Change to xargs to avoid orphaning of utility processes on signal|exit 255 from child

[Jilles Tjoelker]

On Tue, Feb 21, 2012 at 12:50:19PM -0500, Matthew Story wrote:
> On Thu, Feb 16, 2012 at 7:09 PM, Matthew Story <matthewstory at gmail.com>wrote:
> > Apologies if this is the wrong list, I would like to submit a patch that
> > changes the behavior of xargs(1) on signal to child utility process or
> > child utility process exiting 255.  The patch(es) is|are available here:

> > http://axe0.blackskyresearch.net/patches/matt/xargs.no_orphan.patch.txt-- this version will apply to current xargs, and adds diagnostic
> > information for exit 255|signal to utility, as required by POSIX (see
> > PR165155).
> >
> > http://axe0.blackskyresearch.net/patches/matt/xargs.no_orphan.PR165155.patch.txt-- this version will apply on top of the patch in PR165155, as the errx
> > calls in that patch need to be modified to warnx calls.

> I have updated these 2 patches above to branch correctly (the change from
> errx to warnx requires else'ing ... may have to purge browser catch to pick
> up the change), wondering if this patch should be expanded to include all
> of the cases mentioned in the ``Consequences of Errors'' section of the
> POSIX specification:

> > [... snip]
> > If a command line meeting the specified requirements cannot be assembled,
> > the utility cannot be invoked, an invocation of the utility is terminated
> > by a signal, or an invocation of the utility exits with exit status 255,
> > the *xargs* utility shall write a diagnostic message and exit without
> > processing any remaining input.

> This would cause xargs to wait on children if the command line cannot be
> assembled, or utility cannot be invoked, in addition to the 2 cases covered
> by the patch.  This should leave termination via signal to the xargs
> process itself as the only outstanding case where xargs orphans utilities,
> which is congruent with sh(1) behavior, and still allows for signaling all
> processes via signal to the process group, if you actually desire to signal
> all utility processes, along with xargs itself.  Thoughts?

I think that makes sense.

I updated the patch to the recent changes in -current and changed a
local from short to int:

Index: xargs.1
===================================================================
--- xargs.1	(revision 232399)
+++ xargs.1	(working copy)
@@ -297,14 +297,18 @@
 The
 .Nm
 utility exits immediately (without processing any further input) if a
-command line cannot be assembled,
+command line cannot be assembled, or
 .Ar utility
-cannot be invoked, an invocation of
+cannot be invoked. If an invocation of
 .Ar utility
 is terminated by a signal,
 or an invocation of
 .Ar utility
-exits with a value of 255.
+exits with a value of 255, the
+.Nm
+utility stops processing input and exits after all invocations of
+.Ar utility
+finish processing.
 .Sh EXIT STATUS
 The
 .Nm
Index: xargs.c
===================================================================
--- xargs.c	(revision 232399)
+++ xargs.c	(working copy)
@@ -597,6 +597,7 @@
 {
 	pid_t pid;
 	int status;
+	int cause_exit = 0;
 
 	while ((pid = xwait(waitall || pids_full(), &status)) > 0) {
 		/* If we couldn't invoke the utility, exit. */
@@ -604,15 +605,27 @@
 			errno = childerr;
 			err(errno == ENOENT ? 127 : 126, "%s", name);
 		}
-		if (WIFSIGNALED(status))
-			errx(1, "%s: terminated with signal %d; aborting",
+		/*
+		 * If utility exited because of a signal or with a value of
+		 * 255, warn (per POSIX), and then wait until all other
+		 * children have exited before exiting 1-125. POSIX requires
+		 * xargs to stop reading if child exits because of a signal or
+		 * with 255, but it does not require us to exit immediately;
+		 * waiting is preferable to orphaning.
+		 */
+		if (WIFSIGNALED(status)) {
+			waitall = cause_exit = 1;
+			warnx("%s: terminated with signal %d; aborting",
 			    name, WTERMSIG(status));
-		if (WEXITSTATUS(status) == 255)
-			errx(1, "%s: exited with status 255; aborting", name);
-		if (WEXITSTATUS(status))
+		} else if (WEXITSTATUS(status) == 255) {
+			waitall = cause_exit = 1;
+			warnx("%s: exited with status 255; aborting", name);
+		} else if (WEXITSTATUS(status))
 			rval = 1;
 	}
 
+	if (cause_exit)
+		exit(1);
 	if (pid == -1 && errno != ECHILD)
 		err(1, "waitpid");
 }

-- 
Jilles Tjoelker