ReadFile: avoid integer overflow when computing buffer capacity#29000
ReadFile: avoid integer overflow when computing buffer capacity#29000robobun wants to merge 6 commits into
Conversation
this.size is u52 (Blob.SizeType). For character devices or pipes where max_length is near u52 max, this.size + 16 overflows and traps as SIGILL in debug builds. Widen to usize before adding.
WalkthroughCast file-size to Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
then() only inspects this.system_error to propagate errors to JS, but the catch handler for initCapacity in runAsyncWithFD was only setting this.errno. An OOM from the initial buffer allocation silently returned an empty result. Match the ReadFileUV pattern and set both.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/web/fetch/blob-oom.test.ts`:
- Around line 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().
🪄 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: f5052f6f-e342-4a1b-9c18-41b8130cd3a9
📒 Files selected for processing (2)
src/bun.js/webcore/blob/read_file.zigtest/js/web/fetch/blob-oom.test.ts
| expect( | ||
| async () => | ||
| await Bun.file("/dev/null") | ||
| .slice(0, 2 ** 52 - 15) | ||
| .bytes(), | ||
| ).toThrow(); |
There was a problem hiding this comment.
🧹 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.
| 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().
There was a problem hiding this comment.
LGTM — both the u52 overflow fix and the OOM propagation fix are correctly applied, matching the ReadFileUV pattern.
Extended reasoning...
Overview
Two files changed: src/bun.js/webcore/blob/read_file.zig (2-line fix in runAsyncWithFD) and test/js/web/fetch/blob-oom.test.ts (new regression test). The fix targets a narrow, well-understood integer overflow in buffer capacity computation for the non-blocking read path.
Security risks
None. The overflow path could previously cause debug-build panics (SIGILL) or undefined behavior, but is not a security boundary — it's an internal memory allocation. The fix correctly converts from u52 to usize before arithmetic and properly propagates OOM as a JS exception.
Level of scrutiny
Low. The diff is minimal and mechanical: a cast insertion and a missing field assignment. Both changes follow established patterns already present in ReadFileUV and other error paths in the same file. The fix is straightforward and well-tested.
Other factors
My prior inline comment identified the missing system_error assignment that would have caused silent data-loss for character devices with actual data. The author addressed this correctly in commit 8e273937d1, and the regression test now asserts .toThrow() rather than silently accepting empty bytes. No outstanding concerns remain.
There was a problem hiding this comment.
Both fixes are in place and my prior feedback was addressed.
Extended reasoning...
Overview
This PR touches (the ReadFile async path) and a regression test in . The changes are small and focused: a one-line type cast to prevent u52 overflow, plus a assignment in the OOM catch block.
Security risks
None. This is a numeric-type fix to prevent integer overflow in a buffer allocation path. No auth, crypto, or permissions code is involved.
Level of scrutiny
Low-to-medium. The change is mechanical and follows the exact pattern used by . My earlier inline comment flagged the missing — the author resolved it in the same PR. The current code correctly sets both and on OOM, matching all other error paths in the file.
Other factors
- CI failures shown in the timeline are from an intermediate commit (); the current HEAD is (autofix.ci formatting pass).
- The coderabbitai nit about tightening to be OOM-specific is trivial and does not affect correctness or the regression coverage.
- No CODEOWNERS-owned paths are touched.
There was a problem hiding this comment.
My prior concern about the missing system_error assignment in the OOM catch handler has been addressed — LGTM.
Extended reasoning...
Overview
This PR fixes a u52 integer overflow in ReadFile.runAsyncWithFD when computing the initCapacity argument. The change is minimal: a @as(usize, this.size) + 16 cast replaces this.size + 16, and the OOM catch handler now also sets this.system_error to properly propagate the error to JS. A non-Windows regression test is added.
Security risks
None. This is a memory-safety fix (overflow → panic in debug builds) with no authentication, cryptography, or permission surface.
Level of scrutiny
Low. The diff is 3 changed lines in the Zig source plus a new test block. The pattern exactly matches the existing ReadFileUV OOM handler. No novel logic is introduced.
Other factors
My prior inline review flagged that the catch handler only set this.errno and not this.system_error, causing OOM to be silently swallowed. The author addressed this in commit 8e27393. The one remaining open item — the bare .toThrow() in the test instead of an OOM-specific matcher — is a nit already covered by inline comments from CodeRabbit and this review cycle. It does not affect correctness of the fix itself.
| // 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(); | ||
| }); |
There was a problem hiding this comment.
🟡 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:
- Suppose a future change accidentally removes the
this.system_error = ...line from the catch block inread_file.zig:389. - OOM is now silently swallowed —
then()returns empty bytes to JS. - Simultaneously, suppose
/dev/nullraises EPERM for some other reason before reaching the OOM path. .toThrow()sees an exception (EPERM) and passes — even though OOM is not being propagated.- 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
There was a problem hiding this comment.
Both the u52 overflow and the missing system_error assignment on OOM (which I flagged earlier) are now fixed — LGTM.
Extended reasoning...
Overview
This PR touches and . The changes are small and targeted: a single cast to prevent integer overflow in , an added assignment in the same catch block, and a new platform-conditional regression test.
Security risks
None. The fix closes an integer overflow that caused a debug-mode SIGILL trap; it does not touch auth, crypto, or permission code.
Level of scrutiny
Low. Both changes are mechanical corrections that match existing patterns in the codebase ( already used the same assignment pattern). The test is appropriately guarded with .
Other factors
The one remaining open nit — tightening the test assertion from bare to an OOM-specific predicate — was flagged in my prior inline comment and by CodeRabbit, and is documented in the thread. It is trivial and does not affect correctness of the fix. No new bugs were found by the bug hunting system.
|
Pushed four empty retriggers chasing unrelated CI failures:
None touch |
| @@ -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| { | |||
There was a problem hiding this comment.
| 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| { |
|
Superseded by #29284, which fixes the root cause (caps the initial buffer for non-regular files at 4096 so the read actually succeeds instead of rejecting with ENOMEM) and also covers the Windows |
2 participants
this.sizeis au52(Blob.SizeType). InReadFile.runAsyncWithFD,this.size + 16is evaluated inu52arithmetic, and overflows whenthis.sizeis close tou52max. In debug builds this traps as SIGILL and looks like a thread-pool crash:The existing guard
this.size != Blob.max_sizeonly excludes the exact max value, so anysizein[max_u52 - 15, max_u52 - 1]still overflows.Reachable today via e.g.
Bun.file('/dev/null').slice(0, 2**52 - 15).bytes(): for a character deviceresolveSizeAndLastModifiedsetsthis.size = max_length, which is passed through from the blob slice.Fix: widen to
usizebefore adding. The allocator's existing catch handler gracefully handles the subsequentOutOfMemory.