dash race

dash race

[Alexey Gladkov]

Greetings!

Sometimes following script is get 99% CPU and will be never
finished. This happens with dash from git. I can't bisect
this because 05c1076 has this problem.

<================================
#!/bin/dash
trap "echo TRAP" USR1
stub() {
	echo ">>> STUB $1" >&2
	sleep $1
	echo "<<< STUB $1" >&2
	kill -USR1 $$
}
stub 3 &
stub 2 &
until { echo "###"; wait; } do
	echo "*** $?"
done
================================>

strace:
12541 wait4(-1, 0xaf87aca0, 0, NULL)    = -1 ECHILD (No child processes)
12541 wait4(-1, 0xaf87aca0, 0, NULL)    = -1 ECHILD (No child processes)
12541 wait4(-1, 0xaf87aca0, 0, NULL)    = -1 ECHILD (No child processes)

-- 
Rgrds, legion

Re: dash race

[Oleg Verych]

Alexey Gladkov:

>  Sometimes following script is get 99% CPU and will be never
>  finished. This happens with dash from git. I can't bisect
>  this because 05c1076 has this problem.

Sometimes?

If not in monitor mode (-m and/or -i), then IMHO jobs are not
under control at all.
-- 
sed 'sed && sh + olecom = love'  <<  ''
-o--=O`C
 #oo'L O
<___=E M

Re: dash race

[Alexey Gladkov]

Oleg Verych wrote:
>  Sometimes?

This bug is not 100% reproducible.

>  If not in monitor mode (-m and/or -i), then IMHO jobs are not under
>  control at all.

Option -m not helped.

-- 
Rgrds, legion

job control (Re: dash race)

[Oleg Verych]

OK, then.

What script tries to do?

> <================================
> #!/bin/dash
> trap "echo TRAP" USR1
> stub() {
> 	echo ">>> STUB $1" >&2
> 	sleep $1
> 	echo "<<< STUB $1" >&2
> 	kill -USR1 $$
> }
> stub 3 &
> stub 2 &
> until { echo "###"; wait; } do
> 	echo "*** $?"
> done
> ================================>

`dash` is forking; inside functions in "background" $$ is parent's pid:

olecom@flower:/tmp$ /bin/dash -s << '#'
stub(){ exec 2>&1
        echo "STUB $$"
        sleep $1
}
stub 1 & echo $! ; stub 2 & echo $!
exit
#

STUB 15418
15419
STUB 15418
15421
olecom@flower:/tmp$


which is kind of strage. Messing with functions especially in background
isn't a good idea. I wonder what is semantics of functions WRT traps,
forks, signals, etc..


olecom@flower:/tmp$ /bin/dash -c 'fu(){ fu; }; fu'
Segmentation fault
olecom@flower:/tmp$ /bin/bash -c 'fu(){ fu; }; fu'
^C
olecom@flower:/tmp$

Also, `ps` && `kill` is better implementation of job control. dash's
interactivity isn't a good one, so what reason is behind shell's job
control after all?
____

Re: job control (Re: dash race)

[Alexey Gladkov]

Oleg Verych wrote:
> What script tries to do?

This script forking child process and send to parent signals. If you 
are confused with functions it is possible to replace them by other 
script (see below).

> which is kind of strage. Messing with functions especially in background
> isn't a good idea. I wonder what is semantics of functions WRT traps,
> forks, signals, etc..

Maybe this is not good idea, but shell specification allow this.

Another example:

<============ parent.sh =================
#!/bin/dash
trap "echo TRAP" USR1
./child.sh 3 $$ &
./child.sh 2 $$ &
until { echo "###"; wait; } do
	echo "*** $?"
done
echo "parent end"
========================================>

<============ child.sh =================
#!/bin/ash
echo ">>> STUB $1"
sleep $1
echo "<<< STUB $1"
kill -USR1 $2
========================================>

This example have same problem. The problem not in functions that 
executed in background. The problem in the 'wait' command.

> Also, `ps` && `kill` is better implementation of job control. dash's
> interactivity isn't a good one, so what reason is behind shell's job
> control after all?

I don't understand this.

-- 
Rgrds, legion

Re: job control (Re: dash race)

[Oleg Verych]

[...]
> >Also, `ps` && `kill` is better implementation of job control. dash's
> >interactivity isn't a good one, so what reason is behind shell's job
> >control after all?
> 
> I don't understand this.

You don't understand, that `wait` and job control is interactive shell
features adopted from Korn Shell?

Then we talk different languages, sorry.
____

Re: job control (Re: dash race)

[Alexey Gladkov]

Oleg Verych wrote:
> You don't understand, that `wait` and job control is interactive shell
> features adopted from Korn Shell?

What difference whence has come this features?

> Then we talk different languages, sorry.

We definitely talk in different languages.

-- 
Rgrds, legion

Re: job control (Re: dash race)

[Oleg Verych]

Alexey Gladkov @ Sun, Apr 27, 2008 at 12:23 PM:

> > What script tries to do?
>
>  This script forking child process and send to parent signals.

Let's define what "shell script" is. Such script is a text file (string)
containing shell syntax. This text is read by shell interpreter
command after command. Each command gets executed; in
case of asynchronous one (&, "background" is term of interactive
shell), shell doesn't wait for completion and goes further.

So, script doesn't fork anything, shell does; more accurately,
shell process of PID=N executes commands by forking itself
(i.e. PID=M, M != N), establishing requested environment and
then doing replacing `exec` of a command.

Please correct me, if i'm wrong, but asynchronous shell functions
don't have this logic clear: $$ = N, but not M.

So, if $$ = N, what is "the parent"? Making shell scripting look
more like ordinary programming by mean of such confusion
in functions or in pipes[0] is a bad thing. Shell is a mean of
talking with operating system in the first place, not
programming an application.

[0] when shell is on the right side of | pipe, variables don't set in
  parent process -- it's sub shell, different execution environment --
  common problem of ordinary programmers trying to do shell
  scripting.

Signals. This are not a good way of communication between
processes[1]. Unfortunately "shell specification" didn't bother to
introduce anything like select() to make e.g. pipes (or other usual
fd-based means) usable in this case.

[1] http://www.dore.ru/perl/nntp.pl?f=1&gid=22&mid=25149
Thread: http://www.dore.ru/perl/nntp.pl?f=2&gid=22&mid=25126#25126

> If you are confused with functions it is possible to replace them by
> other script (see below).

I've asked about script (yours) and what it tries to do (on your opinion)

> > which is kind of strage. Messing with functions especially in background
> > isn't a good idea. I wonder what is semantics of functions WRT traps,
> > forks, signals, etc..
>
>  Maybe this is not good idea, but shell specification allow this.

I want to say, that function usage here may have side effects.

>  Another example:

I think, that implementation of background processes management
is incomplete and buggy. It was designed around interactive shell usage
with obsolete control terminal interface and must not be used otherwise.
For example, `bash` has `disown` command to explicitly remove a process
from the job list, don't know any such in `dash`. `dash` doesn't send
SIGHUP to its "children", instead every sub shell must be scripted so,
that it ends its execution if and when needed.

-- 
sed 'sed && sh + olecom = love' << ''
-o--=O`C
 #oo'L O
