Skip to content

ReadFile: avoid integer overflow when computing buffer capacity #29000

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

ReadFile: avoid integer overflow when computing buffer capacity #29000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a661ed5
ReadFile: avoid integer overflow when computing buffer capacity
robobun Apr 8, 2026
97d594c
retrigger ci
robobun Apr 8, 2026
8e27393
ReadFile: propagate buffer allocation OOM as system_error
robobun Apr 8, 2026
cf4a0b2
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 8, 2026
f2aa46d
retrigger ci
robobun Apr 8, 2026
1292506
retrigger ci
robobun Apr 8, 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 @@ -384,8 +384,9 @@ pub const ReadFile = struct {

// 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, @as(usize, this.size) + 16) catch |err| {

@Jarred-Sumner Jarred-Sumner Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, @as(usize, this.size) + 16) catch |err| {
this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, @min(@as(usize, this.size) + 16, bun.jsc.VirtualMachine.synthetic_allocation_limit)) 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.
};
Expand Down
18 changes: 17 additions & 1 deletion test/js/web/fetch/blob-oom.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { setSyntheticAllocationLimitForTesting } from "bun:internal-for-testing";
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from "bun:test";
import { unlinkSync } from "fs";
import { tempDirWithFiles } from "harness";
import { isWindows, tempDirWithFiles } from "harness";
import path from "path";
describe("Memory", () => {
beforeAll(() => {
Expand Down Expand Up @@ -142,3 +142,19 @@ describe("Bun.file", () => {
expect(async () => await Bun.file(tmpFile).arrayBuffer()).not.toThrow();
});
});

describe.if(!isWindows)("Bun.file on character devices", () => {
test("slice() with a very large max length throws OOM without crashing", async () => {
// Regression test: reading a character device (e.g. /dev/null) with a
// max_length close to u52 max used to overflow `this.size + 16` in
// ReadFile.runAsyncWithFD and panic in debug builds. Also verifies that
// OOM from the buffer allocation is propagated to JS rather than
// silently returning an empty result.
expect(
async () =>
await Bun.file("/dev/null")
.slice(0, 2 ** 52 - 15)
.bytes(),
).toThrow();
Comment on lines +153 to +158

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

🧹 Nitpick | 🔵 Trivial

Make this regression assertion OOM-specific.

toThrow() is broad and may pass on unrelated exceptions. Since this test is validating OOM propagation, assert an OOM-shaped error message/code.

Suggested assertion tightening 
-    expect(
-      async () =>
-        await Bun.file("/dev/null")
-          .slice(0, 2 ** 52 - 15)
-          .bytes(),
-    ).toThrow();
+    expect(
+      async () =>
+        await Bun.file("/dev/null")
+          .slice(0, 2 ** 52 - 15)
+          .bytes(),
+    ).toThrow(/out of memory|enomem|cannot allocate memory/i);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(
async () =>
await Bun.file("/dev/null")
.slice(0, 2 ** 52 - 15)
.bytes(),
).toThrow();
expect(
async () =>
await Bun.file("/dev/null")
.slice(0, 2 ** 52 - 15)
.bytes(),
).toThrow(/out of memory|enomem|cannot allocate memory/i);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/js/web/fetch/blob-oom.test.ts` around lines 153 - 158, The current
assertion uses a broad toThrow() which can catch unrelated errors; tighten it to
assert an OOM-specific failure by expecting an OOM-shaped message or code from
the Promise returned by Bun.file(...).slice(...).bytes(): replace expect(async
() => await Bun.file("/dev/null").slice(0, 2 ** 52 - 15).bytes()).toThrow() with
an assertion that matches an out-of-memory error (e.g., toThrow(/out of
memory/i) or toThrowErrorMatchingInlineSnapshot with the OOM message), so the
test for blob-oom.test.ts specifically verifies OOM propagation from
Bun.file(...).slice(...).bytes().

});
Comment on lines +151 to +159

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

🟡 The new regression test at line 158 uses bare .toThrow() without an error-message predicate, so it passes for any thrown exception — not specifically the ENOMEM that the OOM propagation fix is meant to surface. The test comment explicitly states it 'verifies that OOM from the buffer allocation is propagated to JS rather than silently returning an empty result', but .toThrow() cannot verify that; it would also pass if an unrelated EPERM or ENOENT were thrown while OOM was silently swallowed. The fix is to use .toThrow(/out of memory|enomem|cannot allocate memory/i), consistent with the OOM-specific assertions in the 'Memory' describe block (lines 34, 60, 91) that use exact messages like 'Out of memory'.

Extended reasoning...

What the bug is: The test at test/js/web/fetch/blob-oom.test.ts:158 uses expect(async () => ...).toThrow() with no error-message argument. This assertion succeeds for any thrown exception, not just the ENOMEM error that bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError() produces.

The specific false-positive scenario: If the OOM propagation fix (this.system_error = ... assignment) were accidentally reverted, reading /dev/null would silently return empty bytes (the pre-fix behaviour for the ReadFile path). In that specific scenario .toThrow() would indeed fail — so the test does guard against the primary regression. However, if some other unrelated exception fires (e.g. a permissions change on /dev/null, a libc change that makes character-device stat behave differently, or a future refactor that introduces an early error return), the test would incorrectly pass even while OOM is silently swallowed.

Why existing code doesn't prevent it: The Memory describe-block OOM tests at lines 34, 60, and 91 all pass an exact string to .toThrow() (e.g., 'Out of memory'). The new test follows the weaker pattern of the Bun.file describe block (lines 130, 134, 138) rather than the stronger OOM-specific pattern used elsewhere in the same file.

Impact: The assertion specificity is inconsistent with the stated test comment. In practice the risk is low because /dev/null is guaranteed accessible on non-Windows systems, but the test does not fully validate the invariant it claims to validate.

How to fix: Replace .toThrow() with .toThrow(/out of memory|enomem|cannot allocate memory/i). The NOMEM error from bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read) typically surfaces in JS as 'ENOMEM' or 'Out of memory'.

Step-by-step proof of the weakness:

  1. Suppose a future change accidentally removes the this.system_error = ... line from the catch block in read_file.zig:389.
  2. OOM is now silently swallowed — then() returns empty bytes to JS.
  3. Simultaneously, suppose /dev/null raises EPERM for some other reason before reaching the OOM path.
  4. .toThrow() sees an exception (EPERM) and passes — even though OOM is not being propagated.
  5. With .toThrow(/out of memory|enomem|cannot allocate memory/i), step 4 would fail, correctly catching the regression.

Addressing refutations: The refutation notes that the false-positive scenario (different exception + silent OOM simultaneously) is implausible for /dev/null. This is correct, which is why the severity is nit rather than normal. The bare .toThrow() is also consistent with lines 130–138 in the same file. CodeRabbit independently flagged this as 'Nitpick | Trivial' inline.

🔬 also observed by coderabbitai

});
Loading