Re: [bug #64451] Unexpected behaviour of xargs when multiple children exit with 255 in parallel

Fixed in git with the attached patch.


On Thu, Jul 20, 2023 at 8:46 AM anonymous <[email protected]> wrote:

> URL:
>   <https://savannah.gnu.org/bugs/?64451>
>
>                  Summary: Unexpected behaviour of xargs when multiple
> children
> exit with 255 in parallel
>                    Group: findutils
>                Submitter: None
>                Submitted: Thu 20 Jul 2023 07:46:06 AM UTC
>                 Category: xargs
>                 Severity: 3 - Normal
>               Item Group: Wrong result
>                   Status: None
>                  Privacy: Public
>              Assigned to: None
>          Originator Name:
>         Originator Email:
>              Open/Closed: Open
>                  Release: 4.9.0
>          Discussion Lock: Any
>            Fixed Release: None
>
>
>     _______________________________________________________
>
> Follow-up Comments:
>
>
> -------------------------------------------------------
> Date: Thu 20 Jul 2023 07:46:06 AM UTC By: Anonymous
> = Synopsis =
>     When xargs is invoked with *-P* option set to a value other than 1 or 2
> and more that one child exits with 255, xargs fails to wait on all children
> before exiting.
>     *-P0* breaks in the same manner on just one exit 255.
>     The case for *-P2* is a degenerate case where the second child exiting
> would cause this bug, but as it is the last running child, exiting with 255
> does not cause any misbehaviour.
> = Examples =
>
> # One exit 255
> seq 10 | xargs -n1 -P4 sh -c 'for i do echo start $i; sleep $i; if [ $i
> -eq 3
> ]; then echo exit $i; exit 255; fi; echo stop $i; done' _; echo xargs
> exited
> here with $?
> start 1
> start 2
> start 3
> start 4
> stop 1
> start 5
> stop 2
> start 6
> exit 3
> xargs: sh: exited with status 255; aborting
> stop 4
> stop 5
> stop 6
> xargs exited here with 124
>
>     After job 3 exited with 255, xargs correctly waits for already started
> jobs to finish.
>
> # Two exit 255s
> seq 10 | xargs -n1 -P4 sh -c 'for i do echo start $i; sleep $i; if [ $i
> -ge 3
> ] && [ $i -le 4 ]; then echo exit $i; exit 255; fi; echo stop $i; done' _;
> echo xargs exited here with $?
> start 1
> start 2
> start 3
> start 4
> stop 1
> start 5
> stop 2
> start 6
> exit 3
> xargs: sh: exited with status 255; aborting
> exit 4
> xargs: sh: exited with status 255; aborting
> xargs exited here with 124
> stop 5
> stop 6
>
>     xargs exits after job 4. Jobs 5 and 6 continue to run in background. It
> can no longer be guaranteed that jobs 5 and 6 will complete before further
> processing in some script.
> = POSIX compliance =
>     This breaks a POSIX requirement.
> From xargs(1p):
>     Implementations wanting to provide parallel operation of the invoked
> utilities are encouraged to add an option enabling parallel invocation, but
> should still wait for termination of all of the children before _xargs_
> terminates normally.
> = Quick debugging =
>     After adding unconditional error messages to the _wait_for_proc_all_()
> function
>
> diff --git a/xargs/xargs.c b/xargs/xargs.c
> index fdede10..9bc7aa7 100644
> --- a/xargs/xargs.c
> +++ b/xargs/xargs.c
> @@ -1597,6 +1597,7 @@ wait_for_proc (bool all, unsigned int minreap)
>  static void
>  wait_for_proc_all (void)
>  {
> +  error (0, 0, _("wait_for_proc_all enter"));
>    static bool waiting = false;
>
>    /* This function was registered by the original, parent, process.
> @@ -1614,6 +1615,7 @@ wait_for_proc_all (void)
>    wait_for_proc (true, 0u);
>    waiting = false;
>
> +  error (0, 0, _("wait_for_proc_all before child_error test"));
>    if (original_exit_value != child_error)
>      {
>        /* wait_for_proc () changed the value of child_error ().  This
>
> and testing xargs again, the following is observed:
>
> # One exit 255
> seq 10 | ./xargs -n1 -P4 sh -c 'for i do echo start $i; sleep $i; if [ $i
> -ge
> 3 ] && [ $i -le 3 ]; then echo exit $i; exit 255; fi; echo stop $i; done'
> _;
> echo xargs exited here with $?
> start 1
> start 2
> start 3
> start 4
> stop 1
> start 5
> stop 2
> start 6
> exit 3
> ./xargs: sh: exited with status 255; aborting
> ./xargs: wait_for_proc_all enter
> stop 4
> stop 5
> stop 6
> ./xargs: wait_for_proc_all before child_error test
> xargs exited here with 124
>
>
> # Two exit 255s
> seq 10 | ./xargs -n1 -P4 sh -c 'for i do echo start $i; sleep $i; if [ $i
> -ge
> 3 ] && [ $i -le 4 ]; then echo exit $i; exit 255; fi; echo stop $i; done'
> _;
> echo xargs exited here with $?
> start 1
> start 2
> start 3
> start 4
> stop 1
> start 5
> stop 2
> start 6
> exit 3
> ./xargs: sh: exited with status 255; aborting
> ./xargs: wait_for_proc_all enter
> exit 4
> ./xargs: sh: exited with status 255; aborting
> xargs exited here with 124
> stop 5
> stop 6
>
>     It can be seen that in the second case _wait_for_proc_all_() did not
> reach
> _before child_error test_ line.
> = My hypothesis =
>     As _wait_for_proc_all_() is registered via _atexit_() it is called when
> the first exit 255 is handled with _error_() (which itself calls _exit_()),
> then _wait_for_proc_all_() calls _wait_for_proc_() inside of which another
> _exit_() is called.
> According to atexit(3):
>     POSIX.1 says that the result of calling *exit*(3) more than once (i.e.,
> calling *exit*(3) within a function registered using *atexit*()) is
> undefined.
> On some systems (but not Linux), this can result in an infinite recursion;
> portable programs should not invoke *exit*(3) inside a function registered
> using *atexit*().
> = In the case of -P0 =
>     If the same command line is run with *-P0* something else happens:
>
> # One exit 255 is now enough to exit xargs
> seq 10 | ./xargs -n1 -P0 sh -c 'for i do echo start $i; sleep $i; if [ $i
> -ge
> 3 ] && [ $i -le 4 ]; then echo exit $i; exit 255; fi; echo stop $i; done'
> _;
> echo xargs exited here with $?
> start 1
> start 2
> start 3
> start 4
> start 5
> start 7
> start 6
> start 8
> start 9
> ./xargs: wait_for_proc_all enter
> start 10
> stop 1
> stop 2
> exit 3
> ./xargs: sh: exited with status 255; aborting
> xargs exited here with 124
> exit 4
> stop 5
> stop 6
> stop 7
> stop 8
> stop 9
> stop 10
>
>     As xargs has read in all the input it enters _wait_for_proc_all_(),
> now a
> single _exit 255_ is enough to exit xargs.
>
>
>
>
>
>
>
>     _______________________________________________________
>
> Reply to this item at:
>
>   <https://savannah.gnu.org/bugs/?64451>
>
> _______________________________________________
> Message sent via Savannah
> https://savannah.gnu.org/
>
>
>

From ad636eae01f21bda602f5e7b85dc1b5d8d72efd2 Mon Sep 17 00:00:00 2001
From: James Youngman <[email protected]>
Date: Sat, 18 May 2024 10:36:17 +0100
Subject: [PATCH] xargs: Wait for all children even if one exited with 255.
To: [email protected]

* xargs/xargs.c: fix Savannah bug #64451 (xargs -P exits before all
children have exited if one exits with status 255).
* xargs/xargs.1 (BUGS): mention this bug and the corrected behaviour.
* doc/find.texi: Likewise.
* NEWS: mention this bugfix.
---
 NEWS          |  3 +++
 doc/find.texi |  4 +++-
 xargs/xargs.1 | 10 ++++++++++
 xargs/xargs.c | 42 +++++++++++++++++++++++++++++++++++-------
 4 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/NEWS b/NEWS
index 2cc48130..762297d1 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)
 
 ** Bug Fixes
 
+  xargs -P now waits for all its child processes to compete before
+  exiting, even if one of them exits with status 255. [#64451]
+
   'find -name /' no longer outputs a warning, because that is a valid pattern
   to match the root directory "/".  Previously, a diagnostic falsely claimed
   that this pattern would not match anything. [#62227]
diff --git a/doc/find.texi b/doc/find.texi
index 294cbaa1..48ecdb68 100644
--- a/doc/find.texi
+++ b/doc/find.texi
@@ -2740,7 +2740,9 @@ Run up to @var{max-procs} processes at a time; the default is 1.  If
 @var{max-procs} is 0, @code{xargs} will run as many processes as
 possible at a time.  Use the @samp{-n}, @samp{-s}, or @samp{-L} option
 with @samp{-P}; otherwise chances are that the command will be run
-only once.
+only once. If a child process exits with status 255, @code{xargs} will
+still wait for all child processes to exit (before version 4.9.0 this
+might not happen).
 @end table
 
 For example, suppose you have a directory tree of large image files
diff --git a/xargs/xargs.1 b/xargs/xargs.1
index e58a2bce..df9ac11f 100644
--- a/xargs/xargs.1
+++ b/xargs/xargs.1
@@ -225,6 +225,9 @@ You cannot decrease it below 1.
 never terminates its commands; when asked to decrease, it merely
 waits for more than one existing command to terminate before starting
 another.
+.B xargs
+always waits for all child processes to exit before exiting itself
+(but see BUGS).
 
 .B Please note
 that it is up to the called processes to properly manage parallel
@@ -532,6 +535,13 @@ which is why this discussion appears in the BUGS section.
 The problem doesn't occur with the output of
 .BR find (1)
 because it emits just one filename per line.
+.P
+In versions of
+.B xargs
+up to and including version 4.9.0,
+.B xargs -P
+would exit while some of its children were still running, if one of
+them exited with status 255.
 .
 .SH "REPORTING BUGS"
 GNU findutils online help: <https://www.gnu.org/software/findutils/#get-help>
diff --git a/xargs/xargs.c b/xargs/xargs.c
index c40096ce..00a5e31c 100644
--- a/xargs/xargs.c
+++ b/xargs/xargs.c
@@ -1494,6 +1494,7 @@ static void
 wait_for_proc (bool all, unsigned int minreap)
 {
   unsigned int reaped = 0;
+  int deferred_exit_status = 0;
 
   while (procs_executing)
     {
@@ -1576,17 +1577,44 @@ wait_for_proc (bool all, unsigned int minreap)
       procs_executing--;
       reaped++;
 
+#define set_deferred_exit_status(n) \
+      do \
+      { \
+	if (deferred_exit_status < n)  \
+	  { \
+	    deferred_exit_status = n; \
+	  } \
+      } while (0)
+
       if (WEXITSTATUS (status) == CHILD_EXIT_PLEASE_STOP_IMMEDIATELY)
-	error (XARGS_EXIT_CLIENT_EXIT_255, 0,
-	       _("%s: exited with status 255; aborting"), bc_state.cmd_argv[0]);
+	{
+	  error (0, 0, _("%s: exited with status 255; aborting"), bc_state.cmd_argv[0]);
+	  set_deferred_exit_status(XARGS_EXIT_CLIENT_EXIT_255);
+	}
       if (WIFSTOPPED (status))
-	error (XARGS_EXIT_CLIENT_FATAL_SIG, 0,
-	       _("%s: stopped by signal %d"), bc_state.cmd_argv[0], WSTOPSIG (status));
+	{
+	  error (0, 0, _("%s: stopped by signal %d"), bc_state.cmd_argv[0], WSTOPSIG (status));
+	  set_deferred_exit_status(XARGS_EXIT_CLIENT_FATAL_SIG);
+	}
       if (WIFSIGNALED (status))
-	error (XARGS_EXIT_CLIENT_FATAL_SIG, 0,
-	       _("%s: terminated by signal %d"), bc_state.cmd_argv[0], WTERMSIG (status));
+	{
+	  error (0, 0,
+		 _("%s: terminated by signal %d"), bc_state.cmd_argv[0], WTERMSIG (status));
+	  set_deferred_exit_status(XARGS_EXIT_CLIENT_FATAL_SIG);
+	}
       if (WEXITSTATUS (status) != 0)
-	child_error = XARGS_EXIT_CLIENT_EXIT_NONZERO;
+	{
+	  child_error = XARGS_EXIT_CLIENT_EXIT_NONZERO;
+	}
+      if (deferred_exit_status && !all)
+	{
+	  break;
+	}
+    }
+  if (deferred_exit_status)
+    {
+      child_error = deferred_exit_status > child_error ? deferred_exit_status : child_error;
+      exit (child_error);
     }
 }
 
-- 
2.39.2