<___=E M

Re: job control (Re: dash race)

[Herbert Xu]

Oleg Verych <olecom@flower.upol.cz> wrote:

Re: job control (Re: dash race)

[Herbert Xu]

Oleg Verych <olecom@flower.upol.cz> wrote:
> [...]
>> >Also, `ps` && `kill` is better implementation of job control. dash's
>> >interactivity isn't a good one, so what reason is behind shell's job
>> >control after all?
>> 
>> I don't understand this.
> 
> You don't understand, that `wait` and job control is interactive shell
> features adopted from Korn Shell?

I use this construct all the time in my scripts so I certainly
consider this to be a grave bug in dash :)

Unfortunately I'm awfully busy right now but I'll try to reproduce
it once I get a chance.  If anyone can get me a patch before then
it'd be much appreciated :)

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: job control (Re: dash race)

[Oleg Verych]

> >> >Also, `ps` && `kill` is better implementation of job control. dash's
> >> >interactivity isn't a good one, so what reason is behind shell's job
> >> >control after all?
[...] 
> > You don't understand, that `wait` and job control is interactive shell
> > features adopted from Korn Shell?
> 
> I use this construct all the time in my scripts so I certainly
> consider this to be a grave bug in dash :)

And if it's really so, your scripts are waiting indefinitely (or forever)
or consume all CPU (with such bugs)?

It is signal-related. Clear example of useless and buggy stuff, that
is used my many without much knowledge.


== bug: `wait` returns non zero if `trap` is not empty ==

(counting is done by keyboard to show, that `wait` actually waits)

