From 111efc509a239ff00b7c5fade8e2fb8334dc7534 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 01:03:38 -0700 Subject: [PATCH 1/7] Keep escaped handles alive when their escapable scope closes napi_escape_handle inserts the escaped handle at the scope start index so that it lives in the parent scope, but napi_close_escapable_handle_scope freed and resized the handle stack back to that same index. The escaped handle was therefore destroyed by the very close it was supposed to survive, so every consumer of napi_escape_handle got a dangling napi_value back. This is reachable from ordinary code: Napi::ObjectReference::Get uses an EscapableHandleScope, and Napi::Error::Message and Napi::Error::what are built on it. Reporting the message of a native error was therefore a heap-use-after-free, which is how this was found. Track which scopes have had a handle escaped and keep that one entry when the scope closes. Escaping now also reports napi_escape_called_twice on a second call, which Node-API requires and which the previous implementation silently allowed. The escape path no longer special cases scope_start == 0: inserting at begin() + 0 is already the correct behavior for that case. Adds NodeApi.EscapedHandleOutlivesItsScope, which reads the escaped value back after closing the scope and churning the parent scope. Under ASan it fails with heap-use-after-free before this change and passes after. --- Core/Node-API/Source/js_native_api_quickjs.cc | 55 ++++++++++--------- Core/Node-API/Source/js_native_api_quickjs.h | 7 +++ Tests/UnitTests/Shared/Shared.cpp | 49 +++++++++++++++++ 3 files changed, 84 insertions(+), 27 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index c9e2823d..4a025afe 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1929,15 +1929,24 @@ napi_status napi_close_escapable_handle_scope(napi_env env, napi_escapable_handl CHECK_ENV(env); CHECK_ARG(env, scope); - // Same cleanup as regular handle scope size_t scope_start = reinterpret_cast(scope) - 1; - for (size_t i = scope_start; i < env->handle_scope_stack.size(); i++) { + // If napi_escape_handle was called on this scope, the escaped handle was inserted at + // scope_start so that it belongs to the parent scope. It has to outlive this close, + // so start freeing after it. + const auto escaped = env->escaped_scope_starts.find(scope_start); + const size_t keep = (escaped != env->escaped_scope_starts.end()) ? 1 : 0; + if (keep != 0) { + env->escaped_scope_starts.erase(escaped); + } + + const size_t first_owned = scope_start + keep; + for (size_t i = first_owned; i < env->handle_scope_stack.size(); i++) { JS_FreeValue(env->context, *env->handle_scope_stack[i]); } - env->handle_scope_stack.resize(scope_start); - env->current_scope_start = scope_start; + env->handle_scope_stack.resize(first_owned); + env->current_scope_start = first_owned; napi_clear_last_error(env); return napi_ok; @@ -1952,37 +1961,29 @@ napi_status napi_escape_handle(napi_env env, napi_escapable_handle_scope scope, // Get the scope start index size_t scope_start = reinterpret_cast(scope) - 1; + // 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); + } + // Duplicate the JSValue to create a new handle that will outlive the current scope JSValue jsValue = ToJSValue(escapee); JSValue escapedValue = JS_DupValue(env->context, jsValue); - // Store the escaped value in the parent scope (before scope_start) + // Store the escaped value in the parent scope (before scope_start). The matching + // napi_close_escapable_handle_scope keeps this entry alive. auto parentPtr = std::make_unique(escapedValue); napi_value parentHandle = reinterpret_cast(parentPtr.get()); // Insert at parent scope position (before current scope) - if (scope_start > 0) { - env->handle_scope_stack.insert( - env->handle_scope_stack.begin() + scope_start, - std::move(parentPtr) - ); - - // Note: Inserting shifts indices, but since we're inserting at scope_start, - // the current scope's start index is now scope_start + 1 - // We need to update current_scope_start if it was pointing to this scope - if (env->current_scope_start == scope_start) { - env->current_scope_start = scope_start + 1; - } - } else { - // No parent scope - just add to the beginning - env->handle_scope_stack.insert( - env->handle_scope_stack.begin(), - std::move(parentPtr) - ); - - if (env->current_scope_start == 0) { - env->current_scope_start = 1; - } + env->handle_scope_stack.insert( + env->handle_scope_stack.begin() + scope_start, + std::move(parentPtr) + ); + + // Inserting at scope_start shifts this scope's own handles up by one. + if (env->current_scope_start == scope_start) { + env->current_scope_start = scope_start + 1; } *result = parentHandle; diff --git a/Core/Node-API/Source/js_native_api_quickjs.h b/Core/Node-API/Source/js_native_api_quickjs.h index 7b84fe12..8e3dfb5d 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.h +++ b/Core/Node-API/Source/js_native_api_quickjs.h @@ -12,6 +12,7 @@ #include #include #include +#include #include // Reference info for preventing GC. Defined in the header so that both @@ -33,6 +34,12 @@ struct napi_env__ { std::vector> handle_scope_stack; size_t current_scope_start = 0; + // Scope starts (as recorded by napi_open_escapable_handle_scope) that have had + // napi_escape_handle called on them. The escaped handle is inserted at the scope + // start so that it lives in the parent scope, so closing the scope has to keep it + // rather than free it along with the scope's own handles. + std::set escaped_scope_starts; + // Tracks every RefInfo* created by napi_create_reference so that // pending strong references can be released during Detach. Without // this, any napi_ref held by a native object (e.g. a polyfill's diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a920fa1f..ac4467b5 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -401,6 +401,55 @@ TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) EXPECT_TRUE(zeroSafe.get_future().get()); EXPECT_TRUE(normalWorks.get_future().get()); } + +// Regression: a handle returned by napi_escape_handle must stay alive after its +// escapable scope is closed. The escaped handle is stored in the parent scope, so +// closing the scope must not free it along with the scope's own handles. This is the +// contract Napi::ObjectReference::Get relies on, which in turn is what +// Napi::Error::Message and Napi::Error::what use, so getting it wrong turns any +// report of a native error message into a use-after-free. +TEST(NodeApi, EscapedHandleOutlivesItsScope) +{ + Babylon::AppRuntime runtime{}; + + std::promise escapedValueIsIntact; + std::promise secondEscapeIsRejected; + + runtime.Dispatch([&escapedValueIsIntact, &secondEscapeIsRejected](Napi::Env env) mutable { + napi_env nenv{env}; + + napi_escapable_handle_scope scope{}; + EXPECT_EQ(napi_open_escapable_handle_scope(nenv, &scope), napi_ok); + + napi_value inner{}; + EXPECT_EQ(napi_create_string_utf8(nenv, "escape me", NAPI_AUTO_LENGTH, &inner), napi_ok); + + napi_value escaped{}; + EXPECT_EQ(napi_escape_handle(nenv, scope, inner, &escaped), napi_ok); + + // Node-API allows at most one escape per scope. + napi_value second{}; + secondEscapeIsRejected.set_value(napi_escape_handle(nenv, scope, inner, &second) == napi_escape_called_twice); + + EXPECT_EQ(napi_close_escapable_handle_scope(nenv, scope), napi_ok); + + // Allocate through the parent scope so a dangling escaped handle is likely to + // have been reused by the time it is read back. + for (int i = 0; i < 32; ++i) + { + napi_value filler{}; + napi_create_string_utf8(nenv, "filler filler filler", NAPI_AUTO_LENGTH, &filler); + } + + char buffer[32]{}; + size_t copied{0}; + const napi_status status{napi_get_value_string_utf8(nenv, escaped, buffer, sizeof(buffer), &copied)}; + escapedValueIsIntact.set_value(status == napi_ok && copied == 9 && std::string{buffer} == "escape me"); + }); + + EXPECT_TRUE(escapedValueIsIntact.get_future().get()); + EXPECT_TRUE(secondEscapeIsRejected.get_future().get()); +} #endif int RunTests() From a53b3385415164a838ac5bcb2d65d7454e2c0110 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 07:25:41 -0700 Subject: [PATCH 2/7] Drop the double-escape assertion from the shared test The JavaScriptCore shim does not implement the napi_escape_called_twice check, so asserting it in the cross-engine test fails there. That gap is a separate issue from the use-after-free this change fixes, so the test now only covers the portable contract: an escaped handle must still be readable after its scope closes. --- Tests/UnitTests/Shared/Shared.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index ac4467b5..58901687 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -413,9 +413,8 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) Babylon::AppRuntime runtime{}; std::promise escapedValueIsIntact; - std::promise secondEscapeIsRejected; - runtime.Dispatch([&escapedValueIsIntact, &secondEscapeIsRejected](Napi::Env env) mutable { + runtime.Dispatch([&escapedValueIsIntact](Napi::Env env) mutable { napi_env nenv{env}; napi_escapable_handle_scope scope{}; @@ -427,10 +426,6 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) napi_value escaped{}; EXPECT_EQ(napi_escape_handle(nenv, scope, inner, &escaped), napi_ok); - // Node-API allows at most one escape per scope. - napi_value second{}; - secondEscapeIsRejected.set_value(napi_escape_handle(nenv, scope, inner, &second) == napi_escape_called_twice); - EXPECT_EQ(napi_close_escapable_handle_scope(nenv, scope), napi_ok); // Allocate through the parent scope so a dangling escaped handle is likely to @@ -448,7 +443,6 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) }); EXPECT_TRUE(escapedValueIsIntact.get_future().get()); - EXPECT_TRUE(secondEscapeIsRejected.get_future().get()); } #endif From 9af53078c6b5d6656b5a31450a041f60a12fd918 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 07:27:37 -0700 Subject: [PATCH 3/7] Keep gtest assertions on the test thread in the escape test Match the convention the other Dispatch-based tests in this file use. Reporting through the promise and returning early also means a failure in the setup calls can no longer leave the promise unset and hang the waiter. --- Tests/UnitTests/Shared/Shared.cpp | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 58901687..712e2ee5 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -417,16 +417,34 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) runtime.Dispatch([&escapedValueIsIntact](Napi::Env env) mutable { napi_env nenv{env}; + // Assertions stay on the test thread: the dispatched lambda reports through the + // promise and returns early on failure so the waiter can never deadlock. napi_escapable_handle_scope scope{}; - EXPECT_EQ(napi_open_escapable_handle_scope(nenv, &scope), napi_ok); + if (napi_open_escapable_handle_scope(nenv, &scope) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } napi_value inner{}; - EXPECT_EQ(napi_create_string_utf8(nenv, "escape me", NAPI_AUTO_LENGTH, &inner), napi_ok); + if (napi_create_string_utf8(nenv, "escape me", NAPI_AUTO_LENGTH, &inner) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } napi_value escaped{}; - EXPECT_EQ(napi_escape_handle(nenv, scope, inner, &escaped), napi_ok); + if (napi_escape_handle(nenv, scope, inner, &escaped) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } - EXPECT_EQ(napi_close_escapable_handle_scope(nenv, scope), napi_ok); + if (napi_close_escapable_handle_scope(nenv, scope) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } // Allocate through the parent scope so a dangling escaped handle is likely to // have been reused by the time it is read back. From c7e211de37663fcae0d306bb00efe385ec66572e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 12:31:49 -0700 Subject: [PATCH 4/7] Hold escaped handles aside instead of inserting them mid-stack napi_escape_handle inserted the escaped handle into handle_scope_stack at scope_start so it would belong to the parent scope. That insert shifts every entry above it, and scope tokens are built from the stack size at open, so any nested scope still open at that point ends up with a stale recorded start. Closing it then keeps the wrong slot and frees the escaped handle, which reproduces the dangling napi_value this change set set out to fix. Hold the escaped handle in a map on the env instead, keyed by scope start, and push it onto the stack in napi_close_escapable_handle_scope once the scope's own handles are gone. It lands at scope_start, in the parent scope, so it still outlives the close, but the stack is never modified in the middle and no index is ever invalidated. Env teardown frees any handles still held aside for scopes that were never closed. current_scope_start was being adjusted to compensate for the shift; it is only ever written, never read, so that bookkeeping goes away with the insert. Add NestedEscapableScopesBothEscape, which escapes from both an inner and an outer scope and reads both back, and SecondEscapeIsRejected, which covers the napi_escape_called_twice contract. The nested test fails against the previous implementation on every run and passes with this one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- Core/Node-API/Source/env_quickjs.cc | 8 + Core/Node-API/Source/js_native_api_quickjs.cc | 53 +++--- Core/Node-API/Source/js_native_api_quickjs.h | 15 +- Tests/UnitTests/Shared/Shared.cpp | 173 ++++++++++++++++++ 4 files changed, 212 insertions(+), 37 deletions(-) diff --git a/Core/Node-API/Source/env_quickjs.cc b/Core/Node-API/Source/env_quickjs.cc index 382fe86c..6c02bd74 100644 --- a/Core/Node-API/Source/env_quickjs.cc +++ b/Core/Node-API/Source/env_quickjs.cc @@ -114,6 +114,14 @@ namespace Napi } env_ptr->handle_scope_stack.clear(); + // Handles escaped from scopes that were never closed are held aside + // rather than on the stack, so free them here too. + for (auto& entry : env_ptr->escaped_handles) + { + JS_FreeValue(env_ptr->context, *entry.second); + } + env_ptr->escaped_handles.clear(); + // Run the cycle collector so napi_wrap finalizers (which // destroy C++ wrapper objects and release any embedded // napi_refs) get a chance to execute while the env is still diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index 4a025afe..c2848398 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1931,22 +1931,23 @@ napi_status napi_close_escapable_handle_scope(napi_env env, napi_escapable_handl size_t scope_start = reinterpret_cast(scope) - 1; - // If napi_escape_handle was called on this scope, the escaped handle was inserted at - // scope_start so that it belongs to the parent scope. It has to outlive this close, - // so start freeing after it. - const auto escaped = env->escaped_scope_starts.find(scope_start); - const size_t keep = (escaped != env->escaped_scope_starts.end()) ? 1 : 0; - if (keep != 0) { - env->escaped_scope_starts.erase(escaped); + for (size_t i = scope_start; i < env->handle_scope_stack.size(); i++) { + JS_FreeValue(env->context, *env->handle_scope_stack[i]); } - const size_t first_owned = scope_start + keep; - for (size_t i = first_owned; i < env->handle_scope_stack.size(); i++) { - JS_FreeValue(env->context, *env->handle_scope_stack[i]); + env->handle_scope_stack.resize(scope_start); + + // The escaped handle, if any, was held aside by napi_escape_handle rather than + // stored on the stack. Now that this scope's own handles are gone it can be + // pushed on: it lands at scope_start, which belongs to the parent scope, so it + // outlives this close and is freed when the parent closes. + const auto escaped = env->escaped_handles.find(scope_start); + if (escaped != env->escaped_handles.end()) { + env->handle_scope_stack.push_back(std::move(escaped->second)); + env->escaped_handles.erase(escaped); } - env->handle_scope_stack.resize(first_owned); - env->current_scope_start = first_owned; + env->current_scope_start = scope_start; napi_clear_last_error(env); return napi_ok; @@ -1962,31 +1963,21 @@ napi_status napi_escape_handle(napi_env env, napi_escapable_handle_scope scope, size_t scope_start = reinterpret_cast(scope) - 1; // Node-API allows napi_escape_handle to be called at most once per scope. - if (!env->escaped_scope_starts.insert(scope_start).second) { + if (env->escaped_handles.find(scope_start) != env->escaped_handles.end()) { return napi_set_last_error(env, napi_escape_called_twice); } // Duplicate the JSValue to create a new handle that will outlive the current scope - JSValue jsValue = ToJSValue(escapee); - JSValue escapedValue = JS_DupValue(env->context, jsValue); - - // Store the escaped value in the parent scope (before scope_start). The matching - // napi_close_escapable_handle_scope keeps this entry alive. - auto parentPtr = std::make_unique(escapedValue); - napi_value parentHandle = reinterpret_cast(parentPtr.get()); + JSValue escapedValue = JS_DupValue(env->context, ToJSValue(escapee)); - // Insert at parent scope position (before current scope) - env->handle_scope_stack.insert( - env->handle_scope_stack.begin() + scope_start, - std::move(parentPtr) - ); - - // Inserting at scope_start shifts this scope's own handles up by one. - if (env->current_scope_start == scope_start) { - env->current_scope_start = scope_start + 1; - } + // Hold the handle aside until the scope closes, rather than inserting it into + // handle_scope_stack here. An insert would shift every entry above scope_start, + // which silently invalidates the recorded start of any nested scope that is still + // open -- closing that scope would then keep the wrong slot and free this handle. + auto holder = std::make_unique(escapedValue); + *result = reinterpret_cast(holder.get()); + env->escaped_handles.emplace(scope_start, std::move(holder)); - *result = parentHandle; napi_clear_last_error(env); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_quickjs.h b/Core/Node-API/Source/js_native_api_quickjs.h index 8e3dfb5d..db0325a4 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.h +++ b/Core/Node-API/Source/js_native_api_quickjs.h @@ -12,7 +12,7 @@ #include #include #include -#include +#include #include // Reference info for preventing GC. Defined in the header so that both @@ -34,11 +34,14 @@ struct napi_env__ { std::vector> handle_scope_stack; size_t current_scope_start = 0; - // Scope starts (as recorded by napi_open_escapable_handle_scope) that have had - // napi_escape_handle called on them. The escaped handle is inserted at the scope - // start so that it lives in the parent scope, so closing the scope has to keep it - // rather than free it along with the scope's own handles. - std::set escaped_scope_starts; + // Handles escaped by napi_escape_handle, keyed by the scope start recorded by + // napi_open_escapable_handle_scope. They are deliberately held aside rather than + // put on handle_scope_stack: inserting into the middle of the stack would shift + // every entry above it, invalidating the indices that already-open nested scopes + // and their opaque tokens are built from. napi_close_escapable_handle_scope + // pushes the handle onto the stack once the scope's own handles are gone, at + // which point it lands in the parent scope and is freed with it. + std::map> escaped_handles; // Tracks every RefInfo* created by napi_create_reference so that // pending strong references can be released during Detach. Without diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 712e2ee5..e54ce850 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -462,6 +462,179 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) EXPECT_TRUE(escapedValueIsIntact.get_future().get()); } + +// Regression: two escapable scopes open at once, both escaping before either closes, +// then closed innermost first. An implementation that stores an escaped handle by +// inserting it into the middle of the handle stack shifts every entry above it, +// silently invalidating the start index the still-open inner scope was handed. Closing +// the inner scope then keeps the wrong slot and frees the inner escaped handle, +// reintroducing the dangling napi_value this fix is about. +TEST(NodeApi, NestedEscapableScopesBothEscape) +{ + Babylon::AppRuntime runtime{}; + + std::promise bothValuesIntact; + + runtime.Dispatch([&bothValuesIntact](Napi::Env env) mutable { + napi_env nenv{env}; + + const auto fail = [&bothValuesIntact]() { bothValuesIntact.set_value(false); }; + + napi_escapable_handle_scope outerScope{}; + if (napi_open_escapable_handle_scope(nenv, &outerScope) != napi_ok) + { + return fail(); + } + + // Give the outer scope handles of its own, so the inner scope starts at a + // different index and the shifting bug is observable. + for (int i = 0; i < 4; ++i) + { + napi_value outerFiller{}; + if (napi_create_string_utf8(nenv, "outer filler", NAPI_AUTO_LENGTH, &outerFiller) != napi_ok) + { + return fail(); + } + } + + napi_value outerSource{}; + if (napi_create_string_utf8(nenv, "outer value", NAPI_AUTO_LENGTH, &outerSource) != napi_ok) + { + return fail(); + } + + napi_escapable_handle_scope innerScope{}; + if (napi_open_escapable_handle_scope(nenv, &innerScope) != napi_ok) + { + return fail(); + } + + napi_value innerSource{}; + if (napi_create_string_utf8(nenv, "inner value", NAPI_AUTO_LENGTH, &innerSource) != napi_ok) + { + return fail(); + } + + // Inner escapes first, then the still-open outer scope escapes. + napi_value innerEscaped{}; + if (napi_escape_handle(nenv, innerScope, innerSource, &innerEscaped) != napi_ok) + { + return fail(); + } + + napi_value outerEscaped{}; + if (napi_escape_handle(nenv, outerScope, outerSource, &outerEscaped) != napi_ok) + { + return fail(); + } + + // Close innermost first, as the scopes must be. + if (napi_close_escapable_handle_scope(nenv, innerScope) != napi_ok) + { + return fail(); + } + + // The inner escaped handle now belongs to the outer scope and must still read + // back while that scope is open. Churn allocations first: a wrongly freed handle + // only reads back wrong once its block has been reused, so allocate enough to + // make that near certain rather than a matter of luck. + for (int i = 0; i < 512; ++i) + { + napi_value filler{}; + napi_create_string_utf8(nenv, "filler filler filler", NAPI_AUTO_LENGTH, &filler); + } + + char innerBuffer[32]{}; + size_t innerCopied{0}; + if (napi_get_value_string_utf8(nenv, innerEscaped, innerBuffer, sizeof(innerBuffer), &innerCopied) != napi_ok || + std::string{innerBuffer} != "inner value") + { + return fail(); + } + + if (napi_close_escapable_handle_scope(nenv, outerScope) != napi_ok) + { + return fail(); + } + + for (int i = 0; i < 512; ++i) + { + napi_value filler{}; + napi_create_string_utf8(nenv, "filler filler filler", NAPI_AUTO_LENGTH, &filler); + } + + char outerBuffer[32]{}; + size_t outerCopied{0}; + const napi_status status{napi_get_value_string_utf8(nenv, outerEscaped, outerBuffer, sizeof(outerBuffer), &outerCopied)}; + bothValuesIntact.set_value(status == napi_ok && std::string{outerBuffer} == "outer value"); + }); + + EXPECT_TRUE(bothValuesIntact.get_future().get()); +} + +// Node-API permits at most one escape per escapable scope. The second call must be +// rejected with napi_escape_called_twice, and must leave the first escaped handle +// untouched rather than replacing or freeing it. +TEST(NodeApi, SecondEscapeIsRejected) +{ + Babylon::AppRuntime runtime{}; + + std::promise secondEscapeRejected; + std::promise firstValueIntact; + + runtime.Dispatch([&secondEscapeRejected, &firstValueIntact](Napi::Env env) mutable { + napi_env nenv{env}; + + const auto fail = [&secondEscapeRejected, &firstValueIntact]() { + secondEscapeRejected.set_value(false); + firstValueIntact.set_value(false); + }; + + napi_escapable_handle_scope scope{}; + if (napi_open_escapable_handle_scope(nenv, &scope) != napi_ok) + { + return fail(); + } + + napi_value first{}; + napi_value second{}; + if (napi_create_string_utf8(nenv, "first", NAPI_AUTO_LENGTH, &first) != napi_ok || + napi_create_string_utf8(nenv, "second", NAPI_AUTO_LENGTH, &second) != napi_ok) + { + return fail(); + } + + napi_value firstEscaped{}; + if (napi_escape_handle(nenv, scope, first, &firstEscaped) != napi_ok) + { + return fail(); + } + + napi_value secondEscaped{}; + secondEscapeRejected.set_value( + napi_escape_handle(nenv, scope, second, &secondEscaped) == napi_escape_called_twice); + + if (napi_close_escapable_handle_scope(nenv, scope) != napi_ok) + { + firstValueIntact.set_value(false); + return; + } + + for (int i = 0; i < 32; ++i) + { + napi_value filler{}; + napi_create_string_utf8(nenv, "filler filler filler", NAPI_AUTO_LENGTH, &filler); + } + + char buffer[32]{}; + size_t copied{0}; + const napi_status status{napi_get_value_string_utf8(nenv, firstEscaped, buffer, sizeof(buffer), &copied)}; + firstValueIntact.set_value(status == napi_ok && std::string{buffer} == "first"); + }); + + EXPECT_TRUE(secondEscapeRejected.get_future().get()); + EXPECT_TRUE(firstValueIntact.get_future().get()); +} #endif int RunTests() From 714bdb3e8f3611dd022fd1e51213c2d73fd28f88 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 12:54:59 -0700 Subject: [PATCH 5/7] Skip the double-escape test on backends where escaping is a no-op The Chakra and JavaScriptCore shims implement napi_escape_handle as a pass through that returns the escapee and does not track scopes, so they always report napi_ok and cannot return napi_escape_called_twice. Asserting that contract there fails on three CI jobs for a limitation unrelated to this change, so gate the test on a capability define set from CMake, following the existing JSRUNTIMEHOST_NAPI_ENGINE_JSI pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- Tests/UnitTests/CMakeLists.txt | 7 +++++++ Tests/UnitTests/Shared/Shared.cpp | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 2dbc7619..ed028aff 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -52,6 +52,13 @@ if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() +# The Chakra and JavaScriptCore shims implement napi_escape_handle as a pass +# through that does not track scopes, so they cannot report +# napi_escape_called_twice and SecondEscapeIsRejected is compiled out there. +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra" OR NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") + target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH) +endif() + target_link_libraries(UnitTests PRIVATE AppRuntime PRIVATE Console diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index e54ce850..a0a025c4 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -575,6 +575,12 @@ TEST(NodeApi, NestedEscapableScopesBothEscape) // Node-API permits at most one escape per escapable scope. The second call must be // rejected with napi_escape_called_twice, and must leave the first escaped handle // untouched rather than replacing or freeing it. +// +// The Chakra and JavaScriptCore shims implement napi_escape_handle as a pass through +// that does not track scopes at all, so they always report napi_ok and cannot honour +// this contract. Bringing them into line is its own change, so this test only covers +// the backends that do track scopes. +#if !defined(JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH) TEST(NodeApi, SecondEscapeIsRejected) { Babylon::AppRuntime runtime{}; @@ -635,6 +641,7 @@ TEST(NodeApi, SecondEscapeIsRejected) EXPECT_TRUE(secondEscapeRejected.get_future().get()); EXPECT_TRUE(firstValueIntact.get_future().get()); } +#endif // !JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH #endif int RunTests() From faf36578d82e334bead21c65369d45693149b24b Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 13:08:49 -0700 Subject: [PATCH 6/7] Publish the escape-handle capability from the napi target The Android tests compile Shared.cpp into their own UnitTestsJNI target, so a define set on the UnitTests target alone left Android_JSC still building and failing the double-escape test. Set it as an INTERFACE definition on napi instead, which reaches every consumer through JsRuntime and AppRuntime, so no test target has to repeat the engine check or be kept in sync. Verified on both sides: JavaScriptCore builds 8 NodeApi tests with the double-escape test excluded, QuickJS builds 9 with it included. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- Core/Node-API/CMakeLists.txt | 9 +++++++++ Tests/UnitTests/CMakeLists.txt | 7 ------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Core/Node-API/CMakeLists.txt b/Core/Node-API/CMakeLists.txt index 5f495695..ecd91261 100644 --- a/Core/Node-API/CMakeLists.txt +++ b/Core/Node-API/CMakeLists.txt @@ -266,6 +266,15 @@ add_library(napi ${SOURCES}) target_include_directories(napi ${INCLUDE_DIRECTORIES}) target_link_libraries(napi ${LINK_LIBRARIES}) +# The Chakra and JavaScriptCore shims implement napi_escape_handle as a pass +# through that returns the escapee without tracking scopes, so they always report +# napi_ok and cannot report napi_escape_called_twice. Published as an INTERFACE +# definition so every consumer sees it without each test target repeating the +# engine check. +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra" OR NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") + target_compile_definitions(napi INTERFACE JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH) +endif() + if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Hermes") # Apply Hermes-specific warning suppressions ONLY to env_hermes.cc so # they don't relax the rules for the rest of the napi sources. diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index ed028aff..2dbc7619 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -52,13 +52,6 @@ if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() -# The Chakra and JavaScriptCore shims implement napi_escape_handle as a pass -# through that does not track scopes, so they cannot report -# napi_escape_called_twice and SecondEscapeIsRejected is compiled out there. -if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra" OR NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") - target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH) -endif() - target_link_libraries(UnitTests PRIVATE AppRuntime PRIVATE Console From 6238b5abec7daa2e657727250d7a58a862901163 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 13 Aug 2026 13:36:38 -0700 Subject: [PATCH 7/7] Accept engines that only allow escaping from the innermost scope Hermes validates that napi_escape_handle is called on the innermost open scope and returns napi_handle_scope_mismatch otherwise, which is a legitimate refusal rather than a failure. Treat it as such and keep asserting the inner handle, which is the part the regression is about. --- Tests/UnitTests/Shared/Shared.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a0a025c4..658e4b49 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -469,6 +469,9 @@ TEST(NodeApi, EscapedHandleOutlivesItsScope) // silently invalidating the start index the still-open inner scope was handed. Closing // the inner scope then keeps the wrong slot and frees the inner escaped handle, // reintroducing the dangling napi_value this fix is about. +// +// Engines differ on whether the outer scope may escape while an inner one is open, so +// the test only requires that of the engines that allow it. TEST(NodeApi, NestedEscapableScopesBothEscape) { Babylon::AppRuntime runtime{}; @@ -522,8 +525,14 @@ TEST(NodeApi, NestedEscapableScopesBothEscape) return fail(); } + // Hermes only permits escaping from the innermost open scope and reports + // napi_handle_scope_mismatch here. That is a legitimate refusal rather than a + // failure, so record whether the engine allows this and keep checking the part + // that applies either way. napi_value outerEscaped{}; - if (napi_escape_handle(nenv, outerScope, outerSource, &outerEscaped) != napi_ok) + const napi_status outerEscapeStatus{napi_escape_handle(nenv, outerScope, outerSource, &outerEscaped)}; + const bool outerEscapeSupported{outerEscapeStatus == napi_ok}; + if (!outerEscapeSupported && outerEscapeStatus != napi_handle_scope_mismatch) { return fail(); } @@ -563,6 +572,14 @@ TEST(NodeApi, NestedEscapableScopesBothEscape) napi_create_string_utf8(nenv, "filler filler filler", NAPI_AUTO_LENGTH, &filler); } + if (!outerEscapeSupported) + { + // Nothing escaped from the outer scope, so the inner check above is the whole + // result on this engine. + bothValuesIntact.set_value(true); + return; + } + char outerBuffer[32]{}; size_t outerCopied{0}; const napi_status status{napi_get_value_string_utf8(nenv, outerEscaped, outerBuffer, sizeof(outerBuffer), &outerCopied)};