…cope closes (#223)
[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](BabylonJS/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.
---------
Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
Copilot-Session: c26bf58d-8462-4ea4-908d-67d366b657c5
No description provided.