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/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 c9e2823d..c2848398 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1929,7 +1929,6 @@ 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++) { @@ -1937,6 +1936,17 @@ napi_status napi_close_escapable_handle_scope(napi_env env, napi_escapable_handl } 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->current_scope_start = scope_start; napi_clear_last_error(env); @@ -1952,40 +1962,22 @@ 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; - // 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) - 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; - } + // Node-API allows napi_escape_handle to be called at most once per scope. + if (env->escaped_handles.find(scope_start) != env->escaped_handles.end()) { + return napi_set_last_error(env, napi_escape_called_twice); } - *result = parentHandle; + // Duplicate the JSValue to create a new handle that will outlive the current scope + JSValue escapedValue = JS_DupValue(env->context, ToJSValue(escapee)); + + // 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)); + 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 7b84fe12..db0325a4 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,15 @@ struct napi_env__ { std::vector> handle_scope_stack; size_t current_scope_start = 0; + // 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 // 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..658e4b49 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -401,6 +401,264 @@ 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; + + 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{}; + if (napi_open_escapable_handle_scope(nenv, &scope) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } + + napi_value inner{}; + if (napi_create_string_utf8(nenv, "escape me", NAPI_AUTO_LENGTH, &inner) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } + + napi_value escaped{}; + if (napi_escape_handle(nenv, scope, inner, &escaped) != napi_ok) + { + escapedValueIsIntact.set_value(false); + return; + } + + 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. + 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()); +} + +// 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. +// +// 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{}; + + 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(); + } + + // 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{}; + 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(); + } + + // 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); + } + + 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)}; + 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. +// +// 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{}; + + 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 // !JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH #endif int RunTests()