Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-09 Thread Jilles Tjoelker
> > POSIX's Shell and Utilities (XCU) 2.12 [1] does say that "[the]
> > environment of the shell process shall not be changed by the utility",
> > and that environment includes open files. My understanding is that
> > dash's new behaviour (and incidentally, ksh93's one) is incorrect.

> As I understand it, the intent of that section was not to prohibit
> changes in process state due to a command like "exec foo" replacing
> the shell process.  The shell execution environment is not changed but
> simply no longer exists once execve() has been called.

The word "process" seems a bit awkward here, yes. For the most part,
POSIX is very careful in writing "shell environment" instead of "shell
process", so that creating a new process can often be avoided by clever
implementations.

Anyway, if the shell has found that it can safely exec a simple command
without forking (or run a subshell without forking), there is no
possibility of any execution in the original shell environment, so there
seems no need to preserve any open files or other resource needed for
such execution.

Note that 2.7 Redirection does not say anything explicit about
preserving the previous content of file descriptors, only that a
redirection on a command (except 'exec') shall be in effect during that
command only. Therefore, the shell only needs to save file descriptors
at high numbers if it may need to execute subsequent commands in the
same process ("may" because a trap for EXIT might be set in the command,
if it is not external).

Also note that 2.9.1 Simple Commands does not explicitly call for
creating a new process. If there are subsequent commands, the shell will
obviously need to create a new process, but otherwise that is not
needed.

> More to the point, POSIX is very easy to change[1], so if this
> behavior is causing a problem in otherwise reasonable application
> code, methinks it would be better to discuss that instead...

The behaviour you are complaining about exists in almost all shells
(tried: FreeBSD /bin/sh, dash, bash, zsh, mksh, ksh93, Heirloom Bourne
shell, kBuild kmk_ash (much like NetBSD /bin/sh)) in some contexts, so
it is unlikely to change.

To demonstrate, where ${SH} is the shell to be tested:

${SH} -c '/bin/sleep 10 >/dev/null &' | { cat; echo EOF; }

The "EOF" line appears immediately in all tested shells, showing that
there is nothing holding onto the pipe in the subshell environment
created by '&'.

On the other hand, if the redirection is 2>/dev/null instead, it takes
ten seconds before "EOF" appears, as in this case the pipe file
descriptor is passed on.

If braces or parentheses are added around the sleep command (including
or not including the redirection), behaviour varies per shell.

Likewise, behaviour for

{ ${SH} -c '/bin/sleep 10 >/dev/null' & } | { cat; echo EOF; }

varies per shell, and I do not see a reason why this should keep the
pipe file descriptor open while shells clearly agree that it should not
be kept open in the first example.

(Note that I write /bin/sleep explicitly, because some shells have a
sleep builtin.)

-- 
Jilles Tjoelker



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-06 Thread Jonathan Nieder
(-cc: debian-devel)
Stéphane Glondu wrote:

> [CC-ing debian-devel to get more opinions]

Sorry for the tone of my last response.  But really, more opinions are
not needed, though the increased review of the ramifications of the
change is welcome.  Facts should suffice. ;-)

> POSIX's Shell and Utilities (XCU) 2.12 [1] does say that "[the]
> environment of the shell process shall not be changed by the utility",
> and that environment includes open files. My understanding is that
> dash's new behaviour (and incidentally, ksh93's one) is incorrect.

As I understand it, the intent of that section was not to prohibit
changes in process state due to a command like "exec foo" replacing
the shell process.  The shell execution environment is not changed but
simply no longer exists once execve() has been called.

More to the point, POSIX is very easy to change[1], so if this
behavior is causing a problem in otherwise reasonable application
code, methinks it would be better to discuss that instead...

From a release management point of view, I'm of course happy with
delaying reinstatement of the change in sid until the fallout is
better understood.

Thanks,
Jonathan

[1] http://austingroupbugs.net/



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-06 Thread Stéphane Glondu
[CC-ing debian-devel to get more opinions]

