Skip to content

fix integer overflow in ReadFile buffer capacity computation #28849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 3 commits into from
Closed

fix integer overflow in ReadFile buffer capacity computation #28849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e98d05a
fix integer overflow when ReadFile computes buffer capacity
robobun Apr 4, 2026
dbac3ef
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 4, 2026
ba08550
test: tighten regression assertion and drop per-test timeout
robobun Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/bun.js/webcore/blob/read_file.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,14 +381,15 @@
this.onFinish();
return;
}

// 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;
Comment thread
claude[bot] marked this conversation as resolved.
};

Check notice on line 392 in src/bun.js/webcore/blob/read_file.zig

View check run for this annotation

Claude / Claude Code Review

doReadLoop bun.handleOom crashes vs. graceful initCapacity OOM path 

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 O
Comment on lines 384 to 392

@claude claude Bot Apr 4, 2026

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.

🟣 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

  1. Bun.file("/dev/zero").text() — no slice, so max_length = Blob.max_size.
  2. resolveSizeAndLastModified: could_block = true, stat.size == 0 → this.size = 4096 (the default).
  3. Guard at line 388 (this.size > synthetic_allocation_limit): 4096 > 4MB is false — guard does not fire.
  4. initCapacity(bun.default_allocator, 4096 +| 16 = 4112): succeeds under normal conditions.
  5. Read loop begins; remainingBuffer returns the 64 KB stack buffer when heap unused capacity < 64 KB.
  6. bun.handleOom(this.buffer.ensureUnusedCapacity(..., read.len)): if allocator returns OOM, bun.outOfMemory() is called → crashHandler(.out_of_memory) → process crash.
  7. 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.

this.read_off = 0;

// If it's not a regular file, it might be something
Expand Down
11 changes: 11 additions & 0 deletions test/js/bun/util/bun-file-read.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect, it } from "bun:test";
import { isPosix } from "harness";
import { tmpdir } from "node:os";

it("offset should work in Bun.file() #4963", async () => {
Expand All @@ -9,3 +10,13 @@ it("offset should work in Bun.file() #4963", async () => {
const contents = await slice.text();
expect(contents).toBe("ntents");
});

it.skipIf(!isPosix)("slicing Bun.file() by a non-zero offset rejects rather than overflowing", async () => {
// Regression: ReadFile.runAsyncWithFD computed buffer capacity as
// `this.size + 16`, which overflowed u52 when `this.size` approached
// Blob.max_size (2^52 - 1). The fix uses saturating addition and
// propagates the OOM via system_error so the promise rejects with ENOMEM.
const file = Bun.file("/dev/zero");
const sliced = file.slice(1);
await expect(sliced.text()).rejects.toMatchObject({ code: "ENOMEM" });
});
Loading