fix(napi): validate status before copying data in env arraybuffer fallback by Brooooooklyn · Pull Request #3077 · napi-rs/napi-rs

Brooooooklyn · 2025-12-30T03:18:37Z

Note

Strengthens Env arraybuffer creation paths to avoid unsafe copies and improve diagnostics.

Validates N-API status before copying memory in create_arraybuffer_with_data and borrowed variant; adds explicit error messages
Properly handles zero-length buffers without unsafe pointers
Fallback from external to internal arraybuffer now copies only when status == napi_ok and length > 0
Ensures hint cleanup and always invokes finalize callback even on errors

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

Summary by CodeRabbit

Release Notes

Bug Fixes
- Improved reliability of buffer creation with automatic fallback to alternative methods when external buffers are unavailable
- Enhanced error reporting with more descriptive error messages during buffer operations and initialization failures
- Better handling of finalization callbacks in constrained environments
Refactor
- Streamlined conditional compilation logic for WebAssembly targets to optimize build output and reduce unnecessary debug checks

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

coderabbitai · 2025-12-30T03:18:48Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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

This pull request adds WebAssembly target exclusions to debug-assertion code paths in ArrayBuffer and Buffer handling modules, while refactoring buffer creation logic in env.rs to handle zero-length buffers separately and implement fallback mechanisms for when external buffer allocation is not permitted.

Changes

Cohort / File(s)	Summary
WASM Conditional Compilation Gates `crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs`, `crates/napi/src/bindgen_runtime/js_values/buffer.rs`	Added `not(target_family = "wasm")` to existing cfg attributes for debug-only sections. Excludes wasm targets from debug assertions in ArrayBuffer/TypedArray creation and BUFFER_DATA initialization.
Buffer Creation Logic Refactoring `crates/napi/src/env.rs`	Separated zero-length buffer handling from non-zero cases. For zero-length: uses null pointer path. For non-zero: attempts external arraybuffer allocation; on `napi_no_external_buffers_allowed` error, falls back to standard arraybuffer with data copying. Added explicit error messages to status checks and ensured finalize callbacks are always invoked in fallback scenarios.

Sequence Diagram

sequenceDiagram
    participant Caller as Caller
    participant Env as env.rs
    participant NAPI as NAPI C API
    
    Caller->>Env: create_arraybuffer(length)
    
    alt length == 0
        Env->>NAPI: napi_create_arraybuffer(null)
        NAPI-->>Env: status
        Env->>Env: check_status with error msg
        Env-->>Caller: result
    else length > 0
        Env->>NAPI: napi_create_external_arraybuffer(hint)
        NAPI-->>Env: status
        
        alt status == napi_no_external_buffers_allowed
            Env->>NAPI: napi_create_arraybuffer(data_ptr)
            NAPI-->>Env: status + buffer
            Env->>Env: copy original data to buffer
            Env->>Env: invoke finalize callback
            Env->>Env: check_status with error msg
            Env-->>Caller: result
        else status == napi_ok
            Env->>Env: check_status with error msg
            Env-->>Caller: result
        end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Wasm we exclude from debug's keen eye,
While buffers dance with fallback supply,
Zero or plenty, the logic now clear,
External or copied—no errors to fear!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title accurately reflects the main change: adding status validation before copying data in the env arraybuffer fallback logic, which is the primary focus of the refactored buffer creation paths in env.rs.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91f19f4 and 7c9aa9f.

📒 Files selected for processing (3)

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

🧰 Additional context used

🧠 Learnings (3)

