Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions src/bun.js/webcore/blob/read_file.zig
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ pub const ReadFile = struct {
}

this.could_block = !bun.isRegularFile(stat.mode);
this.total_size = @truncate(@as(SizeType, @intCast(@max(@as(i64, @intCast(stat.size)), 0))));
this.total_size = @intCast(@min(@max(stat.size, 0), Blob.max_size));

Comment on lines 340 to 344

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 PR fixes the @truncate/@intcast panic in read_file.zig by clamping stat.size to Blob.max_size, but the analogous resolveFileStat function in Blob.zig (lines ~3298 and 3312) still uses the old @truncate pattern — for a file with stat.size just above 2^52 (e.g. 2^52+1), @truncate silently discards the high bits, yielding max_size=1 instead of Blob.max_size. This is a pre-existing issue in adjacent code that the PR did not touch, but it represents the same class of silent size-corruption bug the PR was explicitly written to address.

Extended reasoning...

What the bug is and how it manifests

In Blob.zig, the resolveFileStat function sets store.data.file.max_size via two expressions that use @truncate to convert a u64 stat.size to SizeType (u52):

Line 3298 (path-based): store.data.file.max_size = @truncate(@as(u64, @intCast(@max(stat.size, 0))))
Line 3312 (fd-based): store.data.file.max_size = @as(SizeType, @truncate(@as(u64, @intCast(@max(stat.size, 0)))))

Unlike the @intcast in read_file.zig (which panics on overflow), @truncate silently drops the high 12 bits when converting u64 to u52. For any stat.size greater than maxInt(u52) = 4503599627370496 (~4.5 PB), the resulting value is not Blob.max_size but rather the low 52 bits of the actual size.

The specific code path that triggers it

A file with stat.size = 2^52 + 1 (i.e., 4503599627370497) would produce @truncate result of 1 (the low 52 bits), setting store.data.file.max_size = 1. Downstream code in resolveSize checks max_size != Blob.max_size and uses it as the store_size, so allocation and routing decisions would use a 1-byte max_size for a multi-petabyte file.

Why existing code doesn't prevent it

@truncate is intentionally not a safety check in Zig — it compiles in release and debug modes without panicking. The only guard is that the high bits are dropped. There is no clamping logic like the PR's @intcast(@min(@max(stat.size, 0), Blob.max_size)) pattern.

Addressing the refutation

One verifier correctly noted that Blob.zig is not modified by this PR, read_file.zig does not call resolveFileStat, and the two code paths share no callers. This is accurate — the bug is genuinely pre-existing and the PR does not increase its surface area. However, the PR's stated purpose is specifically to fix this class of issue (stat.size clamping to Blob.max_size), and the PR author clearly recognized the pattern and applied the fix in read_file.zig. The same fix pattern should be applied to resolveFileStat for completeness. The pre_existing severity designation is appropriate.

What the impact would be

Any file whose stat.size is exactly above a 2^52 multiple (e.g. 2^52+1, 2^53+1, etc.) would get an incorrect small max_size. Files above 4.5 PB are essentially nonexistent on real hardware, making this a theoretical issue for the foreseeable future. But as filesystems and storage technology evolve, and especially in virtualized/mocked environments, this silent truncation is a latent data-corruption bug.

How to fix it

Apply the same clamping pattern used in the PR fix:

store.data.file.max_size = @intCast(@min(@max(stat.size, 0), Blob.max_size));

Step-by-step proof

  1. File has stat.size = 4503599627370497 (= 2^52 + 1)
  2. resolveFileStat is called (path or fd variant)
  3. @max(stat.size, 0) = 4503599627370497 (u64)
  4. @truncate(u64 → u52): 4503599627370497 mod 2^52 = 1
  5. store.data.file.max_size = 1 (should be Blob.max_size = 4503599627370496)
  6. resolveSize uses max_size=1, allocates 1-byte buffer for a >4.5 PB file
  7. ReadFile.create gets max_len=1, reads only 1 byte

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed that resolveFileStat in Blob.zig has a similar shape, but it uses @truncate (silent wrap, no panic) rather than @intCast, so it's not part of this crash. Keeping this PR scoped to the fuzzer-found panic in ReadFile; the Blob.zig clamping can be a separate follow-up if desired.