olecom@flower:~$ test_script='
(sleep 2; echo kill; kill -1 $$)&
echo do wait;
wait || ps hc -t `tty`
'
olecom@flower:~$ bug='
trap ":" 1;
'$test_script
olecom@flower:~$ dash -c "$bug"
do wait
1
2
kill
 4256 pts/15   Ss     0:00 bash
 4475 pts/15   S+     0:00 dash
 4479 pts/15   R+     0:00 ps
olecom@flower:~$ 1
bash: 1: command not found
olecom@flower:~$ 2
bash: 2: command not found
olecom@flower:~$
olecom@flower:~$ notrap='
trap "" 1;
'$test_script
olecom@flower:~$ dash -c "$notrap"
do wait
1
2
kill
olecom@flower:~$ 1
bash: 1: command not found
olecom@flower:~$ 2
bash: 2: command not found
olecom@flower:~$

That's why, i'd advise to use `ps` and/or procfs to poll execution
results right from the kernel; `kill` to terminate or clear processes.
Simply and reliable (at least for scripts).

== m flag ==

olecom@flower:~$ dash +m -c "$b"
do wait
1
2
kill
 4256 pts/15   Ss     0:00 bash
 4487 pts/15   S+     0:00 dash
 4491 pts/15   R+     0:00 ps
olecom@flower:~$ 1
bash: 1: command not found
olecom@flower:~$ 2
bash: 2: command not found
olecom@flower:~$ dash +m -c "$s"
do wait
1
2
kill
olecom@flower:~$ 1
bash: 1: command not found
olecom@flower:~$ 2
bash: 2: command not found
olecom@flower:~$

I.e. it doesn't work.

> Unfortunately I'm awfully busy right now but I'll try to reproduce
> it once I get a chance.  If anyone can get me a patch before then
> it'd be much appreciated :)

It's better to design select()-like thing, as Peter did for `read`.
Document it. Remove job control. No joking.
____

Re: job control (Re: dash race)

[Herbert Xu]

On Tue, Apr 29, 2008 at 10:04:37PM +0800, Herbert Xu wrote:
> 
> Unfortunately I'm awfully busy right now but I'll try to reproduce
> it once I get a chance.  If anyone can get me a patch before then
> it'd be much appreciated :)

OK, I finally got to playing with this and here is the patch to
fix it:

commit ed25e9f97e007f684146f729bb5cdeaf91b668b6
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Sun Feb 22 18:16:13 2009 +0800

    [SIGNAL] Remove EXSIG
    
    Now that waitcmd no longer uses EXSIG we can remove it.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 8f58d96..d87e5d4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,7 @@
 2009-02-22  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix dowait signal race.
+	* Remove EXSIG.
 
 2009-01-14  Herbert Xu <herbert@gondor.apana.org.au>
 
diff --git a/src/error.c b/src/error.c
index 7f165c1..e304d3d 100644
--- a/src/error.c
+++ b/src/error.c
@@ -51,7 +51,6 @@
 #include "show.h"
 #include "eval.h"
 #include "parser.h"
-#include "trap.h"
 #include "system.h"
 
 
