From 46343764d5aa4af0a2e939b6f0f1fad798af43ef Mon Sep 17 00:00:00 2001 From: Andrey Mazo Date: Wed, 18 Nov 2015 19:05:26 -0500 Subject: [PATCH] unix: fix uv_fs_event_stop() from fs_event_cb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The following changeset 442b8a5a848e1589520a4d4fd175d7e9aa084c44 "unix: use QUEUE_MOVE when iterating over lists" introduced a new assert failure: `queue_foreach_delete` failed: exit code 6 Output from process `queue_foreach_delete`: run-tests: src/unix/linux-inotify.c:244: uv_fs_event_stop: Assertion `w != ((void *)0)' failed. Simplest test case for this: 1. create and start two uv_fs_event_t for the same path; 2. in the callback for the first one, call uv_close() on it; 3. assert/segfault while accessing the second uv_fs_event_t from uv__inotify_read(). PR-URL: https://github.com/libuv/libuv/pull/621 Reviewed-By: Saúl Ibarra Corretgé --- src/unix/linux-inotify.c | 33 ++++++++++++++---- test/test-queue-foreach-delete.c | 57 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/unix/linux-inotify.c b/src/unix/linux-inotify.c index e3ff6d52..28291211 100644 --- a/src/unix/linux-inotify.c +++ b/src/unix/linux-inotify.c @@ -35,6 +35,7 @@ struct watcher_list { RB_ENTRY(watcher_list) entry; QUEUE watchers; + int iterating; char* path; int wd; }; @@ -113,6 +114,15 @@ static struct watcher_list* find_watcher(uv_loop_t* loop, int wd) { return RB_FIND(watcher_root, CAST(&loop->inotify_watchers), &w); } +static void maybe_free_watcher_list(struct watcher_list* w, uv_loop_t* loop) { + /* if the watcher_list->watchers is being iterated over, we can't free it. */ + if ((!w->iterating) && QUEUE_EMPTY(&w->watchers)) { + /* No watchers left for this path. Clean up. */ + RB_REMOVE(watcher_root, CAST(&loop->inotify_watchers), w); + uv__inotify_rm_watch(loop->inotify_fd, w->wd); + uv__free(w); + } +} static void uv__inotify_read(uv_loop_t* loop, uv__io_t* dummy, @@ -160,6 +170,18 @@ static void uv__inotify_read(uv_loop_t* loop, */ path = e->len ? (const char*) (e + 1) : uv__basename_r(w->path); + /* We're about to iterate over the queue and call user's callbacks. + * What can go wrong? + * A callback could call uv_fs_event_stop() + * and the queue can change under our feet. + * So, we use QUEUE_MOVE() trick to safely iterate over the queue. + * And we don't free the watcher_list until we're done iterating. + * + * First, + * tell uv_fs_event_stop() (that could be called from a user's callback) + * not to free watcher_list. + */ + w->iterating = 1; QUEUE_MOVE(&w->watchers, &queue); while (!QUEUE_EMPTY(&queue)) { q = QUEUE_HEAD(&queue); @@ -170,6 +192,9 @@ static void uv__inotify_read(uv_loop_t* loop, h->cb(h, path, events, 0); } + /* done iterating, time to (maybe) free empty watcher_list */ + w->iterating = 0; + maybe_free_watcher_list(w, loop); } } } @@ -221,6 +246,7 @@ int uv_fs_event_start(uv_fs_event_t* handle, w->wd = wd; w->path = strcpy((char*)(w + 1), path); QUEUE_INIT(&w->watchers); + w->iterating = 0; RB_INSERT(watcher_root, CAST(&handle->loop->inotify_watchers), w); no_insert: @@ -248,12 +274,7 @@ int uv_fs_event_stop(uv_fs_event_t* handle) { uv__handle_stop(handle); QUEUE_REMOVE(&handle->watchers); - if (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); - uv__free(w); - } + maybe_free_watcher_list(w, handle->loop); return 0; } diff --git a/test/test-queue-foreach-delete.c b/test/test-queue-foreach-delete.c index cc4ba013..45da2253 100644 --- a/test/test-queue-foreach-delete.c +++ b/test/test-queue-foreach-delete.c @@ -60,6 +60,9 @@ static const unsigned first_handle_number_idle = 2; static const unsigned first_handle_number_prepare = 2; static const unsigned first_handle_number_check = 2; +#ifdef __linux__ +static const unsigned first_handle_number_fs_event = 0; +#endif #define DEFINE_GLOBALS_AND_CBS(name) \ @@ -119,6 +122,45 @@ DEFINE_GLOBALS_AND_CBS(idle) DEFINE_GLOBALS_AND_CBS(prepare) DEFINE_GLOBALS_AND_CBS(check) +#ifdef __linux__ +DEFINE_GLOBALS_AND_CBS(fs_event) + +static const char watched_dir[] = "."; +static uv_timer_t timer; +static unsigned helper_timer_cb_calls; + + +static void init_and_start_fs_events(uv_loop_t* loop) { + size_t i; + for (i = 0; i < ARRAY_SIZE(fs_event); i++) { + int r; + r = uv_fs_event_init(loop, &fs_event[i]); + ASSERT(r == 0); + + r = uv_fs_event_start(&fs_event[i], + (uv_fs_event_cb)fs_event_cbs[i], + watched_dir, + 0); + ASSERT(r == 0); + } +} + +static void helper_timer_cb(uv_timer_t* thandle) { + int r; + uv_fs_t fs_req; + + /* fire all fs_events */ + r = uv_fs_utime(thandle->loop, &fs_req, watched_dir, 0, 0, NULL); + ASSERT(r == 0); + ASSERT(fs_req.result == 0); + ASSERT(fs_req.fs_type == UV_FS_UTIME); + ASSERT(strcmp(fs_req.path, watched_dir) == 0); + uv_fs_req_cleanup(&fs_req); + + helper_timer_cb_calls++; +} +#endif + TEST_IMPL(queue_foreach_delete) { uv_loop_t* loop; @@ -130,6 +172,17 @@ TEST_IMPL(queue_foreach_delete) { INIT_AND_START(prepare, loop); INIT_AND_START(check, loop); +#ifdef __linux__ + init_and_start_fs_events(loop); + + /* helper timer to trigger async and fs_event callbacks */ + r = uv_timer_init(loop, &timer); + ASSERT(r == 0); + + r = uv_timer_start(&timer, helper_timer_cb, 0, 0); + ASSERT(r == 0); +#endif + r = uv_run(loop, UV_RUN_NOWAIT); ASSERT(r == 1); @@ -137,6 +190,10 @@ TEST_IMPL(queue_foreach_delete) { END_ASSERTS(prepare); END_ASSERTS(check); +#ifdef __linux__ + ASSERT(helper_timer_cb_calls == 1); +#endif + MAKE_VALGRIND_HAPPY(); return 0;