Keep inotify directory baseline when a snapshot fails

A directory watch snapshots the child-name set on each change and diffs it
against the previous baseline to recover the name kqueue omits. dir_snapshot
returned an empty list on any failure (opendir error, a mid-read readdir
error, or an allocation failure), which the diff could not tell apart from a
genuinely empty directory.

On a transient failure the IN_DELETE pass then saw every known child as
missing and queued a spurious IN_DELETE for each, and the baseline was
overwritten with the empty list, so every later change re-reported the whole
directory as IN_CREATE. The corruption was permanent.

Make dir_snapshot return bool: on failure it frees any partial result and
reports false, and readdir read errors are now detected by clearing errno
before each call. process_vnode_event only diffs against and advances to a
snapshot that succeeded; on failure it keeps the previous baseline so the next
successful snapshot reconciles whatever changed in between. The watch-add path
documents that a failed initial snapshot is a best-effort empty baseline.

Validated with make check on Apple Silicon; test-inotify still passes 6/6.

Author: chenhunghan

src/syscall/inotify.c

@@ -303,51 +303,77 @@ static void pipe_drain(inotify_instance_t *inst)
         ;
 }
 
-/* Snapshot the entry names of a directory (excluding "." and ".."). On return
- * *out is a malloc'd array of malloc'd strings with *n_out entries (free with
- * free_dir_snapshot). On any failure the snapshot is left empty.
+static void free_dir_snapshot(char **entries, int n)
+{
+    if (!entries)
+        return;
+    for (int i = 0; i < n; i++)
+        free(entries[i]);
+    free(entries);
+}
+
+/* Snapshot the entry names of a directory (excluding "." and ".."). On success
+ * *out is a malloc'd array of *n_out malloc'd strings (free with
+ * free_dir_snapshot) and the function returns true. On any failure -- the
+ * directory could not be opened, a readdir read error occurred, or an
+ * allocation failed -- it frees any partial result, leaves *out NULL / *n_out
+ * 0, and returns false. Callers must distinguish a failed snapshot from a
+ * genuinely empty directory: treating a failure as "empty" would diff every
+ * known child as deleted and wipe the baseline.
  */
-static void dir_snapshot(const char *path, char ***out, int *n_out)
+static bool dir_snapshot(const char *path, char ***out, int *n_out)
 {
     *out = NULL;
     *n_out = 0;
 
     DIR *d = opendir(path);
     if (!d)
-        return;
+        return false;
 
     char **names = NULL;
     int n = 0, cap = 0;
-    struct dirent *de;
-    while ((de = readdir(d)) != NULL) {
+    bool ok = true;
+    for (;;) {
+        /* readdir returns NULL both at end-of-stream and on error; reset
+         * errno immediately before each call so a non-zero errno afterwards
+         * unambiguously signals a read error rather than EOF.
+         */
+        errno = 0;
+        struct dirent *de = readdir(d);
+        if (!de) {
+            if (errno != 0)
+                ok = false;
+            break;
+        }
         if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
             continue;
         if (n == cap) {
             int ncap = cap ? cap * 2 : 16;
             char **tmp = realloc(names, (size_t) ncap * sizeof(char *));
-            if (!tmp)
+            if (!tmp) {
+                ok = false;
                 break;
+            }
             names = tmp;
             cap = ncap;
         }
         names[n] = strdup(de->d_name);
-        if (!names[n])
+        if (!names[n]) {
+            ok = false;
             break;
+        }
         n++;
     }
     closedir(d);
 
+    if (!ok) {
+        free_dir_snapshot(names, n);
+        return false;
+    }
+
     *out = names;
     *n_out = n;
-}
-
-static void free_dir_snapshot(char **entries, int n)
-{
-    if (!entries)
-        return;
-    for (int i = 0; i < n; i++)
-        free(entries[i]);
-    free(entries);
+    return true;
 }
 
 static bool snapshot_contains(char *const *entries, int n, const char *name)
@@ -379,33 +405,42 @@ static int process_vnode_event(inotify_instance_t *inst,
     if (w->is_dir && (fflags & NOTE_WRITE) && w->path) {
         char **now = NULL;
         int now_n = 0;
-        dir_snapshot(w->path, &now, &now_n);
-
-        for (int j = 0; j < now_n && !overflow; j++) {
-            if ((w->mask & IN_CREATE) &&
-                !snapshot_contains(w->entries, w->n_entries, now[j])) {
-                if (queue_event(inst, w->wd, IN_CREATE, 0, now[j]) < 0)
-                    overflow = true;
-                else
-                    queued++;
+        /* Only diff against -- and advance to -- a snapshot that succeeded.
+         * On a transient opendir/readdir failure dir_snapshot returns false
+         * with an empty result; treating that as the live directory state
+         * would emit a spurious IN_DELETE for every known child and leave the
+         * baseline empty, so every later change would re-report the whole tree
+         * as IN_CREATE. Keeping the previous baseline instead lets the next
+         * successful snapshot reconcile whatever was missed this round.
+         */
+        if (dir_snapshot(w->path, &now, &now_n)) {
+            for (int j = 0; j < now_n && !overflow; j++) {
+                if ((w->mask & IN_CREATE) &&
+                    !snapshot_contains(w->entries, w->n_entries, now[j])) {
+                    if (queue_event(inst, w->wd, IN_CREATE, 0, now[j]) < 0)
+                        overflow = true;
+                    else
+                        queued++;
+                }
             }
-        }
-        for (int j = 0; j < w->n_entries && !overflow; j++) {
-            if ((w->mask & IN_DELETE) &&
-                !snapshot_contains(now, now_n, w->entries[j])) {
-                if (queue_event(inst, w->wd, IN_DELETE, 0, w->entries[j]) < 0)
-                    overflow = true;
-                else
-                    queued++;
+            for (int j = 0; j < w->n_entries && !overflow; j++) {
+                if ((w->mask & IN_DELETE) &&
+                    !snapshot_contains(now, now_n, w->entries[j])) {
+                    if (queue_event(inst, w->wd, IN_DELETE, 0, w->entries[j]) <
+                        0)
+                        overflow = true;
+                    else
+                        queued++;
+                }
             }
-        }
 
-        /* Advance the snapshot regardless: the directory state has moved on,
-         * and any names dropped under overflow are covered by IN_Q_OVERFLOW.
-         */
-        free_dir_snapshot(w->entries, w->n_entries);
-        w->entries = now;
-        w->n_entries = now_n;
+            /* Advance the snapshot: the directory state has moved on, and any
+             * names dropped under overflow are covered by IN_Q_OVERFLOW.
+             */
+            free_dir_snapshot(w->entries, w->n_entries);
+            w->entries = now;
+            w->n_entries = now_n;
+        }
     }
 
     if (!overflow) {
@@ -564,7 +599,12 @@ int64_t sys_inotify_add_watch(guest_t *g,
     int wn = 0;
     if (is_dir) {
         wpath = strdup(path);
-        dir_snapshot(path, &wentries, &wn);
+        /* A failed initial snapshot leaves an empty baseline -- the best we
+         * can do at watch-add time -- so the first change may report existing
+         * children as IN_CREATE once. That self-corrects and never wipes a
+         * populated baseline, which is the case the runtime diff guards.
+         */
+        (void) dir_snapshot(path, &wentries, &wn);
     }
 
     pthread_mutex_lock(&inotify_lock);