Fix integer overflow reading sliced non-regular file blobs by robobun · Pull Request #29284 · oven-sh/bun

robobun · 2026-04-14T00:50:19Z

What does this PR do?

Fixes an integer overflow panic in ReadFile when reading a sliced file blob backed by a non-regular file (pipe/fifo/socket).

Fuzzer fingerprint: 8a544632e2452b59

Repro

await Bun.stdin.slice(1).text();

With stdin piped, this panics in debug builds at read_file.zig:387 with integer overflow.

Root cause

Bun.stdin has an unknown size, represented as Blob.max_size (maxInt(u52)). Slicing it with .slice(1) produces a blob with size = Blob.max_size - 1.

When reading such a blob, doReadFile passes blob.size as max_length to ReadFile. For non-regular files with stat.size == 0, the initial buffer size was set to max_length whenever max_length != Blob.max_size — so max_size - 1 was used directly. Adding the 16-byte slack then overflowed u52.

Even without the overflow, using max_length as the pre-allocation size for pipes is wrong: max_length is a cap, not an expected size.

Fix

Cap the initial buffer for non-regular files at @min(max_length, 4096). The read loop already grows the buffer via the 64KB stack buffer path as data arrives, and still respects max_length as a cap.
Use saturating addition (+|) for the 16-byte slack as defense-in-depth.
Applied the same change to the Windows ReadFileUV path.

How did you verify your code works?

bun bd test test/js/bun/util/bun-stdin-slice.test.ts passes.
Same test run against the unfixed debug build fails (subprocess panics).
Bun.stdin.slice(0, N).text() still correctly caps reads at N bytes.
Existing bun-file-read.test.ts and bun-file.test.ts still pass.

Reading a sliced file blob backed by a pipe/fifo (e.g. Bun.stdin.slice(1).text()) would panic in debug builds with an integer overflow when computing the initial read buffer capacity. Slicing a blob with unknown size produces a max_length just below Blob.max_size, which was then used directly as the initial buffer size for non-regular files, and adding the 16-byte slack overflowed the u52 size type. Cap the initial buffer for non-regular files at 4096 (the read loop already grows it as needed) and use saturating addition for the slack bytes.

robobun · 2026-04-14T00:50:31Z

^{Updated 10:09 PM PT - Apr 13th, 2026}

❌ @robobun, your commit 45a2425 has 1 failures in Build #45602 (All Failures):

test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts - pid 13580 error: addresssanitizer on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 29284

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

bun-29284 --bun

coderabbitai · 2026-04-14T00:53:26Z

Walkthrough

Use of @min for zero-length read buffer sizing and switched to checked addition when computing capacities in file-read logic. Added two tests exercising Bun.stdin.slice against piped stdin to validate slicing and prevent crashes.

Changes

