Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
45 changes: 45 additions & 0 deletions src/jsc/bindings/napi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <JavaScriptCore/Exception.h>
#include <JavaScriptCore/ExceptionHelpers.h>
#include <JavaScriptCore/ExceptionScope.h>
#include <JavaScriptCore/FrameTracers.h>
#include <JavaScriptCore/FunctionConstructor.h>
#include <JavaScriptCore/Heap.h>
#include <JavaScriptCore/Identifier.h>
Expand Down Expand Up @@ -3321,6 +3322,50 @@ extern "C" void napi_internal_check_gc(napi_env env)
env->checkGC();
}

// Node.js does not gate napi_create_string_* behind NAPI_PREAMBLE, so a VM
// exception may already be pending. SuspendExceptionScope stashes it for the
// allocation and restores it on return.
Comment thread
robobun marked this conversation as resolved.
template<typename CharT, typename Maker>
static JSC::EncodedJSValue napiCreateStringWithSuspendedException(napi_env env, const CharT* ptr, size_t length, Maker make)
{
auto& vm = env->vm();
JSC::SuspendExceptionScope suspend(vm);
if (!length) {
return JSValue::encode(jsEmptyString(vm));
}
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
WTF::String str = make(std::span<const CharT>(ptr, length));
if (scope.exception() || str.isNull()) [[unlikely]] {
scope.clearExceptionExceptTermination();
return {};
}
return JSValue::encode(jsString(vm, WTF::move(str)));
}

extern "C" JSC::EncodedJSValue napi_internal_create_string_latin1(napi_env env, const Latin1Character* ptr, size_t length)
{
return napiCreateStringWithSuspendedException(env, ptr, length, [](std::span<const Latin1Character> bytes) {
return WTF::String(bytes);
});
}

extern "C" JSC::EncodedJSValue napi_internal_create_string_utf8(napi_env env, const char* ptr, size_t length)
{
return napiCreateStringWithSuspendedException(env, ptr, length, [](std::span<const char> bytes) {
if (simdutf::validate_ascii(bytes.data(), bytes.size())) {
return WTF::String(std::span<const Latin1Character>(reinterpret_cast<const Latin1Character*>(bytes.data()), bytes.size()));
}
return WTF::String::fromUTF8ReplacingInvalidSequences(std::span<const Latin1Character>(reinterpret_cast<const Latin1Character*>(bytes.data()), bytes.size()));
});
}

extern "C" JSC::EncodedJSValue napi_internal_create_string_utf16(napi_env env, const char16_t* ptr, size_t length)
{
return napiCreateStringWithSuspendedException(env, ptr, length, [](std::span<const char16_t> units) {
return WTF::String(units);
});
}