@@ -98,19 +97,14 @@ exraise(int e)
 
 void
 onint(void) {
-	int i;
 
 	intpending = 0;
 	sigclearmask();
-	i = EXSIG;
-	if (gotsig[SIGINT - 1] && !trap[SIGINT]) {
-		if (!(rootshell && iflag)) {
-			signal(SIGINT, SIG_DFL);
-			raise(SIGINT);
-		}
-		i = EXINT;
+	if (!(rootshell && iflag)) {
+		signal(SIGINT, SIG_DFL);
+		raise(SIGINT);
 	}
-	exraise(i);
+	exraise(EXINT);
 	/* NOTREACHED */
 }
 
diff --git a/src/error.h b/src/error.h
index dd1fc3f..3162e15 100644
--- a/src/error.h
+++ b/src/error.h
@@ -69,7 +69,6 @@ extern int exception;
 #define EXSHELLPROC 2	/* execute a shell procedure */
 #define EXEXEC 3	/* command execution failed */
 #define EXEXIT 4	/* exit the shell */
-#define EXSIG 5		/* trapped signal in wait(1) */
 
 
 /*
@@ -81,7 +80,6 @@ extern int exception;
 
 extern int suppressint;
 extern volatile sig_atomic_t intpending;
-extern int exsig;
 
 #define barrier() ({ __asm__ __volatile__ ("": : :"memory"); })
 #define INTOFF \
@@ -117,15 +115,6 @@ void __inton(void);
 	})
 #define CLEAR_PENDING_INT intpending = 0
 #define int_pending() intpending
-#define EXSIGON() \
-	({ \
-		exsig++; \
-		barrier(); \
-		if (pendingsigs) \
-			exraise(EXSIG); \
-		0; \
-	})
-/* EXSIG is turned off by evalbltin(). */
 
 void exraise(int) __attribute__((__noreturn__));
 #ifdef USE_NORETURN
diff --git a/src/eval.c b/src/eval.c
index 0b449ee..b90a354 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -863,20 +863,13 @@ bail:
 		}
 		if (evalbltin(cmdentry.u.cmd, argc, argv)) {
 			int status;
-			int i, j;
+			int i;
 
 			i = exception;
 			if (i == EXEXIT)
 				goto raise;
 
-			status = 2;
-			j = 0;
-			if (i == EXINT)
-				j = SIGINT;
-			if (i == EXSIG)
-				j = pendingsigs;
-			if (j)
-				status = j + 128;
+			status = (i == EXINT) ? SIGINT + 128 : 2;
 			exitstatus = status;
 
 			if (i == EXINT || spclbltin > 0) {
@@ -926,7 +919,6 @@ cmddone:
 	exitstatus |= outerr(out1);
 	freestdout();
 	commandname = savecmdname;
-	exsig = 0;
 	handler = savehandler;
 
 	return i;
diff --git a/src/trap.c b/src/trap.c
index 53663ae..5b8b046 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -73,11 +73,9 @@ char *trap[NSIG];
 /* current value of signal */
 char sigmode[NSIG - 1];
 /* indicates specified signal received */
-char gotsig[NSIG - 1];
+static char gotsig[NSIG - 1];
 /* last pending signal */
 volatile sig_atomic_t pendingsigs;
-/* do we generate EXSIG events */
-int exsig;
 
 extern char *signal_names[];
 
@@ -278,7 +276,7 @@ onsig(int signo)
 	gotsig[signo - 1] = 1;
 	pendingsigs = signo;
 
-	if (exsig || (signo == SIGINT && !trap[SIGINT])) {
+	if (signo == SIGINT && !trap[SIGINT]) {
 		if (!suppressint)
 			onint();
 		intpending = 1;
diff --git a/src/trap.h b/src/trap.h
index 50c1587..f19a66b 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -37,7 +37,6 @@
 #include <signal.h>
 
 extern char *trap[];
-extern char gotsig[];
 extern char sigmode[];
 extern volatile sig_atomic_t pendingsigs;
 

commit 3800d4934391b144fd261a7957aea72ced7d47ea
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Sun Feb 22 18:10:01 2009 +0800

    [JOBS] Fix dowait signal race
    
    This test program by Alexey Gladkov can cause dash to enter an
    infinite loop in waitcmd.
    
    #!/bin/dash
    trap "echo TRAP" USR1
    stub() {
    echo ">>> STUB $1" >&2
    sleep $1
    echo "<<< STUB $1" >&2
    kill -USR1 $$
    }
    stub 3 &
    stub 2 &
    until { echo "###"; wait; } do
    echo "*** $?"
    done
    
    The problem is that if we get a signal after the wait3 system
    call has returned but before we get to INTON in dowait, then
    we can jump back up to the top and lose the exit status.  So
    if we then wait for the job that has just exited, then it'll
    stay there forever.
    
    I made the original change that caused this bug to fix pretty
    much the same bug but in the opposite direction.  That is, if
    we get a signal after we enter wait3 but before we hit the kernel
    then it too can cause the wait to go on forever (assuming the
    child doesn't exit).
    
    In fact this is pretty much exactly the scenario that you'll
    find in glibc's documentation on pause().  The solution is given
    there too, in the form of sigsuspend, which is the only way to
    do the check and wait atomically.
    
    So this patch fixes Alexey's race without reintroducing the old
    bug by converting the blocking wait3 to a sigsuspend.
    
    In order to do this we need to set a signal handler for SIGCHLD,
    so the code has been modified to always do that.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 1d18cbf..8f58d96 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2009-02-22  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix dowait signal race.
+
 2009-01-14  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Add arith_yacc.h to dash_SOURCES.
diff --git a/src/jobs.c b/src/jobs.c
index 2b6a752..69a84f7 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -75,6 +75,7 @@
 /* mode flags for dowait */
 #define DOWAIT_NORMAL 0
 #define DOWAIT_BLOCK 1
+#define DOWAIT_WAITCMD 2
 
 /* array of jobs */
 static struct job *jobtab;
@@ -589,8 +590,6 @@ waitcmd(int argc, char **argv)
 	int retval;
 	struct job *jp;
 
-	EXSIGON();
-
 	nextopt(nullstr);
 	retval = 0;
 
@@ -609,7 +608,8 @@ waitcmd(int argc, char **argv)
 				jp->waited = 1;
 				jp = jp->prev_job;
 			}
-			dowait(DOWAIT_BLOCK, 0);
+			if (dowait(DOWAIT_WAITCMD, 0) <= 0)
+				goto sigout;
 		}
 	}
 
@@ -631,7 +631,8 @@ start:
 			job = getjob(*argv, 0);
 		/* loop until process terminated or stopped */
 		while (job->state == JOBRUNNING)
-			dowait(DOWAIT_BLOCK, 0);
+			if (dowait(DOWAIT_WAITCMD, 0) <= 0)
+				goto sigout;
 		job->waited = 1;
 		retval = getstatus(job);
 repeat:
@@ -640,6 +641,10 @@ repeat:
 
 out:
 	return retval;
+
+sigout:
+	retval = 128 + pendingsigs;
+	goto out;
 }
 
 
@@ -998,16 +1003,16 @@ dowait(int block, struct job *job)
 	int pid;
 	int status;
 	struct job *jp;
-	struct job *thisjob;
+	struct job *thisjob = NULL;
 	int state;
 
+	INTOFF;
 	TRACE(("dowait(%d) called\n", block));
 	pid = waitproc(block, &status);
 	TRACE(("wait returns pid %d, status=%d\n", pid, status));
 	if (pid <= 0)
-		return pid;
-	INTOFF;
-	thisjob = NULL;
+		goto out;
+
 	for (jp = curjob; jp; jp = jp->prev_job) {
 		struct procstat *sp;
 		struct procstat *spend;
@@ -1115,34 +1120,33 @@ STATIC int onsigchild() {
 STATIC int
 waitproc(int block, int *status)
 {
-#ifdef BSD
-	int flags = 0;
+	sigset_t mask, oldmask;
+	int flags = block == DOWAIT_BLOCK ? 0 : WNOHANG;
+	int err;
+	int sig;
 
 #if JOBS
 	if (jobctl)
 		flags |= WUNTRACED;
 #endif
-	if (block == 0)
-		flags |= WNOHANG;
-	return wait3(status, flags, (struct rusage *)NULL);
-#else
-#ifdef SYSV
-	int (*save)();
 
-	if (block == 0) {
-		gotsigchild = 0;
-		save = signal(SIGCLD, onsigchild);
-		signal(SIGCLD, save);
-		if (gotsigchild == 0)
-			return 0;
-	}
-	return wait(status);
-#else
-	if (block == 0)
-		return 0;
-	return wait(status);
-#endif
-#endif
+	do {
+		err = wait3(status, flags, NULL);
+		if (err || !block)
+			break;
+
+		block = 0;
+
+		sigfillset(&mask);
+		sigprocmask(SIG_SETMASK, &mask, &oldmask);
+
+		while (!(sig = pendingsigs))
+			sigsuspend(&oldmask);
+
+		sigclearmask();
+	} while (sig == SIGCHLD);
+
+	return err;
 }
 
 /*
diff --git a/src/trap.c b/src/trap.c
index 58cd0cc..53663ae 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -71,7 +71,7 @@
 /* trap handler commands */
 char *trap[NSIG];
 /* current value of signal */
-static char sigmode[NSIG - 1];
+char sigmode[NSIG - 1];
 /* indicates specified signal received */
 char gotsig[NSIG - 1];
 /* last pending signal */
@@ -82,9 +82,10 @@ int exsig;
 extern char *signal_names[];
 
 #ifdef mkinit
-INCLUDE <signal.h>
+INCLUDE "trap.h"
 INIT {
-	signal(SIGCHLD, SIG_DFL);
+	sigmode[SIGCHLD - 1] = S_DFL;
+	setsignal(SIGCHLD);
 }
 #endif
 
@@ -207,6 +208,9 @@ setsignal(int signo)
 		}
 	}
 
+	if (signo == SIGCHLD)
+		action = S_CATCH;
+
 	t = &sigmode[signo - 1];
 	tsig = *t;
 	if (tsig == 0) {
diff --git a/src/trap.h b/src/trap.h
index e889136..50c1587 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -38,6 +38,7 @@
 
 extern char *trap[];
 extern char gotsig[];
+extern char sigmode[];
 extern volatile sig_atomic_t pendingsigs;
 
 int trapcmd(int, char **);

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt