dash race

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

Alexey Gladkov:

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

This bug is not 100% reproducible.

Option -m not helped.

Rgrds, legion

OK, then.

What script tries to do?

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?
____

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).

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.

I don't understand this.

Rgrds, legion

[...]

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

Then we talk different languages, sorry.
____

What difference whence has come this features?

We definitely talk in different languages.

Rgrds, legion

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

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

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

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

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

Oleg Verych <[email protected]> wrote:

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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[...]

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.

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

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

commit ed25e9f97e007f684146f729bb5cdeaf91b668b6

Author: Herbert Xu <[email protected]>
Date:   Sun Feb 22 18:16:13 2009 +0800

[SIGNAL] Remove EXSIG

Now that waitcmd no longer uses EXSIG we can remove it.

index 8f58d96..d87e5d4 100644
@@ -1,6 +1,7 @@
 2009-02-22  Herbert Xu <[email protected]>
 
 	* Fix dowait signal race.
+	* Remove EXSIG.
 
 2009-01-14  Herbert Xu <[email protected]>
 

index 7f165c1..e304d3d 100644
@@ -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 */
 }
 

index dd1fc3f..3162e15 100644
@@ -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

index 0b449ee..b90a354 100644
@@ -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;

index 53663ae..5b8b046 100644
@@ -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;

index 50c1587..f19a66b 100644
@@ -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 <[email protected]>
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.

index 1d18cbf..8f58d96 100644
@@ -1,3 +1,7 @@
+2009-02-22  Herbert Xu <[email protected]>
+
+	* Fix dowait signal race.
+
 2009-01-14  Herbert Xu <[email protected]>
 
 	* Add arith_yacc.h to dash_SOURCES.

index 58cd0cc..53663ae 100644
@@ -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) {

index e889136..50c1587 100644
@@ -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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt