Repository navigation
Bun.mmap: return view at requested offset, not page-aligned offset - #34120
Conversation
|
Updated 8:05 PM PT - Jul 18th, 2026
❌ @robobun, your commit 6d6510e has 5 failures in
🧪 To try this PR locally: bunx bun-pr 34120That installs a local version of the PR into your bun-34120 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesBun’s mmap implementation now page-aligns file offsets, returns a mapping delta, exposes the requested mmap offset and cleanup
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 `@test/js/bun/util/mmap.test.js`:
- Around line 73-105: Refactor the parameterized mmap test around the cases
array to use it.each(cases), passing each offset/size case into its own test
invocation. Preserve the existing assertions and gcTick cleanup while removing
the manual for loop so each case is reported and isolated independently.
🪄 Autofix (Beta)
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: 354da5a9-d799-4d84-aaac-2d34b7ec2571
📒 Files selected for processing (3)
src/runtime/api/BunObject.rssrc/sys/lib.rstest/js/bun/util/mmap.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes unsafe mmap/munmap pointer handling (the deallocator now recovers the mapping base via page-rounding), so it's worth a human look.
What was reviewed:
- Verified
bun_sys::mmap_filehas no other callers, so the signature change is safe. - Checked
delta < map_lenholds on the success path (size==0 → EINVAL, elsemap_len = size + delta), soview_ptr/view_lenstay in-bounds. - Confirmed
bun_sys::page_size()andbun_alloc::page_size()are the same function, so map-time and dealloc-time deltas agree. make_typed_array_with_bytes_no_copydocs confirm the deallocator receives the originalptr, so page-rounding recoversmap_baseexactly.
Extended reasoning...
Overview
This PR fixes Bun.mmap({offset}) to return a view starting at the requested byte instead of the page-aligned boundary. It touches three files: src/sys/lib.rs changes mmap_file to compute delta = offset % page_size, map size + delta bytes from the aligned offset, and return (map, delta); src/runtime/api/BunObject.rs slices the returned mapping by delta before handing it to JSC and updates the deallocator to round the view pointer down to a page boundary to recover the munmap base; test/js/bun/util/mmap.test.js adds three test blocks (9 offset/size combinations, shared-write positioning, offset-past-EOF).
Security risks
None identified. The mmap path is only reachable via the explicit Bun.mmap() API on files the process can already open RDWR. Size arithmetic (size + delta) is bounded by stat_size - offset + page_size, which cannot realistically overflow on 64-bit. The requested-size clamp (size.min(wanted_size)) is unchanged.
Level of scrutiny
This is memory-safety-sensitive native code: unsafe pointer arithmetic, a changed syscall-wrapper signature, and a custom JSC deallocator that now relies on a non-local invariant (mmap returns a page-aligned base and delta < page_size) to recover the munmap address. The reasoning is sound and well-commented, and the tests exercise the finalizer via gcTick(), but per this repo's review norms memory-management changes are the most-blocked category and warrant a maintainer sign-off rather than bot approval.
Other factors
There is a minor intentional behavior change: an unaligned offset at/past EOF now throws EINVAL instead of returning the bytes between the page boundary and EOF. This is documented in the PR description and tested, and it makes the aligned/unaligned cases consistent, but it is user-visible. CodeRabbit's only feedback (use it.each) was addressed in f22e3a6. No prior claude[bot] review on this PR.
|
@robobun fix clippy |
|
Merged main in b1f38e2 to pick up the |
There was a problem hiding this comment.
Beyond the inline nit, I checked a few memory-safety concerns and ruled them out: bun_sys::page_size is a re-export of bun_alloc::page_size (src/sys/lib.rs:3554), so the delta computed at map time and the round-down in munmap_dealloc agree; view_ptr = map_base + delta with map_base page-aligned and delta < page means addr - addr % page recovers exactly map_base; and bun_sys::mmap_file has no other callers, so the signature change is contained. Leaving native mmap/munmap pointer arithmetic for a human to sign off on.
Extended reasoning...
This PR changes the deallocator contract for Bun.mmap's backing ArrayBuffer and adds pointer arithmetic on both the map and unmap sides. I traced the invariants and they hold, but per repo guidance native memory-safety changes get a human look. The one finding (tempDir leak) is cosmetic and doesn't affect the fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/util/mmap.test.js (1)
123-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an explicit numeric
size: 0regression case.The EOF tests cover implicit zero-length mappings, but not
{ offset: 0, size: 0 }. The mmap contract maps any resulting zero length toEINVAL; lock this behavior with an explicit assertion.Proposed test
expect(() => Bun.mmap(file, { offset: 100 })).toThrow("EINVAL"); expect(() => Bun.mmap(file, { offset: 50 })).toThrow("EINVAL"); + expect(() => Bun.mmap(file, { offset: 0, size: 0 })).toThrow("EINVAL");As per coding guidelines, every behavioral change must include an automated test in the same PR.
🤖 Prompt for 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. In `@test/js/bun/util/mmap.test.js` around lines 123 - 130, Add an explicit assertion in the “mmap offset past EOF throws EINVAL” test for Bun.mmap with offset 0 and numeric size 0, verifying it throws EINVAL while preserving the existing implicit zero-length offset cases.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/util/mmap.test.js`:
- Around line 123-130: Add an explicit assertion in the “mmap offset past EOF
throws EINVAL” test for Bun.mmap with offset 0 and numeric size 0, verifying it
throws EINVAL while preserving the existing implicit zero-length offset cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a99f3e6c-ce6e-4dad-92cd-2606d4c94297
📒 Files selected for processing (1)
test/js/bun/util/mmap.test.js
|
Skipping the |
There was a problem hiding this comment.
No new issues found — the earlier tempDir-leak nit is now addressed with afterAll(() => dir[Symbol.dispose]()). I'm deferring rather than approving because munmap_dealloc now recovers the mmap base by page-rounding the view pointer JSC hands back; that's unsafe pointer arithmetic in a GC finalizer and worth a human glance even though the invariant (mmap returns page-aligned, delta < page_size, JSC passes back the exact bytes pointer) checks out.
Checked: bun_sys::page_size is a re-export of bun_alloc::page_size (src/sys/lib.rs:3554), so the delta computation and the dealloc round-down use the same value. mmap_file has no other callers, so the signature change is contained. The offset=0 path is byte-identical to the old behavior (delta=0, view_ptr=map_base, map_len=view_len).
Extended reasoning...
Overview
Fixes Bun.mmap returning data at the page-aligned floor of offset instead of the requested byte. Three files: src/sys/lib.rs changes mmap_file to compute the page-alignment delta internally and return (full_map, delta); src/runtime/api/BunObject.rs slices the JSC Uint8Array to [map_base+delta, map_base+map_len) and updates munmap_dealloc to round the view pointer down to a page boundary before munmaping the full map_len carried in the ctx pointer; test/js/bun/util/mmap.test.js adds an it.each over 9 offset/size cases plus a shared-write test and an offset-past-EOF test.
Security risks
None identified. Bun.mmap is already a raw-memory API gated on a real path the caller controls; this change narrows the returned view rather than widening it. size + delta cannot overflow: size ≤ stat_size - offset and delta ≤ offset, so size + delta ≤ stat_size which already fit in usize.
Level of scrutiny
High. This is unsafe native code (raw mmap/munmap, from_raw_parts_mut, pointer .add()) with a JSC finalizer that now does arithmetic to recover a base pointer it wasn't directly given. REVIEW.md flags memory safety as the most-blocked category; a wrong munmap address is UB. The invariants relied on are solid — POSIX guarantees mmap returns a page-aligned address, delta < page_size by construction, and JSC's JSTypedArrayBytesDeallocator contract passes back the exact bytes pointer — but this is the kind of clever-but-load-bearing trick a maintainer should sign off on rather than a bot.
Other factors
- Only caller of
bun_sys::mmap_fileisBun.mmap, so the return-type change is fully contained. bun_sys::page_sizere-exportsbun_alloc::page_size, so map-time and dealloc-time agree.offset=0path (delta=0) reduces exactly to the pre-PR behavior.- The
map_len = if size == 0 { 0 } else { size + delta }branch preserves the existing EINVAL for offset ≥ EOF instead of silently returning the leading page-boundary bytes. - My prior inline nit (describe-scoped tempDir never disposed) was fixed in 25a3d9c with
afterAll; all review threads are resolved. - Test coverage is thorough (aligned/unaligned offsets, boundary at 4095/4096/4097, explicit size clamping, oversize size, shared-write position, past-EOF), and tests hardcode 4096 which is fine since Linux/macOS CI runners use 4K or 16K pages and the delta logic is page-size-agnostic — the assertions compare against file contents, not page math.
- Jarred has engaged on the PR (clippy fix request, now resolved) but hasn't reviewed the substance yet.
|
Build #75548 is failing across ~20 test files (bake, install, http, etc.) on all platforms with None of it touches this diff. The mmap changes passed every buildkite lane in #72723 and |
25a3d9c to
f86852c
Compare
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 `@test/js/bun/util/mmap.test.js`:
- Around line 123-130: Extend the “mmap offset past EOF throws EINVAL” test to
call Bun.mmap with { offset: 1, size: 0 } and assert it throws EINVAL, covering
zero-size mappings at a non-page-aligned offset while preserving the existing
assertions.
🪄 Autofix (Beta)
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: aaebeb38-7086-4874-bb73-bfde73c5e3fb
📒 Files selected for processing (3)
src/runtime/api/BunObject.rssrc/sys/lib.rstest/js/bun/util/mmap.test.js
There was a problem hiding this comment.
The fix looks correct and my earlier tempDir note is resolved, but this reworks the munmap deallocator to recover the mapping base via page-rounding of the JSC-held view pointer — memory-safety-sensitive enough that a maintainer should sign off.
What was reviewed:
- Verified
bun_sys::mmap_filehas no other callers, so the signature change to(map, delta)is contained. - Checked the deallocator invariant:
bun_sys::page_sizere-exportsbun_alloc::page_size(same cached value at map and unmap time),mmap(NULL, …)returns a page-aligned base, anddelta < page, soaddr - addr % pagerecoversmap_baseexactly. - Checked
size + deltacannot overflow (size ≤ stat_size - offsetanddelta ≤ offset), anddelta < map_lenholds whenevermmapsucceeds, so.add(delta)stays in-bounds.
Extended reasoning...
Overview
The PR fixes Bun.mmap's offset option, which previously rounded down to a page boundary and returned the view at that boundary rather than at the requested byte. The fix moves page-alignment into bun_sys::mmap_file (src/sys/lib.rs), which now returns (full_map, delta); BunObject.rs builds the Uint8Array over map_base + delta .. map_base + map_len and passes map_len as the deallocator context. The deallocator rounds the view pointer down to a page boundary to recover map_base and munmaps the full length. Three new test blocks cover read position, shared-write position, and offset-past-EOF across aligned and unaligned offsets.
Security risks
None in the auth/crypto/injection sense. The relevant risk is memory safety: the munmap deallocator now derives the mapping base by page-rounding a pointer JSC hands back, rather than receiving the base directly. The reasoning is sound (mmap with addr=NULL guarantees page-aligned base; delta < page_size; page_size() is a process-wide OnceLock), but a wrong assumption here would be a UAF or a leaked mapping.
Level of scrutiny
Higher than a mechanical fix. This is native code doing raw pointer arithmetic inside a GC-driven deallocator callback, changes a bun_sys function's return type, and is a user-visible behavioral change (returned length shrinks by delta and index 0 moves). REVIEW.md flags native memory safety as the most-blocked category; this isn't the place for bot-only approval.
Other factors
All prior review threads (CodeRabbit's it.each refactor, my tempDir cleanup note, the {offset:1, size:0} coverage ask) are resolved in the current diff. Jarred engaged on the PR (clippy) but hasn't approved. The mmap tests passed on all lanes in build #72723; #75548's failures were an unrelated main-merge regression in node:events; #75561 on the latest commit was still building when I looked. Given the maintainer is already in the loop and the change sits squarely in memory-safety territory, deferring is the right call.
Bun.mmap(path, { offset: N }) with a non-page-aligned N silently rounded
N down to the nearest page boundary and returned the mapping base. A
request for { offset: 100 } returned the file from byte 0 with length
st_size, so reads and shared-mapping writes both landed at the wrong
file position.
mmap(2) requires a page-aligned file offset, so bun_sys::mmap_file now
computes delta = offset % page_size, maps from the aligned offset with
delta extra bytes, and reports delta alongside the full mapping. The
Bun.mmap wrapper hands JSC a Uint8Array view at map_base + delta with
the requested length; the deallocator rounds the view pointer back down
to the page boundary (map_base is always page-aligned, delta < page) and
munmaps the full mapping length carried in the context pointer.
An offset at or past EOF now yields a zero-length mapping request and
throws EINVAL, matching the existing behavior for page-aligned offsets
past EOF.
dc2d9ec to
6d6510e
Compare
There was a problem hiding this comment.
LGTM — the offset-delta bookkeeping and the page-round-down recovery in munmap_dealloc check out.
Reviewed:
munmap_deallocrecoversmap_basecorrectly: kernel returns a page-aligned base foraddr=NULL,delta < page_size, andbun_sys::page_sizeis the same cachedbun_alloc::page_sizeused at map time.size + deltacannot overflow: aftermin(wanted_size),size ≤ stat_size - offsetanddelta ≤ offset, somap_len ≤ stat_size.bun_sys::mmap_file's only caller isBun.mmap; no other sites need the tuple return.- Tests don't assume a 4K page size — assertions key off
buf[offset], so macOS arm64 (16K pages) is covered.
Extended reasoning...
Overview
Fixes Bun.mmap returning a view at the page-aligned offset instead of the requested offset. bun_sys::mmap_file (src/sys/lib.rs) now computes delta = offset % page_size, maps size + delta bytes from the aligned offset, and returns (map, delta). The Bun.mmap wrapper (src/runtime/api/BunObject.rs) hands JSC a Uint8Array over map_base + delta .. map_base + map_len and the deallocator recovers the mmap base by rounding the view pointer down to a page boundary before calling munmap with the full mapping length carried in the ctx pointer. Three new test blocks in test/js/bun/util/mmap.test.js cover read position, shared-write position, and past-EOF/zero-size across aligned and unaligned offsets.
Security risks
None. Input is a local file path already accepted by the existing Bun.mmap surface; no new untrusted-length arithmetic reaches an allocation without being clamped by stat_size. The unsafe pointer arithmetic is bounded: view_ptr = base.add(delta) with delta < map_len (guaranteed since the Ok arm is only reached when map_len = size + delta > 0, i.e. size > 0), and the deallocator's addr - addr % page is exact because the kernel returns page-aligned addresses and delta < page.
Level of scrutiny
Medium-high — this is native code with unsafe pointer arithmetic in a GC-time finalizer, where an off-by-one becomes a UAF or a mapping leak. I traced each invariant: (1) bun_sys::page_size() in the deallocator is a re-export of bun_alloc::page_size() used at map time (same cached OnceLock), so no divergence; (2) make_typed_array_with_bytes_no_copy's contract passes the exact ptr to the deallocator, so view_ptr is what munmap_dealloc receives; (3) size + delta is bounded by stat_size (since size ≤ stat_size - offset after the min, and delta = offset % page ≤ offset), so no overflow; (4) mmap_file has exactly one caller, so the signature change is self-contained.
Other factors
All prior review feedback has been addressed and the threads are resolved: the it.each refactor, the afterAll cleanup for the shared tempDir, and the {offset: 1, size: 0} EINVAL case. Jarred engaged on the PR and asked only for a clippy fix (unrelated to this diff, picked up via main merge). The reported CI failures on build #75548 are an unrelated node:events regression from main. Test coverage is thorough — 9 offset/size combinations plus write-through and error-path assertions, all with gcTick() to exercise the finalizer — and the tests are page-size-agnostic (assertions key off buf[offset], not a hardcoded boundary), so they hold on 16K-page platforms too.
|
Rebased onto 20b4a0b (past the Remaining red is unrelated to this diff:
This diff only touches |
2 participants
Reproduction
Before:
After:
Cause
mmap(2)requires the file offset to be a multiple of the page size.mmap_fileinsrc/runtime/api/BunObject.rshandled this by rounding the requested offset down to a page boundary (offset -= offset % page) and passing that tobun_sys::mmap_file, but then returned the raw mapping base as theUint8Arraydata pointer. The view was never adjusted for the rounding delta, so index 0 pointed at the page boundary instead of the requested byte. Through a shared writable mapping, writes landed at the wrong file position too.Fix
bun_sys::mmap_filenow accepts the requested (unaligned) offset, computesdelta = offset % page_size, mapsview_size + deltabytes from the aligned offset, and returns(full_map, delta). TheBun.mmapwrapper hands JSC aUint8Arrayovermap_base + delta .. map_base + map_len. The deallocator recovers the mmap base by rounding the view pointer down to a page boundary (the kernel guaranteesmap_baseis page-aligned anddelta < page_size) andmunmaps the full mapping length carried in the context pointer.A non-aligned offset at or past EOF now yields a zero-length mapping request and throws
EINVAL, matching the existing behavior for page-aligned offsets past EOF. Previously it returned the bytes between the page boundary and EOF.Verification
test/js/bun/util/mmap.test.jsgains three tests covering read position, shared-write position, and offset past EOF across aligned and unaligned offsets with and withoutsize. All three fail on main and pass with this change; the existing eight mmap tests continue to pass.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/mmap.test.js