Keep escaped handles alive when their escapable scope closes

[bkaradzic-microsoft]

napi_escape_handle inserted the escaped handle at the scope start index so it would live in the parent scope, but napi_close_escapable_handle_scope recomputed scope_start from the token and called resize(scope_start), freeing the very handle the close was supposed to preserve. Every caller of napi_escape_handle got a dangling napi_value back.

This is reachable from ordinary code, not just direct N-API use. Napi::ObjectReference::Get uses an EscapableHandleScope, and Napi::Error::Message() / Napi::Error::what() are built on it, so reading the message of a native error on QuickJS was a heap-use-after-free.

How this was found

BabylonNative #1835 adds tests that make a native module throw. ExternalCallback::Callback calls e.what() when there is no pending QuickJS exception, which walks straight into the freed handle. Its Ubuntu_Clang_QuickJS job segfaulted while every other engine and platform passed.

#0 ToJSValue                     js_native_api_quickjs.cc:302
#1 napi_get_value_string_utf8
#3 Napi::Error::Message
#4 Napi::Error::what
#5 ExternalCallback::Callback    js_native_api_quickjs.cc:164

freed by:
#1 napi_close_escapable_handle_scope  js_native_api_quickjs.cc:1939
#2 Napi::ObjectReference::Get

The change

Escaped handles are held aside in a std::map<size_t, std::unique_ptr<JSValue>> on the env, keyed by scope start, and pushed onto the handle stack by napi_close_escapable_handle_scope once the scope's own handles are gone. They land at scope_start, in the parent scope, so they outlive the close.

The handle stack is never modified in the middle. That matters: scope tokens are derived from the stack size at open, so the original insert-at-scope_start shifted every entry above it and invalidated the recorded start of any nested scope still open — closing that scope would then keep the wrong slot and free the escaped handle, reintroducing the same dangling value. Holding handles aside removes that whole class of bug rather than compensating for it.

napi_escape_handle also now returns napi_escape_called_twice on a second call for the same scope, as Node-API requires. current_scope_start bookkeeping went away with the insert; it was written in several places and never read.

Env teardown frees handles still held aside for scopes that were never closed, which the previous stack-based version got for free.

Testing

Three tests, all in Tests/UnitTests/Shared/Shared.cpp:

• EscapedHandleOutlivesItsScope — reads the escaped value back after closing the scope and churning the parent. Reproduces the original heap-use-after-free under ASan without the fix.
• NestedEscapableScopesBothEscape — escapes from an inner and an outer scope and reads both back. Verified to fail on every run against the previous implementation.
• SecondEscapeIsRejected — covers the napi_escape_called_twice contract. Compiled out on Chakra and JavaScriptCore, whose shims implement napi_escape_handle as a pass through that cannot report it; the capability is published as an INTERFACE define on the napi target so every consumer, including the separate Android test target, picks it up.

Locally: QuickJS 9/9, V8 9/9, JavaScriptCore 8/8.

End to end, on BabylonNative #1835 with clang + QuickJS + RelWithDebInfo, changing only this dependency:

JsRuntimeHost	Result
master	exit 139, 1, 139 — segfault
this branch	exit 0 × 5 — clean, 16/16

[Copilot]

Pull request overview

Fixes a QuickJS Node-API handle-scope lifetime bug where napi_close_escapable_handle_scope could free an escaped handle, producing dangling napi_values and enabling a heap-use-after-free in common node-addon-api paths (e.g., Napi::Error::what()).

Changes:

• Track escapable scopes that have performed an escape and preserve the escaped handle when closing the escapable scope (QuickJS backend).
• Enforce the Node-API rule of at most one napi_escape_handle call per escapable scope (return napi_escape_called_twice).
• Add a regression unit test ensuring escaped handles outlive the escapable scope and that a second escape is rejected.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
Tests/UnitTests/Shared/Shared.cpp	Adds regression coverage for escaped-handle lifetime and double-escape rejection.
Core/Node-API/Source/js_native_api_quickjs.h	Adds env tracking for escapable scopes that have escaped (escaped_scope_starts).
Core/Node-API/Source/js_native_api_quickjs.cc	Preserves escaped handle on scope close; rejects double escape; simplifies insertion logic.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bkaradzic-microsoft]

