From 612c28b89fd39a7348829e0af91de2484406b651 Mon Sep 17 00:00:00 2001 From: Andy Fiddaman Date: Mon, 11 Apr 2022 16:25:59 +0100 Subject: [PATCH] sunos: fs-event callback can be called after uv_close() (#3542) On illumos and Solaris, fs events are implemented with PORT_SOURCE_FILE type event ports. These are one-shot so need re-arming each time they fire. Once they are armed and an event occurs, the kernel removes them from the current cache list and puts them on an event queue to be read by the application. There's a window in closing one of these ports when it could have triggered and be pending delivery. In that case, the attempt to disarm (dissociate) the event will fail with ENOENT but libuv still goes ahead and closes down the handle. In particular, the close callback (uv_close() argument) will be called but then the event will subsequently be delivered if the loop is still active; this should not happen. --- src/unix/core.c | 9 +++++++++ src/unix/sunos.c | 36 +++++++++++++++++++++++++++++------- test/test-fs-event.c | 38 ++++++++++++++++++++++++++++++++++++++ test/test-list.h | 2 ++ 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/unix/core.c b/src/unix/core.c index ae160f0c..d5758de4 100644 --- a/src/unix/core.c +++ b/src/unix/core.c @@ -160,6 +160,15 @@ void uv_close(uv_handle_t* handle, uv_close_cb close_cb) { case UV_FS_EVENT: uv__fs_event_close((uv_fs_event_t*)handle); +#if defined(__sun) + /* + * On Solaris and illumos, we will not be able to dissociate the watcher + * for an event which is pending delivery, so we cannot always call + * uv__make_close_pending() straight away. The backend will call the + * function once the event has cleared. + */ + return; +#endif break; case UV_POLL: diff --git a/src/unix/sunos.c b/src/unix/sunos.c index 2bf297e5..7835bed7 100644 --- a/src/unix/sunos.c +++ b/src/unix/sunos.c @@ -154,7 +154,6 @@ void uv__io_poll(uv_loop_t* loop, int timeout) { sigset_t set; uint64_t base; uint64_t diff; - uint64_t idle_poll; unsigned int nfds; unsigned int i; int saved_errno; @@ -424,7 +423,7 @@ void uv_loadavg(double avg[3]) { #if defined(PORT_SOURCE_FILE) static int uv__fs_event_rearm(uv_fs_event_t *handle) { - if (handle->fd == -1) + if (handle->fd == PORT_DELETED) return UV_EBADF; if (port_associate(handle->loop->fs_fd, @@ -475,6 +474,12 @@ static void uv__fs_event_read(uv_loop_t* loop, handle = (uv_fs_event_t*) pe.portev_user; assert((r == 0) && "unexpected port_get() error"); + if (uv__is_closing(handle)) { + uv__handle_stop(handle); + uv__make_close_pending((uv_handle_t*) handle); + break; + } + events = 0; if (pe.portev_events & (FILE_ATTRIB | FILE_MODIFIED)) events |= UV_CHANGE; @@ -542,12 +547,14 @@ int uv_fs_event_start(uv_fs_event_t* handle, } -int uv_fs_event_stop(uv_fs_event_t* handle) { +static int uv__fs_event_stop(uv_fs_event_t* handle) { + int ret = 0; + if (!uv__is_active(handle)) return 0; - if (handle->fd == PORT_FIRED || handle->fd == PORT_LOADED) { - port_dissociate(handle->loop->fs_fd, + if (handle->fd == PORT_LOADED) { + ret = port_dissociate(handle->loop->fs_fd, PORT_SOURCE_FILE, (uintptr_t) &handle->fo); } @@ -556,13 +563,28 @@ int uv_fs_event_stop(uv_fs_event_t* handle) { uv__free(handle->path); handle->path = NULL; handle->fo.fo_name = NULL; - uv__handle_stop(handle); + if (ret == 0) + uv__handle_stop(handle); + return ret; +} + +int uv_fs_event_stop(uv_fs_event_t* handle) { + (void) uv__fs_event_stop(handle); return 0; } void uv__fs_event_close(uv_fs_event_t* handle) { - uv_fs_event_stop(handle); + /* + * If we were unable to dissociate the port here, then it is most likely + * that there is a pending queued event. When this happens, we don't want + * to complete the close as it will free the underlying memory for the + * handle, causing a use-after-free problem when the event is processed. + * We defer the final cleanup until after the event is consumed in + * uv__fs_event_read(). + */ + if (uv__fs_event_stop(handle) == 0) + uv__make_close_pending((uv_handle_t*) handle); } #else /* !defined(PORT_SOURCE_FILE) */ diff --git a/test/test-fs-event.c b/test/test-fs-event.c index 0e17e7df..a08bfb91 100644 --- a/test/test-fs-event.c +++ b/test/test-fs-event.c @@ -907,6 +907,44 @@ TEST_IMPL(fs_event_close_with_pending_event) { return 0; } +TEST_IMPL(fs_event_close_with_pending_delete_event) { +#if defined(NO_FS_EVENTS) + RETURN_SKIP(NO_FS_EVENTS); +#endif + uv_loop_t* loop; + int r; + + loop = uv_default_loop(); + + create_dir("watch_dir"); + create_file("watch_dir/file"); + + r = uv_fs_event_init(loop, &fs_event); + ASSERT(r == 0); + r = uv_fs_event_start(&fs_event, fs_event_fail, "watch_dir/file", 0); + ASSERT(r == 0); + + /* Generate an fs event. */ + remove("watch_dir/file"); + + /* Allow time for the remove event to propagate to the pending list. */ + /* XXX - perhaps just for __sun? */ + uv_sleep(1100); + uv_update_time(loop); + + uv_close((uv_handle_t*)&fs_event, close_cb); + + uv_run(loop, UV_RUN_DEFAULT); + + ASSERT(close_cb_called == 1); + + /* Clean up */ + remove("watch_dir/"); + + MAKE_VALGRIND_HAPPY(); + return 0; +} + TEST_IMPL(fs_event_close_in_callback) { #if defined(NO_FS_EVENTS) RETURN_SKIP(NO_FS_EVENTS); diff --git a/test/test-list.h b/test/test-list.h index 833047e8..2f023715 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -389,6 +389,7 @@ TEST_DECLARE (fs_event_no_callback_after_close) TEST_DECLARE (fs_event_no_callback_on_close) TEST_DECLARE (fs_event_immediate_close) TEST_DECLARE (fs_event_close_with_pending_event) +TEST_DECLARE (fs_event_close_with_pending_delete_event) TEST_DECLARE (fs_event_close_in_callback) TEST_DECLARE (fs_event_start_and_close) TEST_DECLARE (fs_event_error_reporting) @@ -1057,6 +1058,7 @@ TASK_LIST_START TEST_ENTRY (fs_event_no_callback_on_close) TEST_ENTRY (fs_event_immediate_close) TEST_ENTRY (fs_event_close_with_pending_event) + TEST_ENTRY (fs_event_close_with_pending_delete_event) TEST_ENTRY (fs_event_close_in_callback) TEST_ENTRY (fs_event_start_and_close) TEST_ENTRY_CUSTOM (fs_event_error_reporting, 0, 0, 60000)