PGRP is supported again.
authorlace <>
Tue, 10 Jul 2007 15:28:28 +0000 (15:28 +0000)
committerlace <>
Tue, 10 Jul 2007 15:28:28 +0000 (15:28 +0000)
SIGCHLD race fixed.
Various other races fixed.

src/orphanripper.c

index 59fe41f..cdf3349 100644 (file)
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * Reap any leftover children possibly holding file descriptors.
- * Children are identified by the open file descriptor.
- * PGID or SID may be set by the children on their own.
- * If we find a child by its file descriptor, we kill it will all its process
- * tree (grandchildren).
+ * Children are identified by the stale file descriptor or PGID / SID.
+ * Both can be missed but only the stale file descriptors are important for us.
+ * PGID / SID may be set by the children on their own.
+ * If we fine a candidate we kill it will all its process tree (grandchildren).
  * The child process is run with `2>&1' redirection (due to forkpty(3)).
  * 2007-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
  */
 
-#define _XOPEN_SOURCE 1
-#define _XOPEN_SOURCE_EXTENDED 1
-#define _BSD_SOURCE 1
+#define _GNU_SOURCE 1
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <fcntl.h>
 #include <assert.h>
 #include <pty.h>
+#include <poll.h>
 
 static const char *progname;
 
-static int signal_child_hit = 0;
+static volatile int signal_child_hit = 0;
 
 static void signal_child (int signo)
 {
@@ -52,31 +51,33 @@ static void signal_child (int signo)
 }
 
 static char childptyname[LINE_MAX];
+static pid_t child;
 
 static int spawn (char **argv)
 {
-  pid_t child, child_got;
+  pid_t child_got;
   int status, amaster, i;
-  char buf[LINE_MAX];
-  ssize_t buf_got;
   struct sigaction act;
+  sigset_t sigset;
+
+  /* Never miss SIGCHLD.  */
+  i = sigemptyset (&sigset);
+  assert (i == 0);
+  i = sigaddset (&sigset, SIGCHLD);
+  assert (i == 0);
+  i = sigprocmask (SIG_BLOCK, &sigset, NULL);
+  assert (i == 0);
+  i = sigemptyset (&sigset);
+  assert (i == 0);
 
   /* We do not use signal(2) to be sure we have SA_RESTART unset.  */
   memset (&act, 0, sizeof (act));
   act.sa_handler = signal_child;
   i = sigemptyset (&act.sa_mask);
-  if (i != 0)
-    {
-      perror ("sigemotyset(3)");
-      exit (EXIT_FAILURE);
-    }
+  assert (i == 0);
   act.sa_flags = 0;
   i = sigaction (SIGCHLD, &act, NULL);
-  if (i != 0)
-    {
-      perror ("sigaction(2)");
-      exit (EXIT_FAILURE);
-    }
+  assert (i == 0);
 
   child = forkpty (&amaster, childptyname, NULL, NULL);
   switch (child)
@@ -85,28 +86,74 @@ static int spawn (char **argv)
        perror ("forkpty(3)");
        exit (EXIT_FAILURE);
       case 0:
+       /* Do not setpgrp(2) in the parent process as the process-group
+          is shared for the whole sh(1) pipeline we could be a part
+          of.  The process-group is set according to PID of the first
+          command in the pipeline.
+          We would rip even vi(1) in the case of:
+               ./orphanripper sh -c 'sleep 1&' | vi -
+          */
+       /* Do not setpgrp(2) as our pty would not be ours and we would
+          get `SIGSTOP' later, particularly after spawning gdb(1).
+          setsid(3) was already executed by forkpty(3) and it would fail if
+          executed again.  */
+       if (getpid() != getpgrp ())
+         {
+           perror ("getpgrp(2)");
+           exit (EXIT_FAILURE);
+         }
        execvp (argv[1], argv + 1);
        perror ("execvp(2)");
        exit (EXIT_FAILURE);
       default:
        break;
     }
+  i = fcntl (amaster, F_SETFL, O_RDWR | O_NONBLOCK);
+  if (i != 0)
+    {
+      perror ("fcntl (amaster, F_SETFL, O_NONBLOCK)");
+      exit (EXIT_FAILURE);
+    }
   for (;;)
     {
-      buf_got = read (amaster, buf, sizeof buf);
-      if (buf_got == -1)
+      struct pollfd pollfd;
+      char buf[LINE_MAX];
+      ssize_t buf_got;
+
+      pollfd.fd = amaster;
+      pollfd.events = POLLIN;
+      i = ppoll (&pollfd, 1, NULL, &sigset);
+      assert (i == -1 || i == 1);
+      if (i == -1)
         {
-         assert (signal_child_hit != 0);
-         assert (errno == EINTR || errno == EIO);
+         assert (errno == EINTR && signal_child_hit != 0);
+         break;
        }
-      if (buf_got <= 0)
+      /* i == 1 */
+      assert (pollfd.revents != 0);
+      if (pollfd.revents & POLLHUP)
         break;
-      if (write (STDOUT_FILENO, buf, buf_got) != buf_got)
+      if (pollfd.revents != POLLIN)
         {
+         fprintf (stderr, "%s: ppoll(2): revents 0x%x\n", progname,
+                  (unsigned) pollfd.revents);
+         exit (EXIT_FAILURE);
+       }
+      buf_got = read (amaster, buf, sizeof buf);
+      if (buf_got == 0)
+       break;
+      if (buf_got == -1)
+       {
+         perror ("read (amaster)");
+         exit (EXIT_FAILURE);
+       }
+      if (write (STDOUT_FILENO, buf, buf_got) != buf_got)
+       {
          perror ("write(2)");
          exit (EXIT_FAILURE);
        }
     }
