From a553fda32baf1c67f54e66e7e08aca38b356813e Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Sun, 25 Jan 2026 20:11:52 -0800 Subject: [PATCH] fix(napi): fix use-after-free in property names and external buffer lifetime (#26450) ## Summary - **PROPERTY_NAME_FROM_UTF8 use-after-free:** The macro used `StringImpl::createWithoutCopying` for ASCII strings, which left dangling pointers in JSC's atom string table when the caller freed the input buffer (e.g. napi-rs `CString`). Fixed by using `Identifier::fromString` which copies only when inserting into the atom table, but never retains a reference to the caller's buffer. - **napi_create_external_buffer data lifetime:** `finalize_cb` was attached via `addFinalizer` (tied to GC of the `JSUint8Array` view) instead of the `ArrayBuffer` destructor. Extracting `.buffer` and letting the Buffer get GC'd would free the backing data while the `ArrayBuffer` still referenced it. Fixed by attaching the destructor to the `ArrayBuffer` via `createFromBytes`, using an armed `NapiExternalBufferDestructor` to safely handle the `JSUint8Array::create` error path. Closes #26446 Closes #26423 ## Test plan - [x] Added regression test `test_napi_get_named_property_copied_string` -- strdup/free cycles with GC to reproduce the atom table dangling pointer - [x] Added regression test `test_external_buffer_data_lifetime` -- extracts ArrayBuffer, drops Buffer, GCs, verifies data is intact - [x] Both tests pass with `bun bd test` and match Node.js output via `checkSameOutput` - [x] Verified `test_external_buffer_data_lifetime` fails without the fix (data corrupted) and passes on Node.js - [x] Verified impit reproducer from #26423 works correctly with the fix Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude --- src/bun.js/bindings/headers.h | 2 +- src/bun.js/bindings/napi.cpp | 86 +++++++++----- test/napi/napi-app/standalone_tests.cpp | 145 ++++++++++++++++++++++++ test/napi/napi.test.ts | 34 ++++++ 4 files changed, 239 insertions(+), 28 deletions(-) diff --git a/src/bun.js/bindings/headers.h b/src/bun.js/bindings/headers.h index 3adcacb440..77cb55030a 100644 --- a/src/bun.js/bindings/headers.h +++ b/src/bun.js/bindings/headers.h @@ -31,7 +31,7 @@ class JSString; class JSCell; class JSMap; class JSPromise; -class CatchScope; +class TopExceptionScope; class VM; class ThrowScope; class CallFrame; diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index aa3419a65b..e6a5dc54cb 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -585,6 +585,21 @@ extern "C" napi_status napi_has_own_property(napi_env env, napi_value object, NAPI_RETURN_SUCCESS(env); } +// For ASCII input (the common case), avoids UTF-8 decoding overhead by going +// directly through Identifier::fromString(VM&, span), which uses the +// span for a hash lookup in the atom string table without creating an +// intermediate WTF::String. If the atom already exists, no copy occurs at all. +// If the atom does not exist and gets inserted into the table, the characters +// are cloned because we cannot guarantee the lifetime of the input span. +JSC::Identifier identifierFromUtf8(JSC::VM& vm, const char* utf8Name) +{ + size_t utf8Len = strlen(utf8Name); + std::span utf8Span { reinterpret_cast(utf8Name), utf8Len }; + return WTF::charactersAreAllASCII(utf8Span) + ? JSC::Identifier::fromString(vm, utf8Span) + : JSC::Identifier::fromString(vm, WTF::String::fromUTF8(utf8Span)); +} + extern "C" napi_status napi_set_named_property(napi_env env, napi_value object, const char* utf8name, napi_value value) @@ -605,8 +620,7 @@ extern "C" napi_status napi_set_named_property(napi_env env, napi_value object, JSC::EnsureStillAliveScope ensureAlive(jsValue); JSC::EnsureStillAliveScope ensureAlive2(target); - auto nameStr = WTF::String::fromUTF8({ utf8name, strlen(utf8name) }); - auto name = JSC::PropertyName(JSC::Identifier::fromString(vm, WTF::move(nameStr))); + auto name = identifierFromUtf8(vm, utf8name); PutPropertySlot slot(target, false); target->putInline(globalObject, name, jsValue, slot); @@ -666,17 +680,6 @@ extern "C" napi_status napi_is_typedarray(napi_env env, napi_value value, bool* NAPI_RETURN_SUCCESS(env); } -// This is more efficient than using WTF::String::FromUTF8 -// it doesn't copy the string -// but it's only safe to use if we are not setting a property -// because we can't guarantee the lifetime of it -#define PROPERTY_NAME_FROM_UTF8(identifierName) \ - size_t utf8Len = strlen(utf8Name); \ - WTF::String&& nameString = WTF::charactersAreAllASCII(std::span { reinterpret_cast(utf8Name), utf8Len }) \ - ? WTF::String(WTF::StringImpl::createWithoutCopying({ utf8Name, utf8Len })) \ - : WTF::String::fromUTF8(utf8Name); \ - const JSC::PropertyName identifierName = JSC::Identifier::fromString(vm, nameString); - extern "C" napi_status napi_has_named_property(napi_env env, napi_value object, const char* utf8Name, bool* result) @@ -692,10 +695,10 @@ extern "C" napi_status napi_has_named_property(napi_env env, napi_value object, JSObject* target = toJS(object).toObject(globalObject); NAPI_RETURN_IF_EXCEPTION(env); - PROPERTY_NAME_FROM_UTF8(name); + JSC::Identifier propertyName = identifierFromUtf8(vm, utf8Name); PropertySlot slot(target, PropertySlot::InternalMethodType::HasProperty); - *result = target->getPropertySlot(globalObject, name, slot); + *result = target->getPropertySlot(globalObject, propertyName, slot); NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION(env); } extern "C" napi_status napi_get_named_property(napi_env env, napi_value object, @@ -713,9 +716,9 @@ extern "C" napi_status napi_get_named_property(napi_env env, napi_value object, JSObject* target = toJS(object).toObject(globalObject); NAPI_RETURN_IF_EXCEPTION(env); - PROPERTY_NAME_FROM_UTF8(name); + JSC::Identifier propertyName = identifierFromUtf8(vm, utf8Name); - *result = toNapi(target->get(globalObject, name), globalObject); + *result = toNapi(target->get(globalObject, propertyName), globalObject); NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION(env); } @@ -2014,6 +2017,35 @@ extern "C" napi_status napi_create_buffer(napi_env env, size_t length, NAPI_RETURN_SUCCESS(env); } +// SharedTask subclass with an armed flag so that the destructor can be +// armed only after JSUint8Array::create succeeds. If creation throws, +// the destructor runs disarmed and skips finalize_cb. +class NapiExternalBufferDestructor final : public SharedTask { +public: + NapiExternalBufferDestructor(WTF::Ref&& env, napi_finalize cb, void* hint) + : m_env(WTF::move(env)) + , m_cb(cb) + , m_hint(hint) + { + } + + void run(void* data) override + { + if (m_armed) { + NAPI_LOG("external buffer finalizer"); + m_env->doFinalizer(m_cb, data, m_hint); + } + } + + void arm() { m_armed = true; } + +private: + WTF::Ref m_env; + napi_finalize m_cb; + void* m_hint; + bool m_armed { false }; +}; + extern "C" napi_status napi_create_external_buffer(napi_env env, size_t length, void* data, napi_finalize finalize_cb, @@ -2044,19 +2076,19 @@ extern "C" napi_status napi_create_external_buffer(napi_env env, size_t length, NAPI_RETURN_SUCCESS(env); } - auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(data), length }, createSharedTask([](void*) { - // do nothing - })); + // Uses NapiExternalBufferDestructor instead of createSharedTask because + // JSUint8Array::create can throw, and we must not call finalize_cb on failure. + Ref destructor = adoptRef(*new NapiExternalBufferDestructor(WTF::Ref(*env), finalize_cb, finalize_hint)); + // Get pointer before using WTF::move + auto* destructorPtr = destructor.ptr(); + auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(data), length }, WTF::move(destructor)); auto* buffer = JSC::JSUint8Array::create(globalObject, subclassStructure, WTF::move(arrayBuffer), 0, length); NAPI_RETURN_IF_EXCEPTION(env); - // setup finalizer after creating the array. if it throws callers of napi_create_external_buffer are expected - // to free input - vm.heap.addFinalizer(buffer, [env = WTF::Ref(*env), finalize_cb, data, finalize_hint](JSCell* cell) -> void { - NAPI_LOG("external buffer finalizer"); - env->doFinalizer(finalize_cb, data, finalize_hint); - }); + // Arm only after successful creation: if create threw, the destructor + // runs disarmed and skips finalize_cb (caller retains ownership). + destructorPtr->arm(); *result = toNapi(buffer, globalObject); NAPI_RETURN_SUCCESS(env); @@ -2071,7 +2103,7 @@ extern "C" napi_status napi_create_external_arraybuffer(napi_env env, void* exte Zig::GlobalObject* globalObject = toJS(env); JSC::VM& vm = JSC::getVM(globalObject); - auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(external_data), byte_length }, createSharedTask([env, finalize_hint, finalize_cb](void* p) { + auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(external_data), byte_length }, createSharedTask([env = WTF::Ref(*env), finalize_hint, finalize_cb](void* p) { NAPI_LOG("external ArrayBuffer finalizer"); env->doFinalizer(finalize_cb, p, finalize_hint); })); diff --git a/test/napi/napi-app/standalone_tests.cpp b/test/napi/napi-app/standalone_tests.cpp index 05c5ae621b..46cefd4867 100644 --- a/test/napi/napi-app/standalone_tests.cpp +++ b/test/napi/napi-app/standalone_tests.cpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include @@ -1855,6 +1857,147 @@ static napi_value napi_get_typeof(const Napi::CallbackInfo &info) { return result; } +// Regression test: napi_create_external_buffer must tie the finalize callback +// to the ArrayBuffer's destructor, not addFinalizer on the JSUint8Array. +// With addFinalizer, extracting .buffer (the ArrayBuffer) and then letting the +// Buffer get GC'd would call finalize_cb and free the data while the ArrayBuffer +// still references it. +static napi_value test_external_buffer_data_lifetime(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + + // Allocate data with a known pattern. + const size_t data_size = 4; + uint8_t* ext_data = (uint8_t*)malloc(data_size); + ext_data[0] = 0xDE; ext_data[1] = 0xAD; ext_data[2] = 0xBE; ext_data[3] = 0xEF; + + napi_ref ab_ref = nullptr; + + // Create the buffer inside a handle scope we'll close before GC, + // so the JSUint8Array handle becomes eligible for collection. + napi_handle_scope *hs = new napi_handle_scope; + NODE_API_CALL(env, napi_open_handle_scope(env, hs)); + + // Allocate on the heap so conservative stack scanning can't find it. + napi_value *buffer = new napi_value; + NODE_API_CALL(env, napi_create_external_buffer(env, data_size, ext_data, + +[](napi_env, void* data, void*) { + // Poison the data then free — detectable as use-after-free if + // the ArrayBuffer still tries to read through this pointer. + memset(data, 0x00, 4); + free(data); + }, nullptr, buffer)); + + // Extract the underlying ArrayBuffer and prevent it from being GC'd. + napi_value *arraybuffer = new napi_value; + NODE_API_CALL(env, napi_get_typedarray_info(env, *buffer, nullptr, nullptr, + nullptr, arraybuffer, nullptr)); + NODE_API_CALL(env, napi_create_reference(env, *arraybuffer, 1, &ab_ref)); + + // Drop heap pointers before closing the scope so the stack scanner + // can't keep the Buffer alive. + delete arraybuffer; + delete buffer; + + NODE_API_CALL(env, napi_close_handle_scope(env, *hs)); + delete hs; + + // GC: with the old bug (addFinalizer), collecting the JSUint8Array would + // call finalize_cb and poison the data even though the ArrayBuffer is alive. + run_gc(info); + run_gc(info); + + // Read data through the ArrayBuffer — should still be 0xDEADBEEF. + napi_value ab_value; + NODE_API_CALL(env, napi_get_reference_value(env, ab_ref, &ab_value)); + + void* ab_data; + size_t ab_len; + NODE_API_CALL(env, napi_get_arraybuffer_info(env, ab_value, &ab_data, &ab_len)); + + uint8_t* bytes = (uint8_t*)ab_data; + if (ab_len >= data_size && + bytes[0] == 0xDE && bytes[1] == 0xAD && + bytes[2] == 0xBE && bytes[3] == 0xEF) { + printf("PASS: external buffer data intact through ArrayBuffer after GC\n"); + } else { + printf("FAIL: external buffer data was corrupted (finalize_cb ran too early)\n"); + } + + NODE_API_CALL(env, napi_delete_reference(env, ab_ref)); + return ok(env); +} + +// Regression test: PROPERTY_NAME_FROM_UTF8 must copy string data. +// Previously it used StringImpl::createWithoutCopying for ASCII strings, +// which could leave dangling pointers in JSC's atom string table. +// +// This replicates the pattern from napi-rs / impit that caused a crash: +// napi-rs creates a CString (heap-allocated) for each property name, +// passes it to napi_get_named_property, then frees the CString. +// With createWithoutCopying, the atom table retains a reference to the +// freed CString memory. On the next lookup of the same property name, +// Identifier::fromString compares against the stale atom -> use-after-free. +static napi_value test_napi_get_named_property_copied_string(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + + napi_value global; + NODE_API_CALL(env, napi_get_global(env, &global)); + + // Simulate what impit does: look up properties on JS objects using + // heap-allocated keys (like napi-rs CString), then free them. + // The property names here match what impit uses in its response handling. + const char *property_names[] = { + "ReadableStream", "Response", "arrayBuffer", "then", "eval", + "enqueue", "bind", "close", + }; + const int num_names = sizeof(property_names) / sizeof(property_names[0]); + + // First round: each strdup'd key goes through PROPERTY_NAME_FROM_UTF8 then + // is freed. With createWithoutCopying, the atom table entries now have + // dangling data pointers. + for (int i = 0; i < num_names; i++) { + char *key = strdup(property_names[i]); + napi_value result; + NODE_API_CALL(env, napi_get_named_property(env, global, key, &result)); + free(key); + } + + // Trigger GC - this is critical. In the impit crash, GC occurs between + // the first and second lookups due to many object allocations (ReadableStream + // chunks, Response objects, promises). GC may cause the atom table to + // drop or recreate atoms, exposing the dangling pointers. + run_gc(info); + + // Churn through more strdup/free cycles to increase the chance that + // malloc reuses memory from the freed keys above. + for (int round = 0; round < 30; round++) { + for (int i = 0; i < num_names; i++) { + char *key = strdup(property_names[i]); + napi_value result; + NODE_API_CALL(env, napi_get_named_property(env, global, key, &result)); + free(key); + } + if (round % 10 == 0) { + run_gc(info); + } + } + + run_gc(info); + + // Second round: look up the same property names again. + // With the bug, Identifier::fromString finds stale atoms in the table + // and reads their freed backing memory -> ASAN heap-use-after-free. + for (int i = 0; i < num_names; i++) { + char *key = strdup(property_names[i]); + napi_value result; + NODE_API_CALL(env, napi_get_named_property(env, global, key, &result)); + free(key); + } + + printf("PASS\n"); + return ok(env); +} + void register_standalone_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, test_issue_7685); REGISTER_FUNCTION(env, exports, test_issue_11949); @@ -1888,6 +2031,8 @@ void register_standalone_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, test_napi_create_external_buffer_empty); REGISTER_FUNCTION(env, exports, test_napi_empty_buffer_info); REGISTER_FUNCTION(env, exports, napi_get_typeof); + REGISTER_FUNCTION(env, exports, test_external_buffer_data_lifetime); + REGISTER_FUNCTION(env, exports, test_napi_get_named_property_copied_string); } } // namespace napitests diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index b374c85452..75d5ea81cc 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -275,6 +275,12 @@ describe.concurrent("napi", () => { expect(result).not.toContain("FAIL"); }); + it("finalize_cb is tied to the ArrayBuffer lifetime, not the Buffer view", async () => { + const result = await checkSameOutput("test_external_buffer_data_lifetime", []); + expect(result).toContain("PASS: external buffer data intact through ArrayBuffer after GC"); + expect(result).not.toContain("FAIL"); + }); + it("empty buffer returns null pointer and 0 length from napi_get_buffer_info and napi_get_typedarray_info", async () => { const result = await checkSameOutput("test_napi_empty_buffer_info", []); expect(result).toContain("PASS: napi_get_buffer_info returns null pointer and 0 length for empty buffer"); @@ -553,6 +559,34 @@ describe.concurrent("napi", () => { await checkSameOutput("test_napi_numeric_string_keys", []); }); + it("napi_get_named_property copies utf8 string data", async () => { + // Must spawn bun directly (not via checkSameOutput/main.js) because the + // bug only reproduces when global property names like "Response" haven't + // been pre-atomized. Loading through main.js → module.js pre-initializes + // globals, masking the use-after-free in the atom string table. + const addonPath = join(__dirname, "napi-app", "build", "Debug", "napitests.node"); + await using proc = spawn({ + cmd: [ + bunExe(), + "-e", + `const addon = require(${JSON.stringify(addonPath)}); addon.test_napi_get_named_property_copied_string(() => { Bun.gc(true); });`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + + expect(stderr).toBe(""); + expect(stdout).toInclude("PASS"); + expect(exitCode).toBe(0); + }); + it("behaves as expected when performing operations with default values", async () => { await checkSameOutput("test_napi_get_default_values", []); });