On 10/06/2011 04:07 PM, Jonathan Nieder wrote:
>> ocamlbuild's logic is definitively incorrect, but I'm not sure if dash's
>> new behaviour is correct. "bash -c" doesn't skip fork() when a
>> redirection is set up, I guess for a reason. "dash -c" should probably
>> do the same for the same reason.
> 
> Hold on a second.  Dash is not supposed to be a bash emulator. :)
> 
> ksh93 -c "/bin/sleep 100 >dev/null" does skip a fork().  I suspect
> bash does not skip a fork in this case for the same reason that
> 
>   bash -c 'echo hi; /bin/sleep 100'
> 
> does not skip a fork.

POSIX's Shell and Utilities (XCU) 2.12 [1] does say that "[the]
environment of the shell process shall not be changed by the utility",
and that environment includes open files. My understanding is that
dash's new behaviour (and incidentally, ksh93's one) is incorrect.

[1]
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_12


Cheers,

-- 
Stéphane



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-06 Thread Jonathan Nieder
Stéphane Glondu wrote:

> ocamlbuild's logic is definitively incorrect, but I'm not sure if dash's
> new behaviour is correct. "bash -c" doesn't skip fork() when a
> redirection is set up, I guess for a reason. "dash -c" should probably
> do the same for the same reason.

Hold on a second.  Dash is not supposed to be a bash emulator. :)

ksh93 -c "/bin/sleep 100 >dev/null" does skip a fork().  I suspect
bash does not skip a fork in this case for the same reason that

bash -c 'echo hi; /bin/sleep 100'

does not skip a fork.



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-06 Thread Stéphane Glondu
On 10/05/2011 11:51 PM, Stéphane Glondu wrote:
> It seems that darcs failure was unrelated. I digged a bit more, and
> found some suspicious code in ocamlbuild:
> 
>   http://caml.inria.fr/mantis/view.php?id=5371
> 
> I still don't understand why this corner case is triggered with this
> patched dash, and I'd rather wait for more feedback before unreverting
> the patch in dash.

