diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 884f3c4535..20271b7032 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([env, finalize_hint, finalize_cb](void* p) { + NAPI_LOG("external buffer finalizer"); + env->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/standalone_tests.cpp b/test/napi/napi-app/standalone_tests.cpp index 05c5ae621b..4aa16bd65a 100644 --- a/test/napi/napi-app/standalone_tests.cpp +++ b/test/napi/napi-app/standalone_tests.cpp @@ -1825,6 +1825,118 @@ static napi_value test_napi_empty_buffer_info(const Napi::CallbackInfo &info) { return ok(env); } +// Test for napi_create_external_buffer finalizer correctness +// See: https://github.com/oven-sh/bun/issues/26423 +// This test verifies that the finalizer receives the correct data pointer +// and is called at the right time. +static int external_buffer_finalizer_call_count = 0; +static void *external_buffer_finalizer_received_data = nullptr; +static void *external_buffer_finalizer_received_hint = nullptr; +static const size_t EXTERNAL_BUFFER_TEST_SIZE = 1024; + +static void external_buffer_test_finalizer(napi_env env, void *data, void *hint) { + external_buffer_finalizer_call_count++; + external_buffer_finalizer_received_data = data; + external_buffer_finalizer_received_hint = hint; +} + +static napi_value +test_napi_external_buffer_finalizer(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + + // Reset global state + external_buffer_finalizer_call_count = 0; + external_buffer_finalizer_received_data = nullptr; + external_buffer_finalizer_received_hint = nullptr; + + // Allocate test data + char *test_data = static_cast(malloc(EXTERNAL_BUFFER_TEST_SIZE)); + if (!test_data) { + printf("FAIL: Could not allocate test data\n"); + return env.Undefined(); + } + + // Fill with a known pattern + memset(test_data, 0x42, EXTERNAL_BUFFER_TEST_SIZE); + + // Create an external buffer + napi_value buffer; + void *hint = reinterpret_cast(0xCAFEBABE); + napi_status status = napi_create_external_buffer( + env, EXTERNAL_BUFFER_TEST_SIZE, test_data, external_buffer_test_finalizer, + hint, &buffer); + + if (status != napi_ok) { + printf("FAIL: napi_create_external_buffer failed with status %d\n", status); + free(test_data); + return env.Undefined(); + } + + // Verify the buffer has the correct data + void *buffer_data; + size_t buffer_length; + NODE_API_CALL(env, napi_get_buffer_info(env, buffer, &buffer_data, &buffer_length)); + + if (buffer_length != EXTERNAL_BUFFER_TEST_SIZE) { + printf("FAIL: Buffer length is %zu instead of %zu\n", buffer_length, + EXTERNAL_BUFFER_TEST_SIZE); + return env.Undefined(); + } + + if (buffer_data != test_data) { + printf("FAIL: Buffer data pointer %p doesn't match original %p\n", + buffer_data, test_data); + return env.Undefined(); + } + + // Verify the data wasn't corrupted + char *data_as_char = static_cast(buffer_data); + for (size_t i = 0; i < EXTERNAL_BUFFER_TEST_SIZE; i++) { + if (data_as_char[i] != 0x42) { + printf("FAIL: Data corrupted at index %zu: expected 0x42, got 0x%02x\n", i, + static_cast(data_as_char[i])); + return env.Undefined(); + } + } + + printf("PASS: External buffer created with correct data pointer and length\n"); + + // Return the buffer - JS will trigger GC to test the finalizer + return buffer; +} + +static napi_value +get_external_buffer_finalizer_stats(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + + napi_value result; + NODE_API_CALL(env, napi_create_object(env, &result)); + + napi_value call_count; + NODE_API_CALL(env, napi_create_int32(env, external_buffer_finalizer_call_count, &call_count)); + NODE_API_CALL(env, napi_set_named_property(env, result, "callCount", call_count)); + + napi_value received_data; + if (external_buffer_finalizer_received_data != nullptr) { + NODE_API_CALL(env, napi_create_bigint_uint64( + env, reinterpret_cast(external_buffer_finalizer_received_data), &received_data)); + } else { + NODE_API_CALL(env, napi_get_null(env, &received_data)); + } + NODE_API_CALL(env, napi_set_named_property(env, result, "receivedData", received_data)); + + napi_value received_hint; + if (external_buffer_finalizer_received_hint != nullptr) { + NODE_API_CALL(env, napi_create_bigint_uint64( + env, reinterpret_cast(external_buffer_finalizer_received_hint), &received_hint)); + } else { + NODE_API_CALL(env, napi_get_null(env, &received_hint)); + } + NODE_API_CALL(env, napi_set_named_property(env, result, "receivedHint", received_hint)); + + return result; +} + // 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 +1999,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, get_external_buffer_finalizer_stats); REGISTER_FUNCTION(env, exports, napi_get_typeof); } diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index b374c85452..49262f6374 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -282,6 +282,15 @@ describe.concurrent("napi", () => { expect(result).toContain("PASS: napi_is_detached_arraybuffer returns true for empty buffer's arraybuffer"); expect(result).not.toContain("FAIL"); }); + + // Regression test for https://github.com/oven-sh/bun/issues/26423 + // Verifies that the finalizer for napi_create_external_buffer receives the correct + // data pointer and is properly tied to the ArrayBuffer's lifetime. + it("finalizer receives correct data pointer", async () => { + const result = await checkSameOutput("test_napi_external_buffer_finalizer", []); + expect(result).toContain("PASS: External buffer created with correct data pointer and length"); + expect(result).not.toContain("FAIL"); + }); }); describe("napi_async_work", () => { diff --git a/test/regression/issue/26423.test.ts b/test/regression/issue/26423.test.ts new file mode 100644 index 0000000000..b1e9d05a40 --- /dev/null +++ b/test/regression/issue/26423.test.ts @@ -0,0 +1,171 @@ +// Regression test for https://github.com/oven-sh/bun/issues/26423 +// napi_create_external_buffer was using a two-stage finalization which caused +// a race condition where the native memory could be freed/reused before the +// JavaScript buffer was actually collected. + +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +describe("napi_create_external_buffer", () => { + test("finalizer is properly tied to ArrayBuffer lifecycle", async () => { + // This test verifies that when napi_create_external_buffer is called, + // the finalizer callback receives the correct data pointer when the + // buffer is garbage collected. Previously, the implementation used a + // two-stage finalization approach which caused race conditions. + + using dir = tempDir("napi-buffer-test", { + "binding.gyp": JSON.stringify({ + targets: [ + { + target_name: "test_external_buffer", + sources: ["test_external_buffer.c"], + }, + ], + }), + "test_external_buffer.c": ` +#include +#include +#include +#include + +static int finalizer_called = 0; +static void* received_data = NULL; +static const size_t TEST_SIZE = 1024; + +static void my_finalizer(napi_env env, void* data, void* hint) { + finalizer_called = 1; + received_data = data; + // Free the memory that was allocated + free(data); +} + +static napi_value create_external_buffer(napi_env env, napi_callback_info info) { + // Reset state + finalizer_called = 0; + received_data = NULL; + + // Allocate memory + char* data = (char*)malloc(TEST_SIZE); + if (!data) { + napi_throw_error(env, NULL, "malloc failed"); + return NULL; + } + + // Fill with a pattern + memset(data, 0x42, TEST_SIZE); + + napi_value buffer; + napi_status status = napi_create_external_buffer( + env, TEST_SIZE, data, my_finalizer, NULL, &buffer); + + if (status != napi_ok) { + free(data); + napi_throw_error(env, NULL, "napi_create_external_buffer failed"); + return NULL; + } + + return buffer; +} + +static napi_value get_finalizer_called(napi_env env, napi_callback_info info) { + napi_value result; + napi_get_boolean(env, finalizer_called, &result); + return result; +} + +static napi_value get_original_data_addr(napi_env env, napi_callback_info info) { + napi_value result; + napi_create_bigint_uint64(env, (uint64_t)received_data, &result); + return result; +} + +static napi_value init(napi_env env, napi_value exports) { + napi_property_descriptor props[] = { + { "createExternalBuffer", NULL, create_external_buffer, NULL, NULL, NULL, napi_default, NULL }, + { "getFinalizerCalled", NULL, get_finalizer_called, NULL, NULL, NULL, napi_default, NULL }, + { "getOriginalDataAddr", NULL, get_original_data_addr, NULL, NULL, NULL, napi_default, NULL }, + }; + napi_define_properties(env, exports, 3, props); + return exports; +} + +NAPI_MODULE(test_external_buffer, init) +`, + "test.js": ` +const addon = require('./build/Release/test_external_buffer.node'); + +// Create an external buffer +let buf = addon.createExternalBuffer(); + +// Verify the buffer has the correct length +if (buf.length !== 1024) { + console.log("FAIL: Buffer length is " + buf.length + " instead of 1024"); + process.exit(1); +} + +// Verify the data wasn't corrupted +for (let i = 0; i < buf.length; i++) { + if (buf[i] !== 0x42) { + console.log("FAIL: Data corrupted at index " + i + ": expected 0x42, got " + buf[i].toString(16)); + process.exit(1); + } +} + +console.log("PASS: Buffer created with correct data"); + +// Clear reference and trigger GC +buf = null; +if (process.isBun) { + Bun.gc(true); +} else if (global.gc) { + global.gc(); +} + +// Small delay to let finalizers run +setTimeout(() => { + console.log("PASS: Test completed successfully"); +}, 100); +`, + "package.json": JSON.stringify({ + name: "test-external-buffer", + version: "1.0.0", + private: true, + }), + }); + + // Build the addon + const buildResult = Bun.spawnSync({ + cmd: ["npx", "node-gyp", "rebuild"], + cwd: String(dir), + env: bunEnv, + stdio: ["inherit", "pipe", "pipe"], + }); + + if (!buildResult.success) { + console.log("Build stderr:", buildResult.stderr.toString()); + throw new Error("node-gyp build failed"); + } + + // Run the test + await using proc = Bun.spawn({ + cmd: [bunExe(), "--expose-gc", "test.js"], + cwd: String(dir), + env: bunEnv, + stdio: ["inherit", "pipe", "pipe"], + }); + + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + + if (stderr) { + console.log("Test stderr:", stderr); + } + + expect(stdout).toContain("PASS: Buffer created with correct data"); + expect(stdout).not.toContain("FAIL"); + expect(exitCode).toBe(0); + }, 60000); +});