Skip to content

napi: napi_is_arraybuffer returns false for SharedArrayBuffer#32629

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/452873c2/napi-is-arraybuffer-shared
Jun 25, 2026
Merged

napi: napi_is_arraybuffer returns false for SharedArrayBuffer#32629
Jarred-Sumner merged 1 commit into
mainfrom
farm/452873c2/napi-is-arraybuffer-shared

Conversation

@robobun

@robobun robobun commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

napi_is_arraybuffer returns true for a SharedArrayBuffer. Node returns false: in V8 (which N-API maps onto) napi_is_arraybuffer is value->IsArrayBuffer(), and a SharedArrayBuffer is a distinct type.

// addon.c, called from JS as add(new SharedArrayBuffer(1))
bool is;
napi_is_arraybuffer(env, args[0], &is);
printf("%d\n", is); // Node: 0, Bun: 1
$ node addon.js       $ bun addon.js
1                     1
0                     1   <- should be 0

Cause

In JSC both ArrayBuffer and SharedArrayBuffer carry the same cell type (JSType::ArrayBufferType), so the previous check in src/runtime/napi/napi_body.rs could not tell them apart:

*result = !value.is_number() && value.js_type_loose() == jsc::JSType::ArrayBuffer;

The only distinguisher is the backing buffer's isShared() flag, already surfaced to Rust as ArrayBuffer.shared.

Fix

Resolve the value as an array buffer and additionally require it to be non-shared, matching V8's IsArrayBuffer():

*result = value
    .as_array_buffer(env.to_js())
    .is_some_and(|ab| ab.typed_array_type == jsc::JSType::ArrayBuffer && !ab.shared);

napi_get_arraybuffer_info is left unchanged on purpose. Node's behavior there is asymmetric: it still accepts a SharedArrayBuffer and returns napi_ok, which Bun already matched. The regression test pins both behaviors.

Verification

Added test_is_arraybuffer to the napi-app fixture and a case in test/napi/napi.test.ts that diffs Bun's output against the ABI-matching Node for ArrayBuffer, SharedArrayBuffer, and a typed array (napi_ok is 0, napi_invalid_arg is 1):

napi_is_arraybuffer=true napi_get_arraybuffer_info=0   # ArrayBuffer
napi_is_arraybuffer=false napi_get_arraybuffer_info=0  # SharedArrayBuffer
napi_is_arraybuffer=false napi_get_arraybuffer_info=1  # Uint8Array

Fails before the fix (Bun reports napi_is_arraybuffer=true for the SharedArrayBuffer), passes after.

Fixes #32624

JSC gives a SharedArrayBuffer the same ArrayBuffer cell type as a plain
ArrayBuffer, so the js_type_loose() == ArrayBuffer check reported true for
both. Node's napi_is_arraybuffer maps to V8's IsArrayBuffer(), which is
false for a SharedArrayBuffer, so gate on the buffer's shared flag to match.

napi_get_arraybuffer_info is intentionally left as-is: Node still accepts a
SharedArrayBuffer there (napi_ok), and the regression test pins that.
@robobun

robobun commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:49 PM PT - Jun 22nd, 2026

@robobun, your commit 700ae3f has 2 failures in Build #64060 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32629

That installs a local version of the PR into your bun-32629 executable, so you can run:

bun-32629 --bun

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 84d5a292-010a-4db2-bdb5-dbc186f6ad7a

📥 Commits

Reviewing files that changed from the base of the PR and between 23315f4 and 700ae3f.

📒 Files selected for processing (3)
  • src/runtime/napi/napi_body.rs
  • test/napi/napi-app/standalone_tests.cpp
  • test/napi/napi.test.ts

Walkthrough

napi_is_arraybuffer in napi_body.rs is updated to use as_array_buffer and check typed_array_type == JSType::ArrayBuffer && !ab.shared, replacing a loose predicate that incorrectly matched SharedArrayBuffer. A new C++ standalone test and a TypeScript test suite are added to verify the corrected behavior.

Changes

napi_is_arraybuffer fix and tests

Layer / File(s) Summary
napi_is_arraybuffer implementation fix
src/runtime/napi/napi_body.rs
Replaces the js_type_loose/!is_number guard with an as_array_buffer conversion that checks typed_array_type == JSType::ArrayBuffer and !ab.shared, correctly excluding SharedArrayBuffer.
Standalone C++ test and TypeScript test suite
test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts
Adds test_is_arraybuffer in standalone_tests.cpp to print napi_is_arraybuffer and napi_get_arraybuffer_info status per argument, registers it in register_standalone_tests, and adds a TypeScript describe block asserting exact output lines for ArrayBuffer, SharedArrayBuffer, and Uint8Array.

Possibly related issues

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main fix: correcting napi_is_arraybuffer to return false for SharedArrayBuffer, matching Node.js behavior.
Description check ✅ Passed The description comprehensively covers the problem, root cause, fix, and verification, fully addressing the template sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted Node-compat fix with a test that diffs against Node.

Extended reasoning...

Overview

This PR fixes napi_is_arraybuffer in src/runtime/napi/napi_body.rs to return false for SharedArrayBuffer, matching Node/V8 semantics. The previous implementation only checked the JSC cell type, which is identical for ArrayBuffer and SharedArrayBuffer; the new code resolves the value via as_array_buffer and additionally requires !ab.shared. A C++ test fixture (test_is_arraybuffer) and a checkSameOutput case in test/napi/napi.test.ts pin the behavior for ArrayBuffer, SharedArrayBuffer, and Uint8Array, including the intentionally-asymmetric napi_get_arraybuffer_info status.

Security risks

None. This is a pure type-classification predicate over a JS value; no parsing, no allocation, no auth/permissions, no untrusted input handling. The underlying JSC__JSValue__asArrayBuffer helper is read-only and already used by other napi entry points (e.g. napi_get_arraybuffer_info).

Level of scrutiny

Low. The runtime change is three lines of logic plus a comment, scoped to a single N-API predicate, and follows an existing pattern (the shared flag is already populated by JSC__JSValue__asArrayBuffer in bindings.cpp:3313). The typed_array_type == ArrayBuffer guard preserves the prior false for typed arrays / DataView, and as_array_buffer returning None for non-cells preserves false for primitives. No CODEOWNERS cover this path.

Other factors

The test follows the established test_is_buffer / test_is_typedarray pattern in the same file and uses checkSameOutput to diff against the ABI-matching Node, which is the right harness for a Node-compat fix. The bug-hunting system found no issues, and there are no prior reviewer comments to address.

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to .cpp and it will simplify the. code

@Jarred-Sumner Jarred-Sumner merged commit 237de94 into main Jun 25, 2026
76 of 80 checks passed
@Jarred-Sumner Jarred-Sumner deleted the farm/452873c2/napi-is-arraybuffer-shared branch June 25, 2026 00:06
robobun added a commit that referenced this pull request Jun 25, 2026
The native helper writes via printf, which emits CRLF line endings on
Windows. Splitting on a bare LF left trailing CR on each line and
failed the toEqual check. checkSameOutput already verifies Bun and
Node produce identical bytes; this only fixes the supplementary
per-line assertion. From #32629.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

napi_is_arraybuffer returns true for SharedArrayBuffer

2 participants