Regression in dash 0.5.10 related to subshells

Commit Message

Herbert Xu May 5, 2018, 4:02 p.m. UTC

Leah Neukirchen <[e-mail]> wrote:
> Hi,
> 
> as of dash 0.5.10, this script waits for the sleep to terminate before
> executing the last line, which no other /bin/sh does.
> dash 0.5.9.1 worked fine.
> 
> 
> #!/bin/sh
> sleep 3 &
> echo first in main shell
> ( echo in subshell; )
> echo back to main shell
> 
> 
> Found by users of the Void Linux project:
> https://github.com/voidlinux/void-packages/issues/14123

Thanks for the report!

This patch should fix the problem:

---8<---
Subject: jobs - Do not block when waiting on SIGCHLD

Because of the nature of SIGCHLD, the process may have already been
waited on and therefore we must be prepared for the case that wait
may block.  So ensure that it doesn't by using WNOHANG.

Fixes: 03876c0743a5 ("eval: Reap zombies after built-in...")
Signed-off-by: Herbert Xu <[e-mail]>

Comments

Leah Neukirchen May 5, 2018, 5:22 p.m. UTC | #1

Herbert Xu <[e-mail]> writes:

> Leah Neukirchen <[e-mail]> wrote:
>> Hi,
>> 
>> as of dash 0.5.10, this script waits for the sleep to terminate before
>> executing the last line, which no other /bin/sh does.
>> dash 0.5.9.1 worked fine.
>> 
>> 
>> #!/bin/sh
>> sleep 3 &
>> echo first in main shell
>> ( echo in subshell; )
>> echo back to main shell
>> 
>> 
>> Found by users of the Void Linux project:
>> https://github.com/voidlinux/void-packages/issues/14123
>
> Thanks for the report!
>
> This patch should fix the problem:

It indeed seems to.

Thanks for the quick help,

Jilles Tjoelker May 6, 2018, 1:53 p.m. UTC | #2

On Sun, May 06, 2018 at 12:02:43AM +0800, Herbert Xu wrote:
> Subject: jobs - Do not block when waiting on SIGCHLD

> Because of the nature of SIGCHLD, the process may have already been
> waited on and therefore we must be prepared for the case that wait
> may block.  So ensure that it doesn't by using WNOHANG.

> Fixes: 03876c0743a5 ("eval: Reap zombies after built-in...")
> Signed-off-by: Herbert Xu <[e-mail]>

> diff --git a/src/jobs.c b/src/jobs.c
> index 1a97c54..6dc555f 100644
> --- a/src/jobs.c
> +++ b/src/jobs.c
> @@ -975,8 +975,8 @@ waitforjob(struct job *jp)
>  	int st;
>  
>  	TRACE(("waitforjob(%%%d) called\n", jp ? jobno(jp) : 0));
> -	while ((jp && jp->state == JOBRUNNING) || gotsigchld)
> -		dowait(DOWAIT_BLOCK, jp);
> +	while (jp ? jp->state == JOBRUNNING : gotsigchld)
> +		dowait(jp ? DOWAIT_BLOCK : DOWAIT_NORMAL, jp);
>  	if (!jp)
>  		return exitstatus;
>  	st = getstatus(jp);

Now each of the first four executable lines of waitforjob() does
something different for jp == NULL and jp != NULL. It probably makes
more sense to separate the jp == NULL case into a new function.

Patch

diff --git a/src/jobs.c b/src/jobs.c
index 1a97c54..6dc555f 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -975,8 +975,8 @@  waitforjob(struct job *jp)
 	int st;
 
 	TRACE(("waitforjob(%%%d) called\n", jp ? jobno(jp) : 0));
-	while ((jp && jp->state == JOBRUNNING) || gotsigchld)
-		dowait(DOWAIT_BLOCK, jp);
+	while (jp ? jp->state == JOBRUNNING : gotsigchld)
+		dowait(jp ? DOWAIT_BLOCK : DOWAIT_NORMAL, jp);
 	if (!jp)
 		return exitstatus;
 	st = getstatus(jp);