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: nodejs#32984
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

Author: codebytere

src/node_snapshotable.cc

@@ -858,7 +858,9 @@ static void ResetContextSettingsBeforeSnapshot(Local<Context> context) {
 
 const std::vector<intptr_t>& SnapshotBuilder::CollectExternalReferences() {
   static auto registry = std::make_unique<ExternalReferenceRegistry>();
-  return registry->external_references();
+  static const std::vector<intptr_t>& references =
+      registry->external_references();
+  return references;
 }
 
 void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data,

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 <stdio.h>
+#include <cstdio>
 #include <string>
+#include <thread>  // NOLINT(build/c++11)
 #include "gtest/gtest.h"
 #include "node_test_fixture.h"
-#include <stdio.h>
-#include <cstdio>
 
 using node::AtExit;
 using node::RunAtExit;
@@ -367,6 +369,27 @@ TEST_F(EnvironmentTest, WorkerInEnvironmentWithoutSnapshot) {
   EXPECT_EQ(node::SpinEventLoop(*env).FromJust(), 0);
 }
 
+TEST_F(EnvironmentTest, CollectExternalReferencesFromSeveralThreads) {
+  constexpr int kThreads = 8;
+  const intptr_t* data[kThreads];
+  size_t sizes[kThreads];
+  std::vector<std::thread> threads;
+  for (int i = 0; i < kThreads; i++) {
+    threads.emplace_back([&, i]() {
+      const std::vector<intptr_t>& 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<v8::Context> context = v8::Context::New(isolate_);