+  /* WNOHANG still could fail.  */
   child_got = waitpid (child, &status, 0);
   if (child != child_got)
     {
@@ -198,8 +245,8 @@ static int fd_fs_scan (pid_t pid, int (*func) (pid_t pid, const char *link))
   dir = opendir (dirname);
   if (dir == NULL)
     {
-      if (errno == EACCES)
-        return 0;
+      if (errno == EACCES || errno == ENOENT)
+       return 0;
       fprintf (stderr, "%s: opendir (\"%s\"): %m\n", progname, dirname);
       exit (EXIT_FAILURE);
     }
@@ -216,8 +263,11 @@ static int fd_fs_scan (pid_t pid, int (*func) (pid_t pid, const char *link))
          || (dirent->d_type == DT_LNK && strspn (dirent->d_name, "0123456789")
              != strlen (dirent->d_name)))
        {
-         fprintf (stderr, "Unexpected entry \"%s\" on readdir (\"%s\"): %m\n",
-                  dirent->d_name, dirname);
+         /* There is a race, D_TYPE may be uninitialized if stat(2) fails.  */
+         if (dirent->d_type != DT_UNKNOWN)
+           fprintf (stderr, "Unexpected entry \"%s\" (d_type %u)"
+                            " on readdir (\"%s\"): %m\n",
+                    dirent->d_name, (unsigned) dirent->d_type, dirname);
          continue;
        }
       if (dirent->d_type == DT_DIR)
@@ -233,8 +283,9 @@ static int fd_fs_scan (pid_t pid, int (*func) (pid_t pid, const char *link))
       buf_len = readlink (linkname, buf, sizeof buf - 1);
       if (buf_len <= 0 || buf_len >= sizeof buf - 1)
        {
-         fprintf (stderr, "Error reading link \"%s\": %m\n",
-                  linkname);
+         if (errno != ENOENT && errno != EACCES)
+           fprintf (stderr, "Error reading link \"%s\": %m\n",
+                    linkname);
          continue;
        }
       buf[buf_len] = 0;
@@ -360,7 +411,7 @@ static pid_t pid_get_parent (pid_t pid)
   while (errno = 0, fgets (line, sizeof line, f) == line)
     {
       if (strncmp (line, "PPid:\t", sizeof "PPid:\t" - 1) != 0)
-        continue;
+       continue;
       retval = atoi (line + sizeof "PPid:\t" - 1);
       errno = 0;
       break;
@@ -402,11 +453,16 @@ static void killtree (pid_t pid)
 
 static void rip_pid_fs_scan (pid_t pid, void *data)
 {
+  pid_t pgid;
+
   /* Shouldn't happen.  */
   if (pid == getpid ())
     return;
 
-  if (fd_fs_scan (pid, rip_check) != 0)
+  /* Check both PGID and the stale file descriptors.  */
+  pgid = getpgid (pid);
+  if (pgid == child
+      || fd_fs_scan (pid, rip_check) != 0)
     killtree (pid);
 }
 
@@ -415,16 +471,18 @@ static void killproc (pid_t pid)
   const char *cmdline;
 
   cmdline = read_cmdline (pid);
+  /* Avoid printing the message for already gone processes.  */
   if (kill (pid, 0) != 0 && errno == ESRCH)
     return;
   if (cmdline == NULL)
     cmdline = "<error>";
   fprintf (stderr, "%s: Killed -9 orphan PID %d: %s\n", progname, (int) pid, cmdline);
-  if (kill (pid, SIGKILL)) {
-    fprintf (stderr, "%s: kill (%d, SIGKILL): %m\n", progname,
-       (int) pid);
-    return;
-  }
+  if (kill (pid, SIGKILL))
+    {
+      if (errno != ESRCH)
+       fprintf (stderr, "%s: kill (%d, SIGKILL): %m\n", progname, (int) pid);
+      return;
+    }
   /* Do not waitpid(2) as it cannot be our direct descendant and it gets
      cleaned up by init(8).  */
 #if 0