From a0681196245683fcd04629aa72db1daf22a88b87 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Mon, 26 Jan 2026 00:37:50 +0000 Subject: [PATCH] fix(napi): tie external buffer finalizer to ArrayBuffer lifecycle This fixes a race condition in `napi_create_external_buffer` where the finalizer was added separately via `vm.heap.addFinalizer`, allowing native code to free/reuse the underlying memory while JavaScript still held a reference to the ArrayBuffer, causing data corruption. The fix ties the finalizer directly to the ArrayBuffer via `createFromBytes`, matching the pattern used in `napi_create_external_arraybuffer`. Fixes #26446 Co-Authored-By: Claude Opus 4.5 --- src/bun.js/bindings/napi.cpp | 12 +--- test/napi/napi-app/module.js | 38 ++++++++++ test/napi/napi-app/standalone_tests.cpp | 95 +++++++++++++++++++++++++ test/napi/napi.test.ts | 8 +++ 4 files changed, 144 insertions(+), 9 deletions(-) diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 884f3c4535..b60fb72cc5 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -2044,20 +2044,14 @@ 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 + auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast(data), length }, createSharedTask([envRef = WTF::Ref(*env), finalize_hint, finalize_cb](void* p) { + NAPI_LOG("external buffer finalizer"); + envRef->doFinalizer(finalize_cb, p, finalize_hint); })); 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); - }); - *result = toNapi(buffer, globalObject); NAPI_RETURN_SUCCESS(env); } diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 7462614379..c9f086a5f3 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -768,6 +768,44 @@ nativeTests.test_ref_unref_underflow = () => { } }; +// Test for napi_create_external_buffer finalizer timing +// See: https://github.com/oven-sh/bun/issues/26446 +nativeTests.test_issue_26446 = async () => { + // Create buffer and verify data integrity + let buffer = nativeTests.test_napi_external_buffer_finalizer(); + + if (!buffer) { + console.log("FAIL: Could not create external buffer"); + return; + } + + // Verify we can read the buffer data + { + const data = new Uint8Array(buffer.buffer || buffer); + let dataValid = true; + for (let i = 0; i < data.length; i++) { + if (data[i] !== (i & 0xff)) { + console.log(`FAIL: Data corruption at offset ${i}: expected ${i & 0xff}, got ${data[i]}`); + dataValid = false; + break; + } + } + + if (dataValid) { + console.log("PASS: Buffer data is correct after creation"); + } + } + // data goes out of scope here, allowing GC + + // Clear the buffer reference to allow GC + buffer = null; + + // Use gcUntil to deterministically wait for finalizer to be called + await gcUntil(() => nativeTests.test_napi_external_buffer_finalizer_check()); + + console.log("PASS: External buffer finalizer was called"); +}; + nativeTests.test_get_value_string = () => { function to16Bit(string) { if (typeof Bun != "object") return string; diff --git a/test/napi/napi-app/standalone_tests.cpp b/test/napi/napi-app/standalone_tests.cpp index 05c5ae621b..b6b4ae1880 100644 --- a/test/napi/napi-app/standalone_tests.cpp +++ b/test/napi/napi-app/standalone_tests.cpp @@ -1825,6 +1825,99 @@ static napi_value test_napi_empty_buffer_info(const Napi::CallbackInfo &info) { return ok(env); } +// Test for napi_create_external_buffer finalizer timing +// See: https://github.com/oven-sh/bun/issues/26446 +// The finalizer must be tied directly to the ArrayBuffer lifecycle to prevent +// native code from freeing/reusing memory while JS still holds a reference. +static bool g_external_buffer_finalizer_called = false; +static void *g_external_buffer_finalized_data = nullptr; + +static void external_buffer_timing_finalizer(napi_env env, void *data, + void *hint) { + g_external_buffer_finalizer_called = true; + g_external_buffer_finalized_data = data; + // Free the data that was allocated in the test + free(data); +} + +static napi_value +test_napi_external_buffer_finalizer(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + + // Reset global state + g_external_buffer_finalizer_called = false; + g_external_buffer_finalized_data = nullptr; + + // Allocate data that will be owned by the external buffer + const size_t data_size = 32 * 1024; // 32KB, larger than 16KB boundary + uint8_t *data = static_cast(malloc(data_size)); + if (!data) { + printf("FAIL: Could not allocate test data\n"); + return env.Undefined(); + } + + // Fill with a pattern we can verify + for (size_t i = 0; i < data_size; i++) { + data[i] = static_cast(i & 0xFF); + } + + // Create external buffer - the finalizer should be tied to the ArrayBuffer + napi_value buffer; + napi_status status = napi_create_external_buffer( + env, data_size, data, external_buffer_timing_finalizer, nullptr, &buffer); + + if (status != napi_ok) { + printf("FAIL: napi_create_external_buffer failed with status %d\n", status); + free(data); + return env.Undefined(); + } + + // Verify we can read the data correctly + void *buffer_data; + size_t buffer_length; + status = napi_get_buffer_info(env, buffer, &buffer_data, &buffer_length); + + if (status != napi_ok) { + printf("FAIL: napi_get_buffer_info failed with status %d\n", status); + return env.Undefined(); + } + + if (buffer_length != data_size) { + printf("FAIL: Buffer length is %zu, expected %zu\n", buffer_length, + data_size); + return env.Undefined(); + } + + if (buffer_data != data) { + printf("FAIL: Buffer data pointer does not match original data\n"); + return env.Undefined(); + } + + // Verify the data content is correct (not corrupted) + uint8_t *verify_data = static_cast(buffer_data); + for (size_t i = 0; i < data_size; i++) { + if (verify_data[i] != static_cast(i & 0xFF)) { + printf("FAIL: Data corruption at offset %zu: expected %u, got %u\n", i, + static_cast(i & 0xFF), + static_cast(verify_data[i])); + return env.Undefined(); + } + } + + // Return the buffer for JavaScript to hold + // The finalizer should NOT be called until this buffer is garbage collected + printf("PASS: napi_create_external_buffer created buffer with correct data\n"); + return buffer; +} + +static napi_value +test_napi_external_buffer_finalizer_check(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + + // Return boolean indicating whether finalizer was called + return Napi::Boolean::New(env, g_external_buffer_finalizer_called); +} + // Test for napi_typeof with boxed primitive objects (String, Number, Boolean) // See: https://github.com/oven-sh/bun/issues/25351 static napi_value napi_get_typeof(const Napi::CallbackInfo &info) { @@ -1887,6 +1980,8 @@ void register_standalone_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, test_napi_freeze_seal_indexed); REGISTER_FUNCTION(env, exports, test_napi_create_external_buffer_empty); REGISTER_FUNCTION(env, exports, test_napi_empty_buffer_info); + REGISTER_FUNCTION(env, exports, test_napi_external_buffer_finalizer); + REGISTER_FUNCTION(env, exports, test_napi_external_buffer_finalizer_check); REGISTER_FUNCTION(env, exports, napi_get_typeof); } diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index b374c85452..01f8bae834 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -282,6 +282,14 @@ describe.concurrent("napi", () => { expect(result).toContain("PASS: napi_is_detached_arraybuffer returns true for empty buffer's arraybuffer"); expect(result).not.toContain("FAIL"); }); + + // https://github.com/oven-sh/bun/issues/26446 + it("calls finalizer when buffer is garbage collected", async () => { + const result = await checkSameOutput("test_issue_26446", []); + expect(result).toContain("PASS: napi_create_external_buffer created buffer with correct data"); + expect(result).toContain("PASS: Buffer data is correct after creation"); + expect(result).not.toContain("FAIL"); + }); }); describe("napi_async_work", () => {