Cohort / File(s)	Summary
File-read buffer & capacity changes `src/bun.js/webcore/blob/read_file.zig`	Replaced conditional buffer selection with `@min(max_length, 4096)` for zero-length reads from non-regular sources. Replaced `this.size + 16` with `this.size +
Stdin slice tests `test/js/bun/util/bun-stdin-slice.test.ts`	Added two tests (skipped on Windows) that spawn a Bun child with `stdin: "pipe"`, write input, and assert `Bun.stdin.slice(...).text()` returns expected content and the process exits with code 0.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix integer overflow reading sliced non-regular file blobs' clearly and specifically summarizes the main change—addressing an integer overflow panic when reading sliced non-regular file blobs, which is the core issue resolved by this PR.
Description check	✅ Passed	The PR description fully covers both required template sections: 'What does this PR do?' explains the fix with root cause analysis, and 'How did you verify your code works?' details comprehensive testing including new tests, existing test verification, and unfixed build validation.

_{✏️ 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 · 2026-04-14T00:55:57Z

This PR may be a duplicate of:

fix integer overflow in ReadFile buffer capacity computation #28849 - Fixes the same this.size + 16 integer overflow in ReadFile.runAsyncWithFD using saturating addition (+|)
ReadFile: avoid integer overflow when computing buffer capacity #29000 - Fixes the same this.size + 16 overflow by widening to usize before addition
blob: avoid size overflow when allocating ReadFile buffer #29008 - Fixes the same overflow with @intCast to usize and reorders the max_size guard

🤖 Generated with Claude Code

robobun · 2026-04-14T00:56:56Z

Not a duplicate — this supersedes #28849, #29000, and #29008.

Those three only patch the overflow symptom (saturating add / widen to usize), so Bun.stdin.slice(1).text() rejects with ENOMEM instead of crashing. This PR fixes the root cause: the initial buffer for non-regular files was being sized at max_length (a cap, not an expected size). With @min(max_length, 4096) the read actually succeeds and returns the data. It also applies the same fix to the Windows ReadFileUV path, which the others don't touch.

Closing the other three in favor of this one.

claude

LGTM — minimal, correct fix for the integer overflow with consistent application to both Linux and Windows paths.

Extended reasoning...

Overview

The PR modifies two spots in read_file.zig (the Linux ReadFile and Windows ReadFileUV paths) and adds a new test file. The core change replaces a conditional that used max_length directly as a pre-allocation size for non-regular files with @min(max_length, 4096), and uses saturating addition (+|) for the 16-byte slack.

Security Risks

None. The change only affects buffer pre-allocation size for non-regular files (pipes/FIFOs/sockets). It makes the allocation smaller in the problematic case, eliminating the overflow. No auth, crypto, or permission logic is touched.

Level of Scrutiny

Low. The diff is 8 lines across two nearly-identical code sites (Linux and Windows). The root cause and fix are well-explained in the PR description. The logic is straightforward: @min(max_length, 4096) is strictly safer than the previous conditional, and +| (saturating add) is the idiomatic Zig way to avoid overflow on a constant offset.

Other Factors

The pre-existing read_off-not-incremented issue flagged in the inline comments is a separate, pre-existing bug not introduced by this PR. The test coverage for the primary fix (no crash on Bun.stdin.slice(1).text()) is adequate to validate the reported panic is resolved.

claude · 2026-04-14T01:15:46Z

            // read up to 4k at a time if
            // they didn't explicitly set a size and we're reading from something that's not a regular file
        } else if (stat.size == 0 and this.could_block) {
-            this.size = if (this.max_length == Blob.max_size)
-                4096
-            else
-                this.max_length;
+            this.size = @min(this.max_length, 4096);
        }

        if (this.offset > 0) {


🟣 Pre-existing bug: in the Linux ReadFile.doReadLoop path, this.read_off is initialized to 0 and never incremented during the loop, so remainingBuffer() always caps each individual read at max_length bytes rather than max_length - already_read bytes. When data arrives in partial chunks, the accumulated buffer can exceed max_length by up to max_length-1 bytes. The PR adds a test claiming to verify the N-byte cap, but the test writes all bytes atomically so the OS delivers exactly N bytes in a single read call—the partial-chunk case that triggers the bug is never exercised.

Extended reasoning...

What the bug is: In ReadFile.doReadLoop (the Linux/non-Windows path), this.read_off is a field meant to track how many bytes have been read so far, allowing remainingBuffer() to compute a shrinking per-iteration limit of max_length - already_read. However, read_off is set to 0 in runAsyncWithFD and is never updated anywhere in doReadLoop. Every call to remainingBuffer() therefore computes max_length -| 0 = max_length, allowing up to max_length additional bytes to be read on every iteration regardless of how many bytes have already accumulated.

Specific code path: remainingBuffer (line 184): remaining[0..@min(remaining.len, this.max_length -| this.read_off)]. Since read_off never changes, this bound is constant at max_length. The only exit guard in doReadLoop is if (!this.read_eof and this.buffer.items.len >= this.max_length) break; — this fires after the over-read has already been appended.

Why existing code does not prevent it: The Windows ReadFileUV.onRead correctly does this.read_off += @intCast(result.int()); (line 796), so the Windows path is fine. The Linux path simply omits this update, making the two paths inconsistent.

Impact: With max_length = N on a non-regular file, if the first partial read delivers k bytes (k < N), the loop continues. On the next iteration remainingBuffer again offers an N-byte window (read_off still 0). If N bytes are now available, they are all read and buffer.items.len = k + N. Maximum overshoot is N-1 extra bytes. NewReadFileHandler.run updates blob.size but passes the raw over-sized slice to the Function callback, potentially exposing extra bytes to JavaScript callers.

Step-by-step proof with max_length = 3, data arrives as [1 byte, then 3 bytes]:

runAsyncWithFD: buffer allocated; read_off = 0.

Iter 1: remainingBuffer → stack_buf[0..3]; read returns 1 byte; buffer.items.len = 1; check 1 >= 3 → false; continue.

Iter 2: remainingBuffer → stack_buf[0..3] (read_off still 0); read returns 3 bytes; buffer.items.len = 4; check 4 >= 3 → break.

Caller receives a 4-byte buffer despite max_length = 3.

How to fix: Add this.read_off += @intCast(read_amount); inside doReadLoop after appending bytes to the buffer, mirroring the Windows ReadFileUV.onRead update.

Relationship to this PR: The bug is pre-existing and not introduced by this PR. However, the PR adds a test ('Bun.stdin.slice(0, N).text() caps reads at N bytes') that explicitly claims to validate the N-byte cap. Because proc.stdin.write('0123456789') delivers all 10 bytes atomically into the pipe buffer, the first read() with a 3-byte buffer returns exactly 3 bytes, immediately satisfying items.len >= max_length, so the test passes without exercising the multi-iteration partial-read scenario where the bug manifests.

Agreed this is pre-existing and out of scope for this crash fix — read_off is never incremented in the POSIX doReadLoop, unlike ReadFileUV.onRead on Windows. Leaving that for a separate change.

The second test here is a regression guard that @min(max_length, 4096) didn't break the existing cap behavior for the common single-read case, not a claim of full cap correctness.

The integer overflow being tested is POSIX-only; on Windows, ReadFileUV already bailed with ENOMEM on size > ULONG_MAX before reaching the overflow site. Reading a sliced stdin pipe via uv_fs_read with a non-zero offset is pre-existing behavior unrelated to this fix and behaves inconsistently across Windows versions.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/bun/util/bun-stdin-slice.test.ts`:
- Around line 22-23: The test asserts the unsliced input while the command uses
slice(1), so update the stdout assertion to validate slicing: change the
expected value for stdout from "hello world" to the sliced string "ello world"
(keep the exitCode assertion). Modify the expectation that references stdout in
the test (the lines with expect(stdout).toBe(...)) so it verifies the slice(1)
behavior rather than the full input.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97a26829-2fe5-4e44-82f6-3c8c9cdaf53c

