Skip to content

Fix integer overflow in ReadFile buffer pre-allocation #29207

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 2 commits into from
Closed

Fix integer overflow in ReadFile buffer pre-allocation #29207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bd3812
Fix integer overflow in ReadFile buffer pre-allocation
robobun Apr 12, 2026
d08e2ff
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 12, 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
4 changes: 2 additions & 2 deletions 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,13 +381,13 @@
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.onFinish();
return;

Check failure on line 390 in src/bun.js/webcore/blob/read_file.zig

View check run for this annotation

Claude / Claude Code Review

OOM not propagated as JS error in ReadFile POSIX path 

In `ReadFile.runAsyncWithFD`, the OOM catch block sets only `this.errno` but leaves `this.system_error` null; since `then()` dispatches errors solely via `system_error`, an OOM during buffer pre-allocation silently resolves the JS promise with an empty string instead of rejecting. Add `this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();` alongside the existing `this.errno = err;` at line 388.
Comment on lines +387 to 390

@coderabbitai coderabbitai Bot Apr 12, 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.

⚠️ Potential issue | 🟠 Major

Propagate OOM to system_error on the non-Windows path.

At Line 387, the catch path sets this.errno but not this.system_error; then() rejects only when system_error exists, so allocation failure can surface as a successful empty read instead of an error.

Suggested fix 
-        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.onFinish();
-                return;
-            };
+        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;
+                if (err == error.OutOfMemory) {
+                    this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
+                }
+                this.onFinish();
+                return;
+            };
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/blob/read_file.zig` around lines 387 - 390, The allocation
catch in the ArrayListUnmanaged initCapacity path currently only sets this.errno
and calls this.onFinish(), so then() won't reject; update the catch in the
non-Windows branch to also set this.system_error (e.g., assign the allocation
error or a mapped OOM system_error to this.system_error) before calling
this.onFinish() and returning, ensuring code paths in the read handler (the
this.buffer init, this.errno, this.system_error, onFinish(), and then()
rejection behavior) correctly propagate OOM to the caller.

Comment on lines 384 to 390

@claude claude Bot Apr 12, 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.

🔴 In ReadFile.runAsyncWithFD, the OOM catch block sets only this.errno but leaves this.system_error null; since then() dispatches errors solely via system_error, an OOM during buffer pre-allocation silently resolves the JS promise with an empty string instead of rejecting. Add this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError(); alongside the existing this.errno = err; at line 388.

Extended reasoning...

What the bug is and how it manifests

In ReadFile.runAsyncWithFD (the POSIX/non-UV path), when initCapacity fails with OutOfMemory (now reliably triggered by the PR's saturating-add fix passing maxInt(u52) as the capacity), the catch block executes:

this.errno = err;
this.onFinish();
return;

It sets this.errno but leaves this.system_error = null. The then() dispatcher — the only path that converts the task result into a JS callback — checks exclusively this.system_error to decide whether to call the callback with an error:

const system_error = this.system_error;
// ...
if (system_error) |err| {
    cb(cb_ctx, ReadFileResultType{ .err = err });
    return;
}
cb(cb_ctx, .{ .result = .{ .buf = buf, .total_size = total_size, .is_temporary = true } });

Because system_error is null, then() falls through to the success path, calling cb with buf = this.buffer.items which is an empty slice — the allocation never succeeded. The JS promise therefore resolves with "" instead of rejecting.

The specific code path that triggers it

  1. JS calls Bun.file("/dev/zero").slice(0, 4503599627370490).text()
  2. resolveSizeAndLastModified sets this.size = this.max_length (≈ maxInt(u52))
  3. runAsyncWithFD reaches initCapacity(bun.default_allocator, this.size +| 16) — after the PR fix, this is maxInt(u52)
  4. initCapacity returns error.OutOfMemory
  5. Only this.errno = err is set; this.system_error stays null
  6. onFinish() → then() → success callback with empty buffer

Why existing code doesn't prevent it

The then() function never checks this.errno at all; it is solely gated on this.system_error. This is an oversight: every other OOM handler in the same file (ReadFileUV lines ~702–703, ~731–732, ~764–765, ~791–792, and the ensureUnusedCapacity path) correctly sets both this.errno and this.system_error. Only this one catch block was left incomplete.

What the impact would be

A caller reading a large Blob on POSIX (e.g., a sliced /dev/zero) will receive an empty string from .text() / .arrayBuffer() / etc. without any error indication. This is silent data loss/corruption: code that checks the result value rather than expecting a rejection will silently process an empty string as if it were the file's contents.

How to fix it

Add the missing line inside the catch block:

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

Step-by-step proof

  1. Bun.file("/dev/zero").slice(0, 4503599627370490).text() is awaited in JS.
  2. ReadFile.createWithCtx allocates a ReadFile with max_length = 4503599627370490 (~maxInt(u52)).
  3. resolveSizeAndLastModified: /dev/zero is not a regular file, stat.size == 0, so this.size = this.max_length = 4503599627370490.
  4. runAsyncWithFD: this.size != Blob.max_size (they happen to be equal only at exact maxInt(u52), but after saturating add the capacity request is still enormous) → initCapacity(allocator, 4503599627370507) is called.
  5. The allocator returns error.OutOfMemory. The catch block sets this.errno = error.OutOfMemory only.
  6. onFinish() is called; eventually then() runs on the JS thread.
  7. In then(): this.store != null → skip first two branches. system_error = this.system_error = null. if (system_error) |err| is false. cb is called with .result = .{ .buf = &[]{}, .total_size = 0, .is_temporary = true }.
  8. JS promise resolves with "" — no error thrown, no rejection.

};
this.read_off = 0;

Expand Down Expand Up @@ -698,7 +698,7 @@
return;
}
// add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data
this.buffer.ensureTotalCapacityPrecise(this.byte_store.allocator, @min(this.size + 16, @as(usize, std.math.maxInt(bun.windows.ULONG)))) catch {
this.buffer.ensureTotalCapacityPrecise(this.byte_store.allocator, @min(this.size +| 16, @as(usize, std.math.maxInt(bun.windows.ULONG)))) catch {
this.errno = error.OutOfMemory;
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.onFinish();
Expand Down
17 changes: 17 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,5 +1,6 @@
import { expect, it } from "bun:test";
import { tmpdir } from "node:os";
import { bunEnv, bunExe, isPosix } from "harness";

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

it.skipIf(!isPosix)("reading a file blob sliced to near Blob.max_size should not crash", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`await Bun.file("/dev/zero").slice(0, 4503599627370490).text().then(() => {}, () => {});`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const exitCode = await proc.exited;
expect(proc.signalCode).toBeNull();
expect(exitCode).toBe(0);
});
Loading