extern "C" bool NapiEnv__hasPendingException(napi_env env)
{
if (env->hasPendingException()) {
Expand Down
60 changes: 23 additions & 37 deletions src/runtime/napi/napi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use bun_collections::linear_fifo::DynamicBuffer;
use bun_event_loop::ConcurrentTask::AutoDeinit;
use bun_event_loop::{TaskTag, Taskable, task_tag};
use bun_io::KeepAlive;
use bun_jsc::StringJsc;
use bun_jsc::event_loop::{ConcurrentTaskItem as ConcurrentTask, EventLoop};
use bun_jsc::virtual_machine::VirtualMachine;
use bun_jsc::{
Expand Down Expand Up @@ -74,6 +73,11 @@ unsafe extern "C" {
fn NapiEnv__deref(env: *mut NapiEnv);
fn NapiEnv__ref(env: *mut NapiEnv);
fn napi_set_last_error(env: napi_env, status: NapiStatus) -> napi_status;
fn napi_internal_create_string_latin1(env: *mut NapiEnv, ptr: *const u8, len: usize)
-> JSValue;
fn napi_internal_create_string_utf8(env: *mut NapiEnv, ptr: *const u8, len: usize) -> JSValue;
fn napi_internal_create_string_utf16(env: *mut NapiEnv, ptr: *const u16, len: usize)
-> JSValue;
}

impl NapiEnv {
Expand Down Expand Up @@ -652,22 +656,13 @@ pub(super) extern "C" fn napi_create_string_latin1(
bstr::BStr::new(slice)
);

if slice.is_empty() {
let js = match bun_core::String::empty().to_js(env.to_js()) {
Ok(v) => v,
Err(_) => return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure),
};
result.set(env, js);
return env.ok();
}

let (mut string, bytes) = bun_core::String::create_uninitialized_latin1(slice.len());
bytes.copy_from_slice(slice);

let js = match string.transfer_to_js(env.to_js()) {
Ok(v) => v,
Err(_) => return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure),
// SAFETY: env is non-null (checked above); slice ptr/len from a live &[u8].
let js = unsafe {
napi_internal_create_string_latin1(env.as_mut_ptr(), slice.as_ptr(), slice.len())
};
if js == JSValue::ZERO {
return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure);
}
Comment thread
robobun marked this conversation as resolved.
result.set(env, js);
env.ok()
}
Expand Down Expand Up @@ -704,12 +699,13 @@ pub(super) extern "C" fn napi_create_string_utf8(

bun_output::scoped_log!(napi, "napi_create_string_utf8: {}", bstr::BStr::new(slice));

let global_object = env.to_js();
let string = match jsc::bun_string_jsc::create_utf8_for_js(global_object, slice) {
Ok(v) => v,
Err(_) => return NapiEnv::set_last_error(Some(env), NapiStatus::pending_exception),
};
result.set(env, string);
// SAFETY: env is non-null (checked above); slice ptr/len from a live &[u8].
let js =
unsafe { napi_internal_create_string_utf8(env.as_mut_ptr(), slice.as_ptr(), slice.len()) };
if js == JSValue::ZERO {
return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure);
}
result.set(env, js);
env.ok()
}

Expand Down Expand Up @@ -753,22 +749,12 @@ pub(super) extern "C" fn napi_create_string_utf16(
);
}

if slice.is_empty() {
let js = match bun_core::String::empty().to_js(env.to_js()) {
Ok(v) => v,
Err(_) => return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure),
};
result.set(env, js);
return env.ok();
// SAFETY: env is non-null (checked above); slice ptr/len from a live &[u16].
let js =
unsafe { napi_internal_create_string_utf16(env.as_mut_ptr(), slice.as_ptr(), slice.len()) };
if js == JSValue::ZERO {
return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure);
}

let (mut string, chars) = bun_core::String::create_uninitialized_utf16(slice.len());
chars.copy_from_slice(slice);

let js = match string.transfer_to_js(env.to_js()) {
Ok(v) => v,
Err(_) => return NapiEnv::set_last_error(Some(env), NapiStatus::generic_failure),
};
result.set(env, js);
env.ok()
}
Expand Down
75 changes: 75 additions & 0 deletions test/napi/napi-app/standalone_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2892,6 +2892,80 @@ static napi_value test_pending_exception_gate(const Napi::CallbackInfo &info) {
return ok(env);
}