📥 Commits

Reviewing files that changed from the base of the PR and between 5eabf81 and 45a2425.

📒 Files selected for processing (1)

test/js/bun/util/bun-stdin-slice.test.ts

coderabbitai · 2026-04-14T01:53:47Z

+  expect(stdout).toBe("hello world");
+  expect(exitCode).toBe(0);


⚠️ Potential issue | 🟠 Major

slice(1) assertion currently doesn't validate slicing behavior.

With slice(1) in the command, asserting the full unsliced input allows an offset-ignore bug to pass. Use a sliced expectation so this test guards both crash safety and offset handling.

Suggested fix

- expect(stdout).toBe("hello world"); + expect(stdout).toBe("ello world"); expect(exitCode).toBe(0);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/js/bun/util/bun-stdin-slice.test.ts` around lines 22 - 23, The test asserts the unsliced input while the command uses slice(1), so update the stdout assertion to validate slicing: change the expected value for stdout from "hello world" to the sliced string "ello world" (keep the exitCode assertion). Modify the expectation that references stdout in the test (the lines with expect(stdout).toBe(...)) so it verifies the slice(1) behavior rather than the full input.

This suggestion would make the test fail. Pipes aren't seekable — lseek(pipe_fd, 1, SEEK_SET) returns ESPIPE, and ReadFile intentionally ignores that error (see the existing comment at the setFileOffset call: // we ignore errors because it should continue to work even if its a pipe). So slice(1) on a piped stdin returns the full input; that's pre-existing behavior this PR doesn't change.

The assertion documents the actual current behavior so the test is a crash regression guard, not a spec for pipe offset semantics.

@robobun, you're right — I apologize for the incorrect suggestion. Since pipes aren't seekable (lseek → ESPIPE) and ReadFile intentionally swallows that error, slice(1) on a piped stdin correctly returns the full input. The assertion expect(stdout).toBe("hello world") accurately documents the actual behavior and serves purely as a crash regression guard. No change needed.

✏️ Learnings added

Learnt from: robobun Repo: oven-sh/bun PR: 29284 File: test/js/bun/util/bun-stdin-slice.test.ts:22-23 Timestamp: 2026-04-14T02:25:22.497Z Learning: In `src/bun.js/webcore/blob/read_file.zig`, `ReadFile.setFileOffset` intentionally ignores `ESPIPE` errors so that `Blob.slice(offset)` continues to work on non-seekable fds (pipes, FIFOs, sockets). Consequently, `Bun.stdin.slice(N).text()` on a piped stdin returns the FULL unsliced input — the seek is silently skipped. Tests that assert on `Bun.stdin.slice(N).text()` for piped stdin should therefore expect the full input string, not the offset-trimmed string. Do NOT suggest changing such assertions to the sliced expectation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

Learnt from: Jarred-Sumner Repo: oven-sh/bun PR: 24082 File: test/cli/test/coverage.test.ts:60-112 Timestamp: 2025-10-26T01:32:04.844Z Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Learnt from: robobun Repo: oven-sh/bun PR: 28694 File: test/regression/issue/20488.test.ts:37-41 Timestamp: 2026-03-31T00:56:25.338Z Learning: In oven-sh/bun regression tests (test/regression/issue/*.test.ts), do NOT assert `expect(stderr).toBe("")` on spawned Bun subprocesses. ASAN builds emit "WARNING: ASAN interferes with JSC signal handlers..." to stderr, causing such assertions to fail on ASAN CI targets. Use `expect(exitCode).toBe(0)` as the crash/failure signal instead; only add stderr assertions when checking for a specific expected error message, not to assert emptiness.

Learnt from: robobun Repo: oven-sh/bun PR: 28093 File: src/bun.js/webcore/Blob.zig:1416-1429 Timestamp: 2026-03-14T09:45:21.439Z Learning: In src/bun.js/webcore/Blob.zig, the fast-path in `writeFileInternal` that pipes a locked Response/Request ReadableStream directly via `pipeReadableStreamToBlob` (added in PR `#28093`, around the Response `.Locked` branch) does not respect destination blob offset/size for sliced file targets. This is a known limitation but not a regression: before PR `#28093`, `Bun.write(slicedFile, new Response(req.body))` deadlocked unconditionally, so the sliced-blob + locked-body combination was never reachable without a hang. A follow-up offset guard (analogous to the `blob.offset == 0` check at line 1265) should be added in a separate PR to make the slice semantics correct.

