napi: reject SharedArrayBuffer in napi_detach_arraybuffer

[robobun]

What

napi_detach_arraybuffer on a SharedArrayBuffer returned napi_ok without detaching anything. Node returns napi_arraybuffer_expected (19).

Repro

// sab = new SharedArrayBuffer(8); new Uint8Array(sab)[0] = 7;
napi_status sd = napi_detach_arraybuffer(env, sab);
// node: sd=19 (napi_arraybuffer_expected)
// bun : sd=0  (napi_ok)          <- claims success, nothing detached
// aftermath in BOTH engines: sab.byteLength == 8, view[0] == 7

Cause

JSC backs SharedArrayBuffer with the same JSC::JSArrayBuffer cell type as a plain ArrayBuffer, so dynamicDowncast<JSArrayBuffer> succeeds. The isDetachable() check (which is false for shared buffers) then silently skipped the detach and fell through to NAPI_RETURN_SUCCESS. V8 treats SharedArrayBuffer as a distinct type, so Node's value->IsArrayBuffer() check rejects it up front.

The napi_ok here is a memory-lifetime lie: an addon that trusts it may free or recycle a backing store that JS (and other threads holding the same SAB) still read and write.

Fix

In napi_detach_arraybuffer: reject shared buffers with napi_arraybuffer_expected (matching Node's IsArrayBuffer() gate), then require isDetachable() with napi_detachable_arraybuffer_expected instead of succeeding as a no-op. Detaching an already-detached buffer remains napi_ok.

Also aligns napi_is_detached_arraybuffer with Node: it now returns napi_ok with result=false for any non-ArrayBuffer value (typed arrays, SAB, anything else) instead of napi_arraybuffer_expected.

Verification

New test_detach_arraybuffer in the napi-app addon prints the status for [SharedArrayBuffer, ArrayBuffer, <same ArrayBuffer again>, Uint8Array] and checkSameOutput asserts Bun matches Node byte-for-byte.

Fails on system Bun (SAB row: napi_detach_arraybuffer=0 vs Node's 19; Uint8Array row: napi_is_detached_arraybuffer=19 vs Node's 0), passes with this change.

Also in this PR

Deduplicates get_all_property_names in test/napi/napi-app/js_test_helpers.cpp. #34126 and #34130 each landed a definition with a different return shape and the addon no longer compiled on main; kept the {status, keys} form and updated the one caller that expected a bare array.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts

[robobun]

^{Updated 7:36 AM PT - Jul 14th, 2026}

❌ @robobun, your commit 5cb2d80 has 2 failures in Build #72871 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 34143

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

bun-34143 --bun

[coderabbitai]

Walkthrough

Changes

The PR aligns N-API ArrayBuffer detachment behavior with Node/V8 semantics, adds regression coverage for shared and detached buffers, and updates property-name test helpers to use structured results.

ArrayBuffer detachment behavior

Layer / File(s)	Summary
Detachment validation and state handling src/jsc/bindings/napi.cpp	Non-shared ArrayBuffers now report detached state without errors for other inputs; detachment rejects shared or non-detachable buffers while allowing already-detached buffers.
Detachment regression tests test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts	Adds standalone and TypeScript coverage for shared-buffer rejection, successful detachment, repeated detachment, and resulting buffer lengths and statuses.

Property-name test helper contract

Layer / File(s)	Summary
Property-name helper wiring and usage test/napi/napi-app/js_test_helpers.cpp, test/napi/napi-app/module.js	Removes the obsolete array-returning helper, registers additional helpers, and reads property names from the returned .keys field.

Possibly related PRs

• oven-sh/bun#34131: Also changes napi_detach_arraybuffer validation behavior.
• oven-sh/bun#34141: Also updates the property-name helper result contract and its JavaScript consumer.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title matches the main change: rejecting SharedArrayBuffer in napi_detach_arraybuffer.
Description check	✅ Passed	It covers the PR purpose and verification, though it uses custom headings instead of the template's exact section names.

---

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

[robobun]

The js_test_helpers.cpp / module.js dedup here is the same change as #34141 (needed so the napi addon compiles at all). Whichever of the two lands first, the other will need a trivial rebase on those two files; the napi.cpp fix and the new test_detach_arraybuffer coverage are independent of that.

[github-actions]

This PR may be a duplicate of:

1. test(napi): remove duplicate get_all_property_names helper #34141 - Also removes the duplicate get_all_property_names helper from js_test_helpers.cpp and updates module.js — the same sub-fix bundled into this PR

🤖 Generated with Claude Code

[claude]

No bugs found. This is a focused N-API compat fix with a checkSameOutput test that diffs Bun against Node byte-for-byte, but it changes error-return semantics for two production N-API functions (including relaxing napi_is_detached_arraybuffer from error → napi_ok/false on non-ArrayBuffers, and tightening napi_detach_arraybuffer from napi_ok → error on non-detachable buffers). Worth a human look given N-API's ecosystem-wide reach.

What was reviewed:

• napi_detach_arraybuffer / napi_is_detached_arraybuffer logic in napi.cpp — the isShared() gate matches the established pattern from #32624, and isDetached() || isDetachable() correctly keeps double-detach as napi_ok.
• Confirmed napi_detachable_arraybuffer_expected is a defined status and JSArrayBuffer::isShared() is used elsewhere in bindings.
• Checked the existing test_napi_empty_buffer_info path (calls napi_is_detached_arraybuffer on a real ArrayBuffer) — unaffected by the new short-circuit.
• The get_all_property_names dedup + .keys caller update in module.js is consistent with the surviving {status, keys} helper.

Extended reasoning...

Overview

Two small edits to src/jsc/bindings/napi.cpp aligning napi_detach_arraybuffer and napi_is_detached_arraybuffer with Node.js semantics, plus a new test_detach_arraybuffer C++ helper in the napi-app addon and a checkSameOutput test in napi.test.ts that asserts Bun's status codes match Node's for [SharedArrayBuffer, ArrayBuffer, already-detached ArrayBuffer, Uint8Array]. Also removes a duplicate get_all_property_names definition in js_test_helpers.cpp (two prior PRs each landed one, breaking the addon build) and updates the one caller in module.js to use the surviving {status, keys} return shape.

Security risks

None. No untrusted-input parsing, allocation sizing, or auth/crypto paths are touched. The change only adjusts which napi_status enum value is returned for edge-case inputs.

Level of scrutiny

Medium-high. The diff itself is ~10 lines of production code and follows the exact pattern established in #32624 (!jsArrayBuffer->isShared() to model V8's IsArrayBuffer()). But N-API is load-bearing for every native addon, and this PR changes return semantics in three ways: (1) SAB detach now errors instead of silently succeeding, (2) non-detachable non-shared buffers (e.g. Wasm memory) now error instead of silently succeeding, (3) napi_is_detached_arraybuffer on non-ArrayBuffers now returns napi_ok/false instead of napi_arraybuffer_expected. All three match Node — verified by the checkSameOutput diff test — but they are user-observable behavior changes that a maintainer should acknowledge.

Other factors

• The PR description notes tests could not be run locally ("Platform-specific test(s) that do not run on this machine. Deferring to CI"), so correctness rests on the CI run.
• The test addon dedup is a necessary drive-by build fix, but it's a second concern in the PR that a reviewer should be aware of.
• The isDetached() || isDetachable() disjunct is defensively written to keep double-detach at napi_ok regardless of whether JSC's isDetachable() flips after detach — this looks correct but is exactly the kind of engine-semantics subtlety a maintainer familiar with JSC's ArrayBuffer internals should confirm.

[claude]

I didn't find any bugs. The change is small and well-reasoned, but it alters error-return semantics for two N-API functions in production C++ bindings, and overlaps with #34131/#34141 — worth a human confirming the direction and merge order.

What was reviewed:

• napi_detach_arraybuffer / napi_is_detached_arraybuffer logic vs Node's IsArrayBuffer()/IsDetachable() gates — matches; already-detached path still returns napi_ok.
• Short-circuit ordering in jsArrayBuffer && !isShared() && impl()->isDetached() — no null-deref regression vs old code.
• New checkSameOutput test covers SAB, AB, re-detach, and typed array; existing test_napi_empty_buffer_info still passes the new napi_is_detached_arraybuffer contract.
• get_all_property_names dedup: remaining {status, keys} overload is still registered; the one bare-array caller in module.js was updated.

Extended reasoning...

Overview

Two N-API functions in src/jsc/bindings/napi.cpp are aligned with Node.js semantics:

• napi_detach_arraybuffer: rejects SharedArrayBuffer with napi_arraybuffer_expected (was silently returning napi_ok), and rejects non-detachable buffers with napi_detachable_arraybuffer_expected (was silently succeeding). Already-detached buffers still succeed.
• napi_is_detached_arraybuffer: now always returns napi_ok and writes false for any non-ArrayBuffer/SAB input, instead of returning napi_arraybuffer_expected without writing *result.

Test coverage is added via a new test_detach_arraybuffer addon function driven by checkSameOutput (byte-for-byte diff against Node). A bundled compile fix removes a duplicate get_all_property_names definition in the test addon and updates its one caller.

Security risks

None. This tightens validation (previously lied about detach success on a shared buffer, which the PR correctly frames as a memory-lifetime hazard for addons). No new untrusted-input parsing, no allocation changes.

Level of scrutiny

Medium. The diff is ~15 lines of production C++, follows the exact pattern of the neighboring napi_is_arraybuffer SAB fix (same !isShared() guard, same macro usage), and is verified against Node via checkSameOutput. But it changes user-visible error codes for two N-API entry points — an addon that previously got napi_ok on a non-detachable buffer will now get napi_detachable_arraybuffer_expected. That's the Node-correct behavior, but it's a semantic change a maintainer should sign off on.

Other factors

• The PR notes tests couldn't be run locally (platform-specific; deferred to CI).
• Overlaps with open PR #34131 (also touches napi_detach_arraybuffer validation) and #34141 (same test-helper dedup) — merge order needs human coordination.
• I checked that the new napi_is_detached_arraybuffer contract doesn't break the existing test_napi_empty_buffer_info test (it passes a real non-shared detached ArrayBuffer, so is_detached remains true).
• NAPI_RETURN_EARLY_IF_FALSE correctly routes the new error code through napi_set_last_error, so napi_get_last_error_info stays consistent.

[robobun]

CI status across builds #72841 and #72871: test/napi/napi.test.ts (the suite this PR touches) passes on every lane. Remaining reds are unrelated to this diff:

• test/js/node/test/parallel/test-net-connect-memleak.js (Alpine x64, GC-timing)
• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (Debian x64-asan, JSC getOwnPropertyDescriptor exception-scope assert)
• assorted flaky retries: complex-workspace, es-module-lexer, webview-chrome, s3, proxy-stress-errors, etc.

None of these exercise N-API ArrayBuffer detach. Ready for review.