-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix integer overflow reading sliced non-regular file blobs #29284
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // Reading a sliced non-regular file blob (like stdin from a pipe) with a size | ||
| // close to Blob.max_size used to overflow when computing the initial read | ||
| // buffer capacity. | ||
| test("Bun.stdin.slice(1).text() does not crash when stdin is a pipe", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", `process.stdout.write(await Bun.stdin.slice(1).text());`], | ||
| env: bunEnv, | ||
| stdin: "pipe", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| proc.stdin.write("hello world"); | ||
| await proc.stdin.end(); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("hello world"); | ||
| expect(exitCode).toBe(0); | ||
|
Comment on lines
+22
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With Suggested fix- expect(stdout).toBe("hello world");
+ expect(stdout).toBe("ello world");
expect(exitCode).toBe(0);🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suggestion would make the test fail. Pipes aren't seekable — The assertion documents the actual current behavior so the test is a crash regression guard, not a spec for pipe offset semantics.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
🧠 Learnings used |
||
| }); | ||
|
|
||
| test("Bun.stdin.slice(0, N).text() caps reads at N bytes", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", `process.stdout.write(await Bun.stdin.slice(0, 3).text());`], | ||
| env: bunEnv, | ||
| stdin: "pipe", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| proc.stdin.write("0123456789"); | ||
| await proc.stdin.end(); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("012"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 Pre-existing bug: in the Linux
ReadFile.doReadLooppath,this.read_offis initialized to 0 and never incremented during the loop, soremainingBuffer()always caps each individual read atmax_lengthbytes rather thanmax_length - already_readbytes. When data arrives in partial chunks, the accumulated buffer can exceedmax_lengthby up tomax_length-1bytes. 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_offis a field meant to track how many bytes have been read so far, allowingremainingBuffer()to compute a shrinking per-iteration limit ofmax_length - already_read. However,read_offis set to 0 inrunAsyncWithFDand is never updated anywhere indoReadLoop. Every call toremainingBuffer()therefore computesmax_length -| 0 = max_length, allowing up tomax_lengthadditional 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)]. Sinceread_offnever changes, this bound is constant atmax_length. The only exit guard indoReadLoopisif (!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.onReadcorrectly doesthis.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 = Non a non-regular file, if the first partial read deliverskbytes (k < N), the loop continues. On the next iterationremainingBufferagain offers anN-byte window (read_off still 0). IfNbytes are now available, they are all read andbuffer.items.len = k + N. Maximum overshoot isN-1extra bytes.NewReadFileHandler.runupdatesblob.sizebut passes the raw over-sized slice to theFunctioncallback, 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.remainingBuffer→stack_buf[0..3]; read returns 1 byte;buffer.items.len = 1; check1 >= 3→ false; continue.remainingBuffer→stack_buf[0..3](read_off still 0); read returns 3 bytes;buffer.items.len = 4; check4 >= 3→ break.max_length = 3.How to fix: Add
this.read_off += @intCast(read_amount);insidedoReadLoopafter appending bytes to the buffer, mirroring the WindowsReadFileUV.onReadupdate.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. Becauseproc.stdin.write('0123456789')delivers all 10 bytes atomically into the pipe buffer, the firstread()with a 3-byte buffer returns exactly 3 bytes, immediately satisfyingitems.len >= max_length, so the test passes without exercising the multi-iteration partial-read scenario where the bug manifests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this is pre-existing and out of scope for this crash fix —
read_offis never incremented in the POSIXdoReadLoop, unlikeReadFileUV.onReadon 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.