Learnt from: robobun Repo: oven-sh/bun PR: 28748 File: src/cli.zig:998-1002 Timestamp: 2026-04-02T21:02:25.201Z Learning: In oven-sh/bun `src/cli/run_command.zig`, adding `bootFromStdin` to the `.RunAsNodeCommand` path (i.e., when `argv0 == "node"` and `ctx.positionals.len == 0`) causes REPL test deadlocks. Tests like `test-repl-dynamic-import.js`, `test-repl-close.js`, and `as-node.test.ts` spawn bun-as-node with a piped stdin and write incrementally; `streamRemaining()` blocks until EOF, so the process hangs waiting for input that never ends. Node shim stdin bootstrapping requires an incremental (non-blocking) read approach and is intentionally deferred to a separate PR. Do NOT flag the absence of `bootFromStdin` in the `RunAsNodeCommand` / `execAsIfNode` no-positionals path as a missing feature parity issue — it was deliberately reverted (commit 79adac4 in PR `#28748`) to avoid these deadlocks.

Learnt from: robobun Repo: oven-sh/bun PR: 28481 File: test/regression/issue/28479.test.ts:132-136 Timestamp: 2026-03-23T20:13:52.791Z Learning: In `test/regression/issue/28479.test.ts` (oven-sh/bun PR `#28481`), the `env: { ...process.env }` spread passed to a child `Bun.spawn` in the "loaded vars are visible in child processes" test is intentional. `process.loadEnvFile` updates the JS `process.env` object but does NOT call `setenv`/`putenv` to update the OS-level environment on POSIX (only Windows gets OS-level updates via `SetEnvironmentVariableW`). Therefore, relying on default env inheritance would miss the new variable on Linux/macOS. Spreading `process.env` is the only correct cross-platform way to verify the JS-side update propagates to children. Do not flag this spread as bypassing the inherited-env path.

Learnt from: markovejnovic Repo: oven-sh/bun PR: 24417 File: test/js/bun/spawn/spawn.test.ts:903-918 Timestamp: 2025-11-06T00:58:23.965Z Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Learnt from: CR Repo: oven-sh/bun PR: 0 File: src/unicode/uucode/CLAUDE.md:0-0 Timestamp: 2026-01-23T08:08:06.510Z Learning: Applies to src/unicode/uucode/test/js/bun/util/stringWidth.test.ts : Verify grapheme table changes by running `bun bd test test/js/bun/util/stringWidth.test.ts` after regenerating tables

Learnt from: Jarred-Sumner Repo: oven-sh/bun PR: 24491 File: test/js/bun/transpiler/declare-global.test.ts:17-17 Timestamp: 2025-11-08T04:06:33.198Z Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Learnt from: CR Repo: oven-sh/bun PR: 0 File: test/CLAUDE.md:0-0 Timestamp: 2026-01-22T12:23:39.726Z Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.

Learnt from: robobun Repo: oven-sh/bun PR: 28628 File: test/regression/issue/28627.test.ts:28-31 Timestamp: 2026-03-29T03:13:10.496Z Learning: In `test/regression/issue/28627.test.ts` (oven-sh/bun PR `#28628`), the `expect(stdout).toContain("installed is-number")` assertion is intentional and should NOT be removed. The regression (`#28627`) is specifically that `bun add --production <pkg>` prints "installed is-number" but does not actually install the package. The stdout assertion validates the user-visible confirmation message, while the `existsSync` check validates the filesystem side-effect. Together they guard against both halves of the regression. The general guideline "avoid asserting stdout for bun install commands" does NOT apply here because the stdout message itself is a core part of the regression being tested.

Learnt from: robobun Repo: oven-sh/bun PR: 28112 File: src/bun.js/webcore/Blob.zig:1416-1429 Timestamp: 2026-03-14T17:19:52.471Z Learning: In src/bun.js/webcore/Blob.zig, the missing offset/slice guard for stream-to-file write paths is a pre-existing limitation that spans ALL such paths uniformly: (1) the WriteFileWaitFromLockedValueTask fallback, (2) the locked-body direct piping fast-path added in PR `#28093/28112`, and (3) pipeReadableStreamToBlob itself. Adding an offset check at only one call site would be inconsistent. The correct fix is to add offset awareness uniformly across all stream-to-file write paths in a dedicated follow-up PR.

Learnt from: robobun Repo: oven-sh/bun PR: 28732 File: test/regression/issue/13184.test.ts:16-20 Timestamp: 2026-04-01T16:21:59.448Z Learning: In `test/regression/issue/13184.test.ts` (oven-sh/bun PR `#28732`), do NOT add a `socket.destroyed` assertion inside `testServerCloseCompletes`. The test's regression signal is `server.close()` completing without hanging: if the socket is not destroyed, `_connections` is never decremented and `server.close()` times out. A direct `expect(socket.destroyed).toBe(true)` assertion was tried and caused intermittent failures under ASAN builds. The `server.close()` timeout failure is the correct and sufficient signal for this regression.

