From 9d65ad3ef4884cb888fd83fabdcc2276554742e3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 22:16:23 +0100 Subject: [PATCH 1/2] src: avoid race condition in tracing code `json_trace_writer_` is protected by `stream_mutex_`, but one access to it was not guarded by a lock on said mutex. Refs: https://github.com/nodejs/node/issues/25512 --- src/tracing/node_trace_writer.cc | 6 ++++-- src/tracing/node_trace_writer.h | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/tracing/node_trace_writer.cc b/src/tracing/node_trace_writer.cc index 1d06c942332fcd..a9b80ada23a6ae 100644 --- a/src/tracing/node_trace_writer.cc +++ b/src/tracing/node_trace_writer.cc @@ -138,8 +138,10 @@ void NodeTraceWriter::FlushPrivate() { void NodeTraceWriter::Flush(bool blocking) { Mutex::ScopedLock scoped_lock(request_mutex_); - if (!json_trace_writer_) { - return; + { + Mutex::ScopedLock stream_mutex_lock(stream_mutex_); + if (!json_trace_writer_) + return; } int request_id = ++num_write_requests_; int err = uv_async_send(&flush_signal_); diff --git a/src/tracing/node_trace_writer.h b/src/tracing/node_trace_writer.h index a91176ad4905a9..63a5df4f635ef1 100644 --- a/src/tracing/node_trace_writer.h +++ b/src/tracing/node_trace_writer.h @@ -45,7 +45,8 @@ class NodeTraceWriter : public AsyncTraceWriter { // Triggers callback to close async objects, ending the tracing thread. uv_async_t exit_signal_; // Prevents concurrent R/W on state related to serialized trace data - // before it's written to disk, namely stream_ and total_traces_. + // before it's written to disk, namely stream_ and total_traces_ + // as well as json_trace_writer_. Mutex stream_mutex_; // Prevents concurrent R/W on state related to write requests. Mutex request_mutex_; From acabd3e937df0686c5a80cbd8e3fdd3c92178644 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 27 Jan 2019 17:18:33 +0100 Subject: [PATCH 2/2] fixup! src: avoid race condition in tracing code --- src/tracing/node_trace_writer.cc | 3 +++ src/tracing/node_trace_writer.h | 1 + 2 files changed, 4 insertions(+) diff --git a/src/tracing/node_trace_writer.cc b/src/tracing/node_trace_writer.cc index a9b80ada23a6ae..5979810668c5f5 100644 --- a/src/tracing/node_trace_writer.cc +++ b/src/tracing/node_trace_writer.cc @@ -139,6 +139,9 @@ void NodeTraceWriter::FlushPrivate() { void NodeTraceWriter::Flush(bool blocking) { Mutex::ScopedLock scoped_lock(request_mutex_); { + // We need to lock the mutexes here in a nested fashion; stream_mutex_ + // protects json_trace_writer_, and without request_mutex_ there might be + // a time window in which the stream state changes? Mutex::ScopedLock stream_mutex_lock(stream_mutex_); if (!json_trace_writer_) return; diff --git a/src/tracing/node_trace_writer.h b/src/tracing/node_trace_writer.h index 63a5df4f635ef1..f412587ab93219 100644 --- a/src/tracing/node_trace_writer.h +++ b/src/tracing/node_trace_writer.h @@ -49,6 +49,7 @@ class NodeTraceWriter : public AsyncTraceWriter { // as well as json_trace_writer_. Mutex stream_mutex_; // Prevents concurrent R/W on state related to write requests. + // If both mutexes are locked, request_mutex_ has to be locked first. Mutex request_mutex_; // Allows blocking calls to Flush() to wait on a condition for // trace events to be written to disk.