Fix integer overflow in ReadFile buffer pre-allocation#29207
Conversation
When a file Blob is sliced to a length near Blob.max_size (maxInt(u52)) and then read, computing the initial buffer capacity as size + 16 overflows u52 and panics in debug builds. Use saturating add so the allocation attempt proceeds (and fails with OOM, which is already handled) instead of crashing.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdjusted buffer capacity sizing in file-reading code to use saturating addition for trailing-space allocations, and added a POSIX-only test that verifies reading a slice near Blob.max_size from Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 `@src/bun.js/webcore/blob/read_file.zig`:
- Around line 387-390: The allocation catch in the ArrayListUnmanaged
initCapacity path currently only sets this.errno and calls this.onFinish(), so
then() won't reject; update the catch in the non-Windows branch to also set
this.system_error (e.g., assign the allocation error or a mapped OOM
system_error to this.system_error) before calling this.onFinish() and returning,
ensuring code paths in the read handler (the this.buffer init, this.errno,
this.system_error, onFinish(), and then() rejection behavior) correctly
propagate OOM to the caller.
🪄 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: 77ca85e6-521a-4421-80b9-a1b2dd70f1c1
📒 Files selected for processing (2)
src/bun.js/webcore/blob/read_file.zigtest/js/bun/util/bun-file-read.test.ts
| this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, this.size +| 16) catch |err| { | ||
| this.errno = err; | ||
| this.onFinish(); | ||
| return; |
There was a problem hiding this comment.
Propagate OOM to system_error on the non-Windows path.
At Line 387, the catch path sets this.errno but not this.system_error; then() rejects only when system_error exists, so allocation failure can surface as a successful empty read instead of an error.
Suggested fix
- 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.onFinish();
- return;
- };
+ 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;
+ if (err == error.OutOfMemory) {
+ this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
+ }
+ this.onFinish();
+ return;
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/webcore/blob/read_file.zig` around lines 387 - 390, The allocation
catch in the ArrayListUnmanaged initCapacity path currently only sets this.errno
and calls this.onFinish(), so then() won't reject; update the catch in the
non-Windows branch to also set this.system_error (e.g., assign the allocation
error or a mapped OOM system_error to this.system_error) before calling
this.onFinish() and returning, ensuring code paths in the read handler (the
this.buffer init, this.errno, this.system_error, onFinish(), and then()
rejection behavior) correctly propagate OOM to the caller.
|
Closing as duplicate of #28849 which has the same fix plus proper Note for whoever merges #28849: the Windows |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // 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, this.size +| 16) catch |err| { | ||
| this.errno = err; | ||
| this.onFinish(); | ||
| return; |
There was a problem hiding this comment.
🔴 In ReadFile.runAsyncWithFD, the OOM catch block sets only this.errno but leaves this.system_error null; since then() dispatches errors solely via system_error, an OOM during buffer pre-allocation silently resolves the JS promise with an empty string instead of rejecting. Add this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError(); alongside the existing this.errno = err; at line 388.
Extended reasoning...
What the bug is and how it manifests
In ReadFile.runAsyncWithFD (the POSIX/non-UV path), when initCapacity fails with OutOfMemory (now reliably triggered by the PR's saturating-add fix passing maxInt(u52) as the capacity), the catch block executes:
this.errno = err;
this.onFinish();
return;It sets this.errno but leaves this.system_error = null. The then() dispatcher — the only path that converts the task result into a JS callback — checks exclusively this.system_error to decide whether to call the callback with an error:
const system_error = this.system_error;
// ...
if (system_error) |err| {
cb(cb_ctx, ReadFileResultType{ .err = err });
return;
}
cb(cb_ctx, .{ .result = .{ .buf = buf, .total_size = total_size, .is_temporary = true } });Because system_error is null, then() falls through to the success path, calling cb with buf = this.buffer.items which is an empty slice — the allocation never succeeded. The JS promise therefore resolves with "" instead of rejecting.
The specific code path that triggers it
- JS calls
Bun.file("/dev/zero").slice(0, 4503599627370490).text() resolveSizeAndLastModifiedsetsthis.size = this.max_length(≈maxInt(u52))runAsyncWithFDreachesinitCapacity(bun.default_allocator, this.size +| 16)— after the PR fix, this ismaxInt(u52)initCapacityreturnserror.OutOfMemory- Only
this.errno = erris set;this.system_errorstays null onFinish()→then()→ success callback with empty buffer
Why existing code doesn't prevent it
The then() function never checks this.errno at all; it is solely gated on this.system_error. This is an oversight: every other OOM handler in the same file (ReadFileUV lines ~702–703, ~731–732, ~764–765, ~791–792, and the ensureUnusedCapacity path) correctly sets both this.errno and this.system_error. Only this one catch block was left incomplete.
What the impact would be
A caller reading a large Blob on POSIX (e.g., a sliced /dev/zero) will receive an empty string from .text() / .arrayBuffer() / etc. without any error indication. This is silent data loss/corruption: code that checks the result value rather than expecting a rejection will silently process an empty string as if it were the file's contents.
How to fix it
Add the missing line inside the catch block:
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(); // <-- add this
this.onFinish();
return;
};Step-by-step proof
Bun.file("/dev/zero").slice(0, 4503599627370490).text()is awaited in JS.ReadFile.createWithCtxallocates aReadFilewithmax_length = 4503599627370490(~maxInt(u52)).resolveSizeAndLastModified:/dev/zerois not a regular file,stat.size == 0, sothis.size = this.max_length=4503599627370490.runAsyncWithFD:this.size != Blob.max_size(they happen to be equal only at exactmaxInt(u52), but after saturating add the capacity request is still enormous) →initCapacity(allocator, 4503599627370507)is called.- The allocator returns
error.OutOfMemory. The catch block setsthis.errno = error.OutOfMemoryonly. onFinish()is called; eventuallythen()runs on the JS thread.- In
then():this.store != null→ skip first two branches.system_error = this.system_error= null.if (system_error) |err|is false.cbis called with.result = .{ .buf = &[]{}, .total_size = 0, .is_temporary = true }. - JS promise resolves with
""— no error thrown, no rejection.
1 participant
What does this PR do?
Fixes an integer overflow panic in
ReadFile.runAsyncWithFD(and the equivalent spot inReadFileUV) when computing the initial buffer capacity.ReadFile.sizeis au52. When a fileBlobbacked by a non-regular file (e.g./dev/zero) is sliced to a length nearBlob.max_size(maxInt(u52)) and then read,resolveSizeAndLastModifiedsetsthis.size = this.max_length, and thenthis.size + 16overflows and panics in debug builds.Minimal repro (before this change, panics with
integer overflow):The fix uses a saturating add (
+|) so the value caps atmaxInt(u52); the subsequent allocation then fails withOutOfMemory, which is already handled by the existingcatchblock.How did you verify your code works?
test/js/bun/util/bun-file-read.test.tsthat spawns a subprocess and asserts it exits with code 0 / no signal. Test fails (timeout, subprocess panics and hangs) on the unfixed binary and passes on the fixed binary.Fuzzilli fingerprint:
4540f8005402e562