Skip to content

Fix integer overflow when reading a sliced non-regular file Blob#29293

Closed
robobun wants to merge 2 commits into
mainfrom
farm/c668299a/fix-readfile-slice-overflow
Closed

Fix integer overflow when reading a sliced non-regular file Blob#29293
robobun wants to merge 2 commits into
mainfrom
farm/c668299a/fix-readfile-slice-overflow

Conversation

@robobun

@robobun robobun commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Fuzzilli found a crash (fingerprint 851b2e15154d15ae) in ReadFile.runAsyncWithFD at this.size + 16.

Root cause

Bun.file(path).slice(1) on a file Blob with unknown size (Blob.max_size) produces a sliced Blob with size = max_size - 1. When reading a non-regular file (char device, pipe, etc.) with stat.size == 0, resolveSizeAndLastModified set this.size = this.max_length whenever max_length != Blob.max_size — intending to use the user's explicit slice length as a buffer size hint. But after .slice(1), max_length is max_size - 1 (~4.5 PB), and the subsequent this.size + 16 overflowed u52 and panicked.

Repro

await Bun.file("/dev/null").slice(1).arrayBuffer();
// panic: integer overflow (debug builds)

Fix

this.size here is only the initial buffer capacity — the read loop grows it as needed via a 64 KB stack buffer, and the actual read limit is max_length. Cap the initial capacity to @min(max_length, 4096), which matches what already happens in the unsliced case and avoids trying to pre-allocate petabytes.

Also use saturating add (+|) on the + 16 for the regular-file path as defense in depth.

Applied the same @min fix to the Windows ReadFileUV path (which the comment says to keep in sync) — there it didn't panic but returned a spurious ENOMEM.

Bun.file("/dev/null").slice(1).arrayBuffer() would panic in debug builds.
Slicing a file Blob with unknown size yields a Blob with size close to
Blob.max_size. For non-regular files with stat.size == 0, ReadFile used
this value directly as the initial buffer capacity, and `size + 16`
overflowed u52.

Cap the initial buffer size for non-regular files to 4k (same as the
unsliced case — the read loop grows the buffer via its 64KB stack
buffer as needed), and use saturating add for defense in depth.
@robobun

robobun commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:09 PM PT - Apr 13th, 2026

@autofix-ci[bot], your commit 116b50d has some failures in Build #45615 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 29293

That installs a local version of the PR into your bun-29293 executable, so you can run:

bun-29293 --bun

@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Refactored blob file reading logic to consistently cap max_length at 4096 bytes for non-regular files and updated overflow handling in buffer capacity calculation. Added POSIX test cases validating file slicing behavior on special files.

Changes

Cohort / File(s) Summary
Blob file reading logic
src/bun.js/webcore/blob/read_file.zig
Refactored ResolveSizeAndLastModified and onFileInitialStat to use @min(this.max_length, 4096) instead of conditional for zero-size non-regular files. Changed buffer capacity calculation in runAsyncWithFD from standard addition to saturating addition (+| operator).
File slicing tests
test/js/bun/util/bun-file-read.test.ts
Added POSIX-only test for Bun.file() slicing behavior on special files, verifying zero-byte reads from /dev/null and bounded reads from /dev/zero.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: addressing integer overflow in ReadFile when handling sliced non-regular file Blobs.
Description check ✅ Passed The PR description exceeds the basic template requirements with detailed root cause analysis, reproduction steps, and comprehensive fix explanation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix integer overflow reading sliced non-regular file blobs #29284 - Also fixes the same integer overflow in sliced non-regular file Blob reads in read_file.zig

🤖 Generated with Claude Code

@robobun

robobun commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #29284 — same root cause, same fix in read_file.zig. Closing in favor of that one (it was opened first and its test additionally verifies the max_length cap still works after the change).

Fuzzer fingerprint 851b2e15154d15ae is the same underlying bug as 8a544632e2452b59.

@robobun robobun closed this Apr 14, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, minimal crash fix with a good regression test.

Extended reasoning...

Overview

The PR fixes an integer overflow panic in ReadFile.runAsyncWithFD (and the Windows ReadFileUV counterpart). Two files are changed: the Zig read-file implementation and a TypeScript test file.

Root Cause & Fix Correctness

The old ternary if (this.max_length == Blob.max_size) 4096 else this.max_length was intended to use 4096 for unsliced blobs and the user's slice length for sliced blobs. But .slice(1) on an unknown-size blob yields max_length = max_size - 1 (~4.5 PB), making the else branch pass a near-maximum u52 value as the initial buffer capacity. The subsequent + 16 then overflowed. The replacement @min(this.max_length, 4096) is strictly equivalent for the unsliced case (@min(max_size, 4096) == 4096) and correctly bounds small explicit slices (e.g., .slice(0, 100)@min(100, 4096) == 100), fixing the overflow for all inputs.

Security Risks

None. This is a crash/panic fix with no auth, crypto, or data-exposure implications. The saturating add (+|) on the regular-file path is a sound defense-in-depth measure.

Level of Scrutiny

Low — the change is three deleted lines replaced by one @min call, applied identically in two symmetric locations. The logic is straightforward, the comment in the Windows path explicitly says to keep it in sync, and the PR does so. The regression test is appropriate and spawns a child process to validate both /dev/null and /dev/zero paths.

Other Factors

No CODEOWNER concerns, no design decisions, no outstanding reviewer comments. The fix is a direct response to a Fuzzilli-found crash with a clear fingerprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant