From 442b8a5a848e1589520a4d4fd175d7e9aa084c44 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 8 Oct 2015 17:03:04 +0200 Subject: [PATCH] unix: use QUEUE_MOVE when iterating over lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace uses of QUEUE_FOREACH when the list can get modified while iterating over it, in particular when a callback is made into the user's code. This should fix a number of spurious failures that people have been reporting. PR-URL: https://github.com/libuv/libuv/pull/565 Reviewed-By: Fedor Indutny Reviewed-By: Saúl Ibarra Corretgé --- src/queue.h | 3 +++ src/unix/async.c | 8 +++++++- src/unix/linux-inotify.c | 9 ++++++++- src/unix/loop-watcher.c | 7 ++++++- src/unix/signal.c | 2 ++ src/uv-common.c | 9 ++++++++- 6 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/queue.h b/src/queue.h index f942e13f..ff3540a0 100644 --- a/src/queue.h +++ b/src/queue.h @@ -30,6 +30,9 @@ typedef void *QUEUE[2]; #define QUEUE_DATA(ptr, type, field) \ ((type *) ((char *) (ptr) - offsetof(type, field))) +/* Important note: mutating the list while QUEUE_FOREACH is + * iterating over its elements results in undefined behavior. + */ #define QUEUE_FOREACH(q, h) \ for ((q) = QUEUE_NEXT(h); (q) != (h); (q) = QUEUE_NEXT(q)) diff --git a/src/unix/async.c b/src/unix/async.c index 9ff24aeb..184b5981 100644 --- a/src/unix/async.c +++ b/src/unix/async.c @@ -78,12 +78,18 @@ void uv__async_close(uv_async_t* handle) { static void uv__async_event(uv_loop_t* loop, struct uv__async* w, unsigned int nevents) { + QUEUE queue; QUEUE* q; uv_async_t* h; - QUEUE_FOREACH(q, &loop->async_handles) { + QUEUE_MOVE(&loop->async_handles, &queue); + while (!QUEUE_EMPTY(&queue)) { + q = QUEUE_HEAD(&queue); h = QUEUE_DATA(q, uv_async_t, queue); + QUEUE_REMOVE(q); + QUEUE_INSERT_TAIL(&loop->async_handles, q); + if (cmpxchgi(&h->pending, 1, 0) == 0) continue; diff --git a/src/unix/linux-inotify.c b/src/unix/linux-inotify.c index d9ed9f4b..e3ff6d52 100644 --- a/src/unix/linux-inotify.c +++ b/src/unix/linux-inotify.c @@ -120,6 +120,7 @@ static void uv__inotify_read(uv_loop_t* loop, const struct uv__inotify_event* e; struct watcher_list* w; uv_fs_event_t* h; + QUEUE queue; QUEUE* q; const char* path; ssize_t size; @@ -159,8 +160,14 @@ static void uv__inotify_read(uv_loop_t* loop, */ path = e->len ? (const char*) (e + 1) : uv__basename_r(w->path); - QUEUE_FOREACH(q, &w->watchers) { + QUEUE_MOVE(&w->watchers, &queue); + while (!QUEUE_EMPTY(&queue)) { + q = QUEUE_HEAD(&queue); h = QUEUE_DATA(q, uv_fs_event_t, watchers); + + QUEUE_REMOVE(q); + QUEUE_INSERT_TAIL(&w->watchers, q); + h->cb(h, path, events, 0); } } diff --git a/src/unix/loop-watcher.c b/src/unix/loop-watcher.c index dc25760b..340bb0df 100644 --- a/src/unix/loop-watcher.c +++ b/src/unix/loop-watcher.c @@ -47,9 +47,14 @@ \ void uv__run_##name(uv_loop_t* loop) { \ uv_##name##_t* h; \ + QUEUE queue; \ QUEUE* q; \ - QUEUE_FOREACH(q, &loop->name##_handles) { \ + QUEUE_MOVE(&loop->name##_handles, &queue); \ + while (!QUEUE_EMPTY(&queue)) { \ + q = QUEUE_HEAD(&queue); \ h = QUEUE_DATA(q, uv_##name##_t, queue); \ + QUEUE_REMOVE(q); \ + QUEUE_INSERT_TAIL(&loop->name##_handles, q); \ h->name##_cb(h); \ } \ } \ diff --git a/src/unix/signal.c b/src/unix/signal.c index 0b7a405c..edd9085d 100644 --- a/src/unix/signal.c +++ b/src/unix/signal.c @@ -234,6 +234,8 @@ void uv__signal_loop_cleanup(uv_loop_t* loop) { /* Stop all the signal watchers that are still attached to this loop. This * ensures that the (shared) signal tree doesn't contain any invalid entries * entries, and that signal handlers are removed when appropriate. + * It's safe to use QUEUE_FOREACH here because the handles and the handle + * queue are not modified by uv__signal_stop(). */ QUEUE_FOREACH(q, &loop->handle_queue) { uv_handle_t* handle = QUEUE_DATA(q, uv_handle_t, handle_queue); diff --git a/src/uv-common.c b/src/uv-common.c index 8ce3d1f4..539756c8 100644 --- a/src/uv-common.c +++ b/src/uv-common.c @@ -337,11 +337,18 @@ int uv_udp_recv_stop(uv_udp_t* handle) { void uv_walk(uv_loop_t* loop, uv_walk_cb walk_cb, void* arg) { + QUEUE queue; QUEUE* q; uv_handle_t* h; - QUEUE_FOREACH(q, &loop->handle_queue) { + QUEUE_MOVE(&loop->handle_queue, &queue); + while (!QUEUE_EMPTY(&queue)) { + q = QUEUE_HEAD(&queue); h = QUEUE_DATA(q, uv_handle_t, handle_queue); + + QUEUE_REMOVE(q); + QUEUE_INSERT_TAIL(&loop->handle_queue, q); + if (h->flags & UV__HANDLE_INTERNAL) continue; walk_cb(h, arg); }