[Emacs-diffs] master 6ff8702: Some inotify cleanup

[Paul Eggert]

branch: master
commit 6ff870218dd4bc015cc4115ceb2febd8d807e57c
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Some inotify cleanup
    
    This catches some problems with integer overflow and races
    that I noticed in inotify.c after reviewing the changes
    installed to fix Bug#26126.
    * src/fns.c, src/lisp.h (equal_no_quit): Now extern.
    * src/inotify.c (aspect_to_inotifymask):
    Check for cycles and for improper lists.
    (make_lispy_mask, lispy_mask_match_p): Remove.
    All callers changed to use INTEGER_TO_CONS and CONS_TO_INTEGER.
    (inotifyevent_to_event, add_watch):
    Don’t assume watch descriptors and cookies fit in fixnums.
    (add_watch): Use assoc_no_quit, not Fassoc.
    Avoid integer overflow in (very!) long-running processes where
    the Emacs watch ID could overflow.  Avoid some duplicate code.
    (find_descriptor): New function.
    (remove_descriptor): First arg is now the returned value from
    find_descriptor, rather than the descriptor.  This way, the
    value can be removed without calling Fdelete, which might quit.
    Wait until the end (when watch_list is consistent) before signaling
    any errors.
    (remove_watch, inotify_callback):
    Use find_descriptor to avoid the need for Fdelete.
    (inotify_callback): Use simpler tests for ioctl failure.
    Free temporary buffer if signaled, and put it on the stack if small.
    Use ssize_t to index through read results, to avoid a cast.
    (valid_watch_descriptor): New function, with a tighter check.
    (Finotify_rm_watch, Finotify_valid_p): Use it.
    (Finotify_valid_p): Use assoc_no_quit and ass_no_quit instead
    of Fassoc.  Do not assume the first assoc succeeds.
    * test/src/inotify-tests.el (inotify-valid-p-simple):
    Add inotify-valid-p tests, some of which dump core without
    the fixes noted above.
---
 src/fns.c                 |   3 +-
 src/inotify.c             | 250 +++++++++++++++++++++++++---------------------
 src/lisp.h                |   1 +
 test/src/inotify-tests.el |   9 ++
 4 files changed, 149 insertions(+), 114 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 42e2eec..de7fc1b 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -38,7 +38,6 @@ along with GNU Emacs.  If not, see 
<http://www.gnu.org/licenses/>.  */
 
 static void sort_vector_copy (Lisp_Object, ptrdiff_t,
                              Lisp_Object *restrict, Lisp_Object *restrict);
-static bool equal_no_quit (Lisp_Object, Lisp_Object);
 enum equal_kind { EQUAL_NO_QUIT, EQUAL_PLAIN, EQUAL_INCLUDING_PROPERTIES };
 static bool internal_equal (Lisp_Object, Lisp_Object,
                            enum equal_kind, int, Lisp_Object);
