posix: Fix improper assert in Linux posix_spawn (BZ#22273)

Message ID 1507833562-6945-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • posix: Fix improper assert in Linux posix_spawn (BZ#22273)
Related show

Commit Message

Adhemerval Zanella Oct. 12, 2017, 6:39 p.m.
As noted by Florian Weimer, current Linux posix_spawn implementation
can trigger an assert if the auxiliary process is terminated before
actually setting the err member:

    340   /* Child must set args.err to something non-negative - we rely on
    341      the parent and child sharing VM.  */
    342   args.err = -1;
    [...]
    362   new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
    363                    CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
    364
    365   if (new_pid > 0)
    366     {
    367       ec = args.err;
    368       assert (ec >= 0);

Another possible issue is killing the child between setting the err and
actually calling execve.  In this case the process will not ran, but
posix_spawn also will not report any error:

    269
    270   args->err = 0;
    271   args->exec (args->file, args->argv, args->envp);

As suggested by Andreas Schwab, this patch removes the faulty assert
and also handles any signal that happens before fork and execve as the
spawn was successful (and thus relaying the handling to the caller to
figure this out).  Different than Florian, I can not see why using
atomics to set err would help here, essentially the code runs
sequentially (due CLONE_VFORK) and I think it would not be legal the
compiler evaluate ec without checking for new_pid result (thus there
is no need to compiler barrier).

Checked on x86_64-linux-gnu.

	[BZ #22273]
	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Handle the case where
	the auxiliary process is terminated by a signal before calling _exit
	or execve.
---
 ChangeLog                        |  6 ++++++
 sysdeps/unix/sysv/linux/spawni.c | 12 ++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Florian Weimer Oct. 12, 2017, 6:49 p.m. | #1
* Adhemerval Zanella:

> As suggested by Andreas Schwab, this patch removes the faulty assert

> and also handles any signal that happens before fork and execve as the

> spawn was successful (and thus relaying the handling to the caller to

> figure this out).  Different than Florian, I can not see why using

> atomics to set err would help here, essentially the code runs

> sequentially (due CLONE_VFORK) and I think it would not be legal the

> compiler evaluate ec without checking for new_pid result (thus there

> is no need to compiler barrier).


I thought you'd need to guard against reordering in the child.  Since
the err member is the only thing that is written and the syscalls in
the child act as compiler barriers (so that the store cannot float to
the beginning of the function), this is not actually an issue here.
Adhemerval Zanella Oct. 17, 2017, 1:42 p.m. | #2
On 12/10/2017 15:49, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> As suggested by Andreas Schwab, this patch removes the faulty assert

>> and also handles any signal that happens before fork and execve as the

>> spawn was successful (and thus relaying the handling to the caller to

>> figure this out).  Different than Florian, I can not see why using

>> atomics to set err would help here, essentially the code runs

>> sequentially (due CLONE_VFORK) and I think it would not be legal the

>> compiler evaluate ec without checking for new_pid result (thus there

>> is no need to compiler barrier).

> 

> I thought you'd need to guard against reordering in the child.  Since

> the err member is the only thing that is written and the syscalls in

> the child act as compiler barriers (so that the store cannot float to

> the beginning of the function), this is not actually an issue here.

> 


Right, so are you ok with the patch proposed?
Florian Weimer Oct. 17, 2017, 8:36 p.m. | #3
* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c

> index dea1650..d6acc1e 100644

> --- a/sysdeps/unix/sysv/linux/spawni.c

> +++ b/sysdeps/unix/sysv/linux/spawni.c

> @@ -365,9 +365,17 @@ __spawnix (pid_t * pid, const char *file,

>    if (new_pid > 0)

>      {

>        ec = args.err;

> -      assert (ec >= 0);

>        if (ec != 0)

> -	  __waitpid (new_pid, NULL, 0);

> +	{

> +	  /* It handles the unlikely case where the auxiliary process is

> +	     terminated by a signal before calling _exit or execve.  For

> +	     this case the signal is relayed to the called, as the spawn

> +	     was successful.  */

> +	  int status;

> +	  __waitpid (new_pid, &status, WNOHANG);

> +	  if (WIFSIGNALED (status))

> +	    ec = 0;

> +	}


I'm sorry to say, but this does not look correct to me.  A SIGCHLD
handler may have collected the PID at the point of the waitpid call,
so it can fail with ECHLD or collect the wait notification for another
subprocess due to PID reuse.

I think we need to treat ec == -1 (process did not proceed to the
execve call because it was terminated by a signal) just like ec == 0
(process made a successful execve call, or was terminated just before
the system call, or just after a failed execve, again due to signals).
That is, return the PID to the caller and let it handle the abnormal
process termination due t a signal.

So the assert needs to be removed and the condition changed from ec !=
0 to ec > 0.  And this needs plenty of comments, explaining that we
report subprocess termination due to a signal to the caller and cannot
handle it locally due to the PID reuse race.

I'm not sure if it is necessary to initialize args.err (and ec) to -1
in the first place and reset it in the subprocess.  It seems to me
that this can be simplified a bit because we only need the non-zero
value to carry the information that execve failed, and the error code
with which it failed.
Adhemerval Zanella Oct. 18, 2017, 11:51 a.m. | #4
On 17/10/2017 18:36, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c

>> index dea1650..d6acc1e 100644

>> --- a/sysdeps/unix/sysv/linux/spawni.c

>> +++ b/sysdeps/unix/sysv/linux/spawni.c

>> @@ -365,9 +365,17 @@ __spawnix (pid_t * pid, const char *file,

>>    if (new_pid > 0)

>>      {

>>        ec = args.err;

>> -      assert (ec >= 0);

>>        if (ec != 0)

>> -	  __waitpid (new_pid, NULL, 0);

>> +	{

>> +	  /* It handles the unlikely case where the auxiliary process is

>> +	     terminated by a signal before calling _exit or execve.  For

>> +	     this case the signal is relayed to the called, as the spawn

>> +	     was successful.  */

>> +	  int status;

>> +	  __waitpid (new_pid, &status, WNOHANG);

>> +	  if (WIFSIGNALED (status))

>> +	    ec = 0;

>> +	}

> 

> I'm sorry to say, but this does not look correct to me.  A SIGCHLD

> handler may have collected the PID at the point of the waitpid call,

> so it can fail with ECHLD or collect the wait notification for another

> subprocess due to PID reuse.


Indeed the it does not handle the pid reuse in the case a SIGCHLD handler
is active.  However I think we can't really eliminate all possible races
races in this case (although we can improve its handling).

> 

> I think we need to treat ec == -1 (process did not proceed to the

> execve call because it was terminated by a signal) just like ec == 0

> (process made a successful execve call, or was terminated just before

> the system call, or just after a failed execve, again due to signals).

> That is, return the PID to the caller and let it handle the abnormal

> process termination due t a signal.

> 

> So the assert needs to be removed and the condition changed from ec !=

> 0 to ec > 0.  And this needs plenty of comments, explaining that we

> report subprocess termination due to a signal to the caller and cannot

> handle it locally due to the PID reuse race.

> > I'm not sure if it is necessary to initialize args.err (and ec) to -1

> in the first place and reset it in the subprocess.  It seems to me

> that this can be simplified a bit because we only need the non-zero

> value to carry the information that execve failed, and the error code

> with which it failed.


I think the best approach is to just initialize args.err to 0 to indicate
a default successful case and handle the case where the args.err is set to
positive value with WNOHANG. The args.err check sign is not really important,
we just need to actually check for a value different than 0.  I think we
will have the following scenarios:

  1. For default case with a success execution, args.err will be 0, pid
     won't be collected and it will be reported to caller.

  2. For default failure case, args.err will be positive and the it will
     be collected by the waitpid. An error will be reported to the caller.

  3. For the unlikely case where the process was terminated and not
     collected by a caller signal handler, it will be reported as a 
     successful execution and not be collected by posix_spawn (since 
     args.err will be 0). The caller will need to actually handle this case.

  4. For the unlikely case where the process was terminated and collected
     by caller we have 3 other possible scenarios:

     4.1. The auxiliary process was terminated with args.err equal to 0: 
	  it will handled as 1. (so it does not matter if we hit the pid
          reuse race since we won't possible collect an unexpected 
          process).

     4.2. The auxiliary process was terminated after execve (due a failure
          in calling it) and before setting args.err to -1: it will also
          be handle as 1. but with the issue of not be able to report the
          caller a possible execve failures.

     4.3. The auxiliary process was terminated after args.err is set to -1:
          this is the case where it will be possible to hit the pid reuse
          case where we will need to collected the auxiliary pid but we
          can not be sure if it will be expected one.  I think for this
          case we need to actually change waitpid to use WNOHANG to avoid
          hanging indefinitely on the call and report an error to caller
          since we can't differentiate between a default failure as 2.
          and a possible pid reuse race issue.

What about the patch below:

---

As noted by Florian Weimer, current Linux posix_spawn implementation
can trigger an assert if the auxiliary process is terminated before
actually setting the err member:

    340   /* Child must set args.err to something non-negative - we rely on
    341      the parent and child sharing VM.  */
    342   args.err = -1;
    [...]
    362   new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
    363                    CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
    364
    365   if (new_pid > 0)
    366     {
    367       ec = args.err;
    368       assert (ec >= 0);

Another possible issue is killing the child between setting the err and
actually calling execve.  In this case the process will not ran, but
posix_spawn also will not report any error:

    269
    270   args->err = 0;
    271   args->exec (args->file, args->argv, args->envp);

As suggested by Andreas Schwab, this patch removes the faulty assert
and also handles any signal that happens before fork and execve as the
spawn was successful (and thus relaying the handling to the caller to
figure this out).  Different than Florian, I can not see why using
atomics to set err would help here, essentially the code runs
sequentially (due CLONE_VFORK) and I think it would not be legal the
compiler evaluate ec without checking for new_pid result (thus there
is no need to compiler barrier).

Summarizing the possible scenarios on posix_spawn execution, we
have:

  1. For default case with a success execution, args.err will be 0, pid
     will not be collected and it will be reported to caller.

  2. For default failure case, args.err will be positive and the it will
     be collected by the waitpid.  An error will be reported to the
     caller.

  3. For the unlikely case where the process was terminated and not
     collected by a caller signal handler, it will be reported as succeful
     execution and not be collected by posix_spawn (since args.err will
     be 0). The caller will need to actually handle this case.

  4. For the unlikely case where the process was terminated and collected
     by caller we have 3 other possible scenarios:

     4.1. The auxiliary process was terminated with args.err equal to 0:
	  it will handled as 1. (so it does not matter if we hit the pid
          reuse race since we won't possible collect an unexpected
          process).

     4.2. The auxiliary process was terminated after execve (due a failure
          in calling it) and before setting args.err to -1: it will also
          be handle as 1. but with the issue of not be able to report the
          caller a possible execve failures.

     4.3. The auxiliary process was terminated after args.err is set to -1:
          this is the case where it will be possible to hit the pid reuse
          case where we will need to collected the auxiliary pid but we
          can not be sure if it will be expected one.  I think for this
          case we need to actually change waitpid to use WNOHANG to avoid
          hanging indefinitely on the call and report an error to caller
          since we can't differentiate between a default failure as 2.
          and a possible pid reuse race issue.

Checked on x86_64-linux-gnu.

	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Handle the case where
	the auxiliary process is terminated by a signal before calling _exit
	or execve.
---

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index dea1650..74c26cb 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -17,7 +17,6 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <spawn.h>
-#include <assert.h>
 #include <fcntl.h>
 #include <paths.h>
 #include <string.h>
@@ -268,7 +267,6 @@ __spawni_child (void *arguments)
   __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
 		 ? &attr->__ss : &args->oldmask, 0);
 
-  args->err = 0;
   args->exec (args->file, args->argv, args->envp);
 
   /* This is compatibility function required to enable posix_spawn run
@@ -339,7 +337,7 @@ __spawnix (pid_t * pid, const char *file,
 
   /* Child must set args.err to something non-negative - we rely on
      the parent and child sharing VM.  */
-  args.err = -1;
+  args.err = 0;
   args.file = file;
   args.exec = exec;
   args.fa = file_actions;
@@ -362,12 +360,26 @@ __spawnix (pid_t * pid, const char *file,
   new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
 		   CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
 
+  /* It needs to colect the case where the auxiliary process was created
+     but failed to execute the file (due either any preparation step or
+     for execve itself).  */
   if (new_pid > 0)
     {
+      /* Also, it handles the unlikely case where the auxiliary process was
+	 terminated before calling execve as it was successfully.  The
+	 args.err is set to 0 as default and changed to a positive value
+	 only in case of failure, so in case of premature termination
+	 due a signal args.err will remain zeroed and it will be up to
+	 caller to actually collected it.  */
       ec = args.err;
-      assert (ec >= 0);
-      if (ec != 0)
-	  __waitpid (new_pid, NULL, 0);
+      if (ec > 0)
+	/* There still an unlikely case where the child is cancelled after
+	   setting args.to a positive error value.  Also due a possible
+	   pid reuse race (where the kernel allocated the same pid to
+	   unrelated process) we need not to undefinitely hang expecting
+	   an invalid pid.  In both cases an error is returned to the
+	   caller.  */
+	__waitpid (new_pid, NULL, WNOHANG);
     }
   else
     ec = -new_pid;
-- 
2.7.4
Andreas Schwab Oct. 18, 2017, 12:07 p.m. | #5
On Okt 18 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> 	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Handle the case where

> 	the auxiliary process is terminated by a signal before calling _exit

> 	or execve.


LGTM.

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c

> index dea1650..74c26cb 100644

> --- a/sysdeps/unix/sysv/linux/spawni.c

> +++ b/sysdeps/unix/sysv/linux/spawni.c

> @@ -17,7 +17,6 @@

>     <http://www.gnu.org/licenses/>.  */

>  

>  #include <spawn.h>

> -#include <assert.h>

>  #include <fcntl.h>

>  #include <paths.h>

>  #include <string.h>

> @@ -268,7 +267,6 @@ __spawni_child (void *arguments)

>    __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)

>  		 ? &attr->__ss : &args->oldmask, 0);

>  

> -  args->err = 0;

>    args->exec (args->file, args->argv, args->envp);

>  

>    /* This is compatibility function required to enable posix_spawn run

> @@ -339,7 +337,7 @@ __spawnix (pid_t * pid, const char *file,

>  

>    /* Child must set args.err to something non-negative - we rely on

>       the parent and child sharing VM.  */

> -  args.err = -1;

> +  args.err = 0;

>    args.file = file;

>    args.exec = exec;

>    args.fa = file_actions;

> @@ -362,12 +360,26 @@ __spawnix (pid_t * pid, const char *file,

>    new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,

>  		   CLONE_VM | CLONE_VFORK | SIGCHLD, &args);

>  

> +  /* It needs to colect the case where the auxiliary process was created


collect

> +     but failed to execute the file (due either any preparation step or

> +     for execve itself).  */

>    if (new_pid > 0)

>      {

> +      /* Also, it handles the unlikely case where the auxiliary process was

> +	 terminated before calling execve as it was successfully.  The


as if it was successful

> +	 args.err is set to 0 as default and changed to a positive value

> +	 only in case of failure, so in case of premature termination

> +	 due a signal args.err will remain zeroed and it will be up to

> +	 caller to actually collected it.  */


collect

>        ec = args.err;

> -      assert (ec >= 0);

> -      if (ec != 0)

> -	  __waitpid (new_pid, NULL, 0);

> +      if (ec > 0)

> +	/* There still an unlikely case where the child is cancelled after

> +	   setting args.to a positive error value.  Also due a possible


args.err, due to

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer Oct. 18, 2017, 2:12 p.m. | #6
On 10/18/2017 01:51 PM, Adhemerval Zanella wrote:
> +	/* There still an unlikely case where the child is cancelled after

> +	   setting args.to a positive error value.  Also due a possible

> +	   pid reuse race (where the kernel allocated the same pid to

> +	   unrelated process) we need not to undefinitely hang expecting

> +	   an invalid pid.  In both cases an error is returned to the

> +	   caller.  */

> +	__waitpid (new_pid, NULL, WNOHANG);


It took a while to wrap my head around this one.  I don't think the 
WNOHANG makes much of a difference because in the non-race case, the 
kernel should ensure that the wait notification is ready before vfork 
returns from the kernel in the parent process.  We had some issues with 
wait notifications, but I hope this one is actually properly serialized.

The race case (i.e., PID reuse because a signal handler or another 
thread does a wildcard waitpid) is vaguely undefined anyway.

We could probably do better if we called clone without SIGCHLD.  Not 
sure if that works, but it's a different enhancement and out of scope 
for this patch.  If Andreas is happy with what you've got, so am I.

Thanks,
Florian
Adhemerval Zanella Oct. 18, 2017, 5:34 p.m. | #7
On 18/10/2017 12:12, Florian Weimer wrote:
> On 10/18/2017 01:51 PM, Adhemerval Zanella wrote:

>> +    /* There still an unlikely case where the child is cancelled after

>> +       setting args.to a positive error value.  Also due a possible

>> +       pid reuse race (where the kernel allocated the same pid to

>> +       unrelated process) we need not to undefinitely hang expecting

>> +       an invalid pid.  In both cases an error is returned to the

>> +       caller.  */

>> +    __waitpid (new_pid, NULL, WNOHANG);

> 

> It took a while to wrap my head around this one.  I don't think the WNOHANG makes much of a difference because in the non-race case, the kernel should ensure that the wait notification is ready before vfork returns from the kernel in the parent process.  We had some issues with wait notifications, but I hope this one is actually properly serialized.


Indeed WNOHANG only helps for the race case to just avoid posix_spawn
hang indefinitely (which should be quite rare).  And I am not aware of
any synchronization issues when calling clone (CLONE_VFORK) plus 
waitpid (WNOHANG). I tried some stress run on both kernel 3.2 and
4.4 to check if waitpid would fail in this scenario and it seems to
behave as expected.

> 

> The race case (i.e., PID reuse because a signal handler or another thread does a wildcard waitpid) is vaguely undefined anyway.

> 

> We could probably do better if we called clone without SIGCHLD.  Not sure if that works, but it's a different enhancement and out of scope for this patch.  If Andreas is happy with what you've got, so am I.


I do not think this scenario would help much because caller can still
use a child handler using Linux only flags (__WALL or __WCLONE).

I will commit the patch with the types/grammar mistakes raised by
Andreas.
Florian Weimer Oct. 18, 2017, 7:16 p.m. | #8
On 10/18/2017 07:34 PM, Adhemerval Zanella wrote:
>> We could probably do better if we called clone without SIGCHLD.  Not sure if that works, but it's a different enhancement and out of scope for this patch.  If Andreas is happy with what you've got, so am I.


> I do not think this scenario would help much because caller can still

> use a child handler using Linux only flags (__WALL or __WCLONE).


I was wrong: we need to specify SIGCHLD to get the right semantics for 
the success case.

Internal vfork for system and perhaps popen could use this because these 
interfaces do not expose the PID, but not posix_spawn.

Thanks,
Florian
Tulio Magno Quites Machado Filho Oct. 20, 2017, 11:55 p.m. | #9
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> As noted by Florian Weimer, current Linux posix_spawn implementation

> can trigger an assert if the auxiliary process is terminated before

> actually setting the err member:

>

>     340   /* Child must set args.err to something non-negative - we rely on

>     341      the parent and child sharing VM.  */

>     342   args.err = -1;

>     [...]

>     362   new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,

>     363                    CLONE_VM | CLONE_VFORK | SIGCHLD, &args);

>     364

>     365   if (new_pid > 0)

>     366     {

>     367       ec = args.err;

>     368       assert (ec >= 0);

>

> Another possible issue is killing the child between setting the err and

> actually calling execve.  In this case the process will not ran, but

> posix_spawn also will not report any error:

>

>     269

>     270   args->err = 0;

>     271   args->exec (args->file, args->argv, args->envp);

>

> As suggested by Andreas Schwab, this patch removes the faulty assert

> and also handles any signal that happens before fork and execve as the

> spawn was successful (and thus relaying the handling to the caller to

> figure this out).  Different than Florian, I can not see why using

> atomics to set err would help here, essentially the code runs

> sequentially (due CLONE_VFORK) and I think it would not be legal the

> compiler evaluate ec without checking for new_pid result (thus there

> is no need to compiler barrier).

>

> Summarizing the possible scenarios on posix_spawn execution, we

> have:

>

>   1. For default case with a success execution, args.err will be 0, pid

>      will not be collected and it will be reported to caller.

>

>   2. For default failure case, args.err will be positive and the it will

>      be collected by the waitpid.  An error will be reported to the

>      caller.

>

>   3. For the unlikely case where the process was terminated and not

>      collected by a caller signal handler, it will be reported as succeful

>      execution and not be collected by posix_spawn (since args.err will

>      be 0). The caller will need to actually handle this case.

>

>   4. For the unlikely case where the process was terminated and collected

>      by caller we have 3 other possible scenarios:

>

>      4.1. The auxiliary process was terminated with args.err equal to 0:

> 	  it will handled as 1. (so it does not matter if we hit the pid

>           reuse race since we won't possible collect an unexpected

>           process).

>

>      4.2. The auxiliary process was terminated after execve (due a failure

>           in calling it) and before setting args.err to -1: it will also

>           be handle as 1. but with the issue of not be able to report the

>           caller a possible execve failures.

>

>      4.3. The auxiliary process was terminated after args.err is set to -1:

>           this is the case where it will be possible to hit the pid reuse

>           case where we will need to collected the auxiliary pid but we

>           can not be sure if it will be expected one.  I think for this

>           case we need to actually change waitpid to use WNOHANG to avoid

>           hanging indefinitely on the call and report an error to caller

>           since we can't differentiate between a default failure as 2.

>           and a possible pid reuse race issue.

>

> Checked on x86_64-linux-gnu.


posix/tst-spawn and posix/tst-spawn2 started to fail after this patch.
Reproduced on aarch64 [1], ppc [2], ppc64 [3] and ppc64le.

posix/tst-spawn.out:
error: tst-spawn.c:257: not true: WEXITSTATUS (status) == 0
error: 1 test failures

posix/tst-spawn2.out:
error: tst-spawn2.c:56: waitpid: No such file or directory)
error: 1 test failures

[1] http://glibc-buildbot.reserved-bit.com/builders/glibc-aarch64-linux/builds/760
[2] http://glibc-buildbot.reserved-bit.com/builders/glibc-ppc-linux/builds/152
[3] http://glibc-buildbot.reserved-bit.com/builders/glibc-power8-linux/builds/728

-- 
Tulio Magno
Adhemerval Zanella Oct. 21, 2017, 12:18 a.m. | #10
> Il giorno 20 ott 2017, alle ore 21:55, Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> ha scritto:

> 

> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> 

>> As noted by Florian Weimer, current Linux posix_spawn implementation

>> can trigger an assert if the auxiliary process is terminated before

>> actually setting the err member:

>> 

>>    340   /* Child must set args.err to something non-negative - we rely on

>>    341      the parent and child sharing VM.  */

>>    342   args.err = -1;

>>    [...]

>>    362   new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,

>>    363                    CLONE_VM | CLONE_VFORK | SIGCHLD, &args);

>>    364

>>    365   if (new_pid > 0)

>>    366     {

>>    367       ec = args.err;

>>    368       assert (ec >= 0);

>> 

>> Another possible issue is killing the child between setting the err and

>> actually calling execve.  In this case the process will not ran, but

>> posix_spawn also will not report any error:

>> 

>>    269

>>    270   args->err = 0;

>>    271   args->exec (args->file, args->argv, args->envp);

>> 

>> As suggested by Andreas Schwab, this patch removes the faulty assert

>> and also handles any signal that happens before fork and execve as the

>> spawn was successful (and thus relaying the handling to the caller to

>> figure this out).  Different than Florian, I can not see why using

>> atomics to set err would help here, essentially the code runs

>> sequentially (due CLONE_VFORK) and I think it would not be legal the

>> compiler evaluate ec without checking for new_pid result (thus there

>> is no need to compiler barrier).

>> 

>> Summarizing the possible scenarios on posix_spawn execution, we

>> have:

>> 

>>  1. For default case with a success execution, args.err will be 0, pid

>>     will not be collected and it will be reported to caller.

>> 

>>  2. For default failure case, args.err will be positive and the it will

>>     be collected by the waitpid.  An error will be reported to the

>>     caller.

>> 

>>  3. For the unlikely case where the process was terminated and not

>>     collected by a caller signal handler, it will be reported as succeful

>>     execution and not be collected by posix_spawn (since args.err will

>>     be 0). The caller will need to actually handle this case.

>> 

>>  4. For the unlikely case where the process was terminated and collected

>>     by caller we have 3 other possible scenarios:

>> 

>>     4.1. The auxiliary process was terminated with args.err equal to 0:

>>      it will handled as 1. (so it does not matter if we hit the pid

>>          reuse race since we won't possible collect an unexpected

>>          process).

>> 

>>     4.2. The auxiliary process was terminated after execve (due a failure

>>          in calling it) and before setting args.err to -1: it will also

>>          be handle as 1. but with the issue of not be able to report the

>>          caller a possible execve failures.

>> 

>>     4.3. The auxiliary process was terminated after args.err is set to -1:

>>          this is the case where it will be possible to hit the pid reuse

>>          case where we will need to collected the auxiliary pid but we

>>          can not be sure if it will be expected one.  I think for this

>>          case we need to actually change waitpid to use WNOHANG to avoid

>>          hanging indefinitely on the call and report an error to caller

>>          since we can't differentiate between a default failure as 2.

>>          and a possible pid reuse race issue.

>> 

>> Checked on x86_64-linux-gnu.

> 

> posix/tst-spawn and posix/tst-spawn2 started to fail after this patch.

> Reproduced on aarch64 [1], ppc [2], ppc64 [3] and ppc64le.

> 

> posix/tst-spawn.out:

> error: tst-spawn.c:257: not true: WEXITSTATUS (status) == 0

> error: 1 test failures

> 

> posix/tst-spawn2.out:

> error: tst-spawn2.c:56: waitpid: No such file or directory)

> error: 1 test failures

> 

> [1] http://glibc-buildbot.reserved-bit.com/builders/glibc-aarch64-linux/builds/760

> [2] http://glibc-buildbot.reserved-bit.com/builders/glibc-ppc-linux/builds/152

> [3] http://glibc-buildbot.reserved-bit.com/builders/glibc-power8-linux/builds/728

> 

> -- 

> Tulio Magno


Right, I only indeed checked on x86_64. I will check this out.

Patch

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index dea1650..d6acc1e 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -365,9 +365,17 @@  __spawnix (pid_t * pid, const char *file,
   if (new_pid > 0)
     {
       ec = args.err;
-      assert (ec >= 0);
       if (ec != 0)
-	  __waitpid (new_pid, NULL, 0);
+	{
+	  /* It handles the unlikely case where the auxiliary process is
+	     terminated by a signal before calling _exit or execve.  For
+	     this case the signal is relayed to the called, as the spawn
+	     was successful.  */
+	  int status;
+	  __waitpid (new_pid, &status, WNOHANG);
+	  if (WIFSIGNALED (status))
+	    ec = 0;
+	}
     }
   else
     ec = -new_pid;