Learnt from: robobun Repo: oven-sh/bun PR: 28570 File: test/regression/issue/28035.test.ts:124-156 Timestamp: 2026-03-26T04:35:04.113Z Learning: In `test/regression/issue/28035.test.ts`, the "TransformStream proxy delivers all data" test intentionally uses `expect(body.length).toBe(TOTAL_CHUNKS * 25000)` (total byte count only) rather than per-chunk index encoding + deep equality. The test's scope is end-to-end streaming delivery correctness, not chunk ordering or data-integrity across a proxy. Do not suggest encoding chunk indices or comparing full byte sequences for this test; the length assertion is sufficient for its regression purpose.

Learnt from: robobun Repo: oven-sh/bun PR: 28686 File: test/regression/issue/12306.test.ts:50-55 Timestamp: 2026-04-02T18:42:38.406Z Learning: In `test/regression/issue/12306.test.ts` (oven-sh/bun PR `#28686`), the `done` Promise only rejects via the client `error` event — a genuine test failure. In that case, bun:test catches the unhandled rejection and reports the error, and the test process exits immediately, making the skipped `server.close()` harmless. Do not flag the missing try/finally around `await done` in this test as a resource-leak risk; the process-exit path is the correct and sufficient cleanup signal for this regression test.

Learnt from: robobun Repo: oven-sh/bun PR: 28621 File: src/cli/test_command.zig:1246-1266 Timestamp: 2026-03-28T13:16:34.757Z Learning: In `src/cli/test_command.zig` (`printCodeCoverage`), the `if (byte_ranges.len > 0)` guard before `lcov_writer.flush()` and the temp-file move is intentionally scoped to the case where `ByteRangeMapping.map` was null/empty (no files collected at all — introduced in PR `#28621`). The scenario where `byte_ranges.len > 0` but all entries are skipped by `coveragePathIgnorePatterns` is pre-existing behavior on main, covered by the existing "coveragePathIgnorePatterns - ignore all files" test. Do not suggest replacing this check with a `reported_file_count` counter, as that would be a separate behavioral change outside the scope of the null/empty-map fix.

Learnt from: robobun Repo: oven-sh/bun PR: 28112 File: src/bun.js/webcore/Blob.zig:1416-1429 Timestamp: 2026-03-16T02:00:53.488Z Learning: In `src/bun.js/webcore/Blob.zig`, `pipeReadableStreamToBlob` accepts only `extra_options` (S3/AWS options), not the full `WriteFileOptions` struct. As a result, `mkdirp_if_not_exists` and `mode` are NOT forwarded on any streaming write path — this includes the S3→file path in `writeFileWithSourceDestination` (around the `destination_type == .file and source_type == .s3` branch) and the locked-body fast-path added in PR `#28112`. This is a pre-existing limitation of the `pipeReadableStreamToBlob` API, not a regression. A follow-up would be needed to thread `WriteFileOptions` through `pipeReadableStreamToBlob` uniformly across all call sites.

Learnt from: robobun Repo: oven-sh/bun PR: 28784 File: src/shell/states/Subshell.zig:157-158 Timestamp: 2026-04-02T20:24:23.367Z Learning: In `src/shell/states/Subshell.zig` (and the pre-existing pattern in `src/shell/Builtin.zig` around line 429), `is_pollable` for the `IOWriter` constructed after `openForWritingImpl()` is hard-coded as `if (bun.Environment.isPosix) false else true` instead of using the `pollable` output variable set by `openForWritingImpl()`. This is intentional for now to stay consistent with the Builtin.zig pattern; a follow-up PR should fix both sites to use the `openForWritingImpl()` result instead of the platform constant, so FIFOs, `/dev/fd/*`, and TTY-backed redirect targets are classified correctly.

Learnt from: robobun Repo: oven-sh/bun PR: 28748 File: src/cli/run_command.zig:1609-1626 Timestamp: 2026-04-02T19:16:22.913Z Learning: In `src/cli/run_command.zig` (oven-sh/bun), `std.fs.File.stdin()` is intentional and correct when `.readerStreaming()` is needed (e.g., in `bootFromStdin`). `bun.FD.stdin()` returns a different type that does not have this method, so the "use bun.* APIs instead of std.*" guideline does not apply to `std.fs.File.stdin()` in streaming-reader contexts. Do not flag this as a guidelines violation.

