Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/jsc/JSValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
robobun marked this conversation as resolved.
pub fn materialize_array_buffer_view_buffer(self) -> bool {
Bun__JSValue__materializeArrayBufferViewBuffer(self)
}

// ── Formatting. ────────────────────────────────────
#[inline]
pub fn fmt_string(self, global: &JSGlobalObject) -> StringFormatter<'_> {
Expand Down Expand Up @@ -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,
Expand Down
11 changes: 11 additions & 0 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<JSArrayBufferView>(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);
Expand Down
49 changes: 48 additions & 1 deletion src/jsc/bindings/napi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2274,8 +2274,20 @@
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.
Comment thread
robobun marked this conversation as resolved.
// In Node.js, napi_create_buffer is uninitialized memory.
auto* uint8Array = JSC::JSUint8Array::createUninitialized(globalObject, subclassStructure, length);
RefPtr<ArrayBuffer> arrayBuffer = ArrayBuffer::tryCreateUninitialized(length, 1);
if (!arrayBuffer) {
// Node leaves a pending exception for a failed allocation.
auto scope = DECLARE_THROW_SCOPE(env->vm());
JSC::throwOutOfMemoryError(globalObject, scope);
RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure));
return napi_set_last_error(env, napi_generic_failure);
}

Check warning on line 2288 in src/jsc/bindings/napi.cpp

View check run for this annotation

Claude / Claude Code Review

Nested ThrowScope inside NAPI_PREAMBLE trips validateExceptionChecks (and leaves a dead return)

The OOM blocks in `napi_create_buffer` (here) and `napi_create_buffer_copy` (:2317-2323) nest a block-scoped `DECLARE_THROW_SCOPE` inside `NAPI_PREAMBLE`'s `TopExceptionScope`, which trips `verifyExceptionCheckNeedIsSatisfied` under `BUN_JSC_validateExceptionChecks=1` (the inner `~ThrowScope` runs `simulateThrow()` after `RETURN_IF_EXCEPTION` returns, then the outer `~TopExceptionScope` asserts — same hazard documented at :93-97 and NodeSqlite.cpp:1907). It also leaves the trailing `return napi_
Comment thread
robobun marked this conversation as resolved.

auto* uint8Array = JSC::JSUint8Array::create(globalObject, subclassStructure, WTF::move(arrayBuffer), 0, length);
NAPI_RETURN_STATUS_IF_EXCEPTION(env, napi_generic_failure);

if (data != nullptr) {
Expand All @@ -2289,6 +2301,41 @@
NAPI_RETURN_SUCCESS(env);
}

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(env);
NAPI_CHECK_ARG(env, result);

Zig::GlobalObject* globalObject = toJS(env);
auto* subclassStructure = globalObject->JSBufferSubclassStructure();

// As above: one allocation, never fast mode, stable data pointer.
RefPtr<ArrayBuffer> arrayBuffer = ArrayBuffer::tryCreateUninitialized(length, 1);
if (!arrayBuffer) {
// Node leaves a pending exception for a failed allocation.
auto scope = DECLARE_THROW_SCOPE(env->vm());
JSC::throwOutOfMemoryError(globalObject, scope);
RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_generic_failure));
return napi_set_last_error(env, napi_generic_failure);
}
if (length > 0) {
memcpy(arrayBuffer->data(), data, length);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

auto* uint8Array = JSC::JSUint8Array::create(globalObject, subclassStructure, WTF::move(arrayBuffer), 0, length);
NAPI_RETURN_STATUS_IF_EXCEPTION(env, napi_generic_failure);

if (result_data != nullptr) {
*result_data = length > 0 ? uint8Array->typedVector() : nullptr;
}

*result = toNapi(uint8Array, globalObject);
NAPI_RETURN_SUCCESS(env);
}

// SharedTask subclass with an armed flag so that the destructor can be
// armed only after the wrapping JS object (JSUint8Array / JSArrayBuffer)
// is successfully created. If creation throws, the destructor runs
Expand Down
53 changes: 17 additions & 36 deletions src/runtime/napi/napi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
robobun marked this conversation as resolved.
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();
};
Expand Down Expand Up @@ -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::<c_void>()
} 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" {
Expand All @@ -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);
};
Expand Down
106 changes: 106 additions & 0 deletions test/napi/napi-app/standalone_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t>(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<uint32_t>(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, &copy_data, &copy));
napi_value copy_arraybuffer = nullptr;
NODE_API_CALL(env, napi_get_typedarray_info(env, copy, nullptr, nullptr,
nullptr, &copy_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, &copy_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.
Expand Down Expand Up @@ -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);
Expand Down
64 changes: 50 additions & 14 deletions test/napi/napi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,8 @@
// 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", "[]"],
Expand Down Expand Up @@ -663,7 +665,7 @@
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
Expand Down Expand Up @@ -915,6 +917,36 @@
);
});

// 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 () => {
for (const size of [16, 999, 2048]) {
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 () => {
for (const expr of ["new Uint8Array(0)", "new Uint8Array(64)", "new Int32Array(8)"]) {
const output = await checkSameOutput("test_typedarray_info_byte_offset", `[${expr}]`);
expect(output).toEndWith("data_is_arraybuffer_data_plus_byte_offset=true");
}
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it("napi_get_buffer_info pointer stays valid after the backing ArrayBuffer is materialized", async () => {
for (const expr of ["Buffer.alloc(32)", "new Uint8Array(32)"]) {
const output = await checkSameOutput("test_buffer_info_pointer_stability", `[${expr}]`);
expect(output).toBe("length=32 first=43 last=43");

Check warning on line 941 in test/napi/napi.test.ts

View check run for this annotation

Claude / Claude Code Review

New tests use sequential checkSameOutput loops that this PR eliminates elsewhere

The new tests at lines ~924/931/937 loop 3/3/2 sequential `checkSameOutput` calls — the same shape this PR just converted to `Promise.all([...])` two blocks below (lines ~1021 and ~1038) because ~2s per debug+ASAN spawn stacks past the 5s default. Since each iteration is independent, `await Promise.all([16, 999, 2048].map(size => checkSameOutput(...)))` (or `it.each` under the enclosing `describe.concurrent`) keeps them under the default and matches the restructuring already applied here.
Comment thread
robobun marked this conversation as resolved.
Outdated
}
});

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",
Expand Down Expand Up @@ -986,24 +1018,28 @@
});

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 () => {
Expand Down
Loading