Update to latest UrlLib/XMLHttpRequest from BabylonNative - #4
Merged
Conversation
bghgary
marked this pull request as ready for review
January 19, 2023 18:05
bkaradzic-microsoft
added a commit
that referenced
this pull request
Aug 18, 2026
…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
bkaradzic-microsoft
added a commit
that referenced
this pull request
Aug 20, 2026
### Problem
`napi_throw`, `napi_throw_error`, `napi_throw_type_error` and
`napi_throw_range_error` returned `napi_pending_exception` after
successfully scheduling the throw.
In Node-API that status means *"this call failed because an exception
was already pending"*, not *"a throw is now pending"*. The upstream
implementation returns `napi_clear_last_error(env)` (i.e. `napi_ok`).
Because the QuickJS port reported failure,
`Error::ThrowAsJavaScriptException` in `napi-inl.h` took its failure
branch on **every** native throw:
```cpp
napi_status status = napi_throw(_env, Value());
#ifdef NAPI_CPP_EXCEPTIONS
if (status != napi_ok) {
throw Error::New(_env); // consumes the exception that was just set
}
#endif
```
`Error::New(env)` calls `napi_get_and_clear_last_exception`, so the
pending JS exception is discarded and a fresh C++ exception is thrown
out of `details::WrapCallback`. `ExternalCallback::Callback` then
catches it, observes `!JS_HasException(ctx)`, and rebuilds the error
from `e.what()`.
By that point the `HandleScope` opened by `ThrowAsJavaScriptException`
has been destroyed during unwinding, so stringifying the message reads
freed memory.
### Impact
Two symptoms, both of which reproduce today:
1. **Wrong error surfaced to JS.** The real error is replaced by
`InternalError: Uncaught C++ exception: <message>`. Every native throw
on QuickJS is affected, so `err.name` and `err instanceof TypeError` are
wrong throughout.
2. **Use-after-free.** On Linux this segfaults. Backtrace from a
BabylonNative CI core dump:
```
#0 js_dup quickjs.c:1628 <-- SIGSEGV
#1 js_force_tostring quickjs.c:4813
#3 JS_ToCStringLen
#4 napi_get_value_string_utf8 js_native_api_quickjs.cc:696
#5 Napi::String::Utf8Value napi-inl.h:1118
#7 Napi::Error::Message napi-inl.h:3087
#8 Napi::Error::what napi-inl.h:3157
#9 ExternalCallback::Callback js_native_api_quickjs.cc:164
```
The `JSValue` being stringified carries `JS_TAG_STRING` with an
unaligned, freed pointer.
I instrumented the `catch` in `ExternalCallback::Callback` in a
BabylonNative QuickJS build and confirmed that **all ~50 native throws**
in that test run escaped `WrapCallback` with `hasExc=0`. After this
change the count is 0.
### Fix
Return `napi_ok` from the four throw entry points, matching upstream.
The exception stays pending, `WrapCallback` returns normally, and the
fragile `e.what()` fallback is never entered.
### Test
Added a strict assertion to the existing `URLSearchParams.set()` arity
throw, checking the error type and exact message rather than a
substring. The pre-existing `.to.throw()` test could not catch this,
because `"Uncaught C++ exception: <msg>"` still *contains* the expected
substring.
Verified on Linux QuickJS (RelWithDebInfo):
| | result |
|---|---|
| without the C++ change | `expected 'InternalError' to equal 'Error'` —
212 passing, **1 failing** |
| with the C++ change | **213 passing**, 10/10 gtest |
Also verified in a BabylonNative QuickJS build on Windows: 21/21 gtest,
49 JS assertions, exit 0, and zero escapes from `WrapCallback`.
Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.