Learnt from: robobun Repo: oven-sh/bun PR: 29262 File: src/cli/test/ChangedFilesFilter.zig:155-162 Timestamp: 2026-04-13T15:48:28.953Z Learning: In `src/cli/test/ChangedFilesFilter.zig` (`bun test --changed`), deleted/unresolved local imports are intentionally invisible to the `--changed` reverse-edge BFS. Unresolved `ImportRecord` entries only carry the raw specifier (e.g. `"./foo"`) — not a resolved absolute path — so reconstructing the absolute path of a deleted file would require re-implementing the resolver's extension/index probing against a path that no longer exists on disk. Vitest's `--changed` has the same limitation. Do not flag this as a bug; it is a known, accepted design trade-off. Revisit only if practical user reports surface it.

Learnt from: CR Repo: oven-sh/bun PR: 0 File: src/bun.js/bindings/v8/CLAUDE.md:0-0 Timestamp: 2025-11-24T18:36:59.706Z Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Learnt from: robobun Repo: oven-sh/bun PR: 27554 File: test/regression/issue/27553.test.ts:4-82 Timestamp: 2026-03-18T02:16:22.232Z Learning: In oven-sh/bun regression tests under test/regression/issue/*.test.ts, prefer explicit per-test setup and assertions (including repeated tempDir + Bun.spawn + bunExe/bunEnv + stdout/stderr/exitCode checks) over extracting shared helpers, so each test name maps clearly to its own setup. Avoid suggesting helper extraction unless the suite grows substantially or shared fixture complexity emerges.

Learnt from: robobun Repo: oven-sh/bun PR: 28308 File: test/js/node/net/blocklist-gc.test.ts:4-12 Timestamp: 2026-03-20T09:29:54.739Z Learning: In oven-sh/bun regression test files (e.g. test/js/node/net/blocklist-gc.test.ts and similar), do not suggest adding inline comments that explain the bug context or issue number being tested. Per repo convention, PR descriptions cover that context and test code is kept clean without such explanatory comments.

Learnt from: CR Repo: oven-sh/bun PR: 0 File: test/CLAUDE.md:0-0 Timestamp: 2026-01-22T12:23:39.726Z Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for test files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test/js/node/test/{parallel,sequential}/*.js files without .test extension, use `bun bd <file>` instead of `bun bd test <file>`.

Learnt from: robobun Repo: oven-sh/bun PR: 28116 File: test/regression/issue/23607.test.ts:39-44 Timestamp: 2026-03-14T20:16:40.048Z Learning: In `test/regression/issue/23607.test.ts`, the `bun_scan_*` temp file search is safe from cross-process interference because the test sets a dedicated `BUN_INSTALL_CACHE_DIR` and uses its own `tempDir`, providing full isolation within a single test run. Do not flag this pattern as a concurrency risk.

Learnt from: robobun Repo: oven-sh/bun PR: 29060 File: src/bun.js/web_worker.zig:500-517 Timestamp: 2026-04-09T00:41:01.359Z Learning: In oven-sh/bun `src/bun.js/web_worker.zig` + `src/bun.js/VirtualMachine.zig` (PR `#29060`): when a Worker `error` event listener cancels propagation via `preventDefault()`, the exit-code rollback is handled entirely inside `VirtualMachine.uncaughtException` — it saves `prev_exit_code` before writing `exit_code = 1`, calls `onUnhandledRejection`, then restores `prev_exit_code` if `worker.error_event_prevented` is true. The `onUnhandledRejection` callback in `web_worker.zig` does NOT touch `exit_handler.exit_code` at all. The `.bun`-mode async-rejection path never sets `exit_code = 1`, so no restore is needed there.

Learnt from: robobun Repo: oven-sh/bun PR: 28116 File: src/install/PackageManager/security_scanner.zig:680-683 Timestamp: 2026-03-14T20:16:45.662Z Learning: In `src/install/PackageManager/security_scanner.zig`, `bun.sys.File.writeFile` on Windows expects a wide-char path (`[:0]const u16`), but `std.fmt.bufPrintZ` into a `bun.PathBuffer` produces `[:0]u8`. When writing the temp scan file (`bun_scan_*`) on Windows, the UTF-8 path must first be converted to a wide-char path (e.g., via `bun.strings.toWPath` or equivalent) before passing to `bun.sys.File.writeFile`. This type mismatch also implies Windows backslash escaping for the injected JS string literal must be addressed at the same layer. Both issues are being fixed together in a follow-up push to PR `#28116`.

Learnt from: robobun Repo: oven-sh/bun PR: 29203 File: src/http/websocket_client/WebSocketUpgradeClient.zig:338-340 Timestamp: 2026-04-12T01:56:26.791Z Learning: In `src/http/websocket_client/WebSocketUpgradeClient.zig` (PR `#29203`), both the unix-socket path (`client.hostname = bun.default_allocator.dupeZ(u8, host_slice.slice()) catch ""`) and the TCP path (`out.hostname = bun.default_allocator.dupeZ(u8, display_host_) catch ""`) intentionally use `catch ""` for the hostname dupeZ rather than `bun.handleOom()`. The two branches are deliberately kept symmetric. Any OOM-hardening of this pattern must be applied to both paths together in a dedicated cleanup PR, not to the unix-socket branch alone. Do NOT flag the unix-socket branch's `catch ""` in isolation as an OOM-handling bug in reviews of this file.

Learnt from: robobun Repo: oven-sh/bun PR: 28904 File: src/bun.js/webcore/Blob.zig:437-440 Timestamp: 2026-04-06T08:49:57.622Z Learning: In `src/bun.js/webcore/Blob.zig`, the `errdefer blob.deinit()` added in PR `#28904` inside `_onStructuredCloneDeserialize` is scoped to the `.bytes` label block only. Once `break :bytes` fires and the `*Blob` is assigned to the outer `const blob`, any subsequent failure in the shared `versions` block (e.g. `readFloat`, filename bytes, is_jsdom_file read) does NOT trigger that errdefer and the heap-allocated Blob leaks. This is a pre-existing gap that also applies to `.file` and `.empty` store_tag variants — none of them have a function-scope errdefer on the outer `blob` variable. The correct fix is a function-scope `errdefer blob.deinit()` placed immediately after the outer `switch` assignment, with the inner branch-local errdefer either removed or converted; this fix is tracked as a follow-up to PR `#28904` and should not be re-flagged as a new regression in that PR.

Learnt from: robobun Repo: oven-sh/bun PR: 0 File: :0-0 Timestamp: 2026-04-07T19:13:28.440Z Learning: In `src/bun.js/node/Stat.zig` (oven-sh/bun PR `#28957`), the `toI64` helper converts stat fields for the BigIntStats path to match Node's `AliasedBigInt64Array` semantics exactly: signed platform types are widened via `intCast` (sign-extending, so macOS `dev_t = i32 -1` → `-1n`); unsigned platform types are `bitCast(as(u64, intCast(value)))` so u64 values above INT64_MAX wrap to negative BigInts — identical to Node's `static_cast<int64_t>(uint64_t)`. The `extern fn Bun__createJSBigIntStatsObject` therefore takes `int64_t` for dev/ino/rdev (not uint64_t). The `toF64` helper for the Stats (Number) path mirrors Node's `Float64Array` path: `floatFromInt` with no clamping, precision lost above 2^53 but never saturated to INT64_MAX.

Learnt from: CR Repo: oven-sh/bun PR: 0 File: src/CLAUDE.md:0-0 Timestamp: 2026-04-07T05:08:07.909Z Learning: Applies to src/**/*.zig : Use `bun.PathBuffer` for path buffers and `bun.path_buffer_pool` for pooled buffers to avoid large stack allocations, especially on Windows