📚 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
crates/napi/src/bindgen_runtime/js_values/buffer.rs
crates/napi/src/env.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
crates/napi/src/bindgen_runtime/js_values/buffer.rs
crates/napi/src/env.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(object)]` attribute for plain objects that should be mapped to TypeScript interfaces

Applied to files:

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

⏰ 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). (16)

GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
GitHub Check: Build only test - aarch64-unknown-linux-ohos
GitHub Check: build-and-test-msys2 (MINGW64)
GitHub Check: build - powerpc64le-unknown-linux-gnu
GitHub Check: build-and-test-msys2 (UCRT64)
GitHub Check: build - x86_64-unknown-linux-musl
GitHub Check: Test freebsd target
GitHub Check: ASAN - windows-latest
GitHub Check: ASAN - ubuntu-24.04
GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
GitHub Check: Memory leak detect job
GitHub Check: Cursor Bugbot

🔇 Additional comments (7)

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

151-160: Consistent cfg guard update for wasm exclusion.

The debug assertion block is now correctly guarded to exclude wasm targets, aligning with the BUFFER_DATA definition in buffer.rs. This prevents compilation issues on wasm targets where thread-local storage behavior differs.

232-241: LGTM!

Consistent wasm exclusion applied to from_external debug assertions.

919-928: LGTM!

Macro-generated from_data method correctly excludes wasm targets from the debug assertions.

1024-1033: LGTM!

Macro-generated from_external method correctly excludes wasm targets from the debug assertions.

1638-1647: LGTM!

Uint8ClampedSlice::from_data correctly excludes wasm targets from the debug assertions.

1735-1744: LGTM!

Uint8ClampedSlice::from_external correctly excludes wasm targets from the debug assertions.

crates/napi/src/env.rs (1)

604-614: Improved fallback handling with correct finalize ordering.

The refactored code correctly:

Copies data only on successful arraybuffer creation and non-zero length

Always calls the finalize callback to ensure caller's resources are cleaned up

Checks status after finalize to properly propagate errors

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

crates/napi/src/env.rs

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

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 makes changes to improve WASM compatibility and error handling in the NAPI array buffer creation code. The changes include excluding WASM targets from thread-local buffer tracking and adding defensive checks for data copying operations.

Refactored array buffer creation error handling with explicit status checks and error messages
Added length validation before memory copy operations to prevent potential issues with null pointers
Updated conditional compilation attributes to exclude target_family = "wasm" from thread-local buffer data tracking

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

File	Description
crates/napi/src/env.rs	Refactored `create_arraybuffer_with_data` and `create_arraybuffer_with_borrowed_data` to improve error handling, add length checks before copying, and add error messages to status checks
crates/napi/src/bindgen_runtime/js_values/buffer.rs	Updated cfg attribute to exclude WASM targets from thread-local BUFFER_DATA declaration
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs	Updated cfg attributes across multiple locations to exclude WASM targets from thread-local BUFFER_DATA usage

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

crates/napi/src/env.rs

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

Merge activity

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

…lback (#3077)  > [!NOTE] > Strengthens `Env` arraybuffer creation paths to avoid unsafe copies and improve diagnostics. > > - Validates N-API `status` before copying memory in `create_arraybuffer_with_data` and borrowed variant; adds explicit error messages > - Properly handles zero-length buffers without unsafe pointers > - Fallback from external to internal arraybuffer now copies only when `status == napi_ok` and `length > 0` > - Ensures hint cleanup and always invokes finalize callback even on errors > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 61ea62b. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>   ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved reliability of buffer creation with automatic fallback to alternative methods when external buffers are unavailable * Enhanced error reporting with more descriptive error messages during buffer operations and initialization failures * Better handling of finalization callbacks in constrained environments * **Refactor** * Streamlined conditional compilation logic for WebAssembly targets to optimize build output and reduce unnecessary debug checks <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>

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

chatgpt-codex-connector bot reviewed Dec 30, 2025

View reviewed changes

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

coderabbitai bot reviewed Dec 30, 2025

View reviewed changes

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

crates/napi/src/env.rs Show resolved Hide resolved

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

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

Brooooooklyn changed the base branch from graphite-base/3077 to bisect/cfg-only December 30, 2025 03:30

Copilot AI reviewed Dec 30, 2025

View reviewed changes

crates/napi/src/env.rs Show resolved Hide resolved

Brooooooklyn force-pushed the bisect/cfg-only branch from 791b076 to 2a58dd5 Compare December 30, 2025 03:34

Brooooooklyn force-pushed the bisect/env-changes branch from 7c9aa9f to 4d6a5ac Compare December 30, 2025 03:34

Brooooooklyn force-pushed the bisect/cfg-only branch from 2a58dd5 to ac6a276 Compare December 30, 2025 03:39

Brooooooklyn force-pushed the bisect/env-changes branch from 4d6a5ac to ff47f3e Compare December 30, 2025 03:40

Brooooooklyn changed the title ~~Bisect/env changes~~ fix(napi): validate status before copying data in env arraybuffer fallback Dec 30, 2025

Brooooooklyn changed the base branch from bisect/cfg-only to graphite-base/3077 December 30, 2025 04:17

Brooooooklyn force-pushed the bisect/env-changes branch from ff47f3e to 3148638 Compare December 30, 2025 04:17

Brooooooklyn changed the base branch from graphite-base/3077 to bisect/typed-array-mut December 30, 2025 04:17

Brooooooklyn force-pushed the bisect/typed-array-mut branch from 07b46c7 to f2e1e07 Compare December 30, 2025 04:58

Brooooooklyn force-pushed the bisect/env-changes branch from 3148638 to 61ea62b Compare December 30, 2025 04:59

graphite-app bot force-pushed the bisect/typed-array-mut branch from f2e1e07 to 51e65eb Compare December 30, 2025 05:39

graphite-app bot force-pushed the bisect/env-changes branch from 61ea62b to d78f83d Compare December 30, 2025 05:39

Base automatically changed from bisect/typed-array-mut to main December 30, 2025 05:59

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

graphite-app bot deleted the bisect/env-changes branch December 30, 2025 05:59

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

chore: release #3073

Merged

coderabbitai bot mentioned this pull request Dec 30, 2025

fix(napi): wasi debug compile error #3081

Merged

Uh oh!

Comments

Conversation

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

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

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

Uh oh!

Review skipped

Other AI code review bot(s) detected

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

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!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Reviewed changes

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 cursor 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