src: fix race condition in `~NodeTraceBuffer`

Libuv does not guarantee that handles have their close
callbacks called in the order in which they were added
(and in fact, currently calls them in reverse order).

This patch ensures that the `flush_signal_` handle
is no longer in use (i.e. its close callback has already
been run) when we signal to the main thread that
`~NodeTraceBuffer` may be destroyed.

The same applies for `~NodeTraceWriter`.

Credit for debugging goes to Gireesh Punathil.

Fixes: https://github.com/nodejs/node/issues/25512
Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: https://github.com/nodejs/node/pull/25896
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>

2 files changed, 33 insertions, 17 deletions

diff --git a/src/tracing/node_trace_buffer.cc b/src/tracing/node_trace_buffer.cc
index 3b7119f6d5..796c9f5292 100644
--- a/src/tracing/node_trace_buffer.cc
+++ b/src/tracing/node_trace_buffer.cc
@@ -1,4 +1,5 @@
 #include "tracing/node_trace_buffer.h"
+#include "util-inl.h"
 
 namespace node {
 namespace tracing {
@@ -170,15 +171,25 @@ void NodeTraceBuffer::NonBlockingFlushSignalCb(uv_async_t* signal) {
 
 // static
 void NodeTraceBuffer::ExitSignalCb(uv_async_t* signal) {
-  NodeTraceBuffer* buffer = reinterpret_cast<NodeTraceBuffer*>(signal->data);
-  uv_close(reinterpret_cast<uv_handle_t*>(&buffer->flush_signal_), nullptr);
-  uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
+  NodeTraceBuffer* buffer =
+      ContainerOf(&NodeTraceBuffer::exit_signal_, signal);
+
+  // Close both flush_signal_ and exit_signal_.
+  uv_close(reinterpret_cast<uv_handle_t*>(&buffer->flush_signal_),
            [](uv_handle_t* signal) {
+    NodeTraceBuffer* buffer =
+        ContainerOf(&NodeTraceBuffer::flush_signal_,
+                    reinterpret_cast<uv_async_t*>(signal));
+
+    uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
+             [](uv_handle_t* signal) {
       NodeTraceBuffer* buffer =
-          reinterpret_cast<NodeTraceBuffer*>(signal->data);
-      Mutex::ScopedLock scoped_lock(buffer->exit_mutex_);
-      buffer->exited_ = true;
-      buffer->exit_cond_.Signal(scoped_lock);
+          ContainerOf(&NodeTraceBuffer::exit_signal_,
+                      reinterpret_cast<uv_async_t*>(signal));
+        Mutex::ScopedLock scoped_lock(buffer->exit_mutex_);
+        buffer->exited_ = true;
+        buffer->exit_cond_.Signal(scoped_lock);
+    });
   });
 }
 
diff --git a/src/tracing/node_trace_writer.cc b/src/tracing/node_trace_writer.cc
index 5979810668..b93688cc95 100644
--- a/src/tracing/node_trace_writer.cc
+++ b/src/tracing/node_trace_writer.cc
@@ -218,18 +218,23 @@ void NodeTraceWriter::AfterWrite() {
 void NodeTraceWriter::ExitSignalCb(uv_async_t* signal) {
   NodeTraceWriter* trace_writer =
       ContainerOf(&NodeTraceWriter::exit_signal_, signal);
+  // Close both flush_signal_ and exit_signal_.
   uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->flush_signal_),
-           nullptr);
-  uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
            [](uv_handle_t* signal) {
-      NodeTraceWriter* trace_writer =
-          ContainerOf(&NodeTraceWriter::exit_signal_,
-                      reinterpret_cast<uv_async_t*>(signal));
-      Mutex::ScopedLock scoped_lock(trace_writer->request_mutex_);
-      trace_writer->exited_ = true;
-      trace_writer->exit_cond_.Signal(scoped_lock);
-  });
+             NodeTraceWriter* trace_writer =
+                 ContainerOf(&NodeTraceWriter::flush_signal_,
+                             reinterpret_cast<uv_async_t*>(signal));
+             uv_close(
+                 reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
+                 [](uv_handle_t* signal) {
+                   NodeTraceWriter* trace_writer =
+                       ContainerOf(&NodeTraceWriter::exit_signal_,
+                                   reinterpret_cast<uv_async_t*>(signal));
+                   Mutex::ScopedLock scoped_lock(trace_writer->request_mutex_);
+                   trace_writer->exited_ = true;
+                   trace_writer->exit_cond_.Signal(scoped_lock);
+                 });
+           });
 }
-
 }  // namespace tracing
 }  // namespace node