fix(napi): validate status before copying in TypedArray owned ToNapiValue fallback by Brooooooklyn · Pull Request #3080 · napi-rs/napi-rs

Brooooooklyn · 2025-12-30T03:19:12Z

Note

Strengthens TypedArray owned ToNapiValue path by tightening ArrayBuffer creation and fallback handling.

Refactors TypedArray::<T>::to_napi_value to explicitly handle zero-length buffers via napi_create_arraybuffer
Validates napi_create_external_arraybuffer status and only falls back to napi_create_arraybuffer with data copy when napi_no_external_buffers_allowed
Ensures check_status! occurs before copying and only copies when length > 0; improves error messages for failures

^{Written by Cursor Bugbot for commit 3d92833. This will update automatically on new commits. Configure here.}

Summary by CodeRabbit

Bug Fixes
- Enhanced TypedArray buffer handling with explicit support for empty buffers
- Added automatic fallback mechanism when external buffers are unsupported, ensuring data integrity
- Improved error messages for buffer creation failures

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-30T03:19:52Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

The arraybuffer.rs file's to_napi_value implementation in the impl_typed_array macro was refactored to handle zero-length buffers explicitly and introduce a fallback mechanism when external ArrayBuffers are not permitted by the runtime.

Changes

Cohort / File(s)	Summary
ArrayBuffer TypedArray Path Handling `crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs`	Added explicit branch for zero-length buffers creating ArrayBuffer with null pointer. Refactored non-empty buffer handling to attempt external ArrayBuffer creation with status checks, and fallback to regular ArrayBuffer with data copying if external buffers are disallowed (`napi_no_external_buffers_allowed`). Updated error messages and control flow to ensure proper TypedArray finalization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

fix(napi): validate status before copying in ArrayBuffer ToNapiValue fallback #3079: Makes identical code-level changes to the same file, including zero-length buffer handling, fallback logic for disallowed external buffers, and updated error handling for ArrayBuffer creation paths.

Poem

🐰 ArrayBuffers hop with care today,
Empty or full, they find their way,
When external paths refuse to stay,
Regular buffers copy, come what may—
TypedArrays thrive in every way!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: validating status before copying in TypedArray's ToNapiValue fallback implementation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c541fee and 6ac1d6b.

📒 Files selected for processing (1)

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript

Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces

📚 Learning: 2025-11-25T09:31:23.877Z

Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript

Applied to files:

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

📚 Learning: 2025-11-25T09:31:23.877Z

Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript

Applied to files:

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

🧬 Code graph analysis (1)

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (2)

crates/napi/src/js_values/arraybuffer.rs (2)

into_raw (212-214)

new (208-210)

crates/napi/src/js_values/value_ref.rs (1)

new (19-31)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)

GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: ASAN - windows-latest
GitHub Check: Memory leak detect job
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: Test node wasi target (--cfg tokio_unstable)
GitHub Check: Test node wasi target
GitHub Check: Build only test - armv7-linux-androideabi
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build-and-test-msys2 (CLANG64)
GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-gnu
GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
GitHub Check: Test freebsd target
GitHub Check: build - s390x-unknown-linux-gnu
GitHub Check: Build only test - loongarch64-unknown-linux-gnu
GitHub Check: build - aarch64-unknown-linux-musl
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: build-and-test-msys2 (UCRT64)

🔇 Additional comments (1)

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (1)

747-787: Overall approach is sound but has issues that need addressing.

The refactoring correctly:

Handles zero-length buffers separately to avoid Rust's 0x1 dangling pointer issue with V8

Validates status before copying in the fallback path (line 780 before line 782)

Reclaims hint_ptr in the napi_no_external_buffers_allowed fallback (line 770)

However, the issues flagged in previous comments (misleading error messages and memory leak on unexpected errors) should be addressed before merging.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

crates/napi/src/bindgen_runtime/js_values/buffer.rs

Brooooooklyn · 2025-12-30T03:30:48Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

ready-to-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copilot

Pull request overview

This PR refactors the error handling logic in the ToNapiValue implementation for typed arrays. The changes restructure the control flow for creating array buffers, separating the status checks into explicit branches rather than nesting them within a single check_status! macro call.

Extracts the zero-length buffer case into its own block with immediate error checking
Explicitly stores and checks the status from napi_create_external_arraybuffer using a mutable variable
Separates error handling for the fallback path when external buffers are not allowed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

