From a108fd54305179c0c80d168bbd452cc93d940658 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 14 Jul 2026 05:43:45 +0000 Subject: [PATCH] napi: stop napi_get_property_names poisoning JSC's per-Structure own-keys cache napi_get_all_property_names(include_prototypes) and napi_get_property_names used JSC::allPropertyKeys(), which walks the prototype chain and then writes the chain-walked list into the Structure's own-keys cache via setCachedPropertyNames(). After enough calls on same-shaped objects, Reflect.ownKeys({a,b}) would permanently return 14 keys (all of Object.prototype leaked in) and Object.keys would return inherited enumerable keys, for every object sharing that Structure, process-wide. Replace both call sites with a local helper that walks the chain via JSObject::getPropertyNames into a PropertyNameArrayBuilder and builds the result JSArray directly, bypassing the cache entirely. The own-only path keeps using ownPropertyKeys() since that cache write is correct. --- src/jsc/bindings/napi.cpp | 34 +++++++++++++++++++++-- test/napi/napi-app/js_test_helpers.cpp | 26 ++++++++++++++++++ test/napi/napi-app/module.js | 37 ++++++++++++++++++++++++++ test/napi/napi.test.ts | 9 +++++++ 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 05212f47db96..37be410bf43b 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -48,6 +48,7 @@ #include #include #include +#include #include #include #include "JSFFIFunction.h" @@ -1814,6 +1815,35 @@ extern "C" napi_status napi_create_typedarray( namespace Zig { +// Walk the prototype chain collecting property names without touching JSC's +// per-Structure own-keys cache. JSC::allPropertyKeys() stores the chain-walked +// list there, poisoning Reflect.ownKeys/Object.keys for same-shaped objects. +static JSArray* collectInheritedPropertyKeys(JSGlobalObject* globalObject, JSObject* object, PropertyNameMode propertyNameMode, DontEnumPropertiesMode dontEnumPropertiesMode) +{ + VM& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + PropertyNameArrayBuilder properties(vm, propertyNameMode, PrivateSymbolMode::Exclude); + object->getPropertyNames(globalObject, properties, dontEnumPropertiesMode); + RETURN_IF_EXCEPTION(scope, nullptr); + + unsigned numProperties = properties.size(); + JSArray* keys = JSArray::create(vm, globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous), numProperties); + for (unsigned i = 0; i < numProperties; i++) { + const auto& identifier = properties[i]; + JSValue key; + if (propertyNameMode != PropertyNameMode::Strings && identifier.isSymbol()) { + ASSERT(!identifier.isPrivateName()); + key = Symbol::create(vm, static_cast(*identifier.impl())); + } else { + key = jsOwnedString(vm, identifier.string()); + } + keys->putDirectIndex(globalObject, i, key); + RETURN_IF_EXCEPTION(scope, nullptr); + } + return keys; +} + extern "C" napi_status napi_get_all_property_names( napi_env env, napi_value objectNapi, napi_key_collection_mode key_mode, napi_key_filter key_filter, napi_key_conversion key_conversion, @@ -1842,7 +1872,7 @@ extern "C" napi_status napi_get_all_property_names( JSArray* exportKeys = nullptr; if (key_mode == napi_key_include_prototypes) { - exportKeys = allPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode); + exportKeys = collectInheritedPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode); } else { exportKeys = ownPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode); } @@ -2011,7 +2041,7 @@ extern "C" napi_status napi_get_property_names(napi_env env, napi_value object, Zig::GlobalObject* globalObject = toJS(env); JSC::EnsureStillAliveScope ensureStillAlive(jsValue); - JSValue value = JSC::allPropertyKeys(globalObject, jsObject, PropertyNameMode::Strings, DontEnumPropertiesMode::Exclude); + JSValue value = collectInheritedPropertyKeys(globalObject, jsObject, PropertyNameMode::Strings, DontEnumPropertiesMode::Exclude); NAPI_RETURN_IF_EXCEPTION(env); JSC::EnsureStillAliveScope ensureStillAlive1(value); diff --git a/test/napi/napi-app/js_test_helpers.cpp b/test/napi/napi-app/js_test_helpers.cpp index 18e8ea727dc4..f38788a03a54 100644 --- a/test/napi/napi-app/js_test_helpers.cpp +++ b/test/napi/napi-app/js_test_helpers.cpp @@ -257,6 +257,30 @@ static napi_value make_empty_object(const Napi::CallbackInfo &info) { return object; } +// get_property_names(object) -> array +static napi_value get_property_names(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + napi_value result; + NODE_API_CALL(env, napi_get_property_names(env, info[0], &result)); + return result; +} + +// get_all_property_names(object, key_mode, key_filter, key_conversion) -> array +static napi_value get_all_property_names(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + uint32_t key_mode, key_filter, key_conversion; + NODE_API_CALL(env, napi_get_value_uint32(env, info[1], &key_mode)); + NODE_API_CALL(env, napi_get_value_uint32(env, info[2], &key_filter)); + NODE_API_CALL(env, napi_get_value_uint32(env, info[3], &key_conversion)); + napi_value result; + NODE_API_CALL(env, + napi_get_all_property_names( + env, info[0], (napi_key_collection_mode)key_mode, + (napi_key_filter)key_filter, + (napi_key_conversion)key_conversion, &result)); + return result; +} + // add_tag(object, lower, upper) static napi_value add_tag(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); @@ -447,6 +471,8 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, create_latin1_string); REGISTER_FUNCTION(env, exports, make_empty_array); REGISTER_FUNCTION(env, exports, make_empty_object); + REGISTER_FUNCTION(env, exports, get_property_names); + REGISTER_FUNCTION(env, exports, get_all_property_names); REGISTER_FUNCTION(env, exports, add_tag); REGISTER_FUNCTION(env, exports, try_add_tag); REGISTER_FUNCTION(env, exports, check_tag); diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 955e2922ce15..601aedccf6a6 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -236,6 +236,43 @@ nativeTests.test_set_property = () => { } }; +nativeTests.test_property_names_cache_poisoning = () => { + // napi_key_include_prototypes = 0, napi_key_own_only = 1 + // napi_key_all_properties = 0, napi_key_skip_symbols = 16 + // napi_key_keep_numbers = 0 + const mkA = () => ({ a: 1, b: 2 }); + for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkA(), 0, 0, 0); + console.log("Reflect.ownKeys after get_all_property_names(include_prototypes):", Reflect.ownKeys(mkA()).join(",")); + console.log("Object.keys after get_all_property_names(include_prototypes):", Object.keys(mkA()).join(",")); + + const proto = { pEnum: 9 }; + const mkB = () => { + const o = Object.create(proto); + o.w1 = 1; + o.w2 = 2; + return o; + }; + for (let i = 0; i < 20; i++) nativeTests.get_property_names(mkB()); + console.log("Object.keys after get_property_names:", Object.keys(mkB()).join(",")); + console.log("Reflect.ownKeys after get_property_names:", Reflect.ownKeys(mkB()).join(",")); + + const mkC = () => ({ c: 1, d: 2 }); + for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkC(), 0, 16, 0); + console.log("Object.getOwnPropertyNames after skip_symbols chain walk:", Object.getOwnPropertyNames(mkC()).join(",")); + + // Own-only mode must still return only own keys and not poison anything. + const mkD = () => ({ e: 1, f: 2 }); + for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkD(), 1, 0, 0); + console.log("Reflect.ownKeys after get_all_property_names(own_only):", Reflect.ownKeys(mkD()).join(",")); + + // The napi result itself should still include inherited keys. + const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0); + console.log("napi include_prototypes result has own a,b:", apnResult.includes("a") && apnResult.includes("b")); + console.log("napi include_prototypes result has inherited toString:", apnResult.includes("toString")); + const gpnResult = nativeTests.get_property_names(mkB()); + console.log("napi get_property_names result:", gpnResult.join(",")); +}; + nativeTests.test_number_integer_conversions_from_js = () => { const i32 = { min: -(2 ** 31), max: 2 ** 31 - 1 }; const u32Max = 2 ** 32 - 1; diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 6323afe73de4..45e2702c6b4a 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -574,6 +574,15 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { }); }); + describe("napi_get_property_names / napi_get_all_property_names", () => { + it("does not poison JSC's per-Structure own-keys cache", async () => { + const output = await checkSameOutput("test_property_names_cache_poisoning", []); + expect(output).toContain("Reflect.ownKeys after get_all_property_names(include_prototypes): a,b"); + expect(output).toContain("Object.keys after get_property_names: w1,w2"); + expect(output).toContain("napi get_property_names result: w1,w2,pEnum"); + }); + }); + describe("napi_value <=> integer conversion", () => { it("works", async () => { await checkSameOutput("test_number_integer_conversions_from_js", []);