From 5d80ae3acdd812651b3b193cd31e0b81c214b50e Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 20 Oct 2018 11:51:29 +0200
Subject: [PATCH] trace_events: forbid tracing modifications from worker
 threads

Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the `async_hooks` integration is definitely not thread
safe in its current state.

PR-URL: https://github.com/nodejs/node/pull/23781
Fixes: https://github.com/nodejs/node/issues/22767
Refs: https://github.com/nodejs/node/issues/22513
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
---
 doc/api/tracing.md                            |  3 ++
 doc/api/worker_threads.md                     |  2 ++
 lib/trace_events.js                           |  3 +-
 src/env.cc                                    | 13 ++++++++-
 src/inspector/tracing_agent.cc                |  4 +++
 .../test-trace-events-api-worker-disabled.js  | 11 ++++++++
 ...-events-dynamic-enable-workers-disabled.js | 28 +++++++++++++++++++
 7 files changed, 62 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-trace-events-api-worker-disabled.js
 create mode 100644 test/parallel/test-trace-events-dynamic-enable-workers-disabled.js

diff --git a/doc/api/tracing.md b/doc/api/tracing.md
index 04db3f12f42e2e..10b2f61ea5a5b9 100644
--- a/doc/api/tracing.md
+++ b/doc/api/tracing.md
@@ -82,6 +82,8 @@ as the one used by `process.hrtime()`
 however the trace-event timestamps are expressed in microseconds,
 unlike `process.hrtime()` which returns nanoseconds.
 
+The features from this module are not available in [`Worker`][] threads.
+
 ## The `trace_events` module
 <!-- YAML
 added: v10.0.0
@@ -205,3 +207,4 @@ console.log(trace_events.getEnabledCategories());
 [Performance API]: perf_hooks.html
 [V8]: v8.html
 [`async_hooks`]: async_hooks.html
+[`Worker`]: worker_threads.html#worker_threads_class_worker
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index b73787174cdc96..328bb55c88f3c9 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -253,6 +253,7 @@ Notable differences inside a Worker environment are:
 - Execution may stop at any point as a result of [`worker.terminate()`][]
   being invoked.
 - IPC channels from parent processes are not accessible.
+- The [`trace_events`][] module is not supported.
 
 Currently, the following differences also exist until they are addressed:
 
@@ -489,6 +490,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
 [Signals events]: process.html#process_signal_events
 [`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
+[`trace_events`]: tracing.html
 [browser `MessagePort`]: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
 [child processes]: child_process.html
 [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
diff --git a/lib/trace_events.js b/lib/trace_events.js
index bf303c28b7dcdc..878580c67f5a2f 100644
--- a/lib/trace_events.js
+++ b/lib/trace_events.js
@@ -13,7 +13,8 @@ const {
   ERR_INVALID_ARG_TYPE
 } = require('internal/errors').codes;
 
-if (!hasTracing)
+const { isMainThread } = require('internal/worker');
+if (!hasTracing || !isMainThread)
   throw new ERR_TRACE_EVENTS_UNAVAILABLE();
 
 const { CategorySet, getEnabledCategories } = internalBinding('trace_events');
diff --git a/src/env.cc b/src/env.cc
index 5da673b3e06020..383df37aab88bd 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -128,11 +128,22 @@ void InitThreadLocalOnce() {
 }
 
 void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
+  if (!env_->is_main_thread()) {
+    // Ideally, we’d have a consistent story that treats all threads/Environment
+    // instances equally here. However, tracing is essentially global, and this
+    // callback is called from whichever thread calls `StartTracing()` or
+    // `StopTracing()`. The only way to do this in a threadsafe fashion
+    // seems to be only tracking this from the main thread, and only allowing
+    // these state modifications from the main thread.
+    return;
+  }
+
   env_->trace_category_state()[0] =
       *TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
           TRACING_CATEGORY_NODE1(async_hooks));
 
   Isolate* isolate = env_->isolate();
+  HandleScope handle_scope(isolate);
   Local<Function> cb = env_->trace_category_state_function();
   if (cb.IsEmpty())
     return;
@@ -182,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data,
   AssignToContext(context, ContextInfo(""));
 
   if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
-    trace_state_observer_.reset(new TrackingTraceStateObserver(this));
+    trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);
     v8::TracingController* tracing_controller = writer->GetTracingController();
     if (tracing_controller != nullptr)
       tracing_controller->AddTraceStateObserver(trace_state_observer_.get());
diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc
index 1ad67d9b9e7f71..64bf7457d9f728 100644
--- a/src/inspector/tracing_agent.cc
+++ b/src/inspector/tracing_agent.cc
@@ -65,6 +65,10 @@ DispatchResponse TracingAgent::start(
     return DispatchResponse::Error(
         "Call NodeTracing::end to stop tracing before updating the config");
   }
+  if (!env_->is_main_thread()) {
+    return DispatchResponse::Error(
+        "Tracing properties can only be changed through main thread sessions");
+  }
 
   std::set<std::string> categories_set;
   protocol::Array<std::string>* categories =
diff --git a/test/parallel/test-trace-events-api-worker-disabled.js b/test/parallel/test-trace-events-api-worker-disabled.js
new file mode 100644
index 00000000000000..6140d3a83c5339
--- /dev/null
+++ b/test/parallel/test-trace-events-api-worker-disabled.js
@@ -0,0 +1,11 @@
+// Flags: --experimental-worker
+'use strict';
+
+const common = require('../common');
+const { Worker } = require('worker_threads');
+
+new Worker("require('trace_events')", { eval: true })
+  .on('error', common.expectsError({
+    code: 'ERR_TRACE_EVENTS_UNAVAILABLE',
+    type: Error
+  }));
diff --git a/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js b/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js
new file mode 100644
index 00000000000000..adae50057e8a4e
--- /dev/null
+++ b/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js
@@ -0,0 +1,28 @@
+// Flags: --experimental-worker
+'use strict';
+
+const common = require('../common');
+const { Worker } = require('worker_threads');
+
+common.skipIfInspectorDisabled();
+
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+  new Worker(__filename);
+  return;
+}
+
+const assert = require('assert');
+const { Session } = require('inspector');
+
+const session = new Session();
+session.connect();
+session.post('NodeTracing.start', {
+  traceConfig: { includedCategories: ['node.perf'] }
+}, common.mustCall((err) => {
+  assert.deepStrictEqual(err, {
+    code: -32000,
+    message:
+      'Tracing properties can only be changed through main thread sessions'
+  });
+}));