Fix use-after-free: keep escaped handles alive when their escapable scope closes

[bkaradzic-microsoft]

[Updated by Copilot on behalf of @bghgary]

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() / what() are built on it, so reading the message of a native error on QuickJS was a heap-use-after-free. Napi::FunctionReference::Call and MakeCallback escape as well, which puts every WebSocket, setTimeout, XMLHttpRequest and AbortSignal callback on this path: instrumenting napi_escape_handle counted ~201 escapes in a single JavaScript.All run with no escape-specific test in scope.

How it was found. BabylonNative #1835 adds tests that make a native module throw. ExternalCallback::Callback calls e.what() when there is no pending QuickJS exception, walking 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
#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

Each open escapable scope gets a record on the env, keyed by a monotonic counter that is handed out as the opaque token. The escaped handle lives in that record until napi_close_escapable_handle_scope pushes it onto the handle stack, once the scope's own handles are gone; it lands at scope_start, in the parent scope, so it outlives the close.

The token is a counter rather than a position because two escapable scopes opened with no handle allocated between them occupy the same position. Keyed on that, their escaped handles collide and the second scope to escape is refused with napi_escape_called_twice having never escaped.

The handle stack is never modified in the middle, which matters: inserting at scope_start shifts every entry above it and invalidates the recorded start of any nested scope still open, reintroducing the same dangling value by a different route.

A close whose recorded start is past the end of the stack now reports napi_handle_scope_mismatch rather than resizing, which previously grew the stack with null entries for the next close to dereference. Env teardown frees handles still held for scopes that were never closed.

napi_open_handle_scope keeps its position-derived token: a position is all a regular scope needs, and its comment now says not to key per-scope state on it, which is the mistake the escapable version made.

Chakra and JavaScriptCore

Both returned the escapee without tracking scopes, so neither could report napi_escape_called_twice. Both now track open escapable scopes; values there are rooted independently of any scope, so this is the error contract only. That removes the need for JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH, so SecondEscapeIsRejected runs on every backend rather than being compiled out on two of them.

Testing

Four tests in Tests/UnitTests/Shared/Shared.cpp:

• EscapedHandleOutlivesItsScope — reproduces the original heap-use-after-free under ASan without the fix.
• NestedEscapableScopesBothEscape — fails on every run against the pre-fix implementation.
• SecondEscapeIsRejected — the napi_escape_called_twice contract.
• AdjacentEscapableScopesEscapeIndependently — two scopes with no handle allocated between them; confirmed to fail against the position-derived token and pass with the counter.

Each test closes its escapable scopes on every exit path. Leaving one open made the enclosing Napi::HandleScope fail to close, and Napi::Error::Fatal throws from a destructor that is implicitly noexcept, so a failing assertion terminated the process instead of reporting FAILED.

Verified locally at this head on Windows Release: QuickJS 10/10 and Chakra 10/10. V8, JavaScriptCore and Hermes are covered by CI.

The BabylonNative #1835 end-to-end run (clang + QuickJS + RelWithDebInfo, changing only this dependency: master gives exit 139, 1, 139; this branch gives exit 0 × 5, clean 16/16) was made against 6238b5ab, before the scope-identity change.

[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.

[bghgary]

[Commented by Copilot on behalf of @bghgary]

I pushed four commits here rather than leaving review comments. Two of them change decisions you had made, so calling those out explicitly:

• The escaped-handle map was keyed on the scope start index. Two escapable scopes opened with no handle allocated between them share that index, so their entries collided and the second scope to escape was refused with napi_escape_called_twice having never escaped. Scopes now get a record keyed by a counter, and AdjacentEscapableScopesEscapeIndependently covers it.
• Chakra and JavaScriptCore now track open escapable scopes, which deletes JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH and lets SecondEscapeIsRejected run on every backend instead of being compiled out on two.

The description is updated to match. Say if you disagree with either, particularly the second, since it widens the change past QuickJS.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Pushed to this branch rather than leaving these as asks.

The scope token came from the handle-stack position, so two escapable scopes opened with no handle between them shared a key and the second to escape was refused with napi_escape_called_twice having never escaped; scopes now get a record keyed by a counter. The tests closed their scopes only on success, turning a failing assertion into process termination with no FAILED line; they now close on every path. Chakra and JavaScriptCore track open scopes, which deletes JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH, lets SecondEscapeIsRejected run on every backend, and bounds state that previously only a matching close could remove. A non-LIFO close no longer resizes the QuickJS stack past its end.

QuickJS 10/10, Chakra 10/10 on Windows Release; the new test fails against the pre-fix code. JavaScriptCore does not build here, so its change rests on CI.

napi_open_handle_scope keeps its position-derived token. A position is all a regular scope needs, because nothing stores per-scope state against it: the token only says where the close truncates, and two closes truncating to the same index is a no-op. Giving it a counter would mean recovering the position from a lookup, so a token this code has never seen becomes an error return, and HandleScope's destructor turns any non-napi_ok into Error::Fatal — a new abort path on a scope opened per Dispatch, for no behavioural change. Its comment now records that the token is a position and must not be keyed on, which is the mistake the escapable version made.