diff options
| author | Anna Henningsen <anna@addaleax.net> | 2018-10-20 11:51:29 +0200 |
|---|---|---|
| committer | Matheus Marchini <mat@mmarchini.me> | 2018-10-24 11:20:35 -0300 |
| commit | 5d80ae3acdd812651b3b193cd31e0b81c214b50e (patch) | |
| tree | efb0548ce0f26174dee13feeadab072c2c9461e5 /src | |
| parent | 036fbdb63d603a64bd8562ec9331dfb7a5c5075c (diff) | |
| download | android-node-v8-5d80ae3acdd812651b3b193cd31e0b81c214b50e.tar.gz android-node-v8-5d80ae3acdd812651b3b193cd31e0b81c214b50e.tar.bz2 android-node-v8-5d80ae3acdd812651b3b193cd31e0b81c214b50e.zip | |
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>
Diffstat (limited to 'src')
| -rw-r--r-- | src/env.cc | 13 | ||||
| -rw-r--r-- | src/inspector/tracing_agent.cc | 4 |
2 files changed, 16 insertions, 1 deletions
diff --git a/src/env.cc b/src/env.cc index 5da673b3e0..383df37aab 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 1ad67d9b9e..64bf7457d9 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 = |