Fix integer overflow in ReadFile buffer capacity calculation#29205
Fix integer overflow in ReadFile buffer capacity calculation#29205robobun wants to merge 2 commits into
Conversation
When reading a non-regular file blob (e.g. /dev/null) sliced to a length near maxInt(u52), computing the initial buffer capacity as size + 16 overflowed the u52 SizeType. Use saturating addition so the allocation attempt fails gracefully instead of panicking.
|
Caution Review failedPull request was closed or merged during review WalkthroughA capacity-initialization expression in the blob read operation was changed to use the Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #28849. |
![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.
🔴 When initCapacity fails with OOM in runAsyncWithFD, the catch block (line 388) only sets this.errno but leaves this.system_error null; since then() only checks system_error to decide whether to propagate an error, the OOM is silently swallowed and the caller receives an empty ArrayBuffer as success instead of a rejected promise. Before this PR the catch path was unreachable (the + 16 overflow would panic first), but the +| fix makes it reachable for the first time while exposing this pre-existing incomplete error handling — and the new test inadvertently masks the bug by accepting an empty ArrayBuffer as a valid outcome alongside an Error.
Extended reasoning...
What the bug is and how it manifests
In ReadFile.runAsyncWithFD, when std.ArrayListUnmanaged(u8).initCapacity fails with error.OutOfMemory, the catch block at lines 387–390 sets this.errno = err but never sets this.system_error. The then() function (lines 239–267) exclusively uses this.system_error to decide whether to return an error to the JS caller:
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 after an OOM, then() takes the success path and invokes the callback with buf = this.buffer.items — an empty slice — delivering an empty ArrayBuffer to the JS caller as if the read succeeded.
The specific code path that triggers it
- A
Bun.file("/dev/null").slice(0, N)where N is nearmaxInt(u52)is awaited asarrayBuffer(). resolveSizeAndLastModifiedsetsthis.size = this.max_length(≈ 4.5 PB) becausestat.size == 0andcould_block == true.this.size +| 16saturates tomaxInt(u52), soinitCapacityattempts to allocate ~4 petabytes.- The allocator returns
error.OutOfMemory; the catch block sets onlythis.errno. then()seessystem_error == nulland callscbwith an empty buffer → success.
Why existing code doesn't prevent it
Before this PR, step 3 used this.size + 16 (wrapping/overflowing addition), which panicked at runtime before the allocator was ever called, making the catch block dead code. The +| fix is correct in intent but exposes the pre-existing incomplete error handling in the catch block. Every other OOM path in both ReadFile and ReadFileUV correctly sets both this.errno AND this.system_error (e.g., ReadFileUV.onFileInitialStat lines ~701-705, queueRead lines ~730-733, ~763-766). The initCapacity catch block is the sole exception.
Impact
An application reading a sliced non-regular file near max_size that would OOM the process receives a resolved promise with an empty ArrayBuffer instead of a rejection. This silently hides a critical resource-exhaustion condition, making it appear the file was empty rather than signalling a system error.
How to fix it
Mirror what ReadFileUV.onFileInitialStat does on allocation failure:
this.buffer = std.ArrayListUnmanaged(u8).initCapacity(bun.default_allocator, this.size +| 16) catch {
this.errno = error.OutOfMemory;
this.system_error = bun.sys.Error.fromCode(bun.sys.E.NOMEM, .read).toSystemError();
this.onFinish();
return;
};Step-by-step proof
Bun.file("/dev/null").slice(0, 4503599627370490)→this.max_length ≈ maxInt(u52).- fstat returns size=0, mode=character-device →
this.size = this.max_length. initCapacity(allocator, maxInt(u52) +| 16)→ OOM → catch fires.- catch:
this.errno = error.OutOfMemory;this.system_errorstaysnull. onFinish()→ eventuallythen()is called.then():system_error == null→cb(cb_ctx, .{ .result = .{ .buf = &[][0]u8{}, ... } }).- JS side receives a resolved promise with
ArrayBuffer { byteLength: 0 }— no error thrown. - The new test's
if (result instanceof ArrayBuffer) { expect(result.byteLength).toBe(0); }branch passes, masking the incorrect behavior.
1 participant
Fuzzer fingerprint:
a83967f18cb850d8What
Reading a non-regular file blob (e.g.
/dev/null) that has been sliced to a length nearmaxInt(u52)panicked with an integer overflow on a thread pool worker.Why
In
ReadFile.runAsyncWithFD, when the underlying file is not a regular file andstat.size == 0,this.sizeis set tothis.max_length(the slice length supplied by the user). The initial read buffer is then pre-allocated with capacitythis.size + 16. Sincesizeis au52andmax_lengthcan be any value up toBlob.max_size - 1, adding 16 overflows.Fix
Use saturating addition (
+|) for the capacity hint. If the resulting allocation is too large it is handled by the existingcatchpath instead of crashing the process.