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: #23781 Fixes: #22767 Refs: #22513 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
nodejs · Oct 24, 2018 · 5d80ae3 · 5d80ae3
1 parent 036fbdb
commit 5d80ae3
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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'
+  });
+}));