Skip to content

Fix backing-file offset truncation at 4 GiB - #129

Merged
bpowers merged 1 commit into
plasma-umass:masterfrom
jkookhan03:fix/backing-offset-width
Oct 21, 2026
Merged

bpowers merged 1 commit into
plasma-umass:masterfrom
jkookhan03:fix/backing-offset-width

Conversation

@jkookhan03

Copy link
Copy Markdown
Contributor

Mesh stores page offsets in a 32-bit Offset. The remapping paths shift that value before converting it to off_t, so page offset 0x100000 with 4 KiB pages becomes byte offset 0 instead of 0x100000000. An mmap call can therefore succeed while referring to the wrong part of the arena backing file.

Widen both operands to off_t before multiplication and use the shared conversion in span reset, mesh finalization, and child-after-fork remapping. A compile-time check verifies that off_t can represent the arena's backing-file offsets. The stored offset type and arena capacity stay the same.

Add a small regression to the existing unit-test target, checking immediately below, at, and above 4 GiB for both 4 KiB and 16 KiB pages. Restoring the original shift produces four expected assertion failures; the fixed conversion passes all six checks without a large allocation.

Local validation on Linux x86_64 with Bazel 7.4.1, based on public upstream master 2987f883b8695c07a80dc40da67c61f524460dfd:

  • Focused regression: passes in debug and optimized builds.
  • Full upstream //src:unit-tests target: in this environment, unmodified upstream passes 49/50 tests and this patch passes 50/51, in both modes. Both fail only the existing public ForkTest.RuntimePidInitialized test (runtime PID is 0). The additional test is the new offset regression above.
  • Optimized //src:libmesh.so: builds successfully.
  • git diff --check: passes.

The 16 KiB checks exercise the arithmetic on Linux; macOS runtime execution was not tested.

Convert page offsets to off_t before multiplying by the page size in span
reset, mesh finalization, and child-after-fork remapping. Verify that off_t
can represent the arena offsets at compile time.

Add boundary regression checks for both 4 KiB and 16 KiB pages.
@emeryberger

Copy link
Copy Markdown
Member

@bpowers, would you take a look? Audit summary:

The bug is real and corrupts data. Offset is uint32_t, so offset << kPageShift wraps once a span starts more than 4 GiB into the arena (64 GB on Linux, 32 GB on macOS). mmap still succeeds, but it maps the wrong part of the backing file:

  • In finalizeMesh (and the remap in afterForkChild), the removed span ends up mapped onto some unrelated file page, so its objects read back wrong.
  • In resetSpanMapping, the reset span ends up aliasing another span's physical memory.

#131 adds an end-to-end test that meshes and scavenges spans above 4 GiB. On current master the meshed object reads back as an empty string instead of its contents. With this PR it passes.

The fix is correct and complete. I checked the other places that turn page offsets into byte offsets:

  • copyFile in afterForkChild uses a size_t index.
  • freePhys gets its off_t from pointer subtraction.
  • mini_heap.h already casts to size_t.
  • offsetFor returns 32 bits, which is enough: the arena holds at most 16M pages.

The only other unwidened shift is in a kAdviseDump path that is compiled out; #131 fixes it anyway.

CI (run 39353503541): passes on Ubuntu (CMake and Bazel) and macOS (Bazel). The new PageOffsetToFileOffsetPreservesFourGiBBoundary test runs and passes in every job, including macOS with 16K pages, which the description notes was untested.

I'd merge this as is. #131 stacks the end-to-end test and a small finalizeMesh cleanup on top.

@bpowers

bpowers commented Oct 21, 2026

Copy link
Copy Markdown
Member

thanks! this LGTM - I was initially concerned that moving from bit shifting to a multiply would slow things down, but codegen is the same

@bpowers
bpowers merged commit 1f29838 into plasma-umass:master Oct 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants