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: #45885
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #65779
Refs: #32984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Author: codebytere

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.

src/node.h

@@ -1009,6 +1009,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 <typename... EnvironmentArgs>
   static std::unique_ptr<CommonEnvironmentSetup> Create(
       MultiIsolatePlatform* platform,

src/node_internals.h

@@ -348,6 +348,8 @@ void DefineZlibConstants(v8::Local<v8::Object> 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,

src/node_snapshotable.cc

@@ -697,7 +697,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;
   }
 }

test/embedding/embedtest.cc

@@ -70,8 +70,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<std::string> 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();

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;
+    },
+  });