Learnt from: robobun Repo: oven-sh/bun PR: 27056 File: test/bundler/standalone.test.ts:281-324 Timestamp: 2026-02-16T04:26:25.185Z Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.

Learnt from: cirospaciari Repo: oven-sh/bun PR: 27385 File: test/js/bun/http/tls-keepalive.test.ts:115-140 Timestamp: 2026-02-24T21:02:00.725Z Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.

Learnt from: LawoodDev Repo: oven-sh/bun PR: 27855 File: test/cli/run/concurrency-filter.test.ts:32-32 Timestamp: 2026-03-06T16:21:42.189Z Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.

Learnt from: LawoodDev Repo: oven-sh/bun PR: 27855 File: test/cli/run/concurrency-filter.test.ts:32-32 Timestamp: 2026-03-06T16:22:55.570Z Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.

Learnt from: robobun Repo: oven-sh/bun PR: 28214 File: test/regression/issue/18115.test.ts:1-158 Timestamp: 2026-03-18T15:19:38.407Z Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.

Learnt from: robobun Repo: oven-sh/bun PR: 28425 File: test/regression/issue/28422.test.ts:65-79 Timestamp: 2026-03-22T10:12:05.719Z Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.

Learnt from: dylan-conway Repo: oven-sh/bun PR: 28863 File: scripts/build/deps/webkit.ts:149-161 Timestamp: 2026-04-04T19:43:49.607Z Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.

Learnt from: robobun Repo: oven-sh/bun PR: 28923 File: test/regression/issue/28921.test.ts:0-0 Timestamp: 2026-04-06T19:19:08.790Z Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.

Learnt from: robobun Repo: oven-sh/bun PR: 29050 File: test/regression/issue/29042.test.ts:60-94 Timestamp: 2026-04-08T21:22:00.840Z Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.

claude

LGTM — the root-cause fix is correct and the pre-existing read_off concern is acknowledged and tracked separately.

Extended reasoning...

Overview

Two files changed: src/bun.js/webcore/blob/read_file.zig (4 lines) and a new test file. The fix replaces the conditional that incorrectly used max_length as an initial buffer size for non-regular files with @min(max_length, 4096), and adds saturating addition (+|) for the 16-byte slack. Applied symmetrically to both the POSIX ReadFile and Windows ReadFileUV paths.

Security risks

None. The change reduces the risk of integer overflow and of allocating an enormous buffer when max_length is close to Blob.max_size.

Level of scrutiny

