tail: fix unlikely races with >=2 --pids

Also, fix commentary to talk about “nonexistent” rather than
“dead” processes, since the code looks for the former not the
latter and the difference matters for zombies.
* src/tail.c (some_writers_exist): Rename from writers_are_dead,
negate the sense, don’t have a special and counterintuitive case
for !nbpids, remove PIDs found not to exist, and avoid some
though not all unlikely races when kernels reuse PIDs.
(tail_forever): Optimize via blocking I/O even if --pid was used,
so long as all the writers no longer exist.
(tail_forever, tail_forever_inotify): Simplify the writers_dead
logic; there is no need to have a local var to track this, since
we can use pids and nbpids now.
(parse_options): Also free and clear pids if !forever.

diff --git a/NEWS b/NEWS
index 0bda0eae5954e923bfb5a1c6f0c9fb2ab973540f..aa04c18842217a24f15cc93e34f8d1e9e00ceaaa 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -60,6 +60,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   when the input is seekable.
   [bug introduced in coreutils-9.6]
 
+  'tail --pid' avoids some unlikely races if the kernel reuses PIDs.
+  [bug introduced in coreutils-9.5]
+
   tty now exits with status 4 with a special diagnostic if ttyname
   fails even though standard input is a tty.  Formerly it quietly
   pretended that standard input was not a tty.

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index c5ecf23769e1b554caaac37958c7e8dd234b5465..34ea700855c932c16d40a5d39e59706d55a49ea7 100644 (file)
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -3216,8 +3216,8 @@ Size multiplier suffixes are the same as with the @option{-c} option.
 @opindex --pid
 When following by name or by descriptor, you may specify the process ID,
 @var{pid}, of one or more (by repeating @option{--pid}) writers of the
-@var{file} arguments.  Then, shortly after all the identified
-processes terminate, tail will also terminate.  This will
+@var{file} arguments.  Then, @command{tail} will exit shortly
+after all the identified processes no longer exist.  This will
 work properly only if the writers and the tailing process are running on
 the same machine.  For example, to save the output of a build in a file
 and to watch the file grow, if you invoke @command{make} and @command{tail}