if (stat.size > 0 and !this.could_block) {
this.size = @min(this.total_size, this.max_length);
Expand Down Expand Up @@ -383,6 +383,7 @@ pub const ReadFile = struct {
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.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.onFinish();
return;
};
Comment on lines 383 to 389

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 OOM system_error at line 386 is created via bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError(), which leaves the .path field empty. By contrast, the EISDIR error in the same function (resolveSizeAndLastModified) explicitly populates .path from this.file_store.pathlike. When initCapacity fails on a named file (e.g., Bun.file("/some/huge/file").text()), the resulting JS error will have no .path property, making it slightly harder to identify which file triggered the OOM. This is a minor inconsistency — adding path enrichment matching the EISDIR pattern would make OOM errors more debuggable.

Extended reasoning...

What the bug is and how it manifests

When initCapacity fails in ReadFile.runAsyncWithFD (line 383-389), the PR added a new system_error assignment:

this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();

bun.sys.Error.fromCode constructs a minimal error with only errno and syscall fields; it does not set .path. So the resulting JS Error object thrown to user code will have err.path === undefined or empty, even when reading a named file.

The specific code path that triggers it

  1. User calls Bun.file("/some/large/file").text()
  2. ReadFile.runAsyncWithFD calls resolveSizeAndLastModified, which sets this.size
  3. initCapacity(bun.default_allocator, this.size +| 16) returns an OOM error
  4. The catch block sets this.system_error without a .path field
  5. then() propagates the system_error to the JS promise rejection — no path info

Why existing code doesn't prevent it

The doRead function (lines 218-220) does enrich system_error.path when it is empty for read-syscall errors, but that enrichment only runs inside doRead, not the initCapacity catch block. The EISDIR branch in resolveSizeAndLastModified explicitly sets .path, showing the pattern exists and works — it just wasn't applied here.

Addressing the refutation

One verifier argued this is intentional because all NOMEM errors in ReadFileUV also omit path. That's accurate — the pattern is consistent between ReadFile and ReadFileUV for OOM errors. However, "consistent with ReadFileUV" only means the omission is uniform, not that it's desirable. The EISDIR case (also present in both ReadFile and ReadFileUV) demonstrates that path enrichment is done and valued; there is no principled reason to treat NOMEM differently.

Impact

OOM errors are rare, and the PR already improves things significantly (previously OOM silently yielded an empty read with no error at all). Missing path makes OOM slightly harder to diagnose in production but is not a correctness issue.

Fix

After setting this.system_error, add path enrichment matching the EISDIR pattern:

this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.system_error.?.path = if (this.file_store.pathlike == .path)
    bun.String.cloneUTF8(this.file_store.pathlike.path.slice())
else
    bun.String.empty;

Step-by-step proof

  1. Bun.file("/huge/file").text()ReadFile.createrunAsyncWithFD
  2. resolveSizeAndLastModified succeeds; this.size = e.g. 2 GB
  3. initCapacity(allocator, 2GB + 16)error.OutOfMemory
  4. Catch: this.system_error = bun.sys.Error.fromCode(.NOMEM, .read).toSystemError(){ code: "ENOMEM", syscall: "read", path: "" }
  5. onFinish()then()cb(cb_ctx, .{ .err = system_error })
  6. JS sees Error { code: "ENOMEM", syscall: "read" } — no .path to identify the file

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Keeping this minimal and consistent with the existing ReadFileUV NOMEM pattern (lines 690, 697). The primary goal is propagating the error at all (previously it was a silent empty read); path enrichment for OOM can be a follow-up across both paths if needed.

Expand Down Expand Up @@ -656,7 +657,7 @@ pub const ReadFileUV = struct {
this.onFinish();
return;
}
this.total_size = @truncate(@as(SizeType, @intCast(@max(@as(i64, @intCast(stat.size)), 0))));
this.total_size = @intCast(@min(@max(stat.size, 0), Blob.max_size));
this.is_regular_file = bun.isRegularFile(stat.mode);

log("is_regular_file: {}", .{this.is_regular_file});
Expand Down
52 changes: 52 additions & 0 deletions test/js/bun/util/bun-file-fd-read.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, expect, test } from "bun:test";
import { closeSync, openSync } from "fs";
import { isWindows, tempDir } from "harness";
import { join } from "path";

