GNU C Library master sources branch release/2.25/master updated. glibc-2.25-66-gb0afcf5

[aurel32 at sourceware dot org]

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.25/master has been updated
       via  b0afcf5db758747c2e563a1db0af6db7a1927746 (commit)
       via  59f0c83e71620e2d8bcec055e7788a6632bea805 (commit)
      from  0e6f64d9d04a9a26a048d1fb1bf1ebdb6739c5af (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
http://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=b0afcf5db758747c2e563a1db0af6db7a1927746

commit b0afcf5db758747c2e563a1db0af6db7a1927746
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Sat Oct 21 11:33:27 2017 -0200

    posix: Do not use WNOHANG in waitpid call for Linux posix_spawn
    
    As shown in some buildbot issues on aarch64 and powerpc, calling
    clone (VFORK) and waitpid (WNOHANG) does not guarantee the child
    is ready to be collected.  This patch changes the call back to 0
    as before fe05e1cb6d64 fix.
    
    This change can lead to the scenario 4.3 described in the commit,
    where the waitpid call can hang undefinitely on the call.  However
    this is also a very unlikely and also undefinied situation where
    both the caller is trying to terminate a pid before posix_spawn
    returns and the race pid reuse is triggered.  I don't see how to
    correct handle this specific situation within posix_spawn.
    
    Checked on x86_64-linux-gnu, aarch64-linux-gnu and
    powerpc64-linux-gnu.
    
    	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Use 0 instead of
    	WNOHANG in waitpid call.
    
    (cherry picked from commit aa95a2414e4f664ca740ad5f4a72d9145abbd426)

diff --git a/ChangeLog b/ChangeLog
index 1da1c0f..c1df219 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-10-23  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Use 0 instead of
+	WNOHANG in waitpid call.
+
 2017-10-20  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
 
 	[BZ #22273]
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 93657c4..ee09fb7 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -372,12 +372,12 @@ __spawnix (pid_t * pid, const char *file,
       ec = args.err;
       if (ec > 0)
 	/* There still an unlikely case where the child is cancelled after
-	   setting args.err, due to a positive error value.  Also due a
+	   setting args.err, due to a positive error value.  Also there is
 	   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);
+	   to an unrelated process).  Unfortunately due synchronization
+	   issues where the kernel might not have the process collected
+	   the waitpid below can not use WNOHANG.  */
+	__waitpid (new_pid, NULL, 0);
     }
   else
     ec = -new_pid;

http://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=59f0c83e71620e2d8bcec055e7788a6632bea805

commit 59f0c83e71620e2d8bcec055e7788a6632bea805
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Thu Oct 12 15:20:57 2017 -0300

    posix: Fix improper assert in Linux posix_spawn (BZ#22273)
    
    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.
    
    (cherry picked from commit fe05e1cb6d64dba6172249c79526f1e9af8f2bfd)

diff --git a/ChangeLog b/ChangeLog
index d3c5570..1da1c0f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2017-10-20  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+	[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.
+
 2017-08-09  Andreas Schwab  <schwab@suse.de>
 
 	* nptl/Makefile (tests) [$(build-shared) = yes]: Add
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 29d8f25..93657c4 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>
@@ -266,7 +265,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
@@ -337,7 +335,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;
@@ -360,12 +358,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 collect 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 if 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 collect 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.err, due 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;

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                        |   12 ++++++++++++
 sysdeps/unix/sysv/linux/spawni.c |   24 ++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)


hooks/post-receive
-- 
GNU C Library master sources