@@ -2121,7 +2120,7 @@ of strings.  (`equal' ignores text properties.)  */)
    Use this only on arguments that are cycle-free and not too large and
    are not window configurations.  */
 
-static bool
+bool
 equal_no_quit (Lisp_Object o1, Lisp_Object o2)
 {
   return internal_equal (o1, o2, EQUAL_NO_QUIT, 0, Qnil);
diff --git a/src/inotify.c b/src/inotify.c
index 004689b..a0a89aa 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -41,16 +41,16 @@ along with GNU Emacs.  If not, see 
<http://www.gnu.org/licenses/>.  */
 #ifndef IN_ONLYDIR
 # define IN_ONLYDIR 0
 #endif
-#define INOTIFY_DEFAULT_MASK (IN_ALL_EVENTS|IN_EXCL_UNLINK)
+#define INOTIFY_DEFAULT_MASK (IN_ALL_EVENTS | IN_EXCL_UNLINK)
 
 /* File handle for inotify.  */
 static int inotifyfd = -1;
 
 /* Alist of files being watched.  We want the returned descriptor to
    be unique for every watch, but inotify returns the same descriptor
-   for multiple calls to inotify_add_watch with the same file.  In
-   order to solve this problem, we add a ID, uniquely identifying a
-   watch/file combination.
+   WD for multiple calls to inotify_add_watch with the same file.
+   Supply a nonnegative integer ID, so that WD and ID together
+   uniquely identify a watch/file combination.
 
    For the same reason, we also need to store the watch's mask and we
    can't allow the following flags to be used.
@@ -60,12 +60,21 @@ static int inotifyfd = -1;
    IN_ONESHOT
    IN_ONLYDIR
 
-   Format: (descriptor . ((id filename callback mask) ...))
-*/
+   Each element of this list is of the form (DESCRIPTOR . WATCHES)
+   where no two DESCRIPTOR values are the same.  DESCRIPTOR represents
+   the inotify watch descriptor and WATCHES is a list with elements of
+   the form (ID FILENAME CALLBACK MASK), where ID is the integer
+   described above, FILENAME names the file being watched, CALLBACK is
+   invoked when the event occurs, and MASK represents the aspects
+   being watched.  The WATCHES list is sorted by ID.  Although
+   DESCRIPTOR and MASK are ordinarily integers, they are conses when
+   representing integers outside of fixnum range.  */
+
 static Lisp_Object watch_list;
 
 static Lisp_Object
-mask_to_aspects (uint32_t mask) {
+mask_to_aspects (uint32_t mask)
+{
   Lisp_Object aspects = Qnil;
   if (mask & IN_ACCESS)
     aspects = Fcons (Qaccess, aspects);
@@ -149,15 +158,13 @@ symbol_to_inotifymask (Lisp_Object symb)
 static uint32_t
 aspect_to_inotifymask (Lisp_Object aspect)
 {
-  if (CONSP (aspect))
+  if (CONSP (aspect) || NILP (aspect))
     {
       Lisp_Object x = aspect;
       uint32_t mask = 0;
-      while (CONSP (x))
-        {
-          mask |= symbol_to_inotifymask (XCAR (x));
-          x = XCDR (x);
-        }
+      FOR_EACH_TAIL (x)
+       mask |= symbol_to_inotifymask (XCAR (x));
+      CHECK_LIST_END (x, aspect);
       return mask;
     }
   else
@@ -165,25 +172,13 @@ aspect_to_inotifymask (Lisp_Object aspect)
 }
 
 static Lisp_Object
-make_lispy_mask (uint32_t mask)
-{
-  return Fcons (make_number (mask & 0xffff),
-                make_number (mask >> 16));
-}
-
-static bool
-lispy_mask_match_p (Lisp_Object mask, uint32_t other)
-{
-  return (XINT (XCAR (mask)) & other)
-    || ((XINT (XCDR (mask)) << 16) & other);
-}
-
-static Lisp_Object
 inotifyevent_to_event (Lisp_Object watch, struct inotify_event const *ev)
 {
-  Lisp_Object name = Qnil;
+  Lisp_Object name;
+  uint32_t mask;
+  CONS_TO_INTEGER (Fnth (make_number (3), watch), uint32_t, mask);
 
-  if (! lispy_mask_match_p (Fnth (make_number (3), watch), ev->mask))
+  if (! (mask & ev->mask))
     return Qnil;
 
   if (ev->len > 0)
@@ -195,10 +190,10 @@ inotifyevent_to_event (Lisp_Object watch, struct 
inotify_event const *ev)
   else
     name = XCAR (XCDR (watch));
 
-  return list2 (list4 (Fcons (make_number (ev->wd), XCAR (watch)),
+  return list2 (list4 (Fcons (INTEGER_TO_CONS (ev->wd), XCAR (watch)),
                        mask_to_aspects (ev->mask),
                        name,
-                       make_number (ev->cookie)),
+                      INTEGER_TO_CONS (ev->cookie)),
                Fnth (make_number (2), watch));
 }
 
@@ -209,55 +204,88 @@ static Lisp_Object
 add_watch (int wd, Lisp_Object filename,
           Lisp_Object aspect, Lisp_Object callback)
 {
-  Lisp_Object descriptor = make_number (wd);
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
-  Lisp_Object watches = Fcdr (elt);
+  Lisp_Object descriptor = INTEGER_TO_CONS (wd);
+  Lisp_Object tail = assoc_no_quit (descriptor, watch_list);
   Lisp_Object watch, watch_id;
-  Lisp_Object mask = make_lispy_mask (aspect_to_inotifymask (aspect));
+  uint32_t imask = aspect_to_inotifymask (aspect);
+  Lisp_Object mask = INTEGER_TO_CONS (imask);
 
-  int id = 0;
-
-  while (! NILP (watches))
+  EMACS_INT id = 0;
+  if (NILP (tail))
+    {
+      tail = list1 (descriptor);
+      watch_list = Fcons (tail, watch_list);
+    }
+  else
     {
-      id = max (id, 1 + XINT (XCAR (XCAR (watches))));
-      watches = XCDR (watches);
+      /* Assign a watch ID that is not already in use, by looking
+        for a gap in the existing sorted list.  */
+      for (; ! NILP (XCDR (tail)); tail = XCDR (tail), id++)
+       if (!EQ (XCAR (XCAR (XCDR (tail))), make_number (id)))
+         break;
+      if (MOST_POSITIVE_FIXNUM < id)
+       emacs_abort ();
     }
 
   watch_id = make_number (id);
   watch = list4 (watch_id, filename, callback, mask);
-
-  if (NILP (elt))
-    watch_list = Fcons (Fcons (descriptor, Fcons (watch, Qnil)),
-                        watch_list);
-  else
-    XSETCDR (elt, Fcons (watch, XCDR (elt)));
+  XSETCDR (tail, Fcons (watch, XCDR (tail)));
 
   return Fcons (descriptor, watch_id);
 }
 
-/*  Remove all watches associated with descriptor.  If INVALID_P is
-    true, the descriptor is already invalid, i.e. it received a
-    IN_IGNORED event. In this case skip calling inotify_rm_watch.  */
+/* Find the watch list element (if any) matching DESCRIPTOR.  Return
+   nil if not found.  If found, return t if the first element matches
+   DESCRIPTOR; otherwise, return the cons whose cdr matches
+   DESCRIPTOR.  This lets the caller easily remove the element
+   matching DESCRIPTOR without having to search for it again, and
+   without calling Fdelete (which might quit).  */
+
+static Lisp_Object
+find_descriptor (Lisp_Object descriptor)
+{
+  Lisp_Object tail, prevtail = Qt;
+  for (tail = watch_list; !NILP (tail); prevtail = tail, tail = XCDR (tail))
+    if (equal_no_quit (XCAR (XCAR (tail)), descriptor))
+      return prevtail;
+  return Qnil;
+}
+
+/*  Remove all watches associated with the watch list element after
+    PREVTAIL, or after the first element if PREVTAIL is t.  If INVALID_P
+    is true, the descriptor is already invalid, i.e., it received a
+    IN_IGNORED event.  In this case skip calling inotify_rm_watch.  */
 static void
-remove_descriptor (Lisp_Object descriptor, bool invalid_p)
+remove_descriptor (Lisp_Object prevtail, bool invalid_p)
 {
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
+  Lisp_Object tail = CONSP (prevtail) ? XCDR (prevtail) : watch_list;
 
-  if (! NILP (elt))
+  int inotify_errno = 0;
+  if (! invalid_p)
     {
-      int wd = XINT (descriptor);
+      int wd;
+      CONS_TO_INTEGER (XCAR (XCAR (tail)), int, wd);
+      if (inotify_rm_watch (inotifyfd, wd) != 0)
+       inotify_errno = errno;
+    }
 
-      watch_list = Fdelete (elt, watch_list);
-      if (! invalid_p)
-        if (inotify_rm_watch (inotifyfd, wd) == -1)
-          report_file_notify_error ("Could not rm watch", descriptor);
+  if (CONSP (prevtail))
+    XSETCDR (prevtail, XCDR (tail));
+  else
+    {
+      watch_list = XCDR (tail);
+      if (NILP (watch_list))
+       {
+         delete_read_fd (inotifyfd);
+         emacs_close (inotifyfd);
+         inotifyfd = -1;
+       }
     }
-  /* Cleanup if no more files are watched.  */
-  if (NILP (watch_list))
+
+  if (inotify_errno != 0)
     {
-      emacs_close (inotifyfd);
-      delete_read_fd (inotifyfd);
-      inotifyfd = -1;
+      errno = inotify_errno;
+      report_file_notify_error ("Could not rm watch", XCAR (tail));
     }
 }
 
@@ -265,19 +293,19 @@ remove_descriptor (Lisp_Object descriptor, bool invalid_p)
 static void
 remove_watch (Lisp_Object descriptor, Lisp_Object id)
 {
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
-
-  if (! NILP (elt))
-    {
-      Lisp_Object watch = Fassoc (id, XCDR (elt));
-
-      if (! NILP (watch))
-        XSETCDR (elt, Fdelete (watch, XCDR (elt)));
-
-      /* Remove the descriptor if noone is watching it.  */
-      if (NILP (XCDR (elt)))
-        remove_descriptor (descriptor, false);
-    }
+  Lisp_Object prevtail = find_descriptor (descriptor);
+  if (NILP (prevtail))
+    return;
+
+  Lisp_Object elt = XCAR (CONSP (prevtail) ? XCDR (prevtail) : watch_list);
+  for (Lisp_Object prev = elt; !NILP (XCDR (prev)); prev = XCDR (prev))
+    if (EQ (id, XCAR (XCAR (XCDR (prev)))))
+      {
+       XSETCDR (prev, XCDR (XCDR (prev)));
+       if (NILP (XCDR (elt)))
+         remove_descriptor (prevtail, false);
+       break;
+      }
 }
 
 /* This callback is called when the FD is available for read.  The inotify
@@ -285,52 +313,44 @@ remove_watch (Lisp_Object descriptor, Lisp_Object id)
 static void
 inotify_callback (int fd, void *_)
 {
-  struct input_event event;
   int to_read;
-  char *buffer;
-  ssize_t n;
-  size_t i;
-
-  to_read = 0;
-  if (ioctl (fd, FIONREAD, &to_read) == -1)
+  if (ioctl (fd, FIONREAD, &to_read) < 0)
     report_file_notify_error ("Error while retrieving file system events",
                              Qnil);
-  buffer = xmalloc (to_read);
-  n = read (fd, buffer, to_read);
+  USE_SAFE_ALLOCA;
+  char *buffer = SAFE_ALLOCA (to_read);
+  ssize_t n = read (fd, buffer, to_read);
   if (n < 0)
-    {
-      xfree (buffer);
-      report_file_notify_error ("Error while reading file system events", 
Qnil);
-    }
+    report_file_notify_error ("Error while reading file system events", Qnil);
 
+  struct input_event event;
   EVENT_INIT (event);
   event.kind = FILE_NOTIFY_EVENT;
 
-  i = 0;
-  while (i < (size_t)n)
+  for (ssize_t i = 0; i < n; )
     {
       struct inotify_event *ev = (struct inotify_event *) &buffer[i];
-      Lisp_Object descriptor = make_number (ev->wd);
-      Lisp_Object elt = Fassoc (descriptor, watch_list);
+      Lisp_Object descriptor = INTEGER_TO_CONS (ev->wd);
+      Lisp_Object prevtail = find_descriptor (descriptor);
 
-      if (! NILP (elt))
+      if (! NILP (prevtail))
         {
-          Lisp_Object watches = XCDR (elt);
-          while (! NILP (watches))
+         Lisp_Object tail = CONSP (prevtail) ? XCDR (prevtail) : watch_list;
+         for (Lisp_Object watches = XCDR (XCAR (tail)); ! NILP (watches);
+              watches = XCDR (watches))
             {
               event.arg = inotifyevent_to_event (XCAR (watches), ev);
               if (!NILP (event.arg))
                 kbd_buffer_store_event (&event);
-              watches = XCDR (watches);
             }
           /* If event was removed automatically: Drop it from watch list.  */
           if (ev->mask & IN_IGNORED)
-            remove_descriptor (descriptor, true);
+           remove_descriptor (prevtail, true);
         }
       i += sizeof (*ev) + ev->len;
     }
 
-  xfree (buffer);
+  SAFE_FREE ();
 }
 
 DEFUN ("inotify-add-watch", Finotify_add_watch, Sinotify_add_watch, 3, 3, 0,
@@ -407,7 +427,7 @@ IN_ONLYDIR  */)
 
   if (inotifyfd < 0)
     {
-      inotifyfd = inotify_init1 (IN_NONBLOCK|IN_CLOEXEC);
+      inotifyfd = inotify_init1 (IN_NONBLOCK | IN_CLOEXEC);
       if (inotifyfd < 0)
        report_file_notify_error ("File watching is not available", Qnil);
       watch_list = Qnil;
@@ -416,12 +436,24 @@ IN_ONLYDIR  */)
 
   encoded_file_name = ENCODE_FILE (filename);
   wd = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
-  if (wd == -1)
+  if (wd < 0)
     report_file_notify_error ("Could not add watch for file", filename);
 
   return add_watch (wd, filename, aspect, callback);
 }
 
+static bool
+valid_watch_descriptor (Lisp_Object wd)
+{
+  return (CONSP (wd)
+         && (RANGED_INTEGERP (0, XCAR (wd), INT_MAX)
+             || (CONSP (XCAR (wd))
+                 && RANGED_INTEGERP ((MOST_POSITIVE_FIXNUM >> 16) + 1,
+                                     XCAR (XCAR (wd)), INT_MAX >> 16)
+                 && RANGED_INTEGERP (0, XCDR (XCAR (wd)), (1 << 16) - 1)))
+         && NATNUMP (XCDR (wd)));
+}
+
 DEFUN ("inotify-rm-watch", Finotify_rm_watch, Sinotify_rm_watch, 1, 1, 0,
        doc: /* Remove an existing WATCH-DESCRIPTOR.
 
@@ -433,9 +465,7 @@ See inotify_rm_watch(2) for more information.  */)
 
   Lisp_Object descriptor, id;
 
-  if (! (CONSP (watch_descriptor)
-         && INTEGERP (XCAR (watch_descriptor))
-         && INTEGERP (XCDR (watch_descriptor))))
+  if (! valid_watch_descriptor (watch_descriptor))
     report_file_notify_error ("Invalid descriptor ", watch_descriptor);
 
   descriptor = XCAR (watch_descriptor);
@@ -456,16 +486,12 @@ reason.  Removing the watch by calling `inotify-rm-watch' 
also makes
 it invalid.  */)
      (Lisp_Object watch_descriptor)
 {
-  Lisp_Object elt, watch;
-
-  if (! (CONSP (watch_descriptor)
-         && INTEGERP (XCAR (watch_descriptor))
-         && INTEGERP (XCDR (watch_descriptor))))
+  if (! valid_watch_descriptor (watch_descriptor))
     return Qnil;
-
-  elt = Fassoc (XCAR (watch_descriptor), watch_list);
-  watch = Fassoc (XCDR (watch_descriptor), XCDR (elt));
-
+  Lisp_Object tail = assoc_no_quit (XCAR (watch_descriptor), watch_list);
+  if (NILP (tail))
+    return Qnil;
+  Lisp_Object watch = assq_no_quit (XCDR (watch_descriptor), XCDR (tail));
   return ! NILP (watch) ? Qt : Qnil;
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index 4b9cd3c..3125bd2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3376,6 +3376,7 @@ extern Lisp_Object merge (Lisp_Object, Lisp_Object, 
Lisp_Object);
 extern Lisp_Object do_yes_or_no_p (Lisp_Object);
 extern Lisp_Object concat2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object concat3 (Lisp_Object, Lisp_Object, Lisp_Object);
+extern bool equal_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object nconc2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object assq_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object assoc_no_quit (Lisp_Object, Lisp_Object);
diff --git a/test/src/inotify-tests.el b/test/src/inotify-tests.el
index f30aecc..987e1fc 100644
--- a/test/src/inotify-tests.el
+++ b/test/src/inotify-tests.el
@@ -28,6 +28,13 @@
 (declare-function inotify-add-watch "inotify.c" (file-name aspect callback))
 (declare-function inotify-rm-watch "inotify.c" (watch-descriptor))
 
+(ert-deftest inotify-valid-p-simple ()
+  "Simple tests for `inotify-valid-p'."
+  (skip-unless (featurep 'inotify))
+  (should-not (inotify-valid-p 0))
+  (should-not (inotify-valid-p nil))
+  (should-not (inotify-valid-p '(0 . 0))))
+
 ;; (ert-deftest filewatch-file-watch-aspects-check ()
 ;;   "Test whether `file-watch' properly checks the aspects."
 ;;   (let ((temp-file (make-temp-file "filewatch-aspects")))
@@ -56,7 +63,9 @@
              (insert "Foo\n"))
            (read-event nil nil 5)
            (should (> events 0)))
+       (should (inotify-valid-p wd))
        (inotify-rm-watch wd)
+       (should-not (inotify-valid-p wd))
        (delete-file temp-file)))))
 
 (provide 'inotify-tests)