diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index af72fb3c985a..ed019c91c606 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -2599,6 +2599,14 @@ impl JSValue { Bun__JSValue__getArrayBufferViewByteOffset(self) } + /// Force a typed array out of JSC's "fast" mode so its data pointer stays + /// stable for the view's lifetime (fast-mode storage is abandoned when the + /// backing buffer is materialized). Returns `false` only when `self` is a + /// view whose backing buffer could not be allocated; non-views are a no-op. + pub fn materialize_array_buffer_view_buffer(self) -> bool { + Bun__JSValue__materializeArrayBufferViewBuffer(self) + } + // ── Formatting. ──────────────────────────────────── #[inline] pub fn fmt_string(self, global: &JSGlobalObject) -> StringFormatter<'_> { @@ -2725,6 +2733,7 @@ unsafe extern "C" { global: &JSGlobalObject, ) -> JSValue; safe fn Bun__JSValue__getArrayBufferViewByteOffset(this: JSValue) -> usize; + safe fn Bun__JSValue__materializeArrayBufferViewBuffer(this: JSValue) -> bool; safe fn Bun__Process__queueNextTick1(global: &JSGlobalObject, func: JSValue, arg: JSValue); fn Bun__JSValue__deserialize( global: *const JSGlobalObject, diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index b3f4c90e1823..c084a885ee86 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -6466,6 +6466,17 @@ CPP_DECL JSC::EncodedJSValue Bun__JSValue__getArrayBufferViewBuffer(JSC::Encoded return JSValue::encode(JSValue()); } +CPP_DECL bool Bun__JSValue__materializeArrayBufferViewBuffer(JSC::EncodedJSValue encoded) +{ + JSC::JSValue value = JSValue::decode(encoded); + if (!value || !value.isCell()) + return true; + if (auto* view = dynamicDowncast(value.asCell())) + return view->possiblySharedBuffer() != nullptr; + // Not a view: nothing to materialize; the caller's type checks decide. + return true; +} + CPP_DECL size_t Bun__JSValue__getArrayBufferViewByteOffset(JSC::EncodedJSValue encoded) { JSC::JSValue value = JSValue::decode(encoded); diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 1daf97c087d0..e7f5eebd156c 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -2268,15 +2268,27 @@ extern "C" napi_status napi_create_buffer(napi_env env, size_t length, void** data, napi_value* result) { - NAPI_PREAMBLE(env); + NAPI_PREAMBLE_NO_THROW_SCOPE(env); + Zig::GlobalObject* globalObject = toJS(env); + auto scope = DECLARE_THROW_SCOPE(env->vm()); + NAPI_RETURN_IF_EXCEPTION_WITH_SCOPE(env, scope); NAPI_CHECK_ARG(env, result); - Zig::GlobalObject* globalObject = toJS(env); auto* subclassStructure = globalObject->JSBufferSubclassStructure(); + // Create the backing ArrayBuffer up front and move it into the view, so the + // view is never in fast mode and the data pointer stays valid for the + // buffer's lifetime, as in Node. // In Node.js, napi_create_buffer is uninitialized memory. - auto* uint8Array = JSC::JSUint8Array::createUninitialized(globalObject, subclassStructure, length); - NAPI_RETURN_STATUS_IF_EXCEPTION(env, napi_generic_failure); + RefPtr arrayBuffer = ArrayBuffer::tryCreateUninitialized(length, 1); + if (!arrayBuffer) { + // Node leaves a pending exception for a failed allocation. + JSC::throwOutOfMemoryError(globalObject, scope); + RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure)); + } + + auto* uint8Array = JSC::JSUint8Array::create(globalObject, subclassStructure, WTF::move(arrayBuffer), 0, length); + RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure)); if (data != nullptr) { // Node.js' code looks like this: @@ -2286,7 +2298,42 @@ extern "C" napi_status napi_create_buffer(napi_env env, size_t length, } *result = toNapi(uint8Array, globalObject); - NAPI_RETURN_SUCCESS(env); + return napi_set_last_error(env, napi_ok); +} + +extern "C" napi_status napi_create_buffer_copy(napi_env env, size_t length, + const void* data, + void** result_data, + napi_value* result) +{ + NAPI_PREAMBLE_NO_THROW_SCOPE(env); + Zig::GlobalObject* globalObject = toJS(env); + auto scope = DECLARE_THROW_SCOPE(env->vm()); + NAPI_RETURN_IF_EXCEPTION_WITH_SCOPE(env, scope); + NAPI_CHECK_ARG(env, result); + + auto* subclassStructure = globalObject->JSBufferSubclassStructure(); + + // As above: one allocation, never fast mode, stable data pointer. + RefPtr arrayBuffer = ArrayBuffer::tryCreateUninitialized(length, 1); + if (!arrayBuffer) { + // Node leaves a pending exception for a failed allocation. + JSC::throwOutOfMemoryError(globalObject, scope); + RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure)); + } + if (length > 0) { + memcpy(arrayBuffer->data(), data, length); + } + + auto* uint8Array = JSC::JSUint8Array::create(globalObject, subclassStructure, WTF::move(arrayBuffer), 0, length); + RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure)); + + if (result_data != nullptr) { + *result_data = length > 0 ? uint8Array->typedVector() : nullptr; + } + + *result = toNapi(uint8Array, globalObject); + return napi_set_last_error(env, napi_ok); } // SharedTask subclass with an armed flag so that the destructor can be diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index 990c739f2ca0..4ad87609e8e7 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -1445,6 +1445,12 @@ extern "C" fn napi_get_typedarray_info( } let _keep = jsc::EnsureStillAlive(typedarray); + // Keep the pointer valid for the view's lifetime, as in Node: the + // arraybuffer out-param below would otherwise invalidate it. + if !maybe_data.is_null() && !typedarray.materialize_array_buffer_view_buffer() { + return env.generic_failure(); + } + let Some(array_buffer) = typedarray.as_array_buffer(env.to_js()) else { return env.invalid_arg(); }; @@ -2034,42 +2040,13 @@ unsafe extern "C" { finalize_hint: *mut c_void, result: *mut napi_value, ) -> napi_status; -} - -#[unsafe(no_mangle)] -extern "C" fn napi_create_buffer_copy( - env_: napi_env, - length: usize, - data: *const u8, - result_data: *mut *mut c_void, - result_: *mut napi_value, -) -> napi_status { - bun_output::scoped_log!(napi, "napi_create_buffer_copy: {}", length); - let env = preamble!(env_); - let result = get_out!(env, result_); - let buffer: JSValue = match JSValue::create_buffer_from_length(env.to_js(), length) { - Ok(b) => b, - Err(_) => return env.generic_failure(), - }; - if let Some(mut array_buf) = buffer.as_array_buffer(env.to_js()) { - if length > 0 { - // SAFETY: caller guarantees `data` points to at least `length` bytes. - let src = unsafe { bun_core::ffi::slice(data, length) }; - array_buf.slice_mut()[..length].copy_from_slice(src); - } - write_out( - result_data, - if length > 0 { - array_buf.ptr.cast::() - } else { - ptr::null_mut() - }, - ); - } - - result.set(env, buffer); - - env.ok() + pub(super) fn napi_create_buffer_copy( + env: napi_env, + length: usize, + data: *const c_void, + result_data: *mut *mut c_void, + result: *mut napi_value, + ) -> napi_status; } unsafe extern "C" { @@ -2086,6 +2063,10 @@ extern "C" fn napi_get_buffer_info( bun_output::scoped_log!(napi, "napi_get_buffer_info"); let env = get_env!(env_); let value = value_.get(); + // Keep the pointer valid for the buffer's lifetime, as in Node. + if !data.is_null() && !value.materialize_array_buffer_view_buffer() { + return env.generic_failure(); + } let Some(array_buf) = value.as_array_buffer(env.to_js()) else { return NapiEnv::set_last_error(Some(env), NapiStatus::invalid_arg); }; diff --git a/test/napi/napi-app/standalone_tests.cpp b/test/napi/napi-app/standalone_tests.cpp index 81ac5fcd2b44..d6ecd232fce4 100644 --- a/test/napi/napi-app/standalone_tests.cpp +++ b/test/napi/napi-app/standalone_tests.cpp @@ -3214,6 +3214,109 @@ test_dataview_info_byte_offset(const Napi::CallbackInfo &info) { return ok(env); } +// Writes through the data pointer that napi_get_typedarray_info returns (with +// every out-param requested, like napi-rs does) and reads the bytes back +// through the JS view. Small typed arrays use JSC's "fast" GC-heap storage, +// which is abandoned when the backing ArrayBuffer is materialized; the pointer +// must reflect the storage that survives. +static napi_value +test_typedarray_info_write_visibility(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + napi_value typedarray = info[1]; + + napi_typedarray_type type; + size_t length = 0; + void *data = nullptr; + napi_value arraybuffer = nullptr; + size_t byte_offset = SIZE_MAX; + NODE_API_CALL(env, + napi_get_typedarray_info(env, typedarray, &type, &length, &data, + &arraybuffer, &byte_offset)); + if (length > 0) { + memset(data, 0x2a, length); + } + + uint32_t first = 0, last = 0; + napi_value element; + if (length > 0) { + NODE_API_CALL(env, napi_get_element(env, typedarray, 0, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, &first)); + NODE_API_CALL(env, + napi_get_element(env, typedarray, + static_cast(length) - 1, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, &last)); + } + printf("length=%zu first=%u last=%u\n", length, first, last); + return ok(env); +} + +// The pointer from napi_get_buffer_info must stay valid after the view's +// backing ArrayBuffer is materialized (here via the arraybuffer out-param of +// napi_get_typedarray_info; JS touching .buffer does the same). +static napi_value +test_buffer_info_pointer_stability(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + napi_value buffer = info[1]; + + void *data = nullptr; + size_t length = 0; + NODE_API_CALL(env, napi_get_buffer_info(env, buffer, &data, &length)); + + napi_value arraybuffer = nullptr; + NODE_API_CALL(env, napi_get_typedarray_info(env, buffer, nullptr, nullptr, + nullptr, &arraybuffer, nullptr)); + + uint32_t first = 0, last = 0; + napi_value element; + if (length > 0) { + memset(data, 0x2b, length); + NODE_API_CALL(env, napi_get_element(env, buffer, 0, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, &first)); + NODE_API_CALL(env, + napi_get_element(env, buffer, + static_cast(length) - 1, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, &last)); + } + printf("length=%zu first=%u last=%u\n", length, first, last); + return ok(env); +} + +// Pointers returned by napi_create_buffer and napi_create_buffer_copy must +// also stay valid after the backing ArrayBuffer is materialized. +static napi_value +test_create_buffer_pointer_stability(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + napi_value element; + + void *data = nullptr; + napi_value buffer = nullptr; + NODE_API_CALL(env, napi_create_buffer(env, 64, &data, &buffer)); + napi_value arraybuffer = nullptr; + NODE_API_CALL(env, napi_get_typedarray_info(env, buffer, nullptr, nullptr, + nullptr, &arraybuffer, nullptr)); + memset(data, 0x2c, 64); + uint32_t last = 0; + NODE_API_CALL(env, napi_get_element(env, buffer, 63, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, &last)); + printf("create_buffer last=%u\n", last); + + const uint8_t src[16] = {0}; + void *copy_data = nullptr; + napi_value copy = nullptr; + NODE_API_CALL( + env, napi_create_buffer_copy(env, sizeof src, src, ©_data, ©)); + napi_value copy_arraybuffer = nullptr; + NODE_API_CALL(env, napi_get_typedarray_info(env, copy, nullptr, nullptr, + nullptr, ©_arraybuffer, + nullptr)); + memset(copy_data, 0x2d, sizeof src); + uint32_t copy_last = 0; + NODE_API_CALL(env, napi_get_element(env, copy, 15, &element)); + NODE_API_CALL(env, napi_get_value_uint32(env, element, ©_last)); + printf("create_buffer_copy last=%u\n", copy_last); + return ok(env); +} + static napi_value test_napi_float16_array(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); // napi_float16_array == 11; cast so older headers without the member compile. @@ -4024,6 +4127,9 @@ test_reference_ref_after_collect(const Napi::CallbackInfo &info) { void register_standalone_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, test_typedarray_info_byte_offset); + REGISTER_FUNCTION(env, exports, test_typedarray_info_write_visibility); + REGISTER_FUNCTION(env, exports, test_buffer_info_pointer_stability); + REGISTER_FUNCTION(env, exports, test_create_buffer_pointer_stability); REGISTER_FUNCTION(env, exports, test_dataview_info_byte_offset); REGISTER_FUNCTION(env, exports, test_napi_float16_array); REGISTER_FUNCTION(env, exports, test_create_arraybuffer_zeroed); diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 4ac4b1d4e702..1e044ea4a4bf 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -634,6 +634,8 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { // leaves one behind leaks. An addon that uses the handle after napi_closing // (a release, say) therefore touches freed memory, in node as well: the docs // say to make no further use of it. Bun-only: reads bun's live tsfn count. + // The spawned run takes >10s under a debug+ASAN bun, so the 5s default + // times out on slow machines. it("frees an orphaned threadsafe function whose last reference a call consumed", async () => { await using proc = spawn({ cmd: [bunExe(), join(__dirname, "napi-app/main.js"), "test_threadsafe_function_orphan_leak", "[]"], @@ -663,7 +665,7 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { exitCode: 0, signalCode: null, }); - }); + }, 30_000); // napi_create_threadsafe_function once the env has torn its threadsafe // functions down (here: from a cleanup hook that a threadsafe function's @@ -915,6 +917,49 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { ); }); + // Small typed arrays use JSC's "fast" mode, whose GC-heap storage is + // abandoned when the backing ArrayBuffer is materialized. The data pointer + // NAPI hands out must point at the storage that survives, or native writes + // are silently dropped (see issue #37151). + it("returns a data pointer whose writes are visible through small typed arrays", async () => { + // 999/1000 straddle JSC's fastSizeLimit; 2048 is always wasteful mode. + await Promise.all( + [16, 999, 1000, 2048].map(async size => { + const output = await checkSameOutput("test_typedarray_info_write_visibility", `[new Uint8Array(${size})]`); + expect(output).toBe(`length=${size} first=42 last=42`); + }), + ); + }); + + it("returns a data pointer at the reported byte offset for small typed arrays", async () => { + await Promise.all( + [ + "new Uint8Array(0)", + "new Uint8Array(64)", + "new Int32Array(8)", + // offset view derived from a small array + "new Uint8Array(64).subarray(16)", + ].map(async expr => { + const output = await checkSameOutput("test_typedarray_info_byte_offset", `[${expr}]`); + expect(output).toEndWith("data_is_arraybuffer_data_plus_byte_offset=true"); + }), + ); + }); + + it("napi_get_buffer_info pointer stays valid after the backing ArrayBuffer is materialized", async () => { + await Promise.all( + ["Buffer.alloc(32)", "new Uint8Array(32)"].map(async expr => { + const output = await checkSameOutput("test_buffer_info_pointer_stability", `[${expr}]`); + expect(output).toBe("length=32 first=43 last=43"); + }), + ); + }); + + it("napi_create_buffer and napi_create_buffer_copy pointers stay valid after the backing ArrayBuffer is materialized", async () => { + const output = await checkSameOutput("test_create_buffer_pointer_stability", []); + expect(output.split(/\r?\n/)).toEqual(["create_buffer last=44", "create_buffer_copy last=45"]); + }); + it("reports the view's byte offset into its backing buffer", async () => { const output = await checkSameOutput( "test_typedarray_info_byte_offset", @@ -986,24 +1031,28 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { }); it("has the right lifetime", async () => { - await checkSameOutput("test_wrap_lifetime_without_ref", []); - await checkSameOutput("test_wrap_lifetime_with_weak_ref", []); - await checkSameOutput("test_wrap_lifetime_with_strong_ref", []); - await checkSameOutput("test_remove_wrap_lifetime_with_weak_ref", []); - await checkSameOutput("test_remove_wrap_lifetime_with_strong_ref", []); - // check that napi finalizers also run at VM exit, even if they didn't get run by GC - await checkSameOutput("test_ref_deleted_in_cleanup", []); - // check that calling napi_delete_ref in the ref's finalizer is not use-after-free - await checkSameOutput("test_ref_deleted_in_async_finalize", []); + await Promise.all([ + checkSameOutput("test_wrap_lifetime_without_ref", []), + checkSameOutput("test_wrap_lifetime_with_weak_ref", []), + checkSameOutput("test_wrap_lifetime_with_strong_ref", []), + checkSameOutput("test_remove_wrap_lifetime_with_weak_ref", []), + checkSameOutput("test_remove_wrap_lifetime_with_strong_ref", []), + // napi finalizers also run at VM exit, even if they didn't get run by GC + checkSameOutput("test_ref_deleted_in_cleanup", []), + // calling napi_delete_ref in the ref's finalizer is not use-after-free + checkSameOutput("test_ref_deleted_in_async_finalize", []), + ]); }); }); describe("napi_define_class", () => { it("handles edge cases in the constructor", async () => { - await checkSameOutput("test_napi_class", []); - await checkSameOutput("test_subclass_napi_class", []); - await checkSameOutput("test_napi_class_non_constructor_call", []); - await checkSameOutput("test_reflect_construct_napi_class", []); + await Promise.all([ + checkSameOutput("test_napi_class", []), + checkSameOutput("test_subclass_napi_class", []), + checkSameOutput("test_napi_class_non_constructor_call", []), + checkSameOutput("test_reflect_construct_napi_class", []), + ]); }); it("does not crash with Reflect.construct when newTarget has no prototype", async () => {