From 654a6cf148c925fb73ddc5083f9c2ac06e19a754 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 2 Sep 2026 22:49:53 +0000 Subject: [PATCH 1/2] src: keep the first snapshot blob alive for later isolates `NewIsolate()` creates every isolate from the snapshot blob the first isolate in the process used, because V8 shares the read-only heap between isolates, and did so by keeping a pointer to the first `CreateParams`. When that blob came from an `EmbedderSnapshotData` the embedder had since released, e.g. a second `CommonEnvironmentSetup::CreateFromSnapshot()` after the first setup and its snapshot were destroyed, V8 deserialized freed memory. Record the first blob and external references under a mutex instead of copying the caller's `CreateParams`, and make `~SnapshotData()` leave that one blob allocated, since its owner can go away before the last isolate is created. Nothing is copied and `node` itself is unaffected. embedtest grows an `--embedder-run-twice` switch so the sequence can be tested. Refs: https://github.com/nodejs/node/pull/45885 Signed-off-by: Shelley Vohr --- src/api/environment.cc | 38 +++++++++++++----- src/node.h | 3 ++ src/node_internals.h | 2 + src/node_snapshotable.cc | 3 +- test/embedding/embedtest.cc | 16 +++++++- .../test-embedding-snapshot-twice.js | 39 +++++++++++++++++++ 6 files changed, 89 insertions(+), 12 deletions(-) create mode 100644 test/embedding/test-embedding-snapshot-twice.js diff --git a/src/api/environment.cc b/src/api/environment.cc index 459830a2af80..ff116d40d127 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -312,6 +312,34 @@ IsolateGroup GetOrCreateIsolateGroup() { return IsolateGroup::GetDefault(); } +// V8 shares the read-only heap between isolates and requires them all to be +// created from the same snapshot, so every isolate gets the blob and external +// references the first NewIsolate() call used. ~SnapshotData() leaves that blob +// alone because its owner may be gone before the last isolate is created. +static Mutex first_snapshot_mutex; +static bool first_snapshot_recorded = false; +static v8::StartupData first_snapshot_blob{nullptr, 0}; +static const intptr_t* first_external_references = nullptr; + +static void UseFirstSnapshot(Isolate::CreateParams* params) { + Mutex::ScopedLock lock(first_snapshot_mutex); + if (!first_snapshot_recorded) { + first_snapshot_recorded = true; + if (params->snapshot_blob != nullptr) { + first_snapshot_blob = *params->snapshot_blob; + } + first_external_references = params->external_references; + } + params->snapshot_blob = + first_snapshot_blob.data != nullptr ? &first_snapshot_blob : nullptr; + params->external_references = first_external_references; +} + +bool IsFirstSnapshotBlob(const char* data) { + Mutex::ScopedLock lock(first_snapshot_mutex); + return first_snapshot_recorded && data == first_snapshot_blob.data; +} + // TODO(joyeecheung): we may want to expose this, but then we need to be // careful about what we override in the params. Isolate* NewIsolate(Isolate::CreateParams* params, @@ -327,15 +355,7 @@ Isolate* NewIsolate(Isolate::CreateParams* params, SnapshotBuilder::InitializeIsolateParams(snapshot_data, params); } - { - // Because it uses a shared readonly-heap, V8 requires all snapshots used - // for creating Isolates to be identical. This isn't really memory-safe - // but also otherwise just doesn't work, and the only real alternative - // is disabling shared-readonly-heap mode altogether. - static Isolate::CreateParams first_params = *params; - params->snapshot_blob = first_params.snapshot_blob; - params->external_references = first_params.external_references; - } + UseFirstSnapshot(params); // Register the isolate on the platform before the isolate gets initialized, // so that the isolate can access the platform during initialization. diff --git a/src/node.h b/src/node.h index 8e7d1e6a2516..391ec637a3d9 100644 --- a/src/node.h +++ b/src/node.h @@ -1004,6 +1004,9 @@ class NODE_EXTERN CommonEnvironmentSetup { // will be empty. // env_args will be passed through as arguments to CreateEnvironment(), after // `isolate_data` and `context`. + // `snapshot_data` has to stay alive as long as the setup created from it, + // and every setup in a process has to use the same snapshot: all isolates + // are created from the blob the first one used. template static std::unique_ptr Create( MultiIsolatePlatform* platform, diff --git a/src/node_internals.h b/src/node_internals.h index 17e23ce61d64..cf3ce9e13f4d 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -348,6 +348,8 @@ void DefineZlibConstants(v8::Local target); // addresses, so this should be used with care. v8::IsolateGroup GetOrCreateIsolateGroup(); +// The blob every isolate is created from, see NewIsolate(). It is never freed. +bool IsFirstSnapshotBlob(const char* data); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, uv_loop_t* event_loop, MultiIsolatePlatform* platform, diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index e861e499534c..2291586d4eca 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -701,7 +701,8 @@ bool SnapshotData::Check() const { SnapshotData::~SnapshotData() { if (data_ownership == DataOwnership::kOwned && - v8_snapshot_blob_data.data != nullptr) { + v8_snapshot_blob_data.data != nullptr && + !IsFirstSnapshotBlob(v8_snapshot_blob_data.data)) { delete[] v8_snapshot_blob_data.data; } } diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index 007781754ab9..8f94e6e910cb 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -116,8 +116,20 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { cppgc::InitializeProcess(platform->GetPageAllocator()); V8::Initialize(); - int ret = - RunNodeInstance(platform.get(), result->args(), result->exec_args()); + // --embedder-run-twice: two sequential instances in one process, each + // loading (and freeing) its own copy of the snapshot. + std::vector instance_args = result->args(); + auto twice = std::find( + instance_args.begin(), instance_args.end(), "--embedder-run-twice"); + int runs = 1; + if (twice != instance_args.end()) { + instance_args.erase(twice); + runs = 2; + } + int ret = 0; + for (int i = 0; i < runs && ret == 0; i++) { + ret = RunNodeInstance(platform.get(), instance_args, result->exec_args()); + } V8::Dispose(); V8::DisposePlatform(); diff --git a/test/embedding/test-embedding-snapshot-twice.js b/test/embedding/test-embedding-snapshot-twice.js new file mode 100644 index 000000000000..7d09aab78e10 --- /dev/null +++ b/test/embedding/test-embedding-snapshot-twice.js @@ -0,0 +1,39 @@ +'use strict'; + +// Tests that an embedder can free the EmbedderSnapshotData it created an +// instance from and create a second instance from a fresh copy afterwards. + +const common = require('../common'); +const assert = require('assert'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const { + spawnSyncAndAssert, + spawnSyncAndExitWithoutError, +} = require('../common/child_process'); + +const embedtest = common.resolveBuiltBinary('embedtest'); +const snapshotFixture = fixtures.path('snapshot', 'echo-args.js'); +const blob = tmpdir.resolve('embedder-snapshot.blob'); + +tmpdir.refresh(); + +spawnSyncAndExitWithoutError( + embedtest, + [ + '--', + `eval(require("fs").readFileSync(${JSON.stringify(snapshotFixture)}, "utf8"))`, + 'arg1', 'arg2', '--embedder-snapshot-blob', blob, '--embedder-snapshot-create', + ], + { cwd: tmpdir.path }); + +spawnSyncAndAssert( + embedtest, + ['--', 'arg3', '--embedder-snapshot-blob', blob, '--embedder-run-twice'], + { cwd: tmpdir.path }, + { + stdout(output) { + assert.strictEqual(output.split('arg3').length, 3); + return true; + }, + }); From 0cda701f57727746ee7266214d086d26cb85e9d5 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Thu, 3 Sep 2026 00:07:20 +0000 Subject: [PATCH 2/2] 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: https://github.com/nodejs/node/pull/32984 Signed-off-by: Shelley Vohr --- src/node_snapshotable.cc | 4 +++- test/cctest/test_environment.cc | 27 +++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 2291586d4eca..341e4564d70d 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -862,7 +862,9 @@ static void ResetContextSettingsBeforeSnapshot(Local context) { const std::vector& SnapshotBuilder::CollectExternalReferences() { static auto registry = std::make_unique(); - return registry->external_references(); + static const std::vector& references = + registry->external_references(); + return references; } void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 36fbc0e79d46..90f5f52f83f3 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -2,14 +2,16 @@ #include "node_buffer.h" #include "node_internals.h" #include "node_realm-inl.h" +#include "node_snapshot_builder.h" #include "node_url.h" #include "util.h" +#include +#include #include +#include // NOLINT(build/c++11) #include "gtest/gtest.h" #include "node_test_fixture.h" -#include -#include using node::AtExit; using node::RunAtExit; @@ -328,6 +330,27 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { EXPECT_TRUE(called_cb_2); } +TEST_F(EnvironmentTest, CollectExternalReferencesFromSeveralThreads) { + constexpr int kThreads = 8; + const intptr_t* data[kThreads]; + size_t sizes[kThreads]; + std::vector threads; + for (int i = 0; i < kThreads; i++) { + threads.emplace_back([&, i]() { + const std::vector& references = + node::SnapshotBuilder::CollectExternalReferences(); + data[i] = references.data(); + sizes[i] = references.size(); + }); + } + for (std::thread& thread : threads) thread.join(); + for (int i = 1; i < kThreads; i++) { + EXPECT_EQ(data[i], data[0]); + EXPECT_EQ(sizes[i], sizes[0]); + } + EXPECT_EQ(node::SnapshotBuilder::CollectExternalReferences().back(), 0); +} + TEST_F(EnvironmentTest, NoEnvironmentSanity) { const v8::HandleScope handle_scope(isolate_); v8::Local context = v8::Context::New(isolate_);