Low — this is a targeted, mechanical fix to a crash-inducing integer overflow. The logic change is straightforward: cap a pre-allocation at 4096 instead of using an unbounded cap value, and use saturating arithmetic. The read loop already handles growing the buffer dynamically, so the cap change cannot lose data.

Other factors

The pre-existing read_off-not-incremented issue in doReadLoop that I previously flagged has been acknowledged as out of scope and slated for a separate fix. The two CI failures (cluster kill timeout, v8 heap snapshot illegal instruction) are unrelated flaky tests. The fix supersedes three earlier incomplete patches that only addressed the symptom.

robobun · 2026-04-14T02:32:07Z

CI failures in #45588 and #45602 are unrelated pre-existing flakes:

test/js/node/test/parallel/test-cluster-primary-kill.js timeout on debian-13-x64-asan — also failing on several other open PRs
test/js/bun/util/v8-heap-snapshot.test.ts illegal instruction on windows-11-aarch64

All 53 per-job statuses on #45602 are green; only the aggregate build status is red from the sharded asan failure. Neither failing test touches Blob/ReadFile/stdin.

robobun · 2026-04-14T04:06:36Z

Build #45602 update:

4 macOS test jobs show Expired (agent unavailable) — infra, not a test failure
test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts ASAN error on debian-13-x64-asan — pre-existing flake also hitting ai slop #28794, sys: retry stat-family syscalls on EINTR #29288, fix(http2): add captureRejectionSymbol handler to Http2SecureServer #29051, fix(expect): propagate exception from custom asymmetric matcher in asymmetricMatch #29199, GarbageCollectionController: fire up to 2 Full GCs before going to slow interval #29280 and others; tracked by ai slop #28794

Still nothing related to this change.

…9284) ## What does this PR do? Fixes an integer overflow panic in `ReadFile` when reading a sliced file blob backed by a non-regular file (pipe/fifo/socket). Fuzzer fingerprint: `8a544632e2452b59` ## Repro ```js await Bun.stdin.slice(1).text(); ``` With stdin piped, this panics in debug builds at `read_file.zig:387` with `integer overflow`. ## Root cause `Bun.stdin` has an unknown size, represented as `Blob.max_size` (`maxInt(u52)`). Slicing it with `.slice(1)` produces a blob with `size = Blob.max_size - 1`. When reading such a blob, `doReadFile` passes `blob.size` as `max_length` to `ReadFile`. For non-regular files with `stat.size == 0`, the initial buffer size was set to `max_length` whenever `max_length != Blob.max_size` — so `max_size - 1` was used directly. Adding the 16-byte slack then overflowed `u52`. Even without the overflow, using `max_length` as the *pre-allocation* size for pipes is wrong: `max_length` is a cap, not an expected size. ## Fix - Cap the initial buffer for non-regular files at `@min(max_length, 4096)`. The read loop already grows the buffer via the 64KB stack buffer path as data arrives, and still respects `max_length` as a cap. - Use saturating addition (`+|`) for the 16-byte slack as defense-in-depth. - Applied the same change to the Windows `ReadFileUV` path. ## How did you verify your code works? - `bun bd test test/js/bun/util/bun-stdin-slice.test.ts` passes. - Same test run against the unfixed debug build fails (subprocess panics). - `Bun.stdin.slice(0, N).text()` still correctly caps reads at N bytes. - Existing `bun-file-read.test.ts` and `bun-file.test.ts` still pass. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>

github-actions Bot added the claude label Apr 14, 2026

This was referenced Apr 14, 2026

fix integer overflow in ReadFile buffer capacity computation #28849

Closed

ReadFile: avoid integer overflow when computing buffer capacity #29000

Closed

blob: avoid size overflow when allocating ReadFile buffer #29008

Closed

claude Bot reviewed Apr 14, 2026

View reviewed changes

coderabbitai Bot reviewed Apr 14, 2026

View reviewed changes

claude Bot reviewed Apr 14, 2026

View reviewed changes

github-actions Bot mentioned this pull request Apr 14, 2026

Fix integer overflow when reading a sliced non-regular file Blob #29293

Closed

Jarred-Sumner merged commit d7b18ac into main Apr 14, 2026
60 of 65 checks passed

Jarred-Sumner deleted the farm/579739c9/fix-readfile-slice-overflow branch April 14, 2026 04:45

coderabbitai Bot mentioned this pull request Apr 15, 2026

blob: clamp stat.size to max_size to avoid @intCast panic in ReadFile #29355

Merged

		expect(stdout).toBe("hello world");
		expect(exitCode).toBe(0);

Conversation

robobun commented Apr 14, 2026

What does this PR do?

Repro

Root cause

Fix

How did you verify your code works?

Uh oh!

robobun commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

robobun commented Apr 14, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security Risks

Level of Scrutiny

Other Factors

Uh oh!

claude Bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

robobun Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robobun Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Apr 14, 2026

Uh oh!

robobun commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Apr 14, 2026 •

edited

Loading

coderabbitai Bot commented Apr 14, 2026 •

edited

Loading

coderabbitai Bot Apr 14, 2026 •

edited

Loading