Skip to content

Fix integer overflow in ReadFile buffer preallocation#29254

Closed
robobun wants to merge 2 commits into
mainfrom
farm/16e814de/fix-readfile-size-overflow
Closed

Fix integer overflow in ReadFile buffer preallocation#29254
robobun wants to merge 2 commits into
mainfrom
farm/16e814de/fix-readfile-size-overflow

Conversation

@robobun

@robobun robobun commented Apr 13, 2026

Copy link
Copy Markdown
Collaborator

Fuzzilli found a debug panic (integer overflow) in ReadFile.runAsyncWithFD. Fingerprint: 9cb0cbabf00a5721.

What

When reading a file Blob backed by a non-regular file (pipe, character device) that has been sliced with an end value close to maxInt(u52), this.size is set to max_length and then this.size + 16 overflows the u52 SizeType when computing the initial buffer capacity.

await Bun.file("/dev/null").slice(0, 2**52 - 2).arrayBuffer();
panic: integer overflow
bun.js.webcore.blob.read_file.ReadFile.runAsyncWithFD
src/bun.js/webcore/blob/read_file.zig:387:100

How

Use saturating addition (+|) so the allocation request is passed through to the allocator, which will fail cleanly with OutOfMemory (handled by the existing catch) instead of panicking on overflow.

When a file Blob backed by a non-regular file (pipe, character device)
is sliced with an end value close to maxInt(u52), the max_length is
propagated to this.size, and adding 16 bytes of slop for the initial
capacity overflows the u52 SizeType.

Use saturating addition so the allocation attempt fails cleanly with
OutOfMemory instead of panicking.
@robobun

robobun commented Apr 13, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:09 AM PT - Apr 13th, 2026

@autofix-ci[bot], your commit bfa7938 has 1 failures in Build #45467 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29254

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

bun-29254 --bun

@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Modified buffer capacity calculation in file read operations to use saturating addition instead of plain addition to handle integer overflow, and added a test case for large file read operations via spawned Bun process.

Changes

Cohort / File(s) Summary
Buffer Capacity Calculation
src/bun.js/webcore/blob/read_file.zig
Changed arithmetic operator from + to `+
File Read Integration Test
test/js/bun/util/bun-file-read.test.ts
Added test case (skipped on Windows) that spawns Bun process to read and slice /dev/null to near-maximum size, converting to arrayBuffer and validating process output and exit code.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately describes the main change: fixing integer overflow in ReadFile buffer preallocation, which is exactly what the code modification and test additions address.
Description check ✅ Passed The pull request description follows the required template with both sections completed. It explains what the issue is (integer overflow panic) and how it was fixed (saturating addition), with a concrete example and error trace.

✏️ 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 in ReadFile buffer capacity computation #28849 - Same fix: changes this.size + 16 to saturating addition on the same line in read_file.zig
  2. ReadFile: avoid integer overflow when computing buffer capacity #29000 - Same overflow bug, uses @as(usize, this.size) + 16 widen-then-add approach instead
  3. blob: avoid size overflow when allocating ReadFile buffer #29008 - Same overflow bug, uses @as(usize, @intCast(this.size)) + 16 with an additional guard check

🤖 Generated with Claude Code

@robobun

robobun commented Apr 13, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #28849 (same saturating-add fix on the same line). Closing in favor of the earlier PR.

@robobun robobun closed this Apr 13, 2026
@robobun robobun deleted the farm/16e814de/fix-readfile-size-overflow branch April 13, 2026 09:27
Comment on lines 384 to 390

// 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.onFinish();
return;

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.

🔴 When fails with OOM in , the catch block sets only but not , causing to silently return an empty instead of rejecting the JS promise. The fix is to also set in the catch block, mirroring the Windows () path which correctly sets both fields.

Extended reasoning...

What the bug is and how it manifests

In ReadFile.runAsyncWithFD (src/bun.js/webcore/blob/read_file.zig, lines 387–390), when initCapacity fails with OutOfMemory, the catch block only assigns this.errno = err but never sets this.system_error. However, the then() function (line ~261) exclusively checks this.system_error to decide whether to call the callback with an error. Since system_error remains null, then() falls through to the success path and returns an empty ArrayBuffer to JavaScript with no indication that anything went wrong.

The specific code path that triggers it

The PR introduces saturating addition (+|) to fix the integer overflow panic. Before, this.size + 16 would overflow and panic in debug or wrap in release — either way, this specific error path was rarely reached in practice. After the fix, a near-maxInt(u52) slice value passes through saturating addition to initCapacity, which then legitimately fails with OutOfMemory. The catch block fires, sets only errno, calls onFinish(), and returns early. Later, then() reads this.system_error (null) and dispatches success.

Why existing code doesn't prevent it

Every other error path in ReadFile sets both errno and system_error together (e.g., lines 146–147, 216–217, 316–317, 329–330 in the unmodified file). Only this one catch block is inconsistent. The ReadFileUV (Windows path) equivalent is correctly written: it sets both this.errno = error.OutOfMemory and this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError() in its ensureTotalCapacityPrecise catch block.

What the impact would be

On Linux/macOS, a caller doing await Bun.file('/dev/stdin').slice(0, 2**52 - 2).arrayBuffer() (or any non-regular file with actual data) would receive an empty ArrayBuffer with exitCode 0 instead of a rejected promise or thrown error. The test added in this PR uses /dev/null which always returns 0 bytes anyway, so the silent empty result happens to be correct — this masks the bug in CI.

How to fix it

In the initCapacity catch block, add the missing assignment before this.onFinish():

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;
};

Step-by-step proof

  1. User calls await Bun.file('/dev/stdin').slice(0, 2**52 - 2).arrayBuffer() on Linux.
  2. resolveSizeAndLastModified sees a non-regular file (could_block = true) and sets this.size = this.max_length = 2^52 - 2.
  3. runAsyncWithFD enters the buffer pre-allocation branch: this.size +| 16 saturates to 2^52 - 2 + 16 = 2^52 + 14 (or clamps to maxInt), which is passed to initCapacity.
  4. initCapacity fails with OutOfMemory; the catch block sets this.errno = error.OutOfMemory but leaves this.system_error = null.
  5. onFinish() is called, eventually dispatching then().
  6. In then(): const system_error = this.system_error;null. The if (system_error) |err| branch is skipped.
  7. cb(cb_ctx, .{ .result = .{ .buf = buf, .total_size = total_size, .is_temporary = true } }) is called with buf = &[] (empty), resolving the promise successfully with an empty buffer.

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