Repository navigation
Conversation
…nsafe.arrayBufferToString, node:zlib)
An ArrayBuffer over memory that JSC did not allocate (bun:ffi
toArrayBuffer, an N-API external buffer, Bun.mmap with an offset, a
slice of a large Blob) can start at any address. The console formatter
and Bun.unsafe.arrayBufferToString cast the bytes of a typed array to
`&[T]` with `bytemuck::cast_slice`, which panics when the address is
not aligned for `T`:
panic: cast_slice>TargetAlignmentGreaterAndInputNotAligned
`ArrayBuffer::as_u32` (node:zlib) made the same cast behind a debug
assertion.
`ArrayBuffer::elements::<T>()` and `elements_mut::<T>()` now return
`&[Unaligned<T>]`, built by `Unaligned::slice_from_bytes`. The console
formatter, the bun:test formatter and node:zlib read and write elements
through it. `arrayBufferToString` keeps the cast when the address is
even, and copies the units when it is not. `ArrayBuffer::as_u32`,
`as_u32_unaligned` and `Unaligned::slice_align_cast_mut` are deleted.
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Status: ready for review. #46580 How I reproduced it (linux x64): import { ptr, toArrayBuffer } from "bun:ffi";
const backing = new Uint8Array(64).fill(1);
console.log(new Int32Array(toArrayBuffer(ptr(backing), 1, 16)));
|
|
Updated 7:23 AM PT - Oct 23rd, 2026
✅ @robobun, your commit 0edfa004ab0719fb4cf3c84107c4f046f1e21c32 passed in 🧪 To try this PR locally: bunx bun-pr 46580That installs a local version of the PR into your bun-46580 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe PR adds unaligned element views for ChangesUnaligned Typed-Array Access
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title names three areas (console.log, Bun.unsafe.arrayBufferToString, node:zlib) but the main change is that typed array elements are now read in place through Unaligned so reads work at any ad… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The pull request description clearly explains the problem, the fix, scope, limitations, performance impact, and verification results. It does not use the exact template headings, but it provides compl… |
Full details: Title check
-
Explanation
The title names three areas (console.log, Bun.unsafe.arrayBufferToString, node:zlib) but the main change is that typed array elements are now read in place through Unaligned<T> so reads work at any address. The title states that main idea and the affected call sites. It is specific and not misleading. The summary matches the changeset: ArrayBuffer::elements/elements_mut, Unaligned<T> views in the console formatter, arrayBufferToString's odd-address copy path, and zlib/zstd/brotli parameter reads, with as_u32 removed.
Full details: Description check
-
Explanation
The pull request description clearly explains the problem, the fix, scope, limitations, performance impact, and verification results. It does not use the exact template headings, but it provides complete information for both required sections: what the PR does and how the code was verified.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the refactored paths for behavior drift and ruled these out: flush_write_result keeps the old write order (index 0 gets avail_out, index 1 gets avail_in, same as the previous r0/r1 split); the Pod impl for #[repr(C, packed)] Unaligned<T> is sound and slice_from_bytes cannot hit a cast_slice panic (target align is 1, length is truncated to a multiple of size_of::<T>(), ZST is a const assert); and the new .len < 2 checks in the zlib/brotli/zstd init paths run after the Uint32Array type check, so len is the element count the old as_u32().len() returned.
Extended reasoning...
The change replaces aligned &[T] casts over JS ArrayBuffer storage with &[Unaligned<T>] reads in the console formatter, the bun:test formatter, Bun.unsafe.arrayBufferToString, and the three node:zlib bindings, and deletes ArrayBuffer::as_u32 and Unaligned::slice_align_cast_mut. It touches memory-safety-sensitive helpers (bun_core::Unaligned, ArrayBuffer::elements) but no auth, crypto, or injection surface. The one confirmed inline finding (copying a large odd-address Uint16Array before the string length limit is checked) plus the core-helper scope mean a human should still look, so this is a defer rather than an approve.
…ess view The odd-address arm collected the units into a `Vec<u16>` and only then found that a string cannot hold them. A view of 2^31 units cost a 4 GiB copy to return the empty string, and a failed allocation aborted. The arm now calls `encoding::to_bun_string`, which allocates the string first and copies the bytes into it: one allocation, and no copy when the string cannot be made. The result is the empty string then, as at an even address.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after 0929eba and found no bugs; the second commit addresses the earlier inline concern (the odd-address arrayBufferToString path now goes through to_bun_string, which fails fast in tryCreateUninitialized before any copy, and the sparse-mmap test pins that). Because this changes the generic ArrayBuffer element accessor and adds unsafe impl Pod/Zeroable for Unaligned<T>, a human look from someone who owns the JSC bindings would still be worthwhile.
What was reviewed:
Unaligned<T>Pod/Zeroable impls andslice_from_bytes{,_mut}:repr(C, packed)over a PodThas no padding and align 1, socast_slicecannot panic once the length is truncated to a multiple ofsize_of::<T>().flush_write_resultordering: old code passed&mut r1[0], &mut r0[0]as(avail_in, avail_out), so index 0 = avail_out and index 1 = avail_in — the new locals preserve that; detached/shrunk buffers still return early viabyte_slice_mutreturning empty.- Too-long and odd-address
arrayBufferToStringboth end in an empty string, matching the alignedEncodedSlice::to_jspath (toStringCopyreturns{}ontryCreateUninitializedfailure);is_dead()covers both the Dead and OutOfMemory tags. - Searched for remaining callers of
as_u32/as_u32_unaligned/slice_align_cast_mut: none left. One test nit only: the Response-body case in inspect.test.js pollscollected < 2without a bound, so a never-firing finalizer hangs the child until the test timeout rather than failing with a message.
Extended reasoning...
The change replaces panicking bytemuck::cast_slice reads of JS typed-array storage with &[Unaligned<T>] reads in the console formatter, bun:test formatter, Bun.unsafe.arrayBufferToString, and the three node:zlib handles, deleting the debug-only-checked ArrayBuffer::as_u32 accessor and three unsafe blocks while adding two unsafe impl lines in bun_core. It touches no auth, crypto, or injection surface; the sensitive part is memory safety of a generic reinterpretation helper used across the runtime. I verified the Pod soundness argument, the preserved index ordering in flush_write_result, the detached-buffer path, and that the fallback string path checks length before allocating; the new tests spawn children for every crashing input and cover ffi, mmap, memfd Blob, Response body, zlib/brotli/zstd. Deferring rather than approving because a core jsc accessor and new unsafe impls merit an owner's look, and the bug hunt only ran dry rather than proving the generic helper sound at every future call site.
Also give the Response body test an error when the Blob and its slice are not collected, in place of the test timeout.
|
Changes after the first review round:
|
There was a problem hiding this comment.
I re-reviewed the two new commits and found no bugs; the earlier concern about arrayBufferToString copying before the length check is addressed (the fallback now goes through to_bun_string, whose tryCreateUninitialized rejects an over-long length before allocating, and the sparse-mmap test pins copied: false). Because this adds unsafe impl Pod/Zeroable for Unaligned<T> and reworks slice reinterpretation used by the console formatter and node:zlib, a human look at the soundness argument would still be worthwhile.
What was reviewed:
Unaligned<T>Pod/Zeroable impls andslice_from_bytes[_mut]:repr(C, packed)over a PodThas no padding and align 1, the truncation arithmetic cannot underflow, and thesize_of != 0const assert rules out a zero-sizedT;bun_core::f16already has a Pod impl, so theTypedArrayElement: Podbound compiles for Float16Array.flush_write_result: the newwrite_result[0] = avail_out/[1] = avail_inmatches the oldr1[0]/r0[0]mapping againstupdate_write_result(avail_in, avail_out); thelen < 2checks in the threeinitfunctions run after the Uint32Array type check, so.lenis the element count as before.- No remaining callers of
as_u32/as_u32_unaligned/slice_align_cast_mutinsrc/.
Extended reasoning...
The change replaces aligned &[T] casts over JSC ArrayBuffer bytes with &[Unaligned<T>] views in the console formatter, the bun:test pretty formatter, Bun.unsafe.arrayBufferToString and the three node:zlib bindings, adds Pod/Zeroable impls and two safe constructors on Unaligned<T>, and deletes ArrayBuffer::as_u32. It touches no auth, injection or data-exposure surface, but it does add new unsafe impl blocks and changes slice reinterpretation on memory adopted from bun:ffi, N-API and Bun.mmap, which is the memory-safety category Bun reviewers block most often. The two commits since the last review address the one inline finding previously posted, the bug hunt ran dry with no findings, and the behavior-preserving claims (zlib out-parameter ordering, length checks, dead-code removal) check out, which is why this is a defer rather than a request for changes; the unsafe surface and the ~500-line size are why it is not an approve. No CODEOWNERS entry covers the changed files.
|
Closing this PR. #47037 landed the same change. On main, I added the tests of this PR to main at 9b8347a, without its source changes, and ran them on a debug build. |
Problem
ArrayBufferis at an unaligned address:panic: cast_slice>TargetAlignmentGreaterAndInputNotAligned, exit 134. Repro:console.log(new Int32Array(toArrayBuffer(ptr(u8), 1, 16)))withbun:ffi. v1.3.14 prints it.print_typed_array(src/jsc/ConsoleObject.rs:5557) andBun.unsafe.arrayBufferToString(src/runtime/api/UnsafeObject.rs:35) cast the bytes withbytemuck::cast_slice, which panics.ArrayBuffer::as_u32(node:zlib) checks only in debug builds.Fix
ArrayBuffer::elements::<T>()returns&[Unaligned<T>]. The console formatter, the bun:test formatter and node:zlib read through it.arrayBufferToStringcopies the units when the address is odd.as_u32is deleted.&[T]is formed. x64 and arm64 load unaligned with the same instruction. Node prints these arrays.test/js/bun/util/inspect.test.js,unsafe.test.jsandtest/js/node/zlib/zlib-handle-bounds-check.test.ts. Alsoconsole-log.test.ts,zlib.test.js.cast_sliceon buffer bytes.Background
ArrayBuffercan adopt memory that JSC did not allocate (bun:ffi, N-API,Bun.mmap, a slice of a largeBlob). It starts at any address.Unaligned<T>is#[repr(C, packed)]around oneT, valid at any address. A&[T]at an unaligned address is undefined behaviour.as_u32keeps its debug-only check. Considered aligning in the producers:bun:ffiand N-API addresses are the caller's choice.Downsides
.textsize do not change.cast_slice(buffer.byte_slice())still compiles. A clippydisallowed-methodsentry would reject it (68 call sites, not here).Notes
Origin. Found during the review of #45947. That PR rewrites the memfd mapper and the
BlobTransfer arm and leaves the readers to a separate change, which is this one. There is no GitHub issue for this abort. The bun.report and Sentry counts were not checked.History of this panic. #31417 handled it for
Blob.text()andBlob.json()(it took over #31006). #31188 is a user report of the same cast for an odd length. #45450 added anis_empty()guard toarrayBufferToStringfor the empty slice of a detached view. This PR removes that guard:try_cast_slicefails for that slice and the copy arm returns the empty string.test/js/bun/util/unsafe.test.jspins it.Regression window. The Zig code used an unchecked
@alignCast. The Rust port (#30412) made the cast checked. v1.2.20, v1.3.5 and v1.3.14 print a view of abun:ffibuffer and of a slice of a 12 MiBBlob. v1.4.0 to v1.4.4 abort.Bun.mmapreturns the view at the requested offset since #34120 (v1.4.0), so v1.3.14 had no unaligned view there.Who reaches it. Every entry into the console formatter:
console.*,%o, a table cell,Bun.inspect, the REPL, the printer of an uncaught error, the message of a failed bun:test matcher (the test runner aborts), a Node error that prints the value it received. Withoutbun:ffi: a slice at an odd offset of aBlobof 8 MiB or more (Linux), aResponsebody that is a slice of aBlob,Bun.mmapat an odd offset.Not changed.
bun:ffiand N-API take the pointer of the caller.Bun.mmapis zero-copy by contract. Blob: count the copy-on-write copies of a memfd store, so two reads of one large body do not share memory #45947 owns the memfd mapper and the Transfer arm.src/runtime/webcore/Blob.rs:2339and:2558. They already handle an odd address (Hardening: input validation and bounds tightening across 27 subsystems (round 6) #31417), and fix: decode UTF-16BE source files #34712 merges them into one helper.write_utf16casts. They take the units of a JS string.bytemuck::cast_slice(buffer.byte_slice())still compiles, andcast_slice::<Unaligned<T>, T>now compiles too. Both panic on an unaligned address.Credit. The
Podimpl forUnaligned<T>and thepretty_format.rshunk are from #40235. #44268 deletespretty_format.rsandUnaligned::getand keeps thecast_slicearms ofConsoleObject.rs. The PR that lands second rebases.Measurements. Release builds on linux x64 of 5c6d4e2 (main) and of 67693a9 (this PR).
llvm-objdump)print_typed_arrayper call,Int8Array(4)(gdbnexti)Uint8Array(4)Int32Array(4)print_typed_arrayBun.inspect(Int32Array(513)), instructions per printBun.inspect(Uint8Array(513)), instructions per printflush_write_resultof zlib, brotli, zstd, instructionsdeflateSync(1 KiB), instructions per callarrayBufferToString, even address, allocations per 1000 callsarrayBufferToString, odd address, allocations per 1000 callsbunfile size,.textsizeunsafeblocksunsafe implinstructions:ufromperf_event_open, with main and the PR interleaved 10 times. The table has the lowest count of each side. Main against main differs by 6 to 39 instructions per print and by 71 perdeflateSynccall.print_typed_arrayruns 3 more instructions for a 1-byte array. They are register moves.Uint8Array(513)as a whole is 15 above main in the lowest counts and 2 below main in the medians. An earlier pair of builds, on fec4a88, gave 28 above main. I did not find a cause other than those 3 instructions.arrayBufferToStringat an odd address allocates the string first and copies the bytes into it (encoding::to_bun_string): one allocation and one copy, as at an even address. A view of 2^31 units gives the empty string at both addresses, with no copy.print_typed_array1693 to 1556 B and 1735 to 1571 B, the three node:zlibinitfunctions by -13, -29 and -36 B,arrayBufferToString288 to 486 B.Tests.
The two node:zlib tests fail only on a debug or ASAN build of main. A release build of main stores the right values through a misaligned
&mut [u32].Each child that reads a buffer that Bun makes (
Bun.mmap, a slice of a largeBlob, aResponsebody) prints the address of the buffer modulo 4 first, so the test fails if the buffer is aligned. TheBlobtest runs only where a memfd is available (Linux). The twoBun.mmaptests do not run on Windows, which has noBun.mmap. The other tests run on every platform.aarch64
Atomics: a separate problem, not changed here. JSC runsAtomics.*with atomic instructions, and arm64 faults on an unaligned atomic access. Measured with Bun v1.4.4 linux-aarch64 underqemu-aarch6410.0.13 (user mode), not on arm64 hardware:-cpu cortex-a72and-cpu neoverse-n1(no LSE2):Atomics.loadon element 0 of anInt32Arrayover an unaligned buffer ends inBus error, exit 135.-cpu neoverse-v1and-cpu max(LSE2): an element that crosses a 16-byte boundary ends inBus error.Atomics.add(new Int32Array(await new Blob([new Uint8Array(12 * 1024 * 1024)]).slice(1, 17).arrayBuffer()), 0, 1)ends inBus errorwith-cpu neoverse-n1. Node gives an aligned buffer for a slice of aBlob.node:ffibuffer at an unaligned address.Only an aligned buffer from the producer avoids it. That is a change to the memfd mapper and the Transfer arm (#45947).
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/unsafe.test.js, test/js/bun/util/inspect.test.js