From 70469dcaa65bc4608e896b4df1ad13fcf58ad984 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Tue, 10 Mar 2020 10:58:29 +0100
Subject: [PATCH] unix: fix signal handle closing deferral

The way libuv handled closing of `uv_signal_t` handles with pending
events introduced an infidelity where `uv_loop_alive()` returned false
while the signal handle was still in the closing-but-not-closed state.

Fixes: https://github.com/libuv/libuv/issues/2721
PR-URL: https://github.com/libuv/libuv/pull/2722
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
---
 src/unix/core.c                     | 19 ++++++++++++++++---
 src/unix/signal.c                   | 23 +----------------------
 test/test-list.h                    |  2 ++
 test/test-signal-pending-on-close.c | 27 ++++++++++++++++++++++++++-
 4 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/src/unix/core.c b/src/unix/core.c
index 1e50f7980..6d0063513 100644
--- a/src/unix/core.c
+++ b/src/unix/core.c
@@ -171,9 +171,7 @@ void uv_close(uv_handle_t* handle, uv_close_cb close_cb) {
 
   case UV_SIGNAL:
     uv__signal_close((uv_signal_t*) handle);
-    /* Signal handles may not be closed immediately. The signal code will
-     * itself close uv__make_close_pending whenever appropriate. */
-    return;
+    break;
 
   default:
     assert(0);
@@ -238,6 +236,8 @@ int uv__getiovmax(void) {
 
 
 static void uv__finish_close(uv_handle_t* handle) {
+  uv_signal_t* sh;
+
   /* Note: while the handle is in the UV_HANDLE_CLOSING state now, it's still
    * possible for it to be active in the sense that uv__is_active() returns
    * true.
@@ -260,7 +260,20 @@ static void uv__finish_close(uv_handle_t* handle) {
     case UV_FS_EVENT:
     case UV_FS_POLL:
     case UV_POLL:
+      break;
+
     case UV_SIGNAL:
+      /* If there are any caught signals "trapped" in the signal pipe,
+       * we can't call the close callback yet. Reinserting the handle
+       * into the closing queue makes the event loop spin but that's
+       * okay because we only need to deliver the pending events.
+       */
+      sh = (uv_signal_t*) handle;
+      if (sh->caught_signals > sh->dispatched_signals) {
+        handle->flags ^= UV_HANDLE_CLOSED;
+        uv__make_close_pending(handle);  /* Back into the queue. */
+        return;
+      }
       break;
 
     case UV_NAMED_PIPE:
diff --git a/src/unix/signal.c b/src/unix/signal.c
index ba8fcc204..1e7e8ac57 100644
--- a/src/unix/signal.c
+++ b/src/unix/signal.c
@@ -331,16 +331,7 @@ int uv_signal_init(uv_loop_t* loop, uv_signal_t* handle) {
 
 
 void uv__signal_close(uv_signal_t* handle) {
-
   uv__signal_stop(handle);
-
-  /* If there are any caught signals "trapped" in the signal pipe, we can't
-   * call the close callback yet. Otherwise, add the handle to the finish_close
-   * queue.
-   */
-  if (handle->caught_signals == handle->dispatched_signals) {
-    uv__make_close_pending((uv_handle_t*) handle);
-  }
 }
 
 
@@ -472,17 +463,6 @@ static void uv__signal_event(uv_loop_t* loop,
 
       if (handle->flags & UV_SIGNAL_ONE_SHOT)
         uv__signal_stop(handle);
-
-      /* If uv_close was called while there were caught signals that were not
-       * yet dispatched, the uv__finish_close was deferred. Make close pending
-       * now if this has happened.
-       */
-      if (handle->caught_signals == handle->dispatched_signals) {
-        if (handle->signum == 0)
-          uv__handle_stop(handle);
-        if (handle->flags & UV_HANDLE_CLOSING)
-          uv__make_close_pending((uv_handle_t*) handle);
-      }
     }
 
     bytes -= end;
@@ -572,6 +552,5 @@ static void uv__signal_stop(uv_signal_t* handle) {
   uv__signal_unlock_and_unblock(&saved_sigmask);
 
   handle->signum = 0;
-  if (handle->caught_signals == handle->dispatched_signals)
-    uv__handle_stop(handle);
+  uv__handle_stop(handle);
 }
diff --git a/test/test-list.h b/test/test-list.h
index 482fcc654..003b8d265 100644
--- a/test/test-list.h
+++ b/test/test-list.h
@@ -471,6 +471,7 @@ TEST_DECLARE   (we_get_signal_one_shot)
 TEST_DECLARE   (we_get_signals_mixed)
 TEST_DECLARE   (signal_multiple_loops)
 TEST_DECLARE   (signal_pending_on_close)
+TEST_DECLARE   (signal_close_loop_alive)
 TEST_DECLARE   (closed_fd_events)
 #endif
 #ifdef __APPLE__
@@ -937,6 +938,7 @@ TASK_LIST_START
   TEST_ENTRY  (we_get_signals_mixed)
   TEST_ENTRY  (signal_multiple_loops)
   TEST_ENTRY  (signal_pending_on_close)
+  TEST_ENTRY  (signal_close_loop_alive)
   TEST_ENTRY  (closed_fd_events)
 #endif
 
diff --git a/test/test-signal-pending-on-close.c b/test/test-signal-pending-on-close.c
index bf8d2793d..23b9ec8d1 100644
--- a/test/test-signal-pending-on-close.c
+++ b/test/test-signal-pending-on-close.c
@@ -34,6 +34,11 @@ static char* buf;
 static int close_cb_called;
 
 
+static void stop_loop_cb(uv_signal_t* signal, int signum) {
+  ASSERT(signum == SIGPIPE);
+  uv_stop(signal->loop);
+}
+
 static void signal_cb(uv_signal_t* signal, int signum) {
   ASSERT(0);
 }
@@ -91,4 +96,24 @@ TEST_IMPL(signal_pending_on_close) {
   return 0;
 }
 
-#endif
\ No newline at end of file
+
+TEST_IMPL(signal_close_loop_alive) {
+  ASSERT(0 == uv_loop_init(&loop));
+  ASSERT(0 == uv_signal_init(&loop, &signal_hdl));
+  ASSERT(0 == uv_signal_start(&signal_hdl, stop_loop_cb, SIGPIPE));
+  uv_unref((uv_handle_t*) &signal_hdl);
+
+  ASSERT(0 == uv_kill(uv_os_getpid(), SIGPIPE));
+  ASSERT(0 == uv_run(&loop, UV_RUN_DEFAULT));
+  uv_close((uv_handle_t*) &signal_hdl, close_cb);
+  ASSERT(1 == uv_loop_alive(&loop));
+
+  ASSERT(0 == uv_run(&loop, UV_RUN_DEFAULT));
+  ASSERT(0 == uv_loop_close(&loop));
+  ASSERT(1 == close_cb_called);
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
+
+#endif