Skip to content

Commit

Permalink
Fix platform delegate race condition
Browse files Browse the repository at this point in the history
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
  • Loading branch information
laverdet committed Jan 19, 2024
1 parent aabf3cd commit fba4d3c
Showing 1 changed file with 18 additions and 8 deletions.
26 changes: 18 additions & 8 deletions src/isolate/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 });
}
Expand Down

0 comments on commit fba4d3c

Please sign in to comment.