Repository navigation
Conversation
…w info APIs napi_create_typedarray previously delegated validation to JSGenericTypedArrayView::create, which throws a raw VM exception. The thrown RangeError carried no .code and the call returned napi_pending_exception (10), while Node.js validates first, throws a RangeError with ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT / _LENGTH and returns napi_generic_failure (9). Because the exception landed in the VM rather than env->m_pendingException, a follow-up Rust-side N-API call (e.g. napi_create_string_utf8) tripped the generated wrapper's assert_exception_presence_matches in the asserts build and aborted. Now validate byte_offset alignment and length * size_of_element + byte_offset <= byteLength up front, throw via napi_throw_range_error (env-stashed), and return napi_generic_failure, matching Node.js. napi_get_typedarray_info and napi_get_dataview_info accepted any value as_array_buffer() would unwrap, so a Uint8Array passed to napi_get_dataview_info (or a DataView/ArrayBuffer passed to napi_get_typedarray_info) returned napi_ok with the wrong geometry. Check the JSType first and return napi_invalid_arg, matching Node.js. napi_create_typedarray and napi_create_dataview now return napi_invalid_arg (not napi_arraybuffer_expected) when arraybuffer is not an ArrayBuffer, matching Node.js.
WalkthroughChangesN-API typed array and DataView creation and inspection now apply stricter argument, alignment, and buffer-range validation. New standalone and integration tests cover invalid inputs, exception codes, metadata checks, and successful exact-fit creation. N-API view validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:35 PM PT - Jul 14th, 2026
❌ @robobun, your commit 628353f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34132That installs a local version of the PR into your bun-34132 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #31524. That PR rewrites This PR's primary fix is The only overlap is the view-type check in the two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e length check Address review on #34132: napi_get_typedarray_info now rejects only DataView and ArrayBuffer up front (matching Node's IsTypedArray gate), and maps to napi_typedarray_type only when the caller requested it, so a Float16Array with type=NULL still returns napi_ok with geometry as before. Also reorder the napi_create_typedarray length check to avoid size_t wrap so a pathological length cannot fall through to JSC's throwing path.
There was a problem hiding this comment.
LGTM — both prior findings (Float16Array handling in napi_get_typedarray_info, overflow-safe length check in napi_create_typedarray) are addressed in 15a78e5, and the new checkSameOutput cases lock them in against Node.
Checked: element-size table matches JSC's typed-array widths; the reordered bounds check can't underflow (byte_offset > buffer_len is tested first); js_type_loose() on non-cell primitives returns Cell/NumberObject so the new DataView guard safely rejects them; from_js_type remains gated on maybe_type so Float16Array with type=NULL still returns geometry.
Extended reasoning...
Overview
Four N-API functions adjusted for Node.js parity: napi_create_typedarray now validates alignment/length up front and throws a coded RangeError via napi_throw_range_error (returning napi_generic_failure) instead of letting JSC throw a raw VM exception; napi_get_typedarray_info / napi_get_dataview_info reject wrong view types with napi_invalid_arg; and both napi_create_typedarray / napi_create_dataview return napi_invalid_arg (was napi_arraybuffer_expected) for a non-ArrayBuffer argument. Touches src/jsc/bindings/napi.cpp (~35 lines net), src/runtime/napi/napi_body.rs (~12 lines), plus a new C++ test fixture and two checkSameOutput test blocks.
Security risks
None. Pure input validation added ahead of an existing safe path; no new allocation, ownership transfer, or user-controlled pointer arithmetic. The bounds check was reordered to be overflow-safe. All inputs come from trusted native addon code, not an adversarial boundary.
Level of scrutiny
Moderate — native C++/Rust in the N-API layer that real addons depend on. But the change is narrowly scoped: it mirrors the existing napi_create_dataview validation pattern line-for-line, and every behavior is pinned by a checkSameOutput test that diffs Bun's output against a real Node.js run. The exact-fit case (int32 length=4 offset=0 over a 16-byte buffer → status=0) confirms the new check doesn't over-reject.
Other factors
This is my second pass. The first raised two 🟡 findings — a Float16Array regression in napi_get_typedarray_info and unchecked size_t arithmetic in the length check — both fixed in 15a78e5 exactly as suggested, with a Float16Array type=null case added to the test to prevent re-regression. The bug hunter found nothing new on the updated diff. The macro-generated switch is exhaustive over all napi_typedarray_type values with correct element sizes, and the Rust-side js_type_loose() guard is safe on all non-cell JSValues (returns JSType::Cell, which != DataView).
There was a problem hiding this comment.
LGTM — both earlier findings (Float16Array handling in napi_get_typedarray_info, overflow-safe length check in napi_create_typedarray) were addressed in 15a78e5 and no new issues were found on re-review.
Checked: the NAPI_TYPED_ARRAY_CASE element sizes and error strings match Node's; the reordered length check cannot underflow (byte_offset > buffer_len guards the subtraction); the new DataView | ArrayBuffer reject-list in napi_get_typedarray_info covers every non-typed-array typed_array_type that as_array_buffer() can produce (SharedArrayBuffer maps to JSType::ArrayBuffer); and the napi_get_dataview_info guard mirrors the existing napi_is_dataview check exactly.
Extended reasoning...
Overview
Four files: src/jsc/bindings/napi.cpp adds up-front alignment/length validation to napi_create_typedarray (throwing coded RangeErrors via napi_throw_range_error and returning napi_generic_failure, matching Node) and changes the non-ArrayBuffer status from napi_arraybuffer_expected to napi_invalid_arg for both napi_create_typedarray and napi_create_dataview. src/runtime/napi/napi_body.rs adds view-type guards to napi_get_typedarray_info (reject DataView/ArrayBuffer) and napi_get_dataview_info (reject non-DataView), plus a clippy SAFETY comment. test/napi/napi-app/standalone_tests.cpp and test/napi/napi.test.ts add two checkSameOutput tests that compare Bun's output byte-for-byte against Node.
Security risks
None. This tightens validation on addon-supplied arguments; no new untrusted-input parsing, no allocation-size math on user data (the overflow-safe reordering was applied), no auth/crypto/permissions.
Level of scrutiny
Moderate — native N-API bindings, but the change is narrow: it moves validation earlier (before JSC's own throwing validation) and follows the exact shape napi_create_dataview already used at line 1697. Status-code changes are pure Node-compat and verified against a real Node binary via checkSameOutput. No memory-ownership or lifetime changes.
Other factors
This PR has already been through one review cycle: my two prior findings (Float16Array regression, unchecked size_t arithmetic) were both correctly fixed in 15a78e5 and locked in with new test cases. The bug hunter found nothing on the current revision. The Rust guards reuse the same is_empty_or_undefined_or_null() + js_type_loose() idiom as the neighbouring napi_is_dataview. No CODEOWNERS on this path. Tests use the established checkSameOutput harness so Node parity is enforced by CI, not asserted by hand.
There was a problem hiding this comment.
LGTM — both earlier findings addressed; validation now mirrors the existing napi_create_dataview pattern and is locked to Node via checkSameOutput.
What was reviewed:
napi_create_typedarrayalignment/length pre-validation: element sizes in the macro switch are correct; bounds check is overflow-safe (size_of_element ≥ 1, no wrap);NAPI_RETURN_IF_EXCEPTIONremains as a backstop aftercreateArrayBufferView.napi_get_dataview_infoguard:js_type_loose()is safe on non-cell primitives (returnsCell/NumberObject), same shape as the existingnapi_is_dataviewcheck.napi_get_typedarray_info: Float16Array withtype=NULLnow returnsnapi_okagain (guard rejects onlyDataView/ArrayBuffer); covered by the newcheckSameOutputcase.
Extended reasoning...
Overview
Four files: napi.cpp adds pre-JSC alignment/length validation to napi_create_typedarray (throwing coded RangeErrors via napi_throw_range_error and returning napi_generic_failure, matching Node) and switches the non-ArrayBuffer status to napi_invalid_arg for both napi_create_typedarray/napi_create_dataview. napi_body.rs adds view-type guards to napi_get_typedarray_info/napi_get_dataview_info. standalone_tests.cpp and napi.test.ts add two checkSameOutput tests comparing against Node byte-for-byte.
Security risks
None. This is input validation on the N-API boundary (trusted native addon code, not adversarial). The change adds checks; it does not remove any. JSC's own validateRange is still the memory-safety backstop behind NAPI_RETURN_IF_EXCEPTION.
Level of scrutiny
Moderate — native N-API bindings are a critical compat surface, but this change follows the established pattern already used by napi_create_dataview in the same file (napi_throw_range_error + napi_generic_failure), and every behavioral claim is pinned by a checkSameOutput test that diffs Bun against real Node output. The failure modes here are wrong status codes / missing .code on invalid input, not memory corruption.
Other factors
I previously flagged two issues on the first revision; both were fixed in 15a78e5 and both threads are resolved:
- The
napi_get_typedarray_infoguard was reworked to reject onlyDataView/ArrayBuffer(mirroring V8'sIsTypedArray()), soFloat16Arraywithtype=NULLreturnsnapi_okwith geometry again — now covered by a test case. - The length check was reordered to
byte_offset > buffer_len || length > (buffer_len - byte_offset) / size_of_element, eliminating thesize_twrap.
I also verified: the macro-generated element sizes (1/1/1/2/2/4/4/4/8/8/8) match the JS typed-array spec; size_of_element is always ≥ 1 so the division is safe; the new js_type_loose() guard in napi_get_dataview_info is safe on non-cell primitives (js_type() returns JSType::Cell for non-cells) and matches the existing napi_is_dataview implementation at napi_body.rs:1462. The bug-hunting pass on this revision found nothing.
|
CI build #72808 is red on That duplicate is on The two new tests added here ( |
…napi-typedarray-validation
There was a problem hiding this comment.
Both prior inline concerns are addressed in the current diff — the bounds check is now overflow-safe (byte_offset > buffer_len || length > (buffer_len - byte_offset) / size_of_element), and napi_get_typedarray_info accepts Float16Array with type=NULL again (with a checkSameOutput case locking it in). No new issues found.
Checked: the C++ validation mirrors the sibling napi_create_dataview shape and Node's element-size table; the Rust js_type_loose guard matches the existing napi_is_dataview pattern and is safe on non-cell primitives; both new tests compare byte-for-byte against Node.
Deferring rather than approving — native N-API compat code across C++ and Rust, and CI is currently red (attributed to an unrelated js_test_helpers.cpp redefinition on main). Worth a quick human look and a green build.
Extended reasoning...
Overview
This PR touches four files: src/jsc/bindings/napi.cpp (adds pre-JSC alignment/length validation to napi_create_typedarray via a macro-generated element-size switch, changes two napi_arraybuffer_expected returns to napi_invalid_arg), src/runtime/napi/napi_body.rs (adds view-type guards to napi_get_typedarray_info and napi_get_dataview_info), and two test files adding checkSameOutput coverage that compares Bun against Node.js byte-for-byte.
Prior review follow-up
I left two inline comments on an earlier revision: (1) unchecked size_t overflow in the length check, and (2) an incorrect claim that Node rejects Float16Array in napi_get_typedarray_info. Both were addressed in 15a78e5 and I verified the fixes in the current diff — the bounds check is reordered to avoid any wrapping intermediate, and the Rust guard now rejects only DataView/ArrayBuffer (mirroring V8's IsTypedArray()), with from_js_type moved back inside the maybe_type block so Float16Array with type=NULL returns napi_ok with geometry. A new test case covers it.
Security risks
None. Inputs come from trusted native addon code, validation is being tightened, and the overflow-safe reordering hardens the one arithmetic path. No auth, crypto, or filesystem surface.
Level of scrutiny
Medium. The changes are mechanical Node-compat fixes that closely follow established sibling patterns (napi_create_dataview for the C++ throw-via-napi_throw_range_error shape; napi_is_dataview at napi_body.rs:1480 for the is_empty_or_undefined_or_null() || js_type_loose() != DataView guard). Test coverage is strong — checkSameOutput asserts full parity with Node. But this is native C++/Rust in the N-API layer, which is production-critical for addon compatibility, so it warrants a human sign-off rather than a bot approval.
Other factors
CI build #72808 is red, which the author attributes to a get_all_property_names redefinition in js_test_helpers.cpp introduced on main by two other PRs — not this diff. The two new tests here passed on all lanes in the prior build #72775. There is a related open PR (#31524) with partial overlap and merge conflicts; the author explained the delta convincingly. Given the red CI and the native-code scope, I'm deferring to a human reviewer.
|
Build #72889 on 27b2ee4: the napi tests all pass on every lane now that main's
This diff is ready for review. |
|
fix conflicts |
Resolve conflicts with #34144 (napi_float16_array): add Float16Array to the NAPI_TYPED_ARRAY_CASE switch in napi_create_typedarray, keep both the new view-validation tests and main's test_napi_float16_array, and simplify the napi_get_typedarray_info guard now that from_js_type covers every typed-array JSType.
|
Conflicts resolved in 26761d6. The conflict was with #34144 (Float16Array support): added |
Resolve conflict with #34131 in napi_get_dataview_info: combine its is_empty() NULL-napi_value guard with this PR's DataView type check.
…re paths (#36805) ## What A set of N-API entry points return a different `napi_status` than Node.js 26 for specific inputs. None of these crash; they break addons that branch on `status == X`. This aligns each to Node.js, verified against `js_native_api_v8.cc` / `node_api.cc` on `v26.x`. ## Before / after `test_napi_status_codes_node26` (diffed byte-for-byte against Node via `checkSameOutput`): ``` bun (before) node / bun (after) napi_wrap(number) status=2 status=1 napi_unwrap(number) status=2 status=1 napi_remove_wrap(number) status=2 status=1 napi_add_finalizer(number) status=2 status=1 napi_coerce_to_number(Symbol) status=10 status=6 pending=1 napi_coerce_to_string(Symbol) status=10 status=3 pending=1 napi_coerce_to_object(null) status=10 status=2 pending=1 napi_run_script(throw) status=10 status=9 pending=1 napi_run_script(syntax) status=10 status=9 pending=1 napi_create_bigint_words(INT_MAX+1) status=10 p=1 status=1 pending=0 napi_create_buffer(SIZE_MAX) status=10 status=9 pending=1 napi_create_buffer_copy(SIZE_MAX) status=10 status=9 pending=1 napi_throw_error while one is pending status=0 status=10 (first kept) node_api_post_finalizer(NULL) status=1 status=0 napi_make_callback(recv=NULL) status=0 status=1 napi_make_callback(argc>0,argv=NULL) status=0 status=1 napi_make_callback(func=number) status=5 status=1 napi_ref_threadsafe_function(env=NULL) status=1 status=0 napi_unref_threadsafe_function(env=NULL) status=1 status=0 napi_ref_threadsafe_function last_error cleared preserved ``` ## Changes **`src/jsc/bindings/napi.cpp`** - New `NAPI_RETURN_STATUS_IF_EXCEPTION(env, status)` macro alongside `NAPI_RETURN_IF_VM_EXCEPTION`, for call sites where Node.js reports a specific code while leaving the thrown exception pending. Used by the coerce-to-\*/create-buffer paths below and by the two pre-existing `napi_instanceof` call sites (no behavior change there). - `napi_wrap` / `napi_unwrap` / `napi_remove_wrap` / `napi_add_finalizer`: non-object `js_object` returns `napi_invalid_arg` (was `napi_object_expected`). Node: `RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg)` in `v8impl::Wrap/Unwrap`. - `napi_coerce_to_number` / `_string` / `_object`: when the coercion throws, return `napi_{number,string,object}_expected` (was `napi_pending_exception`). The thrown exception stays pending. Node: `GEN_COERCE_FUNCTION` -> `CHECK_MAYBE_EMPTY(..., napi_X_expected)`. - `napi_run_script`: compile/eval error returns `napi_generic_failure` (was `napi_pending_exception`). Exception stays pending on the env. Node: `CHECK_MAYBE_EMPTY(env, maybe_script, napi_generic_failure)` / same for `Run()`. - `napi_create_buffer` / `napi_create_external_buffer`: allocation failure returns `napi_generic_failure` (was `napi_pending_exception`). Node: `CHECK_MAYBE_EMPTY(env, maybe, napi_generic_failure)`. - `napi_create_bigint_words`: `word_count > INT_MAX` returns `napi_invalid_arg` with no throw (was a thrown `RangeError` + `napi_pending_exception` for `INT_MAX < word_count <= UINT_MAX`). Node: `RETURN_STATUS_IF_FALSE(env, word_count <= INT_MAX, napi_invalid_arg)`. Counts between JSC's bigint limit and `INT_MAX` still throw, guarded before `createFromWords` so we never read past the caller's buffer (V8 checks its limit before reading; JSC trims trailing zeroes first). - `napi_throw_error` / `napi_throw_type_error` / `napi_throw_range_error` / `node_api_throw_syntax_error`: if an exception is already pending, return `napi_pending_exception` and leave it untouched (was `napi_ok`, overwriting the first exception). Node uses `NAPI_PREAMBLE` for all four. The internal call sites in `napi_instanceof` / `napi_create_dataview` sit behind a preamble that has already rejected a pending exception, so they are unaffected. - `node_api_post_finalizer`: accept a NULL `finalize_cb` (was `napi_invalid_arg`). Node uses only `CHECK_ENV` and enqueues unconditionally; `napi_internal_enqueue_finalizer` early-returns on a null callback. **`src/runtime/napi/napi_body.rs`** - `napi_create_string_utf8`: creation failure returns `napi_generic_failure` (was `napi_pending_exception`), consistent with Bun's own latin1/utf16 variants and Node's `CHECK_MAYBE_EMPTY`. - `napi_make_callback`: NULL `recv` / `argc > 0 && argv == NULL` / non-function `func` all return `napi_invalid_arg` (was: accepted / accepted / `napi_function_expected`). Node: `CHECK_ARG(env, recv)`, `if (argc > 0) CHECK_ARG(env, argv)`, `CHECK_TO_FUNCTION` (-> `napi_invalid_arg`). - `napi_resolve_deferred` / `napi_reject_deferred`: resolve/reject failure returns `napi_generic_failure` (was `napi_pending_exception`). Node: `RETURN_STATUS_IF_FALSE(env, success.FromMaybe(false), napi_generic_failure)` in `ConcludeDeferred`. - `napi_create_buffer_copy`: allocation failure returns `napi_generic_failure` (was `napi_pending_exception`). - `napi_ref_threadsafe_function` / `napi_unref_threadsafe_function`: ignore `env` (basic-env functions) and return `napi_ok` without touching `last_error`. Node: `CHECK_NOT_NULL(func); return ...->Ref();` with no env check and no `napi_set_last_error`. ## Not in this PR `napi_create_typedarray` / `napi_create_dataview` non-ArrayBuffer -> `napi_invalid_arg` and the coded-RangeError alignment for typedarray bounds are covered by #34132. ## Coverage `test_napi_status_codes_node26` pins every change that is reachable without engine OOM. The remaining `pending_exception -> generic_failure` edits are alignment-by-inspection only: - `napi_create_string_utf8` creation failure: `length > INT_MAX` is rejected with `napi_invalid_arg` first, so the failure arm needs a true JSC string-allocation OOM. - `napi_create_external_buffer` allocation failure: the empty-buffer and external-bytes paths do not have a size pre-check that can be tripped deterministically. - `napi_resolve_deferred` / `napi_reject_deferred` failure: the underlying `JSPromise::resolve/reject` only `Err`s on VM termination; the deferred handle is freed on first use, so a second call is UB rather than a testable failure. ## Verification ``` USE_SYSTEM_BUN=1 bun test test/napi/napi.test.ts -t 'returns the same napi_status' -> fail (21/21 lines differ from Node) bun bd test test/napi/napi.test.ts -t 'returns the same napi_status' -> pass (Bun output == Node output) ``` The vendored Node test suites for `test_bigint`, `test_conversions`, `test_error`, `test_exception`, `6_object_wrap`, `8_passing_wrapped`, `test_promise`, `test_function`, `test_general`, `test_object` all pass. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
What
napi_create_typedarraynow validates alignment and length before calling into JSC and throws a codedRangeErrorvianapi_throw_range_error, matching Node.js.napi_get_typedarray_info/napi_get_dataview_infonow reject values of the wrong view type instead of returningnapi_okwith the wrong geometry.Why
napi_create_typedarray: raw VM exception, wrong status, no.codes.codes2napi_generic_failure)ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENTnapi_pending_exception)undefinedERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENTPreviously
napi_create_typedarraydelegated validation toJSGenericTypedArrayView::create, which throws directly into the VM. TheRangeErrorcarried no.code, the returned status wasnapi_pending_exception(Node.js returnsnapi_generic_failure), and because the exception sat on the VM rather than inenv->m_pendingException, a follow-up Rust-side N-API call would tripassert_exception_presence_matchesin its generated FFI wrapper and abort on the asserts build:Now we validate
byte_offset % element_size == 0andlength * element_size + byte_offset <= byteLengthup front and throw vianapi_throw_range_error(stashed on the env), returningnapi_generic_failure. This is the same shapenapi_create_dataviewalready used for itsERR_NAPI_INVALID_DATAVIEW_ARGSpath.napi_get_dataview_info/napi_get_typedarray_info: no type checknapi_get_dataview_info(Uint8Array)napi_invalid_arg)napi_get_dataview_info(ArrayBuffer)napi_get_typedarray_info(DataView)typeout-param was non-null, else 0napi_get_typedarray_info(ArrayBuffer)typeout-param was non-null, else 0Both functions accepted anything
as_array_buffer()would unwrap.napi_get_typedarray_infoonly rejected when thetypeout-param was non-null. An addon guarding its element-size math with these statuses would read the wrong memory layout under a cleannapi_ok.Also:
napi_create_typedarrayandnapi_create_dataviewnow returnnapi_invalid_arg(wasnapi_arraybuffer_expected) when thearraybufferargument is not an ArrayBuffer, matching Node.js.Verification
New
checkSameOutputtests intest/napi/napi.test.tscompare Bun against Node.js byte-for-byte:The node-napi
test_typedarray/test_dataviewsuites and existingnapi_get_typedarray_info/napi_get_dataview_infobyte-offset tests still pass.no test proof · iteration 9 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts