blob: clamp stat.size to max_size to avoid @intCast panic in ReadFile#29355
Conversation
resolveSizeAndLastModified computed total_size as @truncate(@as(SizeType, @intcast(@max(stat.size, 0)))). The outer @truncate never runs because @intcast to u52 panics first whenever stat.size exceeds maxInt(u52). Clamp to [0, Blob.max_size] before casting so abnormal fstat results cannot trip the safety check. Also set system_error when initCapacity fails so the error propagates to JS instead of being silently treated as an empty read, matching the ReadFileUV path.
|
Updated 7:16 PM PT - Apr 15th, 2026
❌ @robobun, your commit 31e2d63 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29355That installs a local version of the PR into your bun-29355 --bun |
|
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)
WalkthroughClamps file-size reads into [0, Blob.max_size] for both ReadFile and ReadFileUV, adds an explicit out-of-memory system error and finish path when capacity initialization fails, and introduces a skipped (Windows) test suite for Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/util/bun-file-fd-read.test.ts`:
- Line 3: Replace the import of tmpdirSync with tempDir from "harness" and
update each temp directory usage (calls referencing tmpdirSync in this file) to
create disposable temp dirs with using tempDir(...) so cleanup is automatic;
specifically, change the import to include tempDir instead of tmpdirSync and
replace all tmpdirSync(...) usages at the locations around the tests (the calls
referenced by tmpdirSync in this file) with using const dir = tempDir(...); then
use dir.path (or the same API used elsewhere) in the tests so no manual cleanup
is needed and follow the repo convention of plain using (not await using) for
Bun tests.
🪄 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: 7ad03cca-1788-4d78-a8e8-399af9cf62ce
📒 Files selected for processing (1)
test/js/bun/util/bun-file-fd-read.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix for the intCast panic and OOM propagation.
Extended reasoning...
Overview
Single-file change to src/bun.js/webcore/blob/read_file.zig. Two fixes: (1) replace a broken @truncate(@as(SizeType, @intCast(...))) with a proper @intCast(@min(@max(stat.size, 0), Blob.max_size)) clamp applied symmetrically to both ReadFile and ReadFileUV; (2) add system_error assignment when initCapacity OOMs in the POSIX path, matching ReadFileUV's existing behavior.
Security risks
None. The change is entirely in file-reading size arithmetic and error propagation — no auth, crypto, or permissions code touched.
Level of scrutiny
Low. The root cause is clearly documented (dead @truncate, live @intCast panic), the fix is minimal and symmetric, and three relevant test files pass. The OOM nit (missing .path field) is a minor inconsistency with the EISDIR pattern but is consistent with the existing ReadFileUV NOMEM cases and far better than the prior silent-empty-read behavior.
Other factors
No CODEOWNERS concern, no large design decisions, no behavior change for the normal code path. The fix is structurally verified via objdump and covered by existing tests.
| if (!this.could_block or (this.size > 0 and this.size != Blob.max_size)) | ||
| this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, this.size +| 16) catch |err| { | ||
| this.errno = err; | ||
| this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError(); | ||
| this.onFinish(); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🟡 The new OOM system_error at line 386 is created via bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError(), which leaves the .path field empty. By contrast, the EISDIR error in the same function (resolveSizeAndLastModified) explicitly populates .path from this.file_store.pathlike. When initCapacity fails on a named file (e.g., Bun.file("/some/huge/file").text()), the resulting JS error will have no .path property, making it slightly harder to identify which file triggered the OOM. This is a minor inconsistency — adding path enrichment matching the EISDIR pattern would make OOM errors more debuggable.
Extended reasoning...
What the bug is and how it manifests
When initCapacity fails in ReadFile.runAsyncWithFD (line 383-389), the PR added a new system_error assignment:
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();bun.sys.Error.fromCode constructs a minimal error with only errno and syscall fields; it does not set .path. So the resulting JS Error object thrown to user code will have err.path === undefined or empty, even when reading a named file.
The specific code path that triggers it
- User calls
Bun.file("/some/large/file").text() ReadFile.runAsyncWithFDcallsresolveSizeAndLastModified, which setsthis.sizeinitCapacity(bun.default_allocator, this.size +| 16)returns an OOM error- The catch block sets
this.system_errorwithout a.pathfield then()propagates thesystem_errorto the JS promise rejection — no path info
Why existing code doesn't prevent it
The doRead function (lines 218-220) does enrich system_error.path when it is empty for read-syscall errors, but that enrichment only runs inside doRead, not the initCapacity catch block. The EISDIR branch in resolveSizeAndLastModified explicitly sets .path, showing the pattern exists and works — it just wasn't applied here.
Addressing the refutation
One verifier argued this is intentional because all NOMEM errors in ReadFileUV also omit path. That's accurate — the pattern is consistent between ReadFile and ReadFileUV for OOM errors. However, "consistent with ReadFileUV" only means the omission is uniform, not that it's desirable. The EISDIR case (also present in both ReadFile and ReadFileUV) demonstrates that path enrichment is done and valued; there is no principled reason to treat NOMEM differently.
Impact
OOM errors are rare, and the PR already improves things significantly (previously OOM silently yielded an empty read with no error at all). Missing path makes OOM slightly harder to diagnose in production but is not a correctness issue.
Fix
After setting this.system_error, add path enrichment matching the EISDIR pattern:
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.system_error.?.path = if (this.file_store.pathlike == .path)
bun.String.cloneUTF8(this.file_store.pathlike.path.slice())
else
bun.String.empty;Step-by-step proof
Bun.file("/huge/file").text()→ReadFile.create→runAsyncWithFDresolveSizeAndLastModifiedsucceeds;this.size= e.g. 2 GBinitCapacity(allocator, 2GB + 16)→error.OutOfMemory- Catch:
this.system_error = bun.sys.Error.fromCode(.NOMEM, .read).toSystemError()→{ code: "ENOMEM", syscall: "read", path: "" } onFinish()→then()→cb(cb_ctx, .{ .err = system_error })- JS sees
Error { code: "ENOMEM", syscall: "read" }— no.pathto identify the file
There was a problem hiding this comment.
Keeping this minimal and consistent with the existing ReadFileUV NOMEM pattern (lines 690, 697). The primary goal is propagating the error at all (previously it was a silent empty read); path enrichment for OOM can be a follow-up across both paths if needed.
There was a problem hiding this comment.
LGTM — the clamping fix is correct, applied to both POSIX and Windows paths, and the OOM improvement is a net win even without path enrichment.
Extended reasoning...
Overview
The PR modifies (two one-line stat.size clamping changes + one OOM system_error assignment) and adds (new test file for fd-backed reads). The change is small and self-contained.
Fix correctness
The old expression contained a dead — the inner to u52 (SizeType) runs first and panics on any stat.size > maxInt(u52). The replacement clamps first so the final is always in range. Applied consistently to both (POSIX) and (Windows). The logic is correct.
Security risks
None. This is purely a numeric bounds check; no auth, crypto, or permission-related code is touched.
Level of scrutiny
Low. The diff is three lines of production code and a new test file. The root cause is clearly documented (objdump-verified), the fix is mechanical, and the author verified the fix with objdump confirming zero call sites post-fix. The OOM improvement (propagating error vs. silent empty read) is a strict improvement with no behavioural regressions on the normal path.
Other factors
- coderabbitai's → feedback was addressed in commit fed93fe before this review.
- My earlier inline comment about missing on the OOM system_error was acknowledged; the author's rationale (stay consistent with ReadFileUV, OOM is already rare, this is a minimal fix) is sound.
- The bug report about using is a pre-existing issue on untouched code that only affects files >4.5 PB (effectively theoretical). It does not block this PR.
| } | ||
|
|
||
| this.could_block = !bun.isRegularFile(stat.mode); | ||
| this.total_size = @truncate(@as(SizeType, @intCast(@max(@as(i64, @intCast(stat.size)), 0)))); | ||
| this.total_size = @intCast(@min(@max(stat.size, 0), Blob.max_size)); | ||
|
|
There was a problem hiding this comment.
🟣 The PR fixes the @truncate/@intcast panic in read_file.zig by clamping stat.size to Blob.max_size, but the analogous resolveFileStat function in Blob.zig (lines ~3298 and 3312) still uses the old @truncate pattern — for a file with stat.size just above 2^52 (e.g. 2^52+1), @truncate silently discards the high bits, yielding max_size=1 instead of Blob.max_size. This is a pre-existing issue in adjacent code that the PR did not touch, but it represents the same class of silent size-corruption bug the PR was explicitly written to address.
Extended reasoning...
What the bug is and how it manifests
In Blob.zig, the resolveFileStat function sets store.data.file.max_size via two expressions that use @truncate to convert a u64 stat.size to SizeType (u52):
Line 3298 (path-based): store.data.file.max_size = @truncate(@as(u64, @intCast(@max(stat.size, 0))))
Line 3312 (fd-based): store.data.file.max_size = @as(SizeType, @truncate(@as(u64, @intCast(@max(stat.size, 0)))))
Unlike the @intcast in read_file.zig (which panics on overflow), @truncate silently drops the high 12 bits when converting u64 to u52. For any stat.size greater than maxInt(u52) = 4503599627370496 (~4.5 PB), the resulting value is not Blob.max_size but rather the low 52 bits of the actual size.
The specific code path that triggers it
A file with stat.size = 2^52 + 1 (i.e., 4503599627370497) would produce @truncate result of 1 (the low 52 bits), setting store.data.file.max_size = 1. Downstream code in resolveSize checks max_size != Blob.max_size and uses it as the store_size, so allocation and routing decisions would use a 1-byte max_size for a multi-petabyte file.
Why existing code doesn't prevent it
@truncate is intentionally not a safety check in Zig — it compiles in release and debug modes without panicking. The only guard is that the high bits are dropped. There is no clamping logic like the PR's @intcast(@min(@max(stat.size, 0), Blob.max_size)) pattern.
Addressing the refutation
One verifier correctly noted that Blob.zig is not modified by this PR, read_file.zig does not call resolveFileStat, and the two code paths share no callers. This is accurate — the bug is genuinely pre-existing and the PR does not increase its surface area. However, the PR's stated purpose is specifically to fix this class of issue (stat.size clamping to Blob.max_size), and the PR author clearly recognized the pattern and applied the fix in read_file.zig. The same fix pattern should be applied to resolveFileStat for completeness. The pre_existing severity designation is appropriate.
What the impact would be
Any file whose stat.size is exactly above a 2^52 multiple (e.g. 2^52+1, 2^53+1, etc.) would get an incorrect small max_size. Files above 4.5 PB are essentially nonexistent on real hardware, making this a theoretical issue for the foreseeable future. But as filesystems and storage technology evolve, and especially in virtualized/mocked environments, this silent truncation is a latent data-corruption bug.
How to fix it
Apply the same clamping pattern used in the PR fix:
store.data.file.max_size = @intCast(@min(@max(stat.size, 0), Blob.max_size));Step-by-step proof
- File has stat.size = 4503599627370497 (= 2^52 + 1)
- resolveFileStat is called (path or fd variant)
- @max(stat.size, 0) = 4503599627370497 (u64)
- @truncate(u64 → u52): 4503599627370497 mod 2^52 = 1
- store.data.file.max_size = 1 (should be Blob.max_size = 4503599627370496)
- resolveSize uses max_size=1, allocates 1-byte buffer for a >4.5 PB file
- ReadFile.create gets max_len=1, reads only 1 byte
There was a problem hiding this comment.
Agreed that resolveFileStat in Blob.zig has a similar shape, but it uses @truncate (silent wrap, no panic) rather than @intCast, so it's not part of this crash. Keeping this PR scoped to the fuzzer-found panic in ReadFile; the Blob.zig clamping can be a separate follow-up if desired.
There was a problem hiding this comment.
LGTM — targeted fuzzer fix with correct clamping on both POSIX and Windows paths; the describe.concurrent nit in the test is minor and doesn't affect correctness.
Extended reasoning...
Overview
This PR modifies two files: src/bun.js/webcore/blob/read_file.zig (core fix) and test/js/bun/util/bun-file-fd-read.test.ts (new test coverage). The change is small and scoped: replace @truncate(@as(SizeType, @intCast(...))) with @intCast(@min(@max(stat.size, 0), Blob.max_size)) in both ReadFile.resolveSizeAndLastModified and ReadFileUV.onFileInitialStat, and add system_error propagation when initCapacity fails in the POSIX path.
Security risks
None. This is a purely defensive fix — clamping an integer before a cast to avoid a panic. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low. The fix is mechanically correct: @min(@max(stat.size, 0), Blob.max_size) guarantees the value is in [0, Blob.max_size] before @intCast to u52, which is exactly the invariant needed. The OOM system_error addition is consistent with the ReadFileUV pattern. The test file already uses tempDir with using (addressed in fed93fe). The only remaining nit from the bug hunter is missing describe.concurrent on the test describe block — a convention issue per test/CLAUDE.md but not a correctness problem.
Other factors
Previous inline comments about OOM path enrichment and the analogous Blob.zig @truncate pattern were addressed by the author with sound reasoning (keep scope minimal, consistent with ReadFileUV). No outstanding unresolved concerns.
| // so an abnormal fstat size could trip integerOutOfBounds. Triggering that | ||
| // directly requires fstat to report > 4.5 PB which is not achievable here, | ||
| // but these tests lock in the fd-backed ReadFile path that the fuzzer hit. | ||
| describe.skipIf(isWindows)("Bun.file(fd) read", () => { |
There was a problem hiding this comment.
🟡 The three tests in bun-file-fd-read.test.ts use describe.skipIf(isWindows) instead of describe.concurrent.skipIf(isWindows). Per test/CLAUDE.md, tests that write files should use describe.concurrent unless difficult to make concurrent; since each test creates its own independent tempDir and file descriptors, the fix is a one-word change: describe.concurrent.skipIf(isWindows).
Extended reasoning...
What the issue is
The three tests in bun-file-fd-read.test.ts are wrapped in describe.skipIf(isWindows) rather than describe.concurrent.skipIf(isWindows). The test/CLAUDE.md guideline at line 22 explicitly states: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it is very difficult to make them concurrent." All three tests write files (via tempDir), triggering this rule exactly.
The specific code
Line 13 of bun-file-fd-read.test.ts reads:
describe.skipIf(isWindows)("Bun.file(fd) read", () => {It should be:
describe.concurrent.skipIf(isWindows)("Bun.file(fd) read", () => {Why the fix is trivial and correct
Each test creates its own tempDir with unique file names (fd-read.txt, fd-slice.txt, fd-empty.txt). Each uses its own withFd calls to open/close separate fds. There is zero shared state between tests — no shared directory, no shared fd, no shared variables. The prerequisite for using describe.concurrent is fully satisfied.
Addressing the refutation
The refuter worried that describe.skipIf(isWindows).concurrent() may not be supported syntax. That is correct — but it is the wrong chain order. The supported and already-used-in-this-codebase pattern is describe.concurrent.skipIf(isWindows)(...) (accessing .concurrent first, then .skipIf). This exact pattern is confirmed by test/js/bun/util/symlink-path-traversal.test.ts and test/js/bun/bun-serve-file.test.ts in the codebase. The refutation is based on a misread of the API surface.
Step-by-step proof
test/CLAUDE.mdline 22: tests writing files must usedescribe.concurrentunless difficult.- All three tests call
tempDir(...)which writes files to disk. - Each test has its own
tempDirinstance → no shared state → trivially concurrent. - The fix is exactly one word: change
describe.skipIftodescribe.concurrent.skipIf. - Other test files (e.g.,
symlink-path-traversal.test.ts) already use this exact pattern successfully.
|
CI failures in build 45853 are all pre-existing main issues unrelated to this change:
None reference |
…oven-sh#29355) ## What does this PR do? Fixes a Zig safety-check panic in `ReadFile.resolveSizeAndLastModified` (called from `runAsyncWithFD`) when `fstat` reports a size larger than `maxInt(u52)`. Fuzzer fingerprint: `6e3d2159cadcef3a` ## Root cause ```zig this.total_size = @truncate(@as(SizeType, @intcast(@max(@as(i64, @intcast(stat.size)), 0)))); ``` `SizeType` is `u52`. The outer `@truncate` is dead code — the inner `@intCast` to `u52` runs first and panics with `integerOutOfBounds` whenever the (non-negative) stat size exceeds `maxInt(u52)`. Verified with objdump that `resolveSizeAndLastModified` contained a single call to `debug.FullPanic.integerOutOfBounds` mapping to this line. The fuzzer hit this via an fd-based `ReadFile` task scheduled on the thread pool in a prior REPRL iteration, which then ran after the fd context changed. In a standalone run of the minimized script, `doReadFile` is never invoked — the crash depends on cross-iteration thread-pool state, which is why it's extremely flaky and not directly reproducible outside the REPRL harness. ## Fix - Clamp `stat.size` to `[0, Blob.max_size]` before casting, so the `@intCast` is always in range. Applied to both the POSIX (`ReadFile`) and Windows (`ReadFileUV`) paths. - While here: set `system_error` when `initCapacity` fails in the POSIX path so OOM propagates to JS as an error instead of being silently treated as an empty read. This matches what `ReadFileUV` already does. ## How did you verify your code works? - `objdump` confirms `integerOutOfBounds` is no longer emitted in `resolveSizeAndLastModified` (1 → 0 call sites). - `bun bd test test/js/bun/util/bun-stdin-slice.test.ts` passes (covers fd-based `ReadFile` path). - `bun bd test test/js/bun/util/bun-file-read.test.ts` passes. - `bun bd test test/js/bun/util/bun-file.test.ts` passes. - Manual check: `Bun.file(fd).text()` on a regular file fd still works. No new regression test is added because triggering the original panic requires `fstat` to report a size > 4.5 PB, which is not achievable in the test environment; the fix is verified structurally and the affected code path is already covered by the existing stdin-slice tests. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
…oven-sh#29355) ## What does this PR do? Fixes a Zig safety-check panic in `ReadFile.resolveSizeAndLastModified` (called from `runAsyncWithFD`) when `fstat` reports a size larger than `maxInt(u52)`. Fuzzer fingerprint: `6e3d2159cadcef3a` ## Root cause ```zig this.total_size = @truncate(@as(SizeType, @intcast(@max(@as(i64, @intcast(stat.size)), 0)))); ``` `SizeType` is `u52`. The outer `@truncate` is dead code — the inner `@intCast` to `u52` runs first and panics with `integerOutOfBounds` whenever the (non-negative) stat size exceeds `maxInt(u52)`. Verified with objdump that `resolveSizeAndLastModified` contained a single call to `debug.FullPanic.integerOutOfBounds` mapping to this line. The fuzzer hit this via an fd-based `ReadFile` task scheduled on the thread pool in a prior REPRL iteration, which then ran after the fd context changed. In a standalone run of the minimized script, `doReadFile` is never invoked — the crash depends on cross-iteration thread-pool state, which is why it's extremely flaky and not directly reproducible outside the REPRL harness. ## Fix - Clamp `stat.size` to `[0, Blob.max_size]` before casting, so the `@intCast` is always in range. Applied to both the POSIX (`ReadFile`) and Windows (`ReadFileUV`) paths. - While here: set `system_error` when `initCapacity` fails in the POSIX path so OOM propagates to JS as an error instead of being silently treated as an empty read. This matches what `ReadFileUV` already does. ## How did you verify your code works? - `objdump` confirms `integerOutOfBounds` is no longer emitted in `resolveSizeAndLastModified` (1 → 0 call sites). - `bun bd test test/js/bun/util/bun-stdin-slice.test.ts` passes (covers fd-based `ReadFile` path). - `bun bd test test/js/bun/util/bun-file-read.test.ts` passes. - `bun bd test test/js/bun/util/bun-file.test.ts` passes. - Manual check: `Bun.file(fd).text()` on a regular file fd still works. No new regression test is added because triggering the original panic requires `fstat` to report a size > 4.5 PB, which is not achievable in the test environment; the fix is verified structurally and the affected code path is already covered by the existing stdin-slice tests. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
What does this PR do?
Fixes a Zig safety-check panic in
ReadFile.resolveSizeAndLastModified(called fromrunAsyncWithFD) whenfstatreports a size larger thanmaxInt(u52).Fuzzer fingerprint:
6e3d2159cadcef3aRoot cause
SizeTypeisu52. The outer@truncateis dead code — the inner@intCasttou52runs first and panics withintegerOutOfBoundswhenever the (non-negative) stat size exceedsmaxInt(u52). Verified with objdump thatresolveSizeAndLastModifiedcontained a single call todebug.FullPanic.integerOutOfBoundsmapping to this line.The fuzzer hit this via an fd-based
ReadFiletask scheduled on the thread pool in a prior REPRL iteration, which then ran after the fd context changed. In a standalone run of the minimized script,doReadFileis never invoked — the crash depends on cross-iteration thread-pool state, which is why it's extremely flaky and not directly reproducible outside the REPRL harness.Fix
stat.sizeto[0, Blob.max_size]before casting, so the@intCastis always in range. Applied to both the POSIX (ReadFile) and Windows (ReadFileUV) paths.system_errorwheninitCapacityfails in the POSIX path so OOM propagates to JS as an error instead of being silently treated as an empty read. This matches whatReadFileUValready does.How did you verify your code works?
objdumpconfirmsintegerOutOfBoundsis no longer emitted inresolveSizeAndLastModified(1 → 0 call sites).bun bd test test/js/bun/util/bun-stdin-slice.test.tspasses (covers fd-basedReadFilepath).bun bd test test/js/bun/util/bun-file-read.test.tspasses.bun bd test test/js/bun/util/bun-file.test.tspasses.Bun.file(fd).text()on a regular file fd still works.No new regression test is added because triggering the original panic requires
fstatto report a size > 4.5 PB, which is not achievable in the test environment; the fix is verified structurally and the affected code path is already covered by the existing stdin-slice tests.