From c66f265b5d700bdd922316788545c0fb7956db1d Mon Sep 17 00:00:00 2001 From: Bartosz Sosnowski Date: Mon, 12 Dec 2016 11:34:42 +0100 Subject: [PATCH] win,signal: fix potential deadlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trying to remove the controller from the controller handle itself leads to deadlock. Fix it by always having the global signal handler engaged. Ref: https://github.com/nodejs/node/issues/10165 PR-URL: https://github.com/libuv/libuv/pull/1168 Reviewed-By: Saúl Ibarra Corretgé --- src/win/signal.c | 118 ++------------------------------------------- test/test-list.h | 2 + test/test-signal.c | 35 ++++++++++++-- 3 files changed, 38 insertions(+), 117 deletions(-) diff --git a/src/win/signal.c b/src/win/signal.c index 2c64a55d..af7974c3 100644 --- a/src/win/signal.c +++ b/src/win/signal.c @@ -30,12 +30,14 @@ RB_HEAD(uv_signal_tree_s, uv_signal_s); static struct uv_signal_tree_s uv__signal_tree = RB_INITIALIZER(uv__signal_tree); -static ssize_t volatile uv__signal_control_handler_refs = 0; static CRITICAL_SECTION uv__signal_lock; +static BOOL WINAPI uv__signal_control_handler(DWORD type); void uv_signals_init() { InitializeCriticalSection(&uv__signal_lock); + if (!SetConsoleCtrlHandler(uv__signal_control_handler, TRUE)) + abort(); } @@ -125,102 +127,6 @@ static BOOL WINAPI uv__signal_control_handler(DWORD type) { } -static int uv__signal_register_control_handler() { - /* When this function is called, the uv__signal_lock must be held. */ - - /* If the console control handler has already been hooked, just add a */ - /* reference. */ - if (uv__signal_control_handler_refs > 0) { - uv__signal_control_handler_refs++; - return 0; - } - - if (!SetConsoleCtrlHandler(uv__signal_control_handler, TRUE)) - return GetLastError(); - - uv__signal_control_handler_refs++; - - return 0; -} - - -static void uv__signal_unregister_control_handler() { - /* When this function is called, the uv__signal_lock must be held. */ - BOOL r; - - /* Don't unregister if the number of console control handlers exceeds one. */ - /* Just remove a reference in that case. */ - if (uv__signal_control_handler_refs > 1) { - uv__signal_control_handler_refs--; - return; - } - - assert(uv__signal_control_handler_refs == 1); - - r = SetConsoleCtrlHandler(uv__signal_control_handler, FALSE); - /* This should never fail; if it does it is probably a bug in libuv. */ - assert(r); - - uv__signal_control_handler_refs--; -} - - -static int uv__signal_register(int signum) { - switch (signum) { - case SIGINT: - case SIGBREAK: - case SIGHUP: - return uv__signal_register_control_handler(); - - case SIGWINCH: - /* SIGWINCH is generated in tty.c. No need to register anything. */ - return 0; - - case SIGILL: - case SIGABRT_COMPAT: - case SIGFPE: - case SIGSEGV: - case SIGTERM: - case SIGABRT: - /* Signal is never raised. */ - return 0; - - default: - /* Invalid signal. */ - return ERROR_INVALID_PARAMETER; - } -} - - -static void uv__signal_unregister(int signum) { - switch (signum) { - case SIGINT: - case SIGBREAK: - case SIGHUP: - uv__signal_unregister_control_handler(); - return; - - case SIGWINCH: - /* SIGWINCH is generated in tty.c. No need to unregister anything. */ - return; - - case SIGILL: - case SIGABRT_COMPAT: - case SIGFPE: - case SIGSEGV: - case SIGTERM: - case SIGABRT: - /* Nothing is registered for this signal. */ - return; - - default: - /* Libuv bug. */ - assert(0 && "Invalid signum"); - return; - } -} - - int uv_signal_init(uv_loop_t* loop, uv_signal_t* handle) { uv_req_t* req; @@ -247,8 +153,6 @@ int uv_signal_stop(uv_signal_t* handle) { EnterCriticalSection(&uv__signal_lock); - uv__signal_unregister(handle->signum); - removed_handle = RB_REMOVE(uv_signal_tree_s, &uv__signal_tree, handle); assert(removed_handle == handle); @@ -262,14 +166,9 @@ int uv_signal_stop(uv_signal_t* handle) { int uv_signal_start(uv_signal_t* handle, uv_signal_cb signal_cb, int signum) { - int err; - - /* If the user supplies signum == 0, then return an error already. If the */ - /* signum is otherwise invalid then uv__signal_register will find out */ - /* eventually. */ - if (signum == 0) { + /* Test for invalid signal values. */ + if (signum != SIGWINCH && (signum <= 0 || signum >= NSIG)) return UV_EINVAL; - } /* Short circuit: if the signal watcher is already watching {signum} don't */ /* go through the process of deregistering and registering the handler. */ @@ -289,13 +188,6 @@ int uv_signal_start(uv_signal_t* handle, uv_signal_cb signal_cb, int signum) { EnterCriticalSection(&uv__signal_lock); - err = uv__signal_register(signum); - if (err) { - /* Uh-oh, didn't work. */ - LeaveCriticalSection(&uv__signal_lock); - return uv_translate_sys_error(err); - } - handle->signum = signum; RB_INSERT(uv_signal_tree_s, &uv__signal_tree, handle); diff --git a/test/test-list.h b/test/test-list.h index d2fc022b..3a1e82a9 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -346,6 +346,7 @@ TEST_DECLARE (listen_no_simultaneous_accepts) TEST_DECLARE (fs_stat_root) TEST_DECLARE (spawn_with_an_odd_path) TEST_DECLARE (ipc_listen_after_bind_twice) +TEST_DECLARE (win32_signum_number) #else TEST_DECLARE (emfile) TEST_DECLARE (close_fd) @@ -695,6 +696,7 @@ TASK_LIST_START TEST_ENTRY (fs_stat_root) TEST_ENTRY (spawn_with_an_odd_path) TEST_ENTRY (ipc_listen_after_bind_twice) + TEST_ENTRY (win32_signum_number) #else TEST_ENTRY (emfile) TEST_ENTRY (close_fd) diff --git a/test/test-signal.c b/test/test-signal.c index fcdd8e4d..c0424c60 100644 --- a/test/test-signal.c +++ b/test/test-signal.c @@ -19,13 +19,40 @@ * IN THE SOFTWARE. */ - -/* This test does not pretend to be cross-platform. */ -#ifndef _WIN32 - #include "uv.h" #include "task.h" +/* For Windows we test only signum handling */ +#ifdef _WIN32 +static void signum_test_cb(uv_signal_t* handle, int signum) { + FATAL("signum_test_cb should not be called"); +} + +TEST_IMPL(win32_signum_number) { + uv_signal_t signal; + uv_loop_t* loop; + + loop = uv_default_loop(); + uv_signal_init(loop, &signal); + + ASSERT(uv_signal_start(&signal, signum_test_cb, 0) == UV_EINVAL); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGINT) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGBREAK) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGHUP) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGWINCH) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGILL) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGABRT_COMPAT) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGFPE) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGSEGV) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGTERM) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, SIGABRT) == 0); + ASSERT(uv_signal_start(&signal, signum_test_cb, -1) == UV_EINVAL); + ASSERT(uv_signal_start(&signal, signum_test_cb, NSIG) == UV_EINVAL); + ASSERT(uv_signal_start(&signal, signum_test_cb, 1024) == UV_EINVAL); + MAKE_VALGRIND_HAPPY(); + return 0; +} +#else #include #include #include