// napi_create_string_* must succeed (napi_ok) and preserve a pending
// exception that already lives on the VM. Two ways to put one there without
// relying on Bun internals:
// (a) napi_throw(E) then napi_call_function(), which returns status 10 and
// promotes E into the VM, and
// (b) napi_create_bigint_words() with a length past the engine cap, which
// throws a RangeError directly into the VM and returns status 10.
// In debug/asan builds the Rust string creators previously routed through a
// validation scope that asserts "no VM exception" on success and aborted.
static napi_value test_create_string_with_vm_exception(
const Napi::CallbackInfo &info) {
napi_env env = info.Env();

napi_value global, isNaN;
NODE_API_CALL(env, napi_get_global(env, &global));
NODE_API_CALL(env, napi_get_named_property(env, global, "isNaN", &isNaN));

const char *labels[] = {"call_function", "bigint_words"};
for (int route = 0; route < 2; route++) {
// Arm a VM-level exception.
if (route == 0) {
napi_value msg, err;
NODE_API_CALL(env, napi_create_string_utf8(env, "E1", 2, &msg));
NODE_API_CALL(env, napi_create_error(env, nullptr, msg, &err));
NODE_API_CALL(env, napi_throw(env, err));
napi_value r;
napi_status st = napi_call_function(env, global, isNaN, 0, nullptr, &r);
printf("%s napi_call_function: status=%d\n", labels[route], (int)st);
} else {
static const uint64_t word = 1;
napi_value big;
napi_status st =
napi_create_bigint_words(env, 0, (size_t)INT_MAX, &word, &big);
printf("%s napi_create_bigint_words: status=%d\n", labels[route],
(int)st);
}

// String creators must succeed with the exception still pending.
napi_value s;
static const char16_t wide[] = {'x', 0};
napi_status st = napi_create_string_utf8(env, "x", 1, &s);
printf("%s napi_create_string_utf8: status=%d\n", labels[route], (int)st);
st = napi_create_string_latin1(env, "x", 1, &s);
printf("%s napi_create_string_latin1: status=%d\n", labels[route],
(int)st);
st = napi_create_string_utf16(env, wide, 1, &s);
printf("%s napi_create_string_utf16: status=%d\n", labels[route], (int)st);
// Non-ASCII path (utf8 decoder takes a different branch).
st = napi_create_string_utf8(env, "\xc3\xa9", 2, &s);
printf("%s napi_create_string_utf8_nonascii: status=%d\n", labels[route],
(int)st);

bool pending = false;
napi_is_exception_pending(env, &pending);
printf("%s pending_after_create=%s\n", labels[route],
pending ? "true" : "false");

napi_value exc;
napi_get_and_clear_last_exception(env, &exc);
if (route == 0) {
// Only the napi_throw'd Error has a portable message across engines.
napi_value exc_msg;
if (napi_coerce_to_string(env, exc, &exc_msg) == napi_ok) {
char buf[64];
size_t n;
napi_get_value_string_utf8(env, exc_msg, buf, sizeof(buf), &n);
printf("%s exception=%s\n", labels[route], buf);
}
}
}

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.
Expand Down Expand Up @@ -3562,6 +3636,7 @@ void register_standalone_tests(Napi::Env env, Napi::Object exports) {
REGISTER_FUNCTION(env, exports,
test_external_buffer_with_pending_exception);
REGISTER_FUNCTION(env, exports, test_pending_exception_gate);
REGISTER_FUNCTION(env, exports, test_create_string_with_vm_exception);
REGISTER_FUNCTION(env, exports, test_napi_get_named_property_copied_string);
REGISTER_FUNCTION(env, exports, test_issue_25933);
REGISTER_FUNCTION(env, exports, test_napi_make_callback_status);
Expand Down
23 changes: 23 additions & 0 deletions test/napi/napi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,29 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => {
expect(result).toContain("side_effect arr[7]=undefined");
expect(result).toContain("side_effect script_ran=false");
});

it("napi_create_string_* succeeds while a VM exception is pending", async () => {
// Two routes put an exception into the VM (napi_call_function promotes a
// napi_throw'd value; napi_create_bigint_words over the engine cap
// throws RangeError). The string creators must return napi_ok and leave
// the exception pending, matching Node. In debug/asan builds Bun's
// validation scope previously aborted here.
const result = await checkSameOutput("test_create_string_with_vm_exception", []);
expect(result).toContain("call_function napi_call_function: status=10");
expect(result).toContain("bigint_words napi_create_bigint_words: status=10");
for (const route of ["call_function", "bigint_words"]) {
for (const fn of [
"napi_create_string_utf8",
"napi_create_string_latin1",
"napi_create_string_utf16",
"napi_create_string_utf8_nonascii",
]) {
expect(result).toContain(`${route} ${fn}: status=0`);
}
expect(result).toContain(`${route} pending_after_create=true`);
}
expect(result).toContain("call_function exception=Error: E1");
});
});

describe("napi_async_work", () => {
Expand Down
Loading