From 60311fe2fa8d6e0670d8b51f703ec2738fc35686 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 2 Feb 2019 15:15:35 +0100
Subject: [PATCH 1/2] 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.

Credit for debugging goes to Gireesh Punathil.

Fixes: https://github.com/nodejs/node/issues/25512
---
 src/tracing/node_trace_buffer.cc | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/tracing/node_trace_buffer.cc b/src/tracing/node_trace_buffer.cc
index 3b7119f6d57f68..796c9f529288e5 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);
+    });
   });
 }
 

From 2219a399ad085da15af67080fa2ced0f4ec9855a Mon Sep 17 00:00:00 2001
From: Gireesh Punathil <gpunathi@in.ibm.com>
Date: Sun, 3 Feb 2019 08:17:08 -0500
Subject: [PATCH 2/2] fixup: fix race in NodeTraceWriter too

---
 src/tracing/node_trace_writer.cc | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/tracing/node_trace_writer.cc b/src/tracing/node_trace_writer.cc
index 5979810668c5f5..b93688cc958faa 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