Sitelet https://github.com/nodejs/node/commit/c16ce75d04f76a4dab74661221b6bbfc5f7b0117
Skip to content

Commit c16ce75

Browse files
codebyterecodebytere
committed
src: fix external reference list race between concurrent isolates
Two threads creating their first isolate at the same time (two `CommonEnvironmentSetup`s on their own threads, or an embedder's setup racing a Worker) could corrupt or misread the external reference list handed to V8: `SnapshotBuilder::CollectExternalReferences()` creates its registry in a thread-safe function static, but then calls `external_references()` on every call, and that method appends the terminating nullptr and flips `is_finalized_` the first time through without any locking, so both threads can append, or one can read the vector while the other reallocates it. TSAN reports it for any two concurrent setups. Keep the finalized list in a second function static so finalization runs exactly once, under that static's initialization guard. Refs: #32984 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
1 parent 98530c6 commit c16ce75

2 files changed

Lines changed: 28 additions & 3 deletions

File tree

‎src/node_snapshotable.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -858,7 +858,9 @@ static void ResetContextSettingsBeforeSnapshot(Local<Context> context) {
858858

859859
const std::vector<intptr_t>& SnapshotBuilder::CollectExternalReferences() {
860860
static auto registry = std::make_unique<ExternalReferenceRegistry>();
861-
return registry->external_references();
861+
static const std::vector<intptr_t>& references =
862+
registry->external_references();
863+
return references;
862864
}
863865

864866
void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data,

‎test/cctest/test_environment.cc‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,16 @@
22
#include "node_buffer.h"
33
#include "node_internals.h"
44
#include "node_realm-inl.h"
5+
#include "node_snapshot_builder.h"
56
#include "node_url.h"
67
#include "util.h"
78

9+
#include <stdio.h>
10+
#include <cstdio>
811
#include <string>
12+
#include <thread> // NOLINT(build/c++11)
913
#include "gtest/gtest.h"
1014
#include "node_test_fixture.h"
11-
#include <stdio.h>
12-
#include <cstdio>
1315

1416
using node::AtExit;
1517
using node::RunAtExit;
@@ -388,6 +390,27 @@ TEST_F(EnvironmentTest, StopFromExitHandlerDoesNotLeakIntoNextEnvironment) {
388390
}
389391
}
390392

393+
TEST_F(EnvironmentTest, CollectExternalReferencesFromSeveralThreads) {
394+
constexpr int kThreads = 8;
395+
const intptr_t* data[kThreads];
396+
size_t sizes[kThreads];
397+
std::vector<std::thread> threads;
398+
for (int i = 0; i < kThreads; i++) {
399+
threads.emplace_back([&, i]() {
400+
const std::vector<intptr_t>& references =
401+
node::SnapshotBuilder::CollectExternalReferences();
402+
data[i] = references.data();
403+
sizes[i] = references.size();
404+
});
405+
}
406+
for (std::thread& thread : threads) thread.join();
407+
for (int i = 1; i < kThreads; i++) {
408+
EXPECT_EQ(data[i], data[0]);
409+
EXPECT_EQ(sizes[i], sizes[0]);
410+
}
411+
EXPECT_EQ(node::SnapshotBuilder::CollectExternalReferences().back(), 0);
412+
}
413+
391414
TEST_F(EnvironmentTest, NoEnvironmentSanity) {
392415
const v8::HandleScope handle_scope(isolate_);
393416
v8::Local<v8::Context> context = v8::Context::New(isolate_);

0 commit comments

Comments
 (0)