From aefcb5bc7b61976e2c0755f2fdb0809d6fb1bb1f Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 9 Oct 2026 14:26:18 -0300 Subject: [PATCH 1/2] test(runtime): reproduce a JS block disposed mid-teardown of its worker A worker leaves an NSBlockOperation as the only owner of a block built from a function that interop.FunctionReference registered first. The worker's ~Runtime has already dropped the isolate from the live registry when DisposeAllRegistered releases the operation, so the block's dispose takes the invalid-isolate branch and frees the BlockWrapper without the Locker, while the function's slot still points at it. The walk then reaches the function and reads the freed wrapper (heap-use-after-free in BaseDataWrapper::IsGcProtected under ASan). --- TestRunner/app/tests/BlockCacheRaceTests.js | 16 ++++++++++++++++ .../app/tests/blockTeardownReleaseWorker.js | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 TestRunner/app/tests/blockTeardownReleaseWorker.js diff --git a/TestRunner/app/tests/BlockCacheRaceTests.js b/TestRunner/app/tests/BlockCacheRaceTests.js index 2e0efac5..05fbb512 100644 --- a/TestRunner/app/tests/BlockCacheRaceTests.js +++ b/TestRunner/app/tests/BlockCacheRaceTests.js @@ -140,4 +140,20 @@ describe("JS block outliving its worker", function () { }; worker.postMessage(0); }); + + // The worker's teardown itself drops the block's last reference, while it + // still has the function left to dispose. + it("is released by the teardown that later disposes its function", function (done) { + var worker = new Worker("./blockTeardownReleaseWorker.js"); + worker.onmessage = function (msg) { + expect(msg.data).toBe("held"); + worker.terminate(); + setTimeout(done, 600); + }; + worker.onerror = function (e) { + expect(String(e && e.message ? e.message : e)).toBe(""); + done(); + }; + worker.postMessage(0); + }); }); diff --git a/TestRunner/app/tests/blockTeardownReleaseWorker.js b/TestRunner/app/tests/blockTeardownReleaseWorker.js new file mode 100644 index 00000000..c60e22a5 --- /dev/null +++ b/TestRunner/app/tests/blockTeardownReleaseWorker.js @@ -0,0 +1,16 @@ +// Leaves a native object as the only owner of a block built from a function +// that interop.FunctionReference registered first. Teardown disposes registered +// objects newest first, so it releases the block (and runs its dispose) before +// it reaches the function whose slot still points at the block's wrapper. +onmessage = function () { + var fn = function () {}; + new interop.FunctionReference(fn); + var operation = NSBlockOperation.alloc().init(); + // The pool drains the reference the marshalling call autoreleased. + TNSTestNativeCallbacks.repeatPausingAfter(1, function () { + operation.addExecutionBlock(fn); + }); + globalThis.heldFunction = fn; + globalThis.heldOperation = operation; + postMessage("held"); +}; From 0e40c4d0750402df91e1bcd2f26b6cec91ec4fe6 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 9 Oct 2026 14:38:11 -0300 Subject: [PATCH 2/2] fix(runtime): free a JS block's wrapper only once its isolate's teardown is done The JSBlock dispose helper skipped the Locker and freed the BlockWrapper as soon as IsolateWrapper::IsValid() turned false, but ~Runtime makes it false before taking the Locker. The function's slot still pointed at the wrapper, and both ~Runtime's own DisposeAllRegistered walk and JS already running on another thread under the Locker could read it after the free. A process-wide gate registry, keyed by a per-isolate id that survives InvalidateIsolate and is never reused, records for each isolate a pin count and a closed flag behind an os_unfair_lock (UnfairMutex). Runtime::Init opens the entry right after creating the isolate's Caches; ~Runtime closes it as the last step of its locked section, after every walk that reads wrappers through JS objects, and keeps its existing ordering otherwise. DisposeIsolateWhenPossible defers, through its existing 10 ms re-post, while the entry is pinned, and retires it before Isolate::Dispose, so a pinned thread never waits on or holds the Locker of a disposed isolate. IsolateWrapper keeps only the isolate and that id, staying trivially copyable for the ObjC blocks that capture it by value. The dispose helper pins, takes the Locker and clears the slot unless the gate closed meanwhile; only a refused pin lets it free without the Locker. ArgConverter::MethodCallback pins across its Locker too, keeping its IsValid checks, which closes the window where a callback waiting on the Locker could outlive Isolate::Dispose. Both skip the pin on a thread that has already entered the isolate: an entered isolate stays in use, which already defers its disposal. --- NativeScript/runtime/ArgConverter.mm | 14 ++-- NativeScript/runtime/Caches.cpp | 3 +- NativeScript/runtime/Caches.h | 6 ++ NativeScript/runtime/Interop.mm | 51 ++++++++------- NativeScript/runtime/IsolateWrapper.cpp | 85 ++++++++++++++++++++++++- NativeScript/runtime/IsolateWrapper.h | 76 ++++++++++++++++++---- NativeScript/runtime/Runtime.mm | 31 ++++++--- 7 files changed, 214 insertions(+), 52 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index bae55621..80263ab9 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -293,11 +293,6 @@ Isolate* isolate = data->isolateWrapper_.Isolate(); - if (!data->isolateWrapper_.IsValid()) { - memset(retValue, 0, cif->rtype->size); - return; - } - // Declared before all V8 scopes: an ObjC exception must never unwind through a // live V8 scope (Locker/HandleScope/Context::Scope). A branded escape caught // below is captured here and @thrown only after the inner block closes every @@ -305,6 +300,15 @@ NSException* __strong pendingThrow = nil; { + // Outlives the Locker: a runtime torn down while this thread waits for the + // lock cannot dispose the isolate before the wait is over. Free when this + // thread has already entered the isolate. + IsolatePin pin = data->isolateWrapper_.Pin(); + if (!pin || !data->isolateWrapper_.IsValid()) { + memset(retValue, 0, cif->rtype->size); + return; + } + v8::Locker locker(isolate); // Checked again with the isolate locked: a runtime being destroyed holds // this lock while it removes its caches, so the check above can pass and diff --git a/NativeScript/runtime/Caches.cpp b/NativeScript/runtime/Caches.cpp index 13c86574..868c1b3b 100644 --- a/NativeScript/runtime/Caches.cpp +++ b/NativeScript/runtime/Caches.cpp @@ -12,7 +12,8 @@ namespace tns { Caches::Caches(Isolate* isolate, const int& isolateId) : PromiseRejections(std::make_unique(isolate)), isolate_(isolate), - isolateId_(isolateId) {} + isolateId_(isolateId), + gateId_(isolateId) {} Caches::~Caches() { // Subsystem state may hold v8 handles and reference the core caches below; diff --git a/NativeScript/runtime/Caches.h b/NativeScript/runtime/Caches.h index c7f497a5..01d0b642 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -117,6 +117,11 @@ class Caches { inline int getIsolateId() { return isolateId_; } + // The id assigned at Init; unlike getIsolateId() it survives + // InvalidateIsolate, so it still names this isolate's gate during teardown. + // -1 for the stand-in Get() returns once the isolate's Caches is gone. + inline int getGateId() const { return gateId_; } + inline void InvalidateIsolate() { isolateId_ = -1; } inline bool IsValid() { return isolateId_ != -1; } @@ -317,6 +322,7 @@ class Caches { v8::Isolate* isolate_; std::shared_ptr> context_; int isolateId_; + const int gateId_; }; } // namespace tns diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 9cb484cf..998742e9 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -44,30 +44,36 @@ // resetting it never touches the finalizer drain's bookkeeping, // and a foreign-thread Locker into the block's own isolate is // legitimate now that extended class names are worker-scoped. - if (wrapper->isolateWrapper_.IsValid()) { - Isolate* isolate = wrapper->isolateWrapper_.Isolate(); - v8::Locker locker(isolate); - // Re-checked under the lock: ~Runtime holds it while it tears the - // isolate's caches down, so the check above can predate that. - if (wrapper->isolateWrapper_.IsValid()) { - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - Local callback = wrapper->callback_->Get(isolate); - if (!callback.IsEmpty() && callback->IsObject()) { - // The slot may hold another wrapper by now; only our own is - // cleared from it. - if (tns::GetValue(isolate, callback) == blockWrapper) { - tns::DeleteValue(isolate, callback); + // + // Validity is not the test here: the slot stays reachable after + // the isolate is invalidated, by JS still running under the Locker + // and by the teardown's own walk of registered objects. Until the + // gate closes, the wrapper is only freed once the slot is cleared. + { + IsolatePin pin = wrapper->isolateWrapper_.Pin(); + if (pin) { + Isolate* isolate = wrapper->isolateWrapper_.Isolate(); + v8::Locker locker(isolate); + if (!wrapper->isolateWrapper_.IsTornDown()) { + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + Local callback = wrapper->callback_->Get(isolate); + if (!callback.IsEmpty() && callback->IsObject()) { + // The slot may hold another wrapper by now; only our own is + // cleared from it. + if (tns::GetValue(isolate, callback) == blockWrapper) { + tns::DeleteValue(isolate, callback); + } } + // Unconditional: an already-detached callback still owns its + // node, and dropping the persistent without a reset would leave + // that node rooted forever. + wrapper->callback_->Reset(); } - // Unconditional: an already-detached callback still owns its - // node, and dropping the persistent without a reset would leave - // that node rooted forever. - wrapper->callback_->Reset(); } } - // Outside the isolate guard: once the isolate is gone the cache - // slot is unreachable and nothing else can free the wrapper. + // Outside the gate: once the teardown is done the cache slot is + // unreachable and nothing else can free the wrapper. delete blockWrapper; delete wrapper; ffi_closure_free(block->ffiClosure); @@ -570,8 +576,9 @@ inline bool isBool() { blockPtr = CFAutorelease(Block_copy(wrapper->Block())); } else if (TryRetainJSBlock(static_cast(wrapper->Block()))) { // Reading the block is safe even when its last release raced ahead: - // while the isolate is valid, dispose clears this slot under the - // Locker this thread holds, before libclosure frees the block. + // until the isolate's teardown closes its gate, dispose clears this + // slot under the Locker this thread holds, before libclosure frees + // the block. blockPtr = CFAutorelease(wrapper->Block()); } } diff --git a/NativeScript/runtime/IsolateWrapper.cpp b/NativeScript/runtime/IsolateWrapper.cpp index e076907c..a7679221 100644 --- a/NativeScript/runtime/IsolateWrapper.cpp +++ b/NativeScript/runtime/IsolateWrapper.cpp @@ -7,12 +7,95 @@ // #include "IsolateWrapper.h" + +#include +#include + #include "Runtime.h" +#include "UnfairLock.h" +#include "robin_hood.h" namespace tns { bool IsolateWrapper::IsValid() const { - return Runtime::IsAlive(isolate_) && isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) != nullptr && GetCache()->getIsolateId() == isolateId_; + if (!Runtime::IsAlive(isolate_) || + isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) == nullptr) { + return false; + } + std::shared_ptr cache = GetCache(); + return cache->IsValid() && cache->getGateId() == gateId_; +} + +namespace IsolateGates { + +namespace { + +struct Gate { + uint32_t pins = 0; + bool closed = false; +}; + +// Trivially destructible, so exit() leaves it usable. +static UnfairMutex gatesMutex; + +// Never destroyed: exit() runs static destructors while other threads may +// still be pinning. +robin_hood::unordered_map& Gates() { + static auto* gates = new robin_hood::unordered_map(); + return *gates; +} + +} // namespace + +void Open(int id) { + std::lock_guard lock(gatesMutex); + Gates()[id] = Gate(); +} + +bool TryPin(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it == Gates().end() || it->second.closed) { + return false; + } + it->second.pins++; + return true; +} + +void Unpin(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it != Gates().end()) { + it->second.pins--; + } +} + +void Close(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it != Gates().end()) { + it->second.closed = true; + } +} + +bool IsClosed(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + return it == Gates().end() || it->second.closed; +} + +bool RetireIfUnpinned(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it == Gates().end()) { + return true; + } + if (it->second.pins > 0) { + return false; + } + Gates().erase(it); + return true; } +} // namespace IsolateGates } diff --git a/NativeScript/runtime/IsolateWrapper.h b/NativeScript/runtime/IsolateWrapper.h index 0eb56639..96407565 100644 --- a/NativeScript/runtime/IsolateWrapper.h +++ b/NativeScript/runtime/IsolateWrapper.h @@ -9,36 +9,86 @@ #ifndef IsolateWrapper_h #define IsolateWrapper_h -#include "v8.h" +#include + #include "Caches.h" #include "Constants.h" +#include "v8.h" namespace tns { +// One gate per isolate, keyed by Caches::getGateId() (ids are never reused). +// A thread that has not entered the isolate pins its gate before waiting for +// or holding the isolate's Locker. A pin taken before Close() defers +// Isolate::Dispose until it is released. Once the teardown has closed the +// gate, no wrapper can be reached from JS any more and pins are refused. A +// missing entry reads as closed. +namespace IsolateGates { +void Open(int id); +bool TryPin(int id); +void Unpin(int id); +// Called by the teardown while it holds the isolate's Locker, after the last +// walk that reads wrappers through JS objects. +void Close(int id); +bool IsClosed(int id); +// Drops the entry unless a pin is held; the isolate may be disposed only once +// this returns true. +bool RetireIfUnpinned(int id); +} // namespace IsolateGates + +// Owns a pin for its scope. Declare it before the Locker it guards, so the +// Locker is released first. A thread that has entered the isolate takes no +// pin: an entered isolate stays in use, which already defers its disposal. +class IsolatePin { + public: + IsolatePin(int id, bool entered) + : id_(id), + pinned_(!entered && IsolateGates::TryPin(id)), + usable_(entered || pinned_) {} + ~IsolatePin() { + if (pinned_) { + IsolateGates::Unpin(id_); + } + } + IsolatePin(const IsolatePin&) = delete; + IsolatePin& operator=(const IsolatePin&) = delete; + explicit operator bool() const { return usable_; } + + private: + int id_; + bool pinned_; + bool usable_; +}; + +// Kept trivially copyable: ObjC blocks that live as long as the process (the +// extended classes' synthesized methods) capture it by value. class IsolateWrapper { public: bool IsValid() const; inline std::shared_ptr GetCache() const { return tns::Caches::Get(isolate_); } - inline v8::Isolate* Isolate() { - return isolate_; - } - inline int IsolateId() { - return isolateId_; - } + inline v8::Isolate* Isolate() { return isolate_; } inline IsolateWrapper(v8::Isolate* isolate) { isolate_ = isolate; - isolateId_ = tns::Caches::Get(isolate_)->getIsolateId(); + gateId_ = tns::Caches::Get(isolate_)->getGateId(); + } + // Hold the returned pin across any wait for the isolate's Locker from a + // thread that is not already inside the isolate; an empty pin means the + // isolate may already be disposed and must not be touched at all. + inline IsolatePin Pin() const { + return IsolatePin(gateId_, v8::Isolate::TryGetCurrent() == isolate_); } - -private: + // True once the teardown has finished with every wrapper reachable from + // JS. Read it while pinned. + inline bool IsTornDown() const { return IsolateGates::IsClosed(gateId_); } + + private: v8::Isolate* isolate_; - int isolateId_; - - + int gateId_; }; +static_assert(std::is_trivially_copyable_v); } #endif /* IsolateWrapper_h */ diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm index 216fb710..a358a46f 100644 --- a/NativeScript/runtime/Runtime.mm +++ b/NativeScript/runtime/Runtime.mm @@ -193,15 +193,18 @@ static void InitializeImportMetaObject(Local context, Local mod // CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, // kCFTimeZoneSystemTimeZoneDidChangeNotification, NULL); -void DisposeIsolateWhenPossible(Isolate* isolate) { - // Disposal is deferred only while the isolate is still entered, which happens +void DisposeIsolateWhenPossible(Isolate* isolate, int gateId) { + // Disposal is deferred while the isolate is still entered, which happens // when the runtime is deleted by code running inside its own isolate: an - // embedder calling shutdownRuntime from a JS callback. exit() never deletes a - // runtime, so a process that is dying leaves its isolates to the OS. - if (isolate->IsInUse()) { + // embedder calling shutdownRuntime from a JS callback. It is also deferred + // while another thread holds a pin to wait for or use the isolate's Locker. + // Never block here instead: the pin holder may be waiting for a Locker this + // thread still holds further up the stack. exit() never deletes a runtime, + // so a process that is dying leaves its isolates to the OS. + if (isolate->IsInUse() || !IsolateGates::RetireIfUnpinned(gateId)) { dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(10.0 * NSEC_PER_MSEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_LOW, 0), ^{ - DisposeIsolateWhenPossible(isolate); + DisposeIsolateWhenPossible(isolate, gateId); }); } else { isolate->Dispose(); @@ -234,10 +237,11 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { NativeScriptException::OnIsolateTeardown(isolate_); auto currentIsolate = this->isolate_; + int gateId = Caches::Get(this->isolate_)->getGateId(); { - // make sure we remove the isolate from the list of active isolates first - // this will make sure isAlive(isolate) will return false and prevent locking of the v8 isolate - // after it terminates execution + // First, so IsAlive turns false: native callbacks arriving from here on + // bail before taking the Locker. A block's dispose still pins and locks + // until the gate closes below. SpinLock lock(isolatesMutex_); Runtime::isolates_.erase( std::remove(Runtime::isolates_.begin(), Runtime::isolates_.end(), this->isolate_), @@ -299,9 +303,14 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { Caches::Remove(this->isolate_); this->isolate_->SetData(Constants::RUNTIME_SLOT, nullptr); + + // Last, and under the Locker: from here on nothing reads a wrapper through + // a JS object, so threads that never pinned may free theirs without the + // lock, and threads already pinned see the gate closed once they get it. + IsolateGates::Close(gateId); } - DisposeIsolateWhenPossible(this->isolate_); + DisposeIsolateWhenPossible(this->isolate_, gateId); // Matched erase: only removes the registry entry while it still maps to // this runtime's loop, so a worker isolate that reuses this pointer after @@ -384,6 +393,8 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { void Runtime::Init(Isolate* isolate, bool isWorker) { std::shared_ptr cache = Caches::Init(isolate, nextIsolateId.fetch_add(1, std::memory_order_relaxed)); + // Before anything can build an IsolateWrapper for this isolate. + IsolateGates::Open(cache->getGateId()); cache->isWorker = isWorker; cache->ObjectCtorInitializer = MetadataBuilder::GetOrCreateConstructorFunctionTemplate; cache->StructPrototypeInitializer = MetadataBuilder::GetOrCreateStructPrototype;