From fba4d3c22974b8836f93de2fe61db2546494391b Mon Sep 17 00:00:00 2001
From: Marcel Laverdet <marcel@laverdet.com>
Date: Fri, 19 Jan 2024 17:36:07 -0600
Subject: [PATCH] Fix platform delegate race condition

If a new isolate environment is allocated right after a previous one is
deallocated then the new one could possibly get the same memory pointer
as the previous one. This would cause node's `RegisterIsolate` API to
fail because it already has a registered isolate with that pointer.

I think maybe v8's `Isolate::Dispose()` method should be split out into
`Destroy()` and `Deallocate()` in the same way `Create()` was split into
`Allocate()` and `Initialize()`. The fact that `Dispose()` potentially
invokes callbacks doesn't give us any way to run hooks after callbacks
are done but before memory is freed.

Fixes #447
---
 src/isolate/environment.cc | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/isolate/environment.cc b/src/isolate/environment.cc
index 1823cec..c6bd9f1 100644
--- a/src/isolate/environment.cc
+++ b/src/isolate/environment.cc
@@ -80,6 +80,10 @@ namespace ivm {
 std::atomic<size_t> detail::IsolateSpecificSize{0};
 thread_suspend_handle::initialize suspend_init{};
 
+namespace {
+	std::mutex isolate_allocator_mutex{};
+} // anonymous namespace
+
 /**
  * HeapCheck implementation
  */
@@ -375,8 +379,11 @@ void IsolateEnvironment::IsolateCtor(size_t memory_limit_in_mb, shared_ptr<v8::B
 		startup_data.raw_size = snapshot_length;
 	}
 	task_runner = std::make_shared<IsolateTaskRunner>(holder.lock()->GetIsolate());
-	isolate = Isolate::Allocate();
-	PlatformDelegate::RegisterIsolate(isolate, &*scheduler);
+	{
+		std::lock_guard allocator_lock{isolate_allocator_mutex};
+		isolate = Isolate::Allocate();
+		PlatformDelegate::RegisterIsolate(isolate, &*scheduler);
+	}
 	Isolate::Initialize(isolate, create_params);
 
 	// Various callbacks
@@ -450,13 +457,16 @@ IsolateEnvironment::~IsolateEnvironment() {
 			ExchangeDefault(scheduler_lock->tasks);
 		}
 		{
-			// Dispose() will call destructors for external strings and array buffers, so this lock sets the
-			// "current" isolate for those C++ dtors to function correctly without locking v8
-			Executor::Scope lock{*this};
-			isolate->Dispose();
+			std::lock_guard allocator_lock{isolate_allocator_mutex};
+			{
+				// Dispose() will call destructors for external strings and array buffers, so this lock sets the
+				// "current" isolate for those C++ dtors to function correctly without locking v8
+				Executor::Scope lock{*this};
+				isolate->Dispose();
+			}
+			// Unregister from Platform
+			PlatformDelegate::UnregisterIsolate(isolate);
 		}
-		// Unregister from Platform
-		PlatformDelegate::UnregisterIsolate(isolate);
 		// Unreference from default isolate
 		executor.default_executor.env.owned_isolates->write()->erase({ dispose_wait, holder });
 	}