I got it: the command started with "sh -c" (ocamldep) redirects (with
shell's ">") its output to a file, therefore closing first its current
standard output, which is the writing end of a pipe. When fork() is
skipped, the parent process (ocamlbuild) gets an EOF on the reading end
of the pipe, which is then misinterpreted. When fork() is used, the
writing end of the pipe is closed only when the shell exits, i.e. when
ocamldep finishes.

ocamlbuild's logic is definitively incorrect, but I'm not sure if dash's
new behaviour is correct. "bash -c" doesn't skip fork() when a
redirection is set up, I guess for a reason. "dash -c" should probably
do the same for the same reason.


Cheers,

-- 
Stéphane



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-10-05 Thread Stéphane Glondu
Le 26/09/2011 22:44, Jonathan Nieder a écrit :
>> I beg to differ. ocamlbuild uses libc's system(), it would be insane to
>> change that. It seems that darcs is also affected. There might be also
>> more packages affected...
> 
> Got it --- I'll prepare an upload reverting the "sh -c" patch.
> 
> More understanding of what is actually happening would be welcome, of
> course.

It seems that darcs failure was unrelated. I digged a bit more, and
found some suspicious code in ocamlbuild:

  http://caml.inria.fr/mantis/view.php?id=5371

I still don't understand why this corner case is triggered with this
patched dash, and I'd rather wait for more feedback before unreverting
the patch in dash.


Cheers,

-- 
Stéphane




--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-09-26 Thread Mehdi Dogguy
severity 642922 serious
thanks

On  0, Stéphane Glondu  wrote:
> Le 25/09/2011 20:39, Jonathan Nieder a écrit :
> > Thanks for letting me know.  This is important (and a regression), but
> > it should be possible to work around in ocamlbuild as easily as using
> > "bash", so I don't see why that would make it release-critical.
> 
> I beg to differ. ocamlbuild uses libc's system(), it would be insane to
> change that.

... and for that reason, this bug should be release critical. It is a
severe regression, and led many packages to fail to build (in
unexpected ways).

Regards,

with my Release Team hat on

-- 
Mehdi Dogguy



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-09-26 Thread Jonathan Nieder
tags 642922 + upstream
severity 642922 serious
quit

Stéphane Glondu wrote:
> Le 25/09/2011 20:39, Jonathan Nieder a écrit :

>> Thanks for letting me know.  This is important (and a regression), but
>> it should be possible to work around in ocamlbuild as easily as using
>> "bash", so I don't see why that would make it release-critical.
>
> I beg to differ. ocamlbuild uses libc's system(), it would be insane to
> change that. It seems that darcs is also affected. There might be also
> more packages affected...

Got it --- I'll prepare an upload reverting the "sh -c" patch.

More understanding of what is actually happening would be welcome, of
course.



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-09-26 Thread Stéphane Glondu
Le 25/09/2011 20:39, Jonathan Nieder a écrit :
> Thanks for letting me know.  This is important (and a regression), but
> it should be possible to work around in ocamlbuild as easily as using
> "bash", so I don't see why that would make it release-critical.

I beg to differ. ocamlbuild uses libc's system(), it would be insane to
change that. It seems that darcs is also affected. There might be also
more packages affected...

-- 
Stéphane




--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-09-25 Thread Stéphane Glondu
block 642835 by 642922
block 642706 by 642922
severity 642922 serious
thanks

Le 25/09/2011 19:28, Stéphane Glondu a écrit :
> Bugs #642706 (bin-prot FTBFS) and #642835 (sexplib310 FTBFS) can be
> fixed by reverting the patch submitted at [1]. I don't understand why.
> 
> [1] http://thread.gmane.org/gmane.comp.shells.dash/556
> 
> While investigating #642706, in the failing case, I observed that a
> cpp process called with "sh -c" gets SIGPIPE while writing to
> stderr. In the succeeding case, the write is successful, and is read
> by the ocamlbuild process that started "sh -c cpp ...".

Raising severity, since it blocks serious bugs.


Cheers,

-- 
Stéphane




--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org



Bug#642922: regression: "sh -c" change causes FTBFS

2011-09-25 Thread Stéphane Glondu
Package: dash
Version: 0.5.7-1
Severity: important
Tags: patch

Hello,

Bugs #642706 (bin-prot FTBFS) and #642835 (sexplib310 FTBFS) can be
fixed by reverting the patch submitted at [1]. I don't understand why.

[1] http://thread.gmane.org/gmane.comp.shells.dash/556

While investigating #642706, in the failing case, I observed that a
cpp process called with "sh -c" gets SIGPIPE while writing to
stderr. In the succeeding case, the write is successful, and is read
by the ocamlbuild process that started "sh -c cpp ...".


Cheers,

-- 
Stéphane

-- System Information:
Debian Release: wheezy/sid
  APT prefers testing
  APT policy: (990, 'testing'), (500, 'unstable'), (1, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 3.0.0-1-amd64 (SMP w/4 CPU cores)
Locale: LANG=fr_FR.utf8, LC_CTYPE=fr_FR.utf8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages dash depends on:
ii  debianutils  4.0.2   
ii  dpkg 1.16.0.3
ii  libc62.13-21 

dash recommends no packages.

dash suggests no packages.

-- debconf information:
* dash/sh: true
diff -u dash-0.5.7/debian/changelog dash-0.5.7/debian/changelog
--- dash-0.5.7/debian/changelog
+++ dash-0.5.7/debian/changelog
@@ -1,3 +1,10 @@
+dash (0.5.7-1.1) UNRELEASED; urgency=low
+
+  * Non-maintainer upload.
+  * Revert http://thread.gmane.org/gmane.comp.shells.dash/556
+
+ -- Stéphane Glondu   Sun, 25 Sep 2011 19:05:10 +0200
+
 dash (0.5.7-1) unstable; urgency=low
 
   * new upstream release.
only in patch2:
unchanged:
--- dash-0.5.7.orig/src/main.c
+++ dash-0.5.7/src/main.c
@@ -171,7 +171,7 @@
 state3:
 	state = 4;
 	if (minusc)
-		evalstring(minusc, sflag ? 0 : EV_EXIT);
+		evalstring(minusc, 0);
 
 	if (sflag || minusc == NULL) {
 state4:	/* XXX ??? - why isn't this before the "if" statement */