graphite-app · 2025-12-30T05:30:15Z

Merge activity

Dec 30, 5:30 AM UTC: Brooooooklyn added this pull request to the Graphite merge queue.
Dec 30, 5:58 AM UTC: Merged by the Graphite merge queue.

…alue fallback (#3080)  > [!NOTE] > Strengthens `TypedArray` owned `ToNapiValue` path by tightening ArrayBuffer creation and fallback handling. > > - Refactors `TypedArray::<T>::to_napi_value` to explicitly handle zero-length buffers via `napi_create_arraybuffer` > - Validates `napi_create_external_arraybuffer` status and only falls back to `napi_create_arraybuffer` with data copy when `napi_no_external_buffers_allowed` > - Ensures `check_status!` occurs before copying and only copies when `length > 0`; improves error messages for failures > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 3d92833. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>

Copilot AI review requested due to automatic review settings December 30, 2025 03:19

chatgpt-codex-connector bot reviewed Dec 30, 2025

View reviewed changes

crates/napi/src/bindgen_runtime/js_values/buffer.rs Show resolved Hide resolved

Brooooooklyn changed the base branch from main to graphite-base/3080 December 30, 2025 03:30

Brooooooklyn changed the base branch from graphite-base/3080 to bisect/arraybuffer-struct December 30, 2025 03:30

This was referenced Dec 30, 2025

fix(napi): validate status before copying data in env arraybuffer fallback #3077

Merged

fix(napi): validate status before copying in ArrayBuffer ToNapiValue fallback #3079

Merged

fix(napi): skip debug buffer tracking on wasm targets #3078

Merged

Brooooooklyn mentioned this pull request Dec 30, 2025

fix(napi): validate status before copying in remaining TypedArray fallback paths #3076

Merged

Copilot started reviewing on behalf of Brooooooklyn December 30, 2025 03:31 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

Brooooooklyn force-pushed the bisect/arraybuffer-struct branch from 13f1fe1 to 512153f Compare December 30, 2025 03:36

Brooooooklyn force-pushed the bisect/typed-array-owned branch from 9eb1cc0 to 4cc80d3 Compare December 30, 2025 03:36

Brooooooklyn force-pushed the bisect/arraybuffer-struct branch from 512153f to 72668eb Compare December 30, 2025 03:40

Brooooooklyn force-pushed the bisect/typed-array-owned branch from 4cc80d3 to 55a51d1 Compare December 30, 2025 03:40

Brooooooklyn changed the title ~~Bisect/typed array owned~~ fix(napi): validate status before copying in TypedArray owned ToNapiValue fallback Dec 30, 2025

Brooooooklyn force-pushed the bisect/arraybuffer-struct branch from 72668eb to 9945379 Compare December 30, 2025 04:17

Brooooooklyn force-pushed the bisect/typed-array-owned branch 2 times, most recently from e48088b to 3d92833 Compare December 30, 2025 04:58

Brooooooklyn force-pushed the bisect/arraybuffer-struct branch from 9945379 to 6752c3e Compare December 30, 2025 04:59

graphite-app bot changed the base branch from bisect/arraybuffer-struct to graphite-base/3080 December 30, 2025 05:24

graphite-app bot changed the base branch from graphite-base/3080 to main December 30, 2025 05:38

graphite-app bot force-pushed the bisect/typed-array-owned branch from 3d92833 to 6ac1d6b Compare December 30, 2025 05:38

graphite-app bot merged commit 6ac1d6b into main Dec 30, 2025
75 checks passed

graphite-app bot deleted the bisect/typed-array-owned branch December 30, 2025 05:58

github-actions bot mentioned this pull request Dec 30, 2025

chore: release #3073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(napi): validate status before copying in TypedArray owned ToNapiValue fallback#3080

fix(napi): validate status before copying in TypedArray owned ToNapiValue fallback#3080
graphite-app[bot] merged 1 commit intomainfrom
bisect/typed-array-owned

Brooooooklyn commented Dec 30, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 30, 2025 •

edited

Loading

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

Brooooooklyn commented Dec 30, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graphite-app bot commented Dec 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Comments

Conversation

Brooooooklyn commented Dec 30, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Brooooooklyn commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graphite-app bot commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Brooooooklyn commented Dec 30, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 30, 2025 •

edited

Loading

Brooooooklyn commented Dec 30, 2025 •

edited

Loading

graphite-app bot commented Dec 30, 2025 •

edited

Loading