linux: fix 'two watchers, one path' segfault

Problem: registering two uv_fs_event_t watchers for the same path, then closing
them, caused a segmentation fault. While active, the watchers didn't work right
either, only one would receive events.

Cause: each watcher has a wd (watch descriptor) that's used as its key in a
binary tree. When you call inotify_watch_add() twice with the same path, the
second call doesn't return a new wd - it returns the existing one. That in turn
resulted in the first handle getting ousted from the binary tree, leaving
dangling pointers.

This commit addresses that by storing the watchers in a queue and storing the
queue in the binary tree instead of storing the watchers directly in the tree.

Fixes joyent/node#3789.

include/uv-private/uv-unix.h

@@ -105,11 +105,8 @@ struct uv__io_s {
 
 #if __linux__
 # define UV_LOOP_PRIVATE_PLATFORM_FIELDS              \
-  /* RB_HEAD(uv__inotify_watchers, uv_fs_event_s) */  \
-  struct uv__inotify_watchers {                       \
-    struct uv_fs_event_s* rbh_root;                   \
-  } inotify_watchers;                                 \
   uv__io_t inotify_read_watcher;                      \
+  void* inotify_watchers;                             \
   int inotify_fd;
 #elif defined(PORT_SOURCE_FILE)
 # define UV_LOOP_PRIVATE_PLATFORM_FIELDS              \
@@ -277,15 +274,11 @@ struct uv__io_s {
 #if defined(__linux__)
 
 #define UV_FS_EVENT_PRIVATE_FIELDS    \
-  /* RB_ENTRY(fs_event_s) node; */    \
-  struct {                            \
-    struct uv_fs_event_s* rbe_left;   \
-    struct uv_fs_event_s* rbe_right;  \
-    struct uv_fs_event_s* rbe_parent; \
-    int rbe_color;                    \
-  } node;                             \
+  ngx_queue_t watchers;               \
   uv_fs_event_cb cb;                  \
-  int fd;                             \
+  int wd;                             \
+  void* pad0;                         \
+  void* pad1;                         \
 
 #elif defined(__APPLE__)  \
   || defined(__FreeBSD__) \

src/unix/linux/inotify.c

@@ -33,6 +33,18 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+struct watcher_list {
+  RB_ENTRY(watcher_list) entry;
+  ngx_queue_t watchers;
+  char* path;
+  int wd;
+};
+
+struct watcher_root {
+  struct watcher_list* rbh_root;
+};
+#define CAST(p) ((struct watcher_root*)(p))
+
 
 /* Don't look aghast, this is exactly how glibc's basename() works. */
 static char* basename_r(const char* path) {
@@ -41,14 +53,15 @@ static char* basename_r(const char* path) {
 }
 
 
-static int compare_watchers(const uv_fs_event_t* a, const uv_fs_event_t* b) {
-  if (a->fd < b->fd) return -1;
-  if (a->fd > b->fd) return 1;
+static int compare_watchers(const struct watcher_list* a,
+                            const struct watcher_list* b) {
+  if (a->wd < b->wd) return -1;
+  if (a->wd > b->wd) return 1;
   return 0;
 }
 
 
-RB_GENERATE_STATIC(uv__inotify_watchers, uv_fs_event_s, node, compare_watchers)
+RB_GENERATE_STATIC(watcher_root, watcher_list, entry, compare_watchers)
 
 
 static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int revents);
@@ -95,36 +108,27 @@ static int init_inotify(uv_loop_t* loop) {
 }
 
 
-static void add_watcher(uv_fs_event_t* handle) {
-  RB_INSERT(uv__inotify_watchers, &handle->loop->inotify_watchers, handle);
+static struct watcher_list* find_watcher(uv_loop_t* loop, int wd) {
+  struct watcher_list w;
+  w.wd = wd;
+  return RB_FIND(watcher_root, CAST(&loop->inotify_watchers), &w);
 }
 
 
-static uv_fs_event_t* find_watcher(uv_loop_t* loop, int wd) {
-  uv_fs_event_t handle;
-  handle.fd = wd;
-  return RB_FIND(uv__inotify_watchers, &loop->inotify_watchers, &handle);
-}
-
-
-static void remove_watcher(uv_fs_event_t* handle) {
-  RB_REMOVE(uv__inotify_watchers, &handle->loop->inotify_watchers, handle);
-}
-
-
-static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) {
+static void uv__inotify_read(uv_loop_t* loop, uv__io_t* dummy, int events) {
   const struct uv__inotify_event* e;
-  uv_fs_event_t* handle;
-  const char* filename;
+  struct watcher_list* w;
+  uv_fs_event_t* h;
+  ngx_queue_t* q;
+  const char* path;
   ssize_t size;
   const char *p;
   /* needs to be large enough for sizeof(inotify_event) + strlen(filename) */
   char buf[4096];
 
   while (1) {
-    do {
-      size = read(loop->inotify_fd, buf, sizeof buf);
-    }
+    do
+      size = read(loop->inotify_fd, buf, sizeof(buf));
     while (size == -1 && errno == EINTR);
 
     if (size == -1) {
@@ -144,17 +148,20 @@ static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) {
       if (e->mask & ~(UV__IN_ATTRIB|UV__IN_MODIFY))
         events |= UV_RENAME;
 
-      handle = find_watcher(loop, e->wd);
-      if (handle == NULL)
-        continue; /* Handle has already been closed. */
+      w = find_watcher(loop, e->wd);
+      if (w == NULL)
+        continue; /* Stale event, no watchers left. */
 
       /* inotify does not return the filename when monitoring a single file
        * for modifications. Repurpose the filename for API compatibility.
        * I'm not convinced this is a good thing, maybe it should go.
        */
-      filename = e->len ? (const char*) (e + 1) : basename_r(handle->filename);
+      path = e->len ? (const char*) (e + 1) : basename_r(w->path);
 
-      handle->cb(handle, filename, events, 0);
+      ngx_queue_foreach(q, &w->watchers) {
+        h = ngx_queue_data(q, uv_fs_event_t, watchers);
+        h->cb(h, path, events, 0);
+      }
     }
   }
 }
@@ -162,9 +169,10 @@ static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) {
 
 int uv_fs_event_init(uv_loop_t* loop,
                      uv_fs_event_t* handle,
-                     const char* filename,
+                     const char* path,
                      uv_fs_event_cb cb,
                      int flags) {
+  struct watcher_list* w;
   int events;
   int wd;
 
@@ -184,26 +192,50 @@ int uv_fs_event_init(uv_loop_t* loop,
          | UV__IN_MOVED_FROM
          | UV__IN_MOVED_TO;
 
-  wd = uv__inotify_add_watch(loop->inotify_fd, filename, events);
-  if (wd == -1) return uv__set_sys_error(loop, errno);
+  wd = uv__inotify_add_watch(loop->inotify_fd, path, events);
+  if (wd == -1)
+    return uv__set_sys_error(loop, errno);
 
+  w = find_watcher(loop, wd);
+  if (w)
+    goto no_insert;
+
+  w = malloc(sizeof(*w) + strlen(path) + 1);
+  if (w == NULL)
+    return uv__set_sys_error(loop, ENOMEM);
+
+  w->wd = wd;
+  w->path = strcpy((char*)(w + 1), path);
+  ngx_queue_init(&w->watchers);
+  RB_INSERT(watcher_root, CAST(&loop->inotify_watchers), w);
+
+no_insert:
   uv__handle_init(loop, (uv_handle_t*)handle, UV_FS_EVENT);
   uv__handle_start(handle); /* FIXME shouldn't start automatically */
-  handle->filename = strdup(filename);
+  ngx_queue_insert_tail(&w->watchers, &handle->watchers);
+  handle->filename = w->path;
   handle->cb = cb;
-  handle->fd = wd;
-  add_watcher(handle);
+  handle->wd = wd;
 
   return 0;
 }
 
 
 void uv__fs_event_close(uv_fs_event_t* handle) {
-  uv__inotify_rm_watch(handle->loop->inotify_fd, handle->fd);
-  remove_watcher(handle);
-  handle->fd = -1;
+  struct watcher_list* w;
 
-  free(handle->filename);
+  w = find_watcher(handle->loop, handle->wd);
+  assert(w != NULL);
+
+  handle->wd = -1;
   handle->filename = NULL;
   uv__handle_stop(handle);
+  ngx_queue_remove(&handle->watchers);
+
+  if (ngx_queue_empty(&w->watchers)) {
+    /* No watchers left for this path. Clean up. */
+    RB_REMOVE(watcher_root, CAST(&handle->loop->inotify_watchers), w);
+    uv__inotify_rm_watch(handle->loop->inotify_fd, w->wd);
+    free(w);
+  }
 }

src/unix/loop.c

@@ -54,7 +54,7 @@ int uv__loop_init(uv_loop_t* loop, int default_loop) {
   eio_channel_init(&loop->uv_eio_channel, loop);
 
 #if __linux__
-  RB_INIT(&loop->inotify_watchers);
+  loop->inotify_watchers = NULL;
   loop->inotify_fd = -1;
 #endif
 #if HAVE_PORTS_FS