Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
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 nodejs/node-v0.x-archive#3789.
  • Loading branch information
bnoordhuis committed Jul 28, 2012
1 parent ec76a42 commit 4fe1916
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 52 deletions.
17 changes: 5 additions & 12 deletions include/uv-private/uv-unix.h
Expand Up @@ -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 \
Expand Down Expand Up @@ -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__) \
Expand Down
110 changes: 71 additions & 39 deletions src/unix/linux/inotify.c
Expand Up @@ -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) {
Expand All @@ -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);
Expand Down Expand Up @@ -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 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 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 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) {
Expand All @@ -144,27 +148,31 @@ 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);
}
}
}
}


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;

Expand All @@ -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;

w = find_watcher(handle->loop, handle->wd);
assert(w != NULL);

free(handle->filename);
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);
}
}
2 changes: 1 addition & 1 deletion src/unix/loop.c
Expand Up @@ -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
Expand Down

0 comments on commit 4fe1916

Please sign in to comment.