signal: flag to disable signal control handler

Adds flags that marks uv__signal_control_handler as disabled instead
of removing it when uv__signal_control_handler_refs falls to zero.
Trying to remove the controller from the controller handle itself
leads to deadlock.

Ref: nodejs/node#10165

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);
 

test/test-list.h

@@ -345,6 +345,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)
@@ -694,6 +695,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)

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 <errno.h>
 #include <signal.h>
 #include <stdarg.h>