// Reading a Bun.file() backed by a file descriptor goes through
// ReadFile.runAsync -> getFd (opened_fd already set) -> runAsyncWithFD ->
// resolveSizeAndLastModified, which derives total_size from fstat. That
// computation previously used @intCast to u52 guarded by a dead @truncate,
// so an abnormal fstat size could trip integerOutOfBounds. Triggering that
// directly requires fstat to report > 4.5 PB which is not achievable here,
// but these tests lock in the fd-backed ReadFile path that the fuzzer hit.
describe.skipIf(isWindows)("Bun.file(fd) read", () => {

Check warning on line 13 in test/js/bun/util/bun-file-fd-read.test.ts

View check run for this annotation

Claude / Claude Code Review

Tests should use describe.concurrent per CLAUDE.md convention

The three tests in `bun-file-fd-read.test.ts` use `describe.skipIf(isWindows)` instead of `describe.concurrent.skipIf(isWindows)`. Per `test/CLAUDE.md`, tests that write files should use `describe.concurrent` unless difficult to make concurrent; since each test creates its own independent `tempDir` and file descriptors, the fix is a one-word change: `describe.concurrent.skipIf(isWindows)`.

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 three tests in bun-file-fd-read.test.ts use describe.skipIf(isWindows) instead of describe.concurrent.skipIf(isWindows). Per test/CLAUDE.md, tests that write files should use describe.concurrent unless difficult to make concurrent; since each test creates its own independent tempDir and file descriptors, the fix is a one-word change: describe.concurrent.skipIf(isWindows).

Extended reasoning...

What the issue is

The three tests in bun-file-fd-read.test.ts are wrapped in describe.skipIf(isWindows) rather than describe.concurrent.skipIf(isWindows). The test/CLAUDE.md guideline at line 22 explicitly states: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it is very difficult to make them concurrent." All three tests write files (via tempDir), triggering this rule exactly.

The specific code

Line 13 of bun-file-fd-read.test.ts reads:

describe.skipIf(isWindows)("Bun.file(fd) read", () => {

It should be:

describe.concurrent.skipIf(isWindows)("Bun.file(fd) read", () => {

Why the fix is trivial and correct

Each test creates its own tempDir with unique file names (fd-read.txt, fd-slice.txt, fd-empty.txt). Each uses its own withFd calls to open/close separate fds. There is zero shared state between tests — no shared directory, no shared fd, no shared variables. The prerequisite for using describe.concurrent is fully satisfied.

Addressing the refutation

The refuter worried that describe.skipIf(isWindows).concurrent() may not be supported syntax. That is correct — but it is the wrong chain order. The supported and already-used-in-this-codebase pattern is describe.concurrent.skipIf(isWindows)(...) (accessing .concurrent first, then .skipIf). This exact pattern is confirmed by test/js/bun/util/symlink-path-traversal.test.ts and test/js/bun/bun-serve-file.test.ts in the codebase. The refutation is based on a misread of the API surface.

Step-by-step proof

  1. test/CLAUDE.md line 22: tests writing files must use describe.concurrent unless difficult.
  2. All three tests call tempDir(...) which writes files to disk.
  3. Each test has its own tempDir instance → no shared state → trivially concurrent.
  4. The fix is exactly one word: change describe.skipIf to describe.concurrent.skipIf.
  5. Other test files (e.g., symlink-path-traversal.test.ts) already use this exact pattern successfully.

async function withFd<T>(path: string, fn: (fd: number) => Promise<T>): Promise<T> {
const fd = openSync(path, "r");
try {
return await fn(fd);
} finally {
closeSync(fd);
}
}

test("text() and arrayBuffer() on a regular-file fd return file contents", async () => {
using dir = tempDir("bun-file-fd-read", { "fd-read.txt": "hello from fd" });
const path = join(String(dir), "fd-read.txt");

// Each read needs a fresh fd because Bun.file(fd) does not own or rewind
// the descriptor, and a completed read leaves it positioned at EOF.
expect(await withFd(path, fd => Bun.file(fd).text())).toBe("hello from fd");

const buf = await withFd(path, fd => Bun.file(fd).arrayBuffer());
expect(new Uint8Array(buf)).toEqual(new TextEncoder().encode("hello from fd"));
});

test("slice() with an end beyond the real size reads the actual file contents", async () => {
using dir = tempDir("bun-file-fd-read", { "fd-slice.txt": "0123456789" });
const path = join(String(dir), "fd-slice.txt");

// total_size should come from fstat (10), not from the requested slice
// end, so the initial buffer allocation stays small.
expect(await withFd(path, fd => Bun.file(fd).slice(0, Number.MAX_SAFE_INTEGER).text())).toBe("0123456789");
expect(await withFd(path, fd => Bun.file(fd).slice(2, 5).text())).toBe("234");
});

test("empty regular file via fd resolves with empty content", async () => {
using dir = tempDir("bun-file-fd-read", { "fd-empty.txt": "" });
const path = join(String(dir), "fd-empty.txt");

expect(await withFd(path, fd => Bun.file(fd).text())).toBe("");
expect((await withFd(path, fd => Bun.file(fd).arrayBuffer())).byteLength).toBe(0);
});
});
Loading