Skip to content

Commit b6004b3

Browse files
addaleaxrvagg
authored andcommitted
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 <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
1 parent 5061610 commit b6004b3

7 files changed

+62
-2
lines changed

doc/api/tracing.md

+3
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ as the one used by `process.hrtime()`
8282
however the trace-event timestamps are expressed in microseconds,
8383
unlike `process.hrtime()` which returns nanoseconds.
8484

85+
The features from this module are not available in [`Worker`][] threads.
86+
8587
## The `trace_events` module
8688
<!-- YAML
8789
added: v10.0.0
@@ -205,3 +207,4 @@ console.log(trace_events.getEnabledCategories());
205207
[Performance API]: perf_hooks.html
206208
[V8]: v8.html
207209
[`async_hooks`]: async_hooks.html
210+
[`Worker`]: worker_threads.html#worker_threads_class_worker

doc/api/worker_threads.md

+2
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ Notable differences inside a Worker environment are:
253253
- Execution may stop at any point as a result of [`worker.terminate()`][]
254254
being invoked.
255255
- IPC channels from parent processes are not accessible.
256+
- The [`trace_events`][] module is not supported.
256257

257258
Currently, the following differences also exist until they are addressed:
258259

@@ -489,6 +490,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
489490
[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
490491
[Signals events]: process.html#process_signal_events
491492
[`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
493+
[`trace_events`]: tracing.html
492494
[browser `MessagePort`]: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
493495
[child processes]: child_process.html
494496
[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

lib/trace_events.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ const {
1313
ERR_INVALID_ARG_TYPE
1414
} = require('internal/errors').codes;
1515

16-
if (!hasTracing)
16+
const { isMainThread } = require('internal/worker');
17+
if (!hasTracing || !isMainThread)
1718
throw new ERR_TRACE_EVENTS_UNAVAILABLE();
1819

1920
const { CategorySet, getEnabledCategories } = internalBinding('trace_events');

src/env.cc

+12-1
Original file line numberDiff line numberDiff line change
@@ -128,11 +128,22 @@ void InitThreadLocalOnce() {
128128
}
129129

130130
void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
131+
if (!env_->is_main_thread()) {
132+
// Ideally, we’d have a consistent story that treats all threads/Environment
133+
// instances equally here. However, tracing is essentially global, and this
134+
// callback is called from whichever thread calls `StartTracing()` or
135+
// `StopTracing()`. The only way to do this in a threadsafe fashion
136+
// seems to be only tracking this from the main thread, and only allowing
137+
// these state modifications from the main thread.
138+
return;
139+
}
140+
131141
env_->trace_category_state()[0] =
132142
*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
133143
TRACING_CATEGORY_NODE1(async_hooks));
134144

135145
Isolate* isolate = env_->isolate();
146+
HandleScope handle_scope(isolate);
136147
Local<Function> cb = env_->trace_category_state_function();
137148
if (cb.IsEmpty())
138149
return;
@@ -182,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data,
182193
AssignToContext(context, ContextInfo(""));
183194

184195
if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
185-
trace_state_observer_.reset(new TrackingTraceStateObserver(this));
196+
trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);
186197
v8::TracingController* tracing_controller = writer->GetTracingController();
187198
if (tracing_controller != nullptr)
188199
tracing_controller->AddTraceStateObserver(trace_state_observer_.get());

src/inspector/tracing_agent.cc

+4
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,10 @@ DispatchResponse TracingAgent::start(
6565
return DispatchResponse::Error(
6666
"Call NodeTracing::end to stop tracing before updating the config");
6767
}
68+
if (!env_->is_main_thread()) {
69+
return DispatchResponse::Error(
70+
"Tracing properties can only be changed through main thread sessions");
71+
}
6872

6973
std::set<std::string> categories_set;
7074
protocol::Array<std::string>* categories =
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
4+
const common = require('../common');
5+
const { Worker } = require('worker_threads');
6+
7+
new Worker("require('trace_events')", { eval: true })
8+
.on('error', common.expectsError({
9+
code: 'ERR_TRACE_EVENTS_UNAVAILABLE',
10+
type: Error
11+
}));
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
4+
const common = require('../common');
5+
const { Worker } = require('worker_threads');
6+
7+
common.skipIfInspectorDisabled();
8+
9+
if (!process.env.HAS_STARTED_WORKER) {
10+
process.env.HAS_STARTED_WORKER = 1;
11+
new Worker(__filename);
12+
return;
13+
}
14+
15+
const assert = require('assert');
16+
const { Session } = require('inspector');
17+
18+
const session = new Session();
19+
session.connect();
20+
session.post('NodeTracing.start', {
21+
traceConfig: { includedCategories: ['node.perf'] }
22+
}, common.mustCall((err) => {
23+
assert.deepStrictEqual(err, {
24+
code: -32000,
25+
message:
26+
'Tracing properties can only be changed through main thread sessions'
27+
});
28+
}));

0 commit comments

Comments
 (0)