diff --git a/src/tail.c b/src/tail.c
index d154330c299af10fce59dbac638fb3037c133e9b..2579f4e13b4fc252fbe3571aa0d672177a6b178e 100644 (file)
--- a/src/tail.c
+++ b/src/tail.c
@@ -313,7 +313,7 @@ With more than one FILE, precede each with a header giving the file name.\n\
              DEFAULT_MAX_N_UNCHANGED_STATS_BETWEEN_OPENS
              );
      fputs (_("\
-      --pid=PID            with -f, terminate after process ID, PID dies;\n\
+      --pid=PID            with -f, exit after PID no longer exists;\n\
                              can be repeated to watch multiple processes\n\
   -q, --quiet, --silent    never output headers giving file names\n\
       --retry              keep trying to open a file if it is inaccessible\n\
@@ -1122,23 +1122,26 @@ any_live_files (const struct File_spec *f, int n_files)
   return false;
 }
 
-/* Determine whether all watched writers are dead.
-   Returns true only if all processes' states can be determined,
-   and all processes no longer exist.  */
+/* Return true if any writers exist, even as zombies.  */
 
 static bool
-writers_are_dead (void)
+some_writers_exist (void)
 {
-  if (!nbpids)
-    return false;
-
-  for (int i = 0; i < nbpids; i++)
+  /* To avoid some (though not all) races if the kernel reuses PIDs,
+     check all PIDs instead of returning merely because one PID exists.  */
+  for (idx_t i = 0; i < nbpids; )
     {
-      if (! (kill (pids[i], 0) < 0 && errno != EPERM))
-        return false;
+      if (kill (pids[i], 0) < 0 && errno == ESRCH)
+        {
+          nbpids--;
+          memmove (&pids[i], &pids[i + 1], (nbpids - i) * sizeof pids[i]);
+        }
+      else
+        i++;
     }
 
-  return true;
+
+  return 0 < nbpids;
 }
 
 /* Tail N_FILES files forever, or until killed.
@@ -1151,14 +1154,14 @@ writers_are_dead (void)
 static void
 tail_forever (struct File_spec *f, int n_files, double sleep_interval)
 {
-  /* Use blocking I/O as an optimization, when it's easy.  */
-  bool blocking = (!nbpids && follow_mode == Follow_descriptor
-                   && n_files == 1 && 0 <= f[0].fd && ! S_ISREG (f[0].mode));
-  bool writers_dead = false;
   int last = n_files - 1;
 
   while (true)
     {
+      /* Use blocking I/O as an optimization, when it's easy.  */
+      bool blocking = (!nbpids && follow_mode == Follow_descriptor
+                       && n_files == 1 && 0 <= f[0].fd && !S_ISREG (f[0].mode));
+
       bool any_input = false;
 
       for (int i = 0; i < n_files; i++)
@@ -1296,19 +1299,21 @@ tail_forever (struct File_spec *f, int n_files, double sleep_interval)
 
       check_output_alive ();
 
-      /* If nothing was read, sleep and/or check for dead writers.  */
       if (!any_input)
         {
-          if (writers_dead)
-            break;
-
-          /* Once the writer is dead, read the files once more to
-             avoid a race condition.  */
-          writers_dead = writers_are_dead ();
+          if (pids)
+            {
+              /* Exit loop if it was already known that no writers exist.
+                 Otherwise, to avoid a race, loop one more time
+                 without sleeping if no writers exist now.  */
+              if (!nbpids)
+                break;
+              if (!some_writers_exist ())
+                continue;
+            }
 
-          if (!writers_dead && xnanosleep (sleep_interval))
+          if (xnanosleep (sleep_interval))
             error (EXIT_FAILURE, errno, _("cannot read realtime clock"));
-
         }
     }
 }
@@ -1462,7 +1467,6 @@ tail_forever_inotify (int wd, struct File_spec *f, int n_files,
   bool tailed_but_unwatchable = false;
   bool found_unwatchable_dir = false;
   bool no_inotify_resources = false;
-  bool writers_dead = false;
   struct File_spec *prev_fspec;
   idx_t evlen = 0;
   char *evbuf;
@@ -1627,14 +1631,12 @@ tail_forever_inotify (int wd, struct File_spec *f, int n_files,
               /* How many ms to wait for changes.  -1 means wait forever.  */
               int delay = -1;
 
-              if (nbpids)
+              if (pids)
                 {
-                  if (writers_dead)
+                  if (!nbpids)
                     exit (EXIT_SUCCESS);
 
-                  writers_dead = writers_are_dead ();
-
-                  if (writers_dead || sleep_interval <= 0)
+                  if (!some_writers_exist () || sleep_interval <= 0)
                     delay = 0;
                   else if (sleep_interval < INT_MAX / 1000 - 1)
                     {
@@ -2270,14 +2272,15 @@ parse_options (int argc, char **argv,
         error (0, 0, _("warning: --retry only effective for the initial open"));
     }
 
-  if (nbpids && !forever)
-    error (0, 0,
-           _("warning: PID ignored; --pid=PID is useful only when following"));
-  else if (nbpids && kill (pids[0], 0) < 0 && errno == ENOSYS)
+  if (pids && (!forever || (kill (pids[0], 0) < 0 && errno == ENOSYS)))
     {
-      error (0, 0, _("warning: --pid=PID is not supported on this system"));
-      nbpids = 0;
+      error (0, 0,
+             _(forever
+               ? "warning: --pid=PID is not supported on this system"
+               : ("warning: PID ignored;"
+                  " --pid=PID is useful only when following")));
       free (pids);
+      pids = nullptr;
     }
 }