[PATCH] re-fix child reaping in wordexp

[PATCH] re-fix child reaping in wordexp

[Alexander Monakov]

Do not retry waitpid if the child was terminated by a signal. Do not
examine status: since we are not passing any flags, we will not receive
stop or continue notifications.
---

In general retrying waitpid on EINTR is not robust in case pid reuse is
possible, but fixing that requires changing waitpid call sites to only
do that with signals blocked (where that's not already the case).

 src/misc/wordexp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
index db39b5b8..d123cf75 100644
--- a/src/misc/wordexp.c
+++ b/src/misc/wordexp.c
@@ -14,13 +14,7 @@
 static void reap(pid_t pid)
 {
 	int status;
-	for (;;) {
-		if (waitpid(pid, &status, 0) < 0) {
-			if (errno != EINTR) return;
-		} else {
-			if (WIFEXITED(status)) return;
-		}
-	}
+	while (waitpid(pid, &status, 0) < 0 && errno == EINTR);
 }
 
 static char *getword(FILE *f)
-- 
2.11.0

Re: [PATCH] re-fix child reaping in wordexp

[Rich Felker]

On Mon, Feb 05, 2018 at 05:38:37PM +0300, Alexander Monakov wrote:
> Do not retry waitpid if the child was terminated by a signal. Do not
> examine status: since we are not passing any flags, we will not receive
> stop or continue notifications.

Looks fine.

> ---
> 
> In general retrying waitpid on EINTR is not robust in case pid reuse is
> possible, but fixing that requires changing waitpid call sites to only
> do that with signals blocked (where that's not already the case).

I don't follow this. Unless there's a bug in the kernel, this should
not be functionally different from SA_RESTART. A return with EINTR
means the child was not reaped.

Rich


> 
>  src/misc/wordexp.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/misc/wordexp.c b/src/misc/wordexp.c
> index db39b5b8..d123cf75 100644
> --- a/src/misc/wordexp.c
> +++ b/src/misc/wordexp.c
> @@ -14,13 +14,7 @@
>  static void reap(pid_t pid)
>  {
>  	int status;
> -	for (;;) {
> -		if (waitpid(pid, &status, 0) < 0) {
> -			if (errno != EINTR) return;
> -		} else {
> -			if (WIFEXITED(status)) return;
> -		}
> -	}
> +	while (waitpid(pid, &status, 0) < 0 && errno == EINTR);
>  }
>  
>  static char *getword(FILE *f)
> -- 
> 2.11.0

Re: [PATCH] re-fix child reaping in wordexp

[Alexander Monakov]

On Mon, 5 Feb 2018, Rich Felker wrote:
> > In general retrying waitpid on EINTR is not robust in case pid reuse is
> > possible, but fixing that requires changing waitpid call sites to only
> > do that with signals blocked (where that's not already the case).
> 
> I don't follow this. Unless there's a bug in the kernel, this should
> not be functionally different from SA_RESTART. A return with EINTR
> means the child was not reaped.

The problem I had in mind is that you don't know if a signal handler or
another thread had (yes, incorrectly) already reaped that child when you
are about to retry waitpid.

With signals blocked, you issue just one waitpid, and you need very rapid
pid reuse to happen, after someone successfully reaps your child
even before you enter waitpid.

Of course this is a bit moot since the other thread/sighandler shouldn't
be issuing wildcard waits in the first place, and if rapid pid reuse
does not happen you safely leave the retry loop with ECHILD. But this is
why I said "not robust" rather than "incorrect".

Alexander

Re: [PATCH] re-fix child reaping in wordexp

[Rich Felker]

On Mon, Feb 05, 2018 at 08:07:44PM +0300, Alexander Monakov wrote:
> On Mon, 5 Feb 2018, Rich Felker wrote:
> > > In general retrying waitpid on EINTR is not robust in case pid reuse is
> > > possible, but fixing that requires changing waitpid call sites to only
> > > do that with signals blocked (where that's not already the case).
> > 
> > I don't follow this. Unless there's a bug in the kernel, this should
> > not be functionally different from SA_RESTART. A return with EINTR
> > means the child was not reaped.
> 
> The problem I had in mind is that you don't know if a signal handler or
> another thread had (yes, incorrectly) already reaped that child when you
> are about to retry waitpid.
> 
> With signals blocked, you issue just one waitpid, and you need very rapid
> pid reuse to happen, after someone successfully reaps your child
> even before you enter waitpid.
> 
> Of course this is a bit moot since the other thread/sighandler shouldn't
> be issuing wildcard waits in the first place, and if rapid pid reuse
> does not happen you safely leave the retry loop with ECHILD. But this is
> why I said "not robust" rather than "incorrect".

OK, that makes sense -- it's a matter of tiny window vs
unboundedly-large window. And in this case EINTR is not relevant; the
same unboundedly-large window can happen if you have a long-running
signal handler with SA_RESTART.

Rich

Re: [PATCH] re-fix child reaping in wordexp

[Alexander Monakov]

On Mon, 5 Feb 2018, Rich Felker wrote:
> OK, that makes sense -- it's a matter of tiny window vs
> unboundedly-large window. And in this case EINTR is not relevant; the
> same unboundedly-large window can happen if you have a long-running
> signal handler with SA_RESTART.

Hm, not sure I follow - can you elaborate? What is the timeline of events
leading to an issue?

Thanks.
Alexander

Re: [PATCH] re-fix child reaping in wordexp

[Rich Felker]

On Mon, Feb 05, 2018 at 11:18:45PM +0300, Alexander Monakov wrote:
> On Mon, 5 Feb 2018, Rich Felker wrote:
> > OK, that makes sense -- it's a matter of tiny window vs
> > unboundedly-large window. And in this case EINTR is not relevant; the
> > same unboundedly-large window can happen if you have a long-running
> > signal handler with SA_RESTART.
> 
> Hm, not sure I follow - can you elaborate? What is the timeline of events
> leading to an issue?

1. waitpid starts waiting for pid 42
2. signal arrives and handler begins running
3. pid 42 exits, gets reaped, and a new child with pid 42 appears
4. signal handler returns
5. waitpid resumes waiting for pid 42

For step 3, think of the signal handler as handling SIGCHLD, reaping
the child, and then doing some operation that might block, and the new
child with pid 42 getting created concurrently from another thread.

Rich