Re: [bug #64442] xargs closes stdin on receiving SIGUSR1 or SIGUSR2

From b560e3f911c1eb0095a2c07a461bc28f7335fbd1 Mon Sep 17 00:00:00 2001
From: James Youngman <[email protected]>
Date: Sun, 19 May 2024 11:05:38 +0100
Subject: [PATCH] [xargs] POSIX requires SIGUSR1/2 to be fatal if not blocked.
To: [email protected]

If the user uses -P we're not using only POSIX options, so in that
case it is OK to handle SIGUSR1/2.  Otherwise (without -P) don't
handle these signals, meaning that they could cause xargs to
terminate.

* xargs/xargs.c: set signal handlers for SIGUSR1 and SIGUSR2 only if
the -P option was used.  Warn if SIGUSR1 or SIGUSR2 is not defined.
* tests/xargs/test-sigusr: a new test for this.
* tests/local.mk: build and run the new test.
* NEWS: mention these changes.
---
 NEWS                      |   6 +
 tests/local.mk            |   3 +
 tests/xargs/test-sigusr.c | 352 ++++++++++++++++++++++++++++++++++++++
 xargs/xargs.c             |  42 +++--
 4 files changed, 387 insertions(+), 16 deletions(-)
 create mode 100644 tests/xargs/test-sigusr.c

diff --git a/NEWS b/NEWS
index 762297d1..1ef56c5a 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,12 @@ GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
 ** Bug Fixes
+  If the -P option to xargs is not used, xargs will not change the way
+  in which the SIGUSR1 and SIGUSR2 signals are handled.  This means that
+  they will cause the program to terminate if the signals were not
+  ignored in the process which started xargs.  If you want to depend
+  on xargs not being killed by these signals but don't want
+  parallel execution, use "-P 1".
 
   xargs -P now waits for all its child processes to compete before
   exiting, even if one of them exits with status 255. [#64451]
diff --git a/tests/local.mk b/tests/local.mk
index 9357ccb9..7a7e0aa8 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -82,6 +82,8 @@ EXTRA_DIST += \
 all_root_tests = \
   tests/find/type_list.sh
 
+noinst_PROGRAMS = \
+	tests/xargs/test-sigusr
 
 ALL_RECURSIVE_TARGETS += check-root
 .PHONY: check-root
@@ -108,6 +110,7 @@ all_tests = \
   tests/misc/help-version.sh \
   tests/find/depth-unreadable-dir.sh \
   tests/find/inode-zero.sh \
+  tests/xargs/test-sigusr$(EXEEXT) \
   tests/find/many-dir-entries-vs-OOM.sh \
   tests/find/name-lbracket-literal.sh \
   tests/find/name-slash.sh \
diff --git a/tests/xargs/test-sigusr.c b/tests/xargs/test-sigusr.c
new file mode 100644
index 00000000..616e06bd
--- /dev/null
+++ b/tests/xargs/test-sigusr.c
@@ -0,0 +1,352 @@
+/* test-sigusr -- Verify that xargs SIGUSR1/SIGUSR2 handling is correct.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+   Written by James Youngman <[email protected]>
+*/
+
+/* System headers */
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+/* Gnulib modules */
+#include "error.h"
+
+
+enum { FLAGFILE_MAX = 128 };
+static char flagfile_name[FLAGFILE_MAX];
+static const char* tmpdir = NULL;
+
+
+static void
+remove_temporary_files(void)
+{
+  if (tmpdir)
+    {
+      unlink(flagfile_name);
+      rmdir(tmpdir);
+    }
+  tmpdir = NULL;
+}
+
+static void
+wait_for_flagfile(void)
+{
+  struct stat st;
+  for (;;)
+    {
+      if (0 == stat(flagfile_name, &st))
+	{
+	  return;
+	}
+      sleep(1);
+    }
+}
+
+static void
+delete_flagfile(void)
+{
+  if (0 != unlink(flagfile_name))
+    {
+      if (ENOENT != errno)
+	{
+	  error (2, errno, "Failed to unlink %s", flagfile_name);
+	}
+    }
+}
+
+
+static void
+ignore_signal(int signum)
+{
+  struct sigaction sa;
+  sa.sa_handler = SIG_IGN;
+  sa.sa_flags = 0;
+  sigemptyset(&sa.sa_mask);
+  if (0 != sigaction(signum, &sa, (struct sigaction*)NULL))
+    {
+      error(2, errno, "sigaction failed");
+    }
+}
+
+
+static void handler(int sig)
+{
+  /* Actually does nothing. */
+}
+
+static void
+catch_signal(int signum)
+{
+  struct sigaction sa;
+  sa.sa_handler = handler;
+  sa.sa_flags = 0;
+  if (0 != sigaction(signum, &sa, (struct sigaction*)NULL))
+    {
+      error(2, errno, "sigaction failed");
+    }
+}
+
+static void
+run_child(char * const argv[], int errno_fd)
+{
+  int fd;
+  close(0);
+
+  fd = open("/dev/null", O_RDONLY);
+  if (fd < 0)
+    {
+      error(2, errno, "failed to open /dev/null");
+    }
+  else if (fd != 0)
+    {
+      error(2, errno, "failed to close stdin");
+    }
+
+  execvp("xargs", argv);
+
+  /* Exec failed, tell the parent why. */
+  if (write(errno_fd, &errno, sizeof errno) != sizeof errno)
+    {
+      error (2, errno, "child failed to write errno value though pipe");
+    }
+
+  error(2, errno, "unable to exec xargs in child process");
+
+  /*NOTREACHED*/
+}
+
+static void
+set_close_on_exec(int fd)
+{
+  if (0 != fcntl(fd, F_SETFD, FD_CLOEXEC))
+    {
+      error(2, errno, "F_SETFD failed");
+    }
+}
+
+
+
+struct status
+{
+  int retval;
+  int fatalsig;
+};
+
+/* Run xargs and return its exit status */
+static struct status
+run_xargs(const char *option, const char *optarg, int send_signal)
+{
+  int i = 0;
+  enum { ARGV_MAX = 9 };
+  char *argv[ARGV_MAX];
+  pid_t child, dead;
+  int wstatus;
+  struct status result = {0};
+  int pipefd[2];
+  int child_errno;
+
+  argv[i++] = (char*)"xargs";
+  if (option)
+    {
+      argv[i++] = (char*)option;
+    }
+  if (optarg)
+    {
+      argv[i++] = (char*)optarg;
+    }
+  argv[i++] = (char*)"sh";
+  argv[i++] = (char*)"-c";
+  argv[i++] = (char*)"touch \"$1\" && sleep 4";
+  argv[i++] = (char*)"fnord";
+  argv[i++] = flagfile_name;
+  argv[i++] = NULL;
+  assert(i <= ARGV_MAX);
+
+  /* Create a pipe so that we can detect an exec call. */
+  /* read end is pipefd[0], write end is pipefd[1]. */
+  if (0 != pipe(pipefd))
+    {
+      error(2, errno, "pipe");
+    }
+  set_close_on_exec(pipefd[0]);
+  set_close_on_exec(pipefd[1]);
+  delete_flagfile();
+
+  while ((child=fork()) < 0 && errno == EAGAIN)
+    {
+      perror("unable to fork yet, will try again");
+      sleep(1);
+    }
+
+  switch (child)
+    {
+    case -1:
+      error(EXIT_FAILURE, errno, "cannot fork");
+      break;
+    case 0:			/* child */
+      close(pipefd[0]);		/* close read end */
+      /* The child will close the write end of the pipe on successful exec. */
+      run_child(argv, pipefd[1]);
+      abort();
+      break;
+    default:			/* parent */
+      close(pipefd[1]);		/* close write end */
+      if (read(pipefd[0], &child_errno, sizeof child_errno) < sizeof child_errno)
+	{
+	  /* The exec succeded in the child, and its write end of the pipe was closed. */
+	}
+      else
+	{
+	  /* exec failed in the child and its errno value is now in child_errno. */
+	  error(2, child_errno, "execvp failed in the child process");
+	}
+      break;
+    }
+
+  /* We now know that the exec succeeded and xargs is running.  We
+   * should give it a short time to set up its signal handlers.
+   */
+  fputs("exec succeeded in the child...", stdout);
+  fflush(stdout);
+  /* Wait for the child xargs to launch its command (so we know it
+     will have set any signal handlers it is going to set). */
+  wait_for_flagfile();
+
+  if (send_signal)
+    {
+      if (0 != kill (child, send_signal))
+	{
+	  error(2, errno, "kill failed");
+	}
+    }
+
+  wstatus = 0;
+  dead = waitpid(child, &wstatus, 0);
+
+  if (dead < 0)
+    {
+      error(2, errno, "waitpid failed");
+    }
+  else if (dead == 0)
+    {
+      error(2, 0, "test unexpectedly has more than one child");
+    }
+  else
+    {
+      if (WIFEXITED(wstatus))
+	{
+	  result.retval = WEXITSTATUS(wstatus);
+	}
+      else if (WIFSIGNALED(wstatus))
+	{
+	  result.retval = -1;
+	  result.fatalsig = WTERMSIG(wstatus);
+	}
+      else if (WIFSTOPPED(wstatus))
+	{
+	  error(2, 0, "child was unexpectedly stopped");
+	}
+      return result;
+    }
+  /*NOTREACHED*/
+  abort();
+}
+
+
+static void
+verify_signal_ignored(int signum)
+{
+  struct status status;
+  printf("verifying that xargs can ignore signal %d...", signum);
+  fflush(stdout);
+
+  ignore_signal(signum);
+  status = run_xargs(NULL, NULL, signum);
+  if (status.fatalsig)
+    {
+      fprintf(stderr, "xargs should not have exited fatally on receipt of signal %d\n", signum);
+      exit(1);
+    }
+  if (status.retval)
+    {
+      fprintf(stderr, "xargs should not have returned a nonzero exit status %d\n", status.retval);
+      exit(1);
+    }
+  fputs("OK\n", stdout);
+}
+
+static void
+verify_signal_is_fatal(int signum)
+{
+  struct status status;
+  printf("verifying that signal %d will kill xargs (without -P) if it is not blocked when xargs starts...", signum);
+  fflush(stdout);
+  catch_signal(signum);
+  status = run_xargs(NULL, NULL, signum);
+  if (!status.fatalsig)
+    {
+      fprintf(stderr, "xargs should have exited fatally on receipt of signal %d\n", signum);
+      exit(1);
+    }
+  fputs("OK\n", stdout);
+}
+
+static void
+verify_signal_is_nonfatal_with_p(int signum)
+{
+  struct status status;
+  printf("verifying that signal %d will not kill xargs -P if it is not blocked when xargs starts...", signum);
+  fflush(stdout);
+  catch_signal(signum);
+  status = run_xargs("-P", "5", signum);
+  if (status.fatalsig)
+    {
+      fprintf(stderr, "xargs -P should not have exited fatally on receipt of signal %d\n", signum);
+      exit(1);
+    }
+  fputs("OK\n", stdout);
+}
+
+int
+main(int argc, char *argv[])
+{
+  char tmpdir_tmpl[] = "/tmp/test-sigusr.XXXXXX";
+  tmpdir = mkdtemp(tmpdir_tmpl);
+  if (NULL == tmpdir)
+    {
+      error(2, errno, "failed to create temporary directory");
+    }
+  snprintf(flagfile_name, FLAGFILE_MAX, "%s/flagfile", tmpdir);
+  atexit(remove_temporary_files);
+
+  verify_signal_ignored(SIGUSR1);
+  verify_signal_ignored(SIGUSR2);
+  fputs("\n", stdout);
+
+  verify_signal_is_fatal(SIGUSR1);
+  verify_signal_is_fatal(SIGUSR2);
+  fputs("\n", stdout);
+  verify_signal_is_nonfatal_with_p(SIGUSR1);
+  verify_signal_is_nonfatal_with_p(SIGUSR2);
+  return 0;
+}
diff --git a/xargs/xargs.c b/xargs/xargs.c
index 00a5e31c..56425d55 100644
--- a/xargs/xargs.c
+++ b/xargs/xargs.c
@@ -408,7 +408,7 @@ main (int argc, char **argv)
   void (*act_on_init_result)(void) = noop;
   enum BC_INIT_STATUS bcstatus;
   enum { XARGS_POSIX_HEADROOM = 2048u };
-  struct sigaction sigact;
+  bool catch_usr_signals = false;
 
   /* We #define __STDC_LIMIT_MACROS above for its side effect on
    * <limits.h>, but we use it here to avoid getting what would
@@ -671,6 +671,12 @@ main (int argc, char **argv)
 	case 'P':
 	  /* Allow only up to MAX_PROC_MAX child processes. */
 	  proc_max = parse_num (optarg, 'P', 0L, MAX_PROC_MAX, 1);
+#if !(defined(SIGUSR1) && defined(SIGUSR2))
+	  error (0, 0, _("SIGUSR1 and SIGUSR2 are not both defined, so the -P option does nothing."));
+	  proc_max = 1;
+#else
+	  catch_usr_signals = true;
+#endif
 	  break;
 
         case 'a':
@@ -723,25 +729,29 @@ main (int argc, char **argv)
   act_on_init_result ();
   assert (BC_INIT_OK == bcstatus);
 
+  if (catch_usr_signals)
+    {
 #ifdef SIGUSR1
 # ifdef SIGUSR2
-  /* Accept signals to increase or decrease the number of running
-     child processes.  Do this as early as possible after setting
-     proc_max.  */
-  sigact.sa_handler = increment_proc_max;
-  sigemptyset(&sigact.sa_mask);
-  sigact.sa_flags = 0;
-  if (0 != sigaction (SIGUSR1, &sigact, (struct sigaction *)NULL))
-	  error (0, errno, _("Cannot set SIGUSR1 signal handler"));
-
-  sigact.sa_handler = decrement_proc_max;
-  sigemptyset(&sigact.sa_mask);
-  sigact.sa_flags = 0;
-  if (0 != sigaction (SIGUSR2, &sigact, (struct sigaction *)NULL))
-	  error (0, errno, _("Cannot set SIGUSR2 signal handler"));
+      struct sigaction sigact;
+
+      /* Accept signals to increase or decrease the number of running
+	 child processes.  Do this as early as possible after setting
+	 proc_max.  */
+      sigact.sa_handler = increment_proc_max;
+      sigemptyset(&sigact.sa_mask);
+      sigact.sa_flags = 0;
+      if (0 != sigaction (SIGUSR1, &sigact, (struct sigaction *)NULL))
+	error (0, errno, _("Cannot set SIGUSR1 signal handler"));
+
+      sigact.sa_handler = decrement_proc_max;
+      sigemptyset(&sigact.sa_mask);
+      sigact.sa_flags = 0;
+      if (0 != sigaction (SIGUSR2, &sigact, (struct sigaction *)NULL))
+	error (0, errno, _("Cannot set SIGUSR2 signal handler"));
 # endif /* SIGUSR2 */
 #endif /* SIGUSR1 */
-
+    }
 
   if (0 == strcmp (input_file, "-"))
     {
-- 
2.39.2