Good catch, thanks. Reworked the test to report through the promise and return early, matching the convention the other Dispatch-based tests here use. As you note it also removes a real hazard: an assertion failure in the setup calls would previously have left the promise unset and hung the waiter rather than failing the test.

Also dropped the double-escape assertion in a separate commit. The JavaScriptCore shim does not implement the napi_escape_called_twice check, so it failed \Ubuntu_clang. That is a separate gap from the use-after-free fixed here, so the test now covers only the portable contract. The QuickJS implementation still returns napi_escape_called_twice, which it needs to, since a second escape would insert a second handle while close only preserves one.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Core/Node-API/Source/js_native_api_quickjs.cc:1966

• The newly added napi_escape_called_twice branch is not exercised by the regression test, which calls napi_escape_handle only once. Add a QuickJS-specific assertion that a second call for the same still-open scope returns napi_escape_called_twice and does not disturb the first escaped handle.

  // Node-API allows napi_escape_handle to be called at most once per scope.
  if (!env->escaped_scope_starts.insert(scope_start).second) {
    return napi_set_last_error(env, napi_escape_called_twice);

[bkaradzic-microsoft]

The nested-scope report was correct, and it was a real bug rather than a theoretical one. Thanks — I've reworked the fix.

What was wrong. napi_escape_handle inserted the escaped handle into handle_scope_stack at scope_start. Scope tokens are derived from the stack size at open, so that insert shifts every entry above it and silently invalidates the recorded start of any nested scope that is still open. Closing that inner scope then keeps the wrong slot and frees the escaped handle — which reproduces exactly the dangling napi_value this PR set out to fix.

How it's fixed. Rather than patching up the indices, escaped handles are now held aside in a map on the env keyed by scope start, and pushed onto the stack in napi_close_escapable_handle_scope once the scope's own handles are gone. They land at scope_start, in the parent scope, so they still outlive the close — but the stack is never modified in the middle, so no index is ever invalidated and the whole class of bug goes away. The result is a bit smaller than what it replaces. Env teardown now also frees handles still held aside for scopes that were never closed, which the stack-based version got for free.

current_scope_start was being adjusted to compensate for the shift. It turns out to be written in several places and never read anywhere in the repo, so that bookkeeping went away with the insert.

Tests. Both suggestions are in:

• NestedEscapableScopesBothEscape — escapes from an inner and an outer scope and reads both back. I verified it's a real guard: against the previous implementation it fails on every run; against this one it passes.
• SecondEscapeIsRejected — covers the napi_escape_called_twice contract.

QuickJS and V8 are both 9/9 locally.

One pre-existing limitation I'll note but haven't changed here: two scopes opened at the same stack size get identical tokens and are indistinguishable. Fixing that needs stable scope IDs instead of index-derived tokens, which is a bigger change than this PR should carry.

[bkaradzic-microsoft]

Some real-world evidence for this fix, which I ran into by accident.

BabylonNative #1835 has been failing Ubuntu_Clang_QuickJS consistently. It turns out to be this exact bug, hit through a path nobody writes by hand:

#0  js_force_tostring                       quickjs.c:4813
#3  napi_get_value_string_utf8              js_native_api_quickjs.cc:696
#6  Napi::Error::Message                    napi-inl.h:3087
#7  Napi::Error::what
#8  ExternalCallback::Callback              js_native_api_quickjs.cc:164

Napi::Error::what() reads the error's message, and ObjectReference::Get fetches properties through an escapable handle scope:

inline MaybeOrValue<Napi::Value> ObjectReference::Get(const char* utf8name) const {
  EscapableHandleScope scope(_env);
  ...
  return scope.Escape(result);
}

So any native code that throws a Napi::Error which reaches the shim's catch-all lands on the escaped handle after its scope has closed. At the crash the JSValue reads tag = -7 (string) with ptr = 0x7ff8dec9a216 — not pointer-aligned, i.e. reused memory.

That makes this reachable from ordinary Napi::Error use rather than only from addons that explicitly open escapable scopes, which I think raises the priority a bit.

A/B on the same tree, CI flags (clang, QuickJS, RelWithDebInfo, no sanitizers), only this dependency swapped:

JsRuntimeHost	Result
master	exit 139, 1, 139 — segfault
this branch	exit 0 × 5 — clean, 16/16

Worth noting the crash is intermittent in the way use-after-free usually is (139/1/139), so it reads as flaky CI rather than as a clear bug.