fix integer overflow in ReadFile buffer capacity computation by robobun · Pull Request #28849 · oven-sh/bun

robobun · 2026-04-04T08:33:48Z

this.size is a u52 (Blob.SizeType) bounded to Blob.max_size (2^52 - 1). Adding 16 to a value close to max overflows.

Use saturating addition (+|); the allocator will reject the oversized allocation and the error is propagated through the existing catch.

Fixes fuzzer crash:

panic: integer overflow
bun.js.webcore.blob.read_file.ReadFile.runAsyncWithFD
/workspace/bun/src/bun.js/webcore/blob/read_file.zig:387:100

robobun · 2026-04-04T08:33:59Z

^{Updated 7:33 AM PT - Apr 4th, 2026}

❌ @robobun, your commit ba08550 has 3 failures in Build #43677 (All Failures):

test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 13 aarch64
test/js/bun/webview/webview.test.ts - timeout on 🍎 14 x64
test/js/bun/webview/webview.test.ts - timeout on 🍎 14 aarch64
test/js/bun/webview/webview.test.ts - timeout on 🍎 13 x64
test/js/bun/webview/webview.test.ts - timeout on 🍎 13 aarch64
test/js/third_party/@azure/service-bus/azure-service-bus.test.ts - SIGILL on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 28849

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

bun-28849 --bun

coderabbitai · 2026-04-04T08:38:21Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

Added an early synthetic-allocation limit guard in the blob file-read path that returns ENOMEM without allocating, replaced buffer-capacity addition with saturating add, and added a POSIX-gated regression test that forces the synthetic limit and asserts slice(1).text() rejects with { code: "ENOMEM" }.

Changes

