napi: fix dropped writes through data pointers for small typed arrays - #37154
Conversation
…a pointers JSC keeps typed arrays under 1000 bytes in fast mode, where the contents live in GC-heap storage. Materializing the backing ArrayBuffer (e.g. via the arraybuffer out-param of napi_get_typedarray_info, or JS touching .buffer) copies the contents into a fresh ArrayBuffer and repoints the view, abandoning the old storage. napi_get_typedarray_info captured vector() before fetching the arraybuffer out-param, so the data pointer it returned pointed at the abandoned storage and native writes through it were silently dropped. napi_get_buffer_info, napi_create_buffer and napi_create_buffer_copy handed out fast-mode pointers that went stale as soon as anything materialized the buffer. Force the view into wasteful mode before capturing the pointer in all four, matching Node, where the pointer stays valid for the lifetime of the object. Fixes #37151
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe change materializes ArrayBufferView backing storage before N-API APIs expose data pointers. It adds the JSC-to-N-API binding, stable buffer allocation paths, and regression tests for pointer writes, byte offsets, and pointer stability. ChangesArrayBufferView pointer stability
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes the pointer-lifetime contract NAPI hands to native addons (forcing small typed arrays out of JSC fast mode before returning vector()), which is memory-safety-adjacent enough that a human look is worthwhile.
What was reviewed:
- Sibling coverage:
napi_get_dataview_infodoesn't need the fix (DataView always has a materialized buffer);napi_get_arraybuffer_infoandnapi_create_typedarrayoperate on already-materialized buffers. Bun__JSValue__materializeArrayBufferViewBufferguards non-cell/non-view inputs, so the pre-validation call innapi_get_buffer_infois safe on empty/ArrayBuffer/primitive values.possiblySharedBuffer()returning nullptr on OOM leaves the view in fast mode — same as pre-PR behavior, no new failure path.- Perf trade-off (every NAPI-touched small array now allocates + copies into wasteful mode) is gated on the caller requesting the data pointer.
Extended reasoning...
Overview
The PR fixes silent data corruption where native addons writing through data pointers returned by napi_get_typedarray_info / napi_get_buffer_info / napi_create_buffer / napi_create_buffer_copy see their writes dropped for typed arrays under ~1KB. Root cause: JSC keeps small typed arrays in fast mode (GC-heap storage, no ArrayBuffer); materializing the backing buffer copies contents into a fresh ArrayBuffer and abandons the old storage. Bun captured vector() before materialization, handing out a soon-to-be-stale pointer. The fix adds a helper that forces possiblySharedBuffer() before capturing the pointer in all four entry points, and adds C++ test fixtures + checkSameOutput-vs-Node tests covering write visibility and pointer stability across the fast/wasteful boundary.
Security risks
None new. The change moves in the direction of stronger memory safety — it eliminates a case where native code writes to abandoned storage. The new C++ binding is defensive (dynamicDowncast, null/non-cell guards). No untrusted-input parsing is introduced.
Level of scrutiny
High. This is native memory-safety code that hands raw pointers to arbitrary third-party addons with a lifetime guarantee. REVIEW.md flags this as the most-blocked category. The fix is small and the mechanism is clearly explained, but the correctness depends on JSC internals (slowDownAndWasteMemory repointing m_vector, idempotence of possiblySharedBuffer(), DataView never being fast-mode) that a maintainer should confirm.
Other factors
- Sibling analysis looks complete: I checked
napi_get_dataview_info(DataView requires an existing ArrayBuffer, never fast-mode),napi_get_arraybuffer_info(bare ArrayBuffers have no fast mode), andnapi_create_typedarray/node_api_create_buffer_from_arraybuffer(both take an existing buffer). - The materialize call in
napi_get_buffer_inforuns before the ArrayBuffer/view type check, but the C++ helper'sdynamicDowncast<JSArrayBufferView>makes it a no-op for bare ArrayBuffers and non-cells, so the reject path is unchanged. - There is a real performance trade-off: every small typed array whose data pointer NAPI hands out now incurs an allocation + memcpy into wasteful mode. This is required for Node parity and is gated on
!data.is_null(), but a maintainer should sign off on it. - Tests are Node-diffed via
checkSameOutput, cover the 999-byte fast-mode boundary and the 2048-byte already-wasteful case, and include the pointer-stability-after-.buffer-access scenario. The three unrelated timeout bumps are commented with the debug+ASAN justification per REVIEW.md. - The PR description notes tests couldn't run locally (platform-specific), so CI is the verification of record — another reason for a human check before merge.
|
Thanks for the review. One correction to the summary: the tests did run locally. The new fixtures fail against the released bun (writes invisible, node shows them) and pass with this change, and the full test/napi/napi.test.ts (164 tests) plus Node's js-native-api typedarray and dataview suites are green with a debug+ASAN build. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This isn't the correct fix. The correct fix is to create the JSC::ArrayBuffer and WTFMove that. What you have here allocates the ArrayBuffer twice.
|
Thanks @Jarred-Sumner This is essential for my framework to work. Rust Engine -> Bun |
|
@robobun use JSC::ArrayBuffer. Fix. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 1448-1454: Propagate backing-storage materialization failures
instead of returning success with invalidated pointers: in
src/runtime/napi/napi_body.rs lines 1448-1454, preserve invalid-input handling
and fail before writing maybe_data when
typedarray.materialize_array_buffer_view_buffer() fails; likewise fail before
writing result_data at lines 2062-2066 and data at lines 2102-2106 when buffer
materialization fails. In src/jsc/bindings/napi.cpp lines 2282-2285, check
possiblySharedBuffer() and return failure before reading typedVector().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f65df2ba-9692-4258-9a14-9b39d9711f1c
📒 Files selected for processing (6)
src/jsc/JSValue.rssrc/jsc/bindings/bindings.cppsrc/jsc/bindings/napi.cppsrc/runtime/napi/napi_body.rstest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.ts
Instead of creating a fast-mode view and converting it (two allocations plus a copy), napi_create_buffer and napi_create_buffer_copy now create the JSC::ArrayBuffer up front and move it into the view. Throw OOM when the allocation fails, matching Node's pending-exception behavior. The accessor paths keep the one-time materialization for JS-created views and now fail instead of handing out a pointer when it fails.
|
Reworked in ce36053: napi_create_buffer and napi_create_buffer_copy now create the JSC::ArrayBuffer up front and move it into the view, so it is one allocation with no copy and the view is never in fast mode. The accessor paths (napi_get_typedarray_info, napi_get_buffer_info) keep the one-time possiblySharedBuffer() materialization since those views were created by JS and already sit in fast-mode GC storage; that conversion is the same one a JS .buffer access performs. Full napi suite passes locally, including the Node status-code alignment test (allocation failure now leaves a pending OOM exception like Node). |
…ze new tests Nesting a ThrowScope inside NAPI_PREAMBLE's TopExceptionScope trips validateExceptionChecks; follow the napi_create_bigint_words shape instead and drop the unreachable trailing return. Run the new tests' independent checkSameOutput calls concurrently.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/napi.cpp`:
- Around line 2324-2326: In the relevant N-API binding function, validate that
data is non-null whenever length > 0 before allocating the ArrayBuffer or
calling memcpy; return napi_invalid_arg immediately for a null source, while
preserving the existing copy behavior for valid inputs.
In `@test/napi/napi.test.ts`:
- Around line 925-930: Add the exact 1000-byte case to the size variants in the
Promise.all test for checkSameOutput("test_typedarray_info_write_visibility"),
preserving the existing expected length/visibility assertions so the
fast-storage cutoff boundary is covered.
- Around line 933-940: Update the test case around checkSameOutput for typed
arrays to include views created with nonzero byte offsets, such as subarrays or
typed-array constructors over an offset buffer. Keep the existing zero-offset
and small-size cases, and assert the same data-pointer relationship for the
offset views so incorrect byte-offset calculations are detected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 719e2a51-0f43-4b5a-82bf-4c6099a49e97
📒 Files selected for processing (2)
src/jsc/bindings/napi.cpptest/napi/napi.test.ts
|
CI status: the remaining red lane is test/napi/napi.test.ts on macOS 14 x64, failing in the pre-existing "napi_reference_unref is blocked from finalizers in experimental modules" test (also failing on main, reported separately). The other failures passed on retry or when run alone. The changes in this PR are green on every lane. |
3 participants
Fixes #37151
NAPI-RS users reported that native writes into
Uint8Arrays under ~1KB were silently dropped (data corruption), while the same code works in Node.Repro
A native function that requests every out-param of
napi_get_typedarray_info(as napi-rs does), writes through the returned data pointer, then reads the array back from JS:The existing
test_typedarray_info_byte_offsetfixture also exposes it: fornew Uint8Array(64)(no explicit ArrayBuffer), bun printeddata_is_arraybuffer_data_plus_byte_offset=falsewhere node printstrue.Cause
JSC keeps typed arrays under 1000 bytes (
fastSizeLimit) in fast mode: the contents live in GC-heap storage with no ArrayBuffer. Materializing the backing buffer (possiblySharedBuffer()) copies the contents into a fresh ArrayBuffer and repoints the view, abandoning the old storage.napi_get_typedarray_infocapturedvector()first and fetched the arraybuffer out-param afterwards, so the pointer it returned pointed at the abandoned storage; writes through it never reach the view.napi_get_buffer_info,napi_create_bufferandnapi_create_buffer_copyhad the sibling bug: they handed out fast-mode pointers that go stale as soon as anything later materializes the buffer (JS touching.bufferis enough), while Node guarantees the pointer stays valid for the object's lifetime.Fix
Add
Bun__JSValue__materializeArrayBufferViewBuffer(forces the view into wasteful mode viapossiblySharedBuffer()) and call it before capturing the data pointer in the four NAPI entry points, matching Node's semantics.Verification
New napi-app fixtures compare output against Node: write visibility through
napi_get_typedarray_infodata pointers (16/999/2048 bytes), and pointer stability fornapi_get_buffer_info/napi_create_buffer/napi_create_buffer_copyafter the backing buffer is materialized. All fail on the released bun and pass with this change; fulltest/napi/napi.test.ts(164 tests) and Node'sjs-native-apitypedarray/dataview suites pass.Also bumps the timeout on three pre-existing tests in this file that spawn 4+ sequential debug+ASAN bun processes and exceed the 5s default on slow machines.
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