From 2f334c0de39089602ac0cdaf9fc0ee12be85c43e Mon Sep 17 00:00:00 2001 From: Yuri Neves Silveira Date: Thu, 21 May 2026 00:14:45 -0300 Subject: [PATCH] fix: serialize CreateCodeCache against Isolate.Dispose via RWMutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V8 14.x shares process-global state between isolates (ReadOnlySpace, StringForwardingTable, pointer compression cage). snapshotDeserMu already serializes NewIsolate and Dispose against each other, but CreateCodeCache was unprotected — it reads SharedFunctionInfo which references shared-heap objects that Dispose can tear down concurrently. Under 12-worker concurrency this causes SIGSEGV at address 0x0 in UnboundScriptCreateCodeCache when the ISOLATE_SCOPE macro dereferences a null or freed isolate pointer. Changes: - Upgrade snapshotDeserMu from sync.Mutex to sync.RWMutex. Dispose/NewIsolate take Lock (exclusive writer); CreateCodeCache takes RLock (shared reader). Multiple concurrent CreateCodeCache calls on different isolates remain parallel. - Move i.ptr = nil inside the write lock in Dispose to close the TOCTOU window where a dangling pointer was visible after unlock. - Add nil guards in CreateCodeCache (Go layer) before and after acquiring RLock, plus a null check in the C layer before ISOLATE_SCOPE to prevent dereferencing NULL. --- isolate.go | 12 ++++++++---- snapshot.go | 21 +++++++++++++++------ unbound_script.cc | 1 + unbound_script.go | 25 ++++++++++++++++++++++++- 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/isolate.go b/isolate.go index bac63e1..c0cdd88 100644 --- a/isolate.go +++ b/isolate.go @@ -342,14 +342,18 @@ func (i *Isolate) Dispose() { unregisterIsolate(i) // Serialise dispose against snapshotDeserMu: V8 14.x corrupts its // shared-heap teardown state if a parallel goroutine is constructing - // a new isolate at the same moment we are tearing one down. The - // critical section is short (microseconds) and idiomatic v8::Locker - // rules continue to apply for everyday API calls. + // a new isolate at the same moment we are tearing one down. + // CreateCodeCache takes an RLock on the same mutex so it cannot + // overlap with Dispose — see unbound_script.go. + // + // i.ptr is set to nil inside the lock so that any RLock holder + // that re-checks after acquiring the lock sees the nil and bails + // instead of passing a dangling pointer to CGo. snapshotDeserMu.Lock() C.IsolateDisposeSnapshot(i.ptr) C.IsolateDispose(i.ptr) - snapshotDeserMu.Unlock() i.ptr = nil + snapshotDeserMu.Unlock() if i.snapshotData != nil { C.free(i.snapshotData) i.snapshotData = nil diff --git a/snapshot.go b/snapshot.go index 83739d9..079a173 100644 --- a/snapshot.go +++ b/snapshot.go @@ -86,14 +86,23 @@ var ErrSnapshotCreatorConsumed = errors.New( // lets the embedder use the wrapper freely from any goroutine. var snapshotCreatorMu sync.Mutex -// snapshotDeserMu serialises every Isolate::New call. The V8 binaries we -// ship via tommie/v8go/deps assert and abort when two isolates are being +// snapshotDeserMu serialises Isolate::New and Isolate::Dispose against +// each other AND against CreateCodeCache. The V8 binaries we ship via +// tommie/v8go/deps assert and abort when two isolates are being // constructed in parallel because the shared-heap initialiser (string // table forwarding, read-only heap shrink, etc) is not thread-safe in -// the absence of v8::Locker. Holding this mutex around every isolate -// construction keeps the wrapper safe to call from any goroutine, with -// no cost outside the construction critical section. -var snapshotDeserMu sync.Mutex +// the absence of v8::Locker. +// +// RWMutex semantics: +// - NewIsolate / Dispose take Lock (exclusive writer) because they +// mutate V8 process-global shared-heap state. +// - CreateCodeCache takes RLock (shared reader) because it reads +// shared-heap objects (ReadOnlySpace roots, StringTable) that +// Dispose can tear down concurrently. +// +// Multiple CreateCodeCache calls on different isolates remain parallel; +// they only block while an isolate is being created or disposed. +var snapshotDeserMu sync.RWMutex // NewSnapshotCreator returns a new SnapshotCreator wired to the frozen // process-wide external_references registry. The very first call to this diff --git a/unbound_script.cc b/unbound_script.cc index 7a30f4a..d3265b5 100644 --- a/unbound_script.cc +++ b/unbound_script.cc @@ -12,6 +12,7 @@ using namespace v8; ScriptCompilerCachedData* UnboundScriptCreateCodeCache( Isolate* iso, UnboundScriptPtr us_ptr) { + if (!iso || !us_ptr) return nullptr; ISOLATE_SCOPE(iso); Local unbound_script = us_ptr->ptr.Get(iso); diff --git a/unbound_script.go b/unbound_script.go index dbe5ff9..da36f12 100644 --- a/unbound_script.go +++ b/unbound_script.go @@ -22,9 +22,32 @@ func (u *UnboundScript) Run(ctx *Context) (*Value, error) { return valueResult(ctx, rtn) } -// Create a code cache from the unbound script. +// CreateCodeCache serialises the compiled bytecode so it can be fed to +// a future CompileUnboundScript call (on any isolate) to skip parsing. +// +// An RLock on snapshotDeserMu is held for the duration of the CGo call +// so that Isolate.Dispose (which takes a write Lock) cannot tear down +// V8 shared-heap state while CreateCodeCache is reading it. Multiple +// concurrent CreateCodeCache calls on different isolates remain parallel. +// +// Returns nil without calling into V8 if the isolate has already been +// disposed (ptr set to nil inside Dispose's write lock). func (u *UnboundScript) CreateCodeCache() *CompilerCachedData { + if u == nil || u.iso == nil { + return nil + } + + snapshotDeserMu.RLock() + defer snapshotDeserMu.RUnlock() + + if u.iso.ptr == nil { + return nil + } + rtn := C.UnboundScriptCreateCodeCache(u.iso.ptr, u.ptr) + if rtn == nil { + return nil + } cachedData := &CompilerCachedData{ Bytes: []byte(C.GoBytes(unsafe.Pointer(rtn.data), rtn.length)),