From 68d70c1634132423da8070c06a3024738c904f83 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 30 Mar 2017 11:08:23 -0700 Subject: [PATCH] Some inotify cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 . */ 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 . */ #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) -- 2.9.3