Cohort / File(s)	Summary
Blob read implementation `src/bun.js/webcore/blob/read_file.zig`	Added an early pre-allocation guard: if `this.size` exceeds `jsc.VirtualMachine.synthetic_allocation_limit` the code sets `this.errno = E.NOMEM`, populates `this.system_error` (syscall `.read`), calls `onFinish()`, and returns. Changed capacity allocation from `this.size + 16` to saturating add `this.size +
Regression test `test/js/bun/util/bun-file-read.test.ts`	Added a POSIX-gated (skipped on Windows) test that uses `setSyntheticAllocationLimitForTesting` (restored in `finally`), opens `Bun.file("/dev/zero")`, slices with `slice(1)`, and asserts `sliced.text()` rejects with an error containing `{ code: "ENOMEM" }`.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing an integer overflow issue in ReadFile's buffer capacity computation using saturating addition.
Description check	✅ Passed	The description addresses the required sections: it explains what the PR does (fixes integer overflow using saturating addition) and references the fuzzer crash it resolves. However, it lacks explicit verification details.

_{✏️ 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.}

claude

LGTM — the one-character saturating-addition fix is correct and minimal; the test weakness is captured in the inline comment.

Extended reasoning...

Overview

The PR modifies two files: a one-character change in src/bun.js/webcore/blob/read_file.zig (replacing + with +| for saturating addition) and a new regression test in test/js/bun/util/bun-file-read.test.ts.

Security risks

None. The change prevents an integer overflow that could cause a panic; it does not touch authentication, crypto, permissions, or any data-exposure surface.

Level of scrutiny

Low. This is a one-character, well-understood Zig primitive change. Saturating addition +| is the canonical fix for this class of overflow: when this.size is near Blob.max_size (2^52-1), the result saturates at max rather than wrapping, and the allocator then rejects the oversized request through the already-present catch handler. The fix is correct, self-contained, and does not change any control-flow paths for the non-overflow case.

Other factors

The regression test assertion (expect(results.length).toBe(2)) is trivially always true and provides no regression safety, as noted in the inline bug comment. That is a nit and does not affect correctness of the fix itself.

claude

LGTM — all three previously flagged issues are addressed: saturating add prevents the overflow, the catch block now sets system_error so OOM propagates as a rejection, and the test uses /dev/zero.slice(1) with a proper ENOMEM assertion.

Extended reasoning...

Overview

The PR fixes an integer overflow in ReadFile.runAsyncWithFD (read_file.zig:387) where this.size + 16 could wrap on a u52 value near Blob.max_size (2^52 - 1). Two changes land in the Zig file and a regression test is added.

Changes in the current revision

this.size +| 16: saturating addition prevents the overflow panic — correct and minimal.
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError() added to the initCapacity catch block — this mirrors the Windows ReadFileUV path and ensures OOM propagates as a proper rejection rather than silently resolving to "".
Regression test uses Bun.file("/dev/zero").slice(1) which produces max_length = Blob.max_size - 1, triggering the could_block path and the ~4 PB allocation that correctly fails with ENOMEM. The assertion rejects.toMatchObject({ code: "ENOMEM" }) is meaningful and would fail if the fix were reverted.

Security risks

None — this is a purely defensive fix preventing a crash in the file-reading path. No auth, crypto, or permission code is touched.

Level of scrutiny

Low. The changes are minimal, self-contained, and follow established patterns in the codebase (the Windows path already does the same thing). The test is gated to non-Windows and exercises a real code path.

Other factors

All three issues flagged in my prior inline comments have been resolved in the updated revision: (1) trivially-true assertion removed, (2) fd 9999 replaced with /dev/zero to actually reach the fixed line, (3) missing system_error assignment added to the catch block. No outstanding reviewer concerns remain.

claude

LGTM — both the saturating-add overflow fix and the missing system_error assignment are in place.

Extended reasoning...

Overview

Single-file change to src/bun.js/webcore/blob/read_file.zig. Two lines added to runAsyncWithFD: (1) this.size + 16 → this.size +| 16 (Zig saturating addition), and (2) this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError() in the initCapacity catch block.

Security risks

None. This is a crash-fix in a file-read buffer allocation path. It does not touch auth, crypto, or permissions.

Level of scrutiny

Low. The change is a mechanical 2-line patch in a well-understood code path. The saturating-add operator is the canonical Zig idiom for avoiding integer overflow in arithmetic that feeds an allocator. The system_error addition mirrors exactly the pattern already used in ReadFileUV on Windows (lines 702-703), making it consistent across platforms. Both lines are independently verifiable by reading the surrounding code.

Other factors

My earlier reviews flagged (a) a trivially-true test assertion, (b) a test that didn't actually exercise the fixed code path, and (c) the missing system_error in the catch block. All three inline comments are now marked resolved. The current diff addresses concern (c) directly; concerns (a) and (b) are moot because no test file is included in the final PR. No bugs were found by the current bug-hunting run.

claude

LGTM — all three issues I previously flagged have been addressed in the updated diff.

Extended reasoning...

Overview

This PR fixes an integer overflow in ReadFile.runAsyncWithFD where this.size + 16 could overflow for u52 values near Blob.max_size (2^52 - 1). Two files are touched: the Zig implementation and a TypeScript regression test.

What Changed Since My Previous Reviews

All three substantive issues I raised in prior review passes have been resolved:

Trivially-true assertion (results.length === 2) — replaced with a meaningful rejects.toMatchObject({ code: "ENOMEM" }).
Test not reaching the overflow path (fd 9999 → fstat EBADF short-circuits before line 397) — replaced with /dev/zero + setSyntheticAllocationLimitForTesting(4MB), which correctly exercises the guard path.
Silent OOM swallowing (catch block set only this.errno, not this.system_error, so then() resolved to empty string) — the catch block now sets both fields, matching the Windows ReadFileUV path.

Security Risks

None. This is a bug fix in local file I/O error handling. No auth, crypto, or permission-sensitive code is involved.

Level of Scrutiny

Moderate. The Zig changes are localized to the ReadFile struct's runAsyncWithFD method. The fix is layered (early guard + saturating add + proper error propagation), all three layers are correct, and the pattern matches the existing Windows codepath. The remaining inline nit (test comment not precisely describing which path is exercised) is accurately noted and is a documentation issue only — it does not affect correctness.

Other Factors

The remaining bug report is a nit: the test exercises the synthetic-allocation-limit guard path, not the +| saturating-add path directly (which is physically impossible to unit-test since it requires a ~4.5 PB allocation). The +| is confirmed defense-in-depth verified by the fuzzer. The production code is correct and safe.

claude

LGTM — previous feedback has been addressed: the test now properly asserts ENOMEM rejection via /dev/zero, the catch block sets both errno and system_error, and the saturating add provides correct defense-in-depth.

Extended reasoning...

Overview

The PR fixes an integer overflow in ReadFile.runAsyncWithFD (src/bun.js/webcore/blob/read_file.zig) where this.size + 16 could overflow for u52 values near Blob.max_size (2^52 - 1). Two complementary fixes are applied: (1) an early guard that rejects reads exceeding synthetic_allocation_limit before reaching the allocator, and (2) saturating addition +| as defense-in-depth. The catch block for initCapacity failure is also fixed to set system_error (not just errno), matching the Windows ReadFileUV path. A POSIX-gated regression test is added.

Security Risks

No security risks. This is a memory-safety fix that converts a panic/wrap into a clean ENOMEM rejection. The change makes the error path more correct, not less.

Level of Scrutiny

This is a targeted bug fix in a non-security-critical path (file reading). The core production code is straightforward and the changes are minimal. The test now meaningfully validates behavior rather than checking a structural property of Promise.allSettled.

Other Factors

All inline comments from prior review rounds have been marked resolved. The remaining observation (that the test exercises the guard path rather than the +| path directly) was acknowledged in the 4th comment — testing the +| path directly is impractical since it would require a blob of size near 2^52, which cannot be constructed from any real file. The test comment accurately describes both mechanisms without overclaiming. No outstanding reviewer concerns remain.

claude

LGTM — all previously raised issues have been addressed.

Extended reasoning...

Overview

This PR fixes an integer overflow in ReadFile.runAsyncWithFD where this.size + 16 could overflow since this.size is a u52 bounded to Blob.max_size (2^52 - 1). The fix is two-pronged: an early guard that rejects sizes exceeding synthetic_allocation_limit with ENOMEM, and a saturating addition (+|) as defense-in-depth. The PR also fixes a pre-existing bug where the initCapacity catch block set this.errno but not this.system_error, causing silent OOM swallowing.

Security risks

No security-sensitive code is touched. This is a crash/panic fix in an async file-read path. The change makes error handling more robust (OOM properly propagates as a rejection rather than silently resolving to empty string).

Level of scrutiny

Low-to-medium. The Zig changes are small and mechanical, touching only the buffer-capacity logic in one function. The change mirrors the pattern already used in the Windows ReadFileUV path. The test uses an internal API (setSyntheticAllocationLimitForTesting) correctly and is properly gated to POSIX.

Other factors

Three rounds of prior review feedback (trivially true assertion, wrong fd causing early exit, missing system_error in catch block) have all been addressed in the current revision. The remaining unresolved inline comment from the last review was advisory only — noting that the +| path cannot be directly unit-tested since it requires a blob of size ~2^52, which is acknowledged as defense-in-depth verified by the fuzzer.

claude

LGTM — the overflow fix, OOM propagation, and regression test all look correct.

Extended reasoning...

Overview

The PR modifies src/bun.js/webcore/blob/read_file.zig and adds a regression test in test/js/bun/util/bun-file-read.test.ts. The fix addresses a fuzzer-found integer overflow in ReadFile.runAsyncWithFD where this.size + 16 could overflow since this.size is a u52 bounded to Blob.max_size (2^52 - 1).

Security risks

No security-sensitive code (auth, crypto, permissions) is touched. The fix reduces attack surface by converting a panic/undefined-behavior crash into a clean ENOMEM rejection.

Level of scrutiny

Low — this is a targeted, minimal bug fix with a clear root cause, a straightforward fix (guard + saturating add + error propagation), and a meaningful regression test. All three defensive layers (guard, +|, catch with system_error) are correct and consistent with the Windows ReadFileUV path.

Other factors

Prior review iterations raised concerns about the test (trivially-true assertion, EBADF short-circuit, missing system_error in catch). The current version addresses all of these: the test uses /dev/zero + setSyntheticAllocationLimitForTesting + a proper rejects.toMatchObject({ code: "ENOMEM" }) assertion, and the catch block now sets both this.errno and this.system_error.

this.size is a u52 (Blob.SizeType) bounded to Blob.max_size (2^52 - 1). Adding 16 to a value close to max could overflow. Use saturating addition and set system_error when the allocator rejects the oversized allocation so the promise rejects with ENOMEM instead of silently resolving to an empty string.

claude

LGTM — the core fixes are correct: saturating addition prevents the overflow panic, and system_error is now properly set in the catch block so OOM rejects the promise. The two remaining nit comments (weak assertion and timeout convention) are minor.

Extended reasoning...

Overview\nThis PR modifies two files: `src/bun.js/webcore/blob/read_file.zig` (one-line arithmetic fix + one-line system_error assignment) and a regression test in `test/js/bun/util/bun-file-read.test.ts`.\n\n### Security risks\nNone. This is a memory-safety fix that prevents a Zig integer overflow panic when a blob whose size approaches `Blob.max_size` (2^52 - 1) is read. There are no auth, crypto, or permission changes.\n\n### Level of scrutiny\nLow-to-medium. The arithmetic fix (`+` → `+|`) and the `system_error` addition are each single lines with clear, targeted intent. The test correctly exercises both changes: `Bun.file("/dev/zero").slice(1)` gives a blob with `max_length ≈ 2^52 - 2`, which flows through `resolveSizeAndLastModified` to `this.size ≈ 2^52 - 2`, hits the `initCapacity` call with a saturated ~4.5 PB request, triggers the allocator OOM, and the now-correct catch block propagates rejection via `system_error`.\n\n### Other factors\nMy prior reviews (08:46, 09:02, 09:31, 10:52, 12:35) raised progressively more targeted concerns. The current diff addresses the two substantive issues: test now targets the correct code path (/dev/zero + slice(1) instead of invalid fd 9999), and `system_error` is now set in the catch block. Two nit-level inline comments have been posted by the bug-hunting system (weak assertion and 5000 ms timeout violating CLAUDE.md), which are genuine minor style issues but do not affect the correctness of the regression coverage.

claude

LGTM — the saturating add, OOM guard, and system_error propagation are all correct, and the test properly asserts ENOMEM rejection.

Extended reasoning...

Overview

This PR touches src/bun.js/webcore/blob/read_file.zig and test/js/bun/util/bun-file-read.test.ts. The changes are: (1) replace this.size + 16 with saturating this.size +| 16 to prevent u52 integer overflow, (2) add an early guard that rejects with ENOMEM when this.size > synthetic_allocation_limit, (3) set this.system_error in the initCapacity catch block so the OOM is properly propagated as a promise rejection rather than silently resolving to an empty string, and (4) a POSIX-gated regression test that validates the ENOMEM rejection path using /dev/zero.

Security risks

None. The change only affects memory allocation size computation and error propagation — no auth, crypto, or permission-sensitive paths are involved.

Level of scrutiny

Low-to-medium. The fix is targeted and mechanical: a one-character arithmetic operator change plus two OOM-handling lines. The guard mirrors the existing Windows ReadFileUV pattern. The test makes a meaningful assertion (rejects.toMatchObject({ code: "ENOMEM" })) and is properly POSIX-gated with no per-test timeout.

Other factors

Previous review rounds surfaced several test quality issues (trivially-true assertions, unreachable fix paths, missing system_error, forbidden per-test timeout). The current diff shows all of those have been addressed in subsequent commits. The one pre-existing concern — doReadLoop using bun.handleOom for buffer growth — is out of scope and pre-dates this PR.

claude · 2026-04-04T13:32:28Z


        // add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data
        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.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;
            };


🟣 This PR correctly makes the initCapacity OOM path graceful (sets system_error, rejects with ENOMEM), but doReadLoop still uses bun.handleOom for buffer growth during streaming reads — a pre-existing crash-on-OOM path. The fix makes the inconsistency more visible: Bun.file("/dev/zero").slice(1).text() now rejects gracefully, while Bun.file("/dev/zero").text() (no slice, this.size=4096, initCapacity succeeds) would still crash the process via bun.handleOom if the allocator runs OOM during the read loop.

Extended reasoning...

What the bug is and how it manifests

ReadFile.doReadLoop uses bun.handleOom for two heap-buffer growth calls when read data arrives via the 64 KB stack buffer:

bun.handleOom(this.buffer.ensureTotalCapacityPrecise(bun.default_allocator, read.len)); // or bun.handleOom(this.buffer.ensureUnusedCapacity(bun.default_allocator, read.len));

bun.handleOom (confirmed in handle_oom.zig) calls bun.outOfMemory() on allocation failure, which invokes crash_handler.crashHandler(.out_of_memory) — a fatal process termination. This is in sharp contrast to the initCapacity path fixed by this PR, which now gracefully sets this.system_error and rejects the promise with ENOMEM.

The specific code path that triggers it

When reading a streaming/blocking source like /dev/zero without a slice (e.g., Bun.file("/dev/zero").text()): resolveSizeAndLastModified sees stat.size == 0 and could_block = true, so it sets this.size = 4096 (the default for unknown-size non-regular files). This is small enough for initCapacity to succeed, bypassing the PR's new guard. The read loop then begins. remainingBuffer returns the 64 KB stack buffer when heap remaining capacity < 64 KB (which is the case immediately after a 4096+16 byte initial allocation when more data arrives). ensureUnusedCapacity is then called under bun.handleOom. Under real memory pressure, this crashes.

Why existing code does not prevent it

The PR's graceful OOM handling is applied only to initCapacity at line 387-392. The doReadLoop buffer-growth paths at the bun.handleOom call sites are not touched by this PR and were crash-on-OOM before and after it. Nothing in the new guard (lines 388-393) or the saturating-add fix (line 397) intercepts OOM in the read loop itself.

What the impact would be

An observable behavioral asymmetry exists post-merge: a user who slices a streaming file with a large offset (e.g., Bun.file("/dev/zero").slice(1).text()) gets a graceful ENOMEM rejection, while the same file read without a slice (Bun.file("/dev/zero").text()) crashes the entire Bun process under real memory pressure. Both code paths are exercised by the same ReadFile struct; only the allocation site differs.

Step-by-step proof

Bun.file("/dev/zero").text() — no slice, so max_length = Blob.max_size.

resolveSizeAndLastModified: could_block = true, stat.size == 0 → this.size = 4096 (the default).

Guard at line 388 (this.size > synthetic_allocation_limit): 4096 > 4MB is false — guard does not fire.

initCapacity(bun.default_allocator, 4096 +| 16 = 4112): succeeds under normal conditions.

Read loop begins; remainingBuffer returns the 64 KB stack buffer when heap unused capacity < 64 KB.

bun.handleOom(this.buffer.ensureUnusedCapacity(..., read.len)): if allocator returns OOM, bun.outOfMemory() is called → crashHandler(.out_of_memory) → process crash.

No graceful rejection occurs; the promise never settles; Bun exits.

How to fix it

Replace the two bun.handleOom calls in doReadLoop with graceful error handling that mirrors the initCapacity path: catch OOM, set this.errno and this.system_error, and break/return to allow onFinish to propagate the error as a rejection. This is consistent with how ReadFileUV.queueRead already handles ensureUnusedCapacity gracefully on Windows.

robobun · 2026-04-14T00:57:03Z

Superseded by #29284, which fixes the root cause (caps the initial buffer for non-regular files at 4096 so the read actually succeeds instead of rejecting with ENOMEM) and also covers the Windows ReadFileUV path.

github-actions Bot added the claude label Apr 4, 2026