node:fs: fix FileHandle.readableWebStream BYOB read into a subarray view - #35355
node:fs: fix FileHandle.readableWebStream BYOB read into a subarray view#35355robobun wants to merge 2 commits into
Conversation
The pull() handler was passing view.byteOffset as the offset argument to handle.read(). That argument is the write offset within the view itself, so a BYOB reader supplying a view with a nonzero byteOffset would throw ERR_OUT_OF_RANGE because offset + length exceeded the view's byteLength. The underlying read already writes at index 0 of the typed-array view, which is view.byteOffset in the backing ArrayBuffer.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 11:44 PM PT - Jul 23rd, 2026
❌ @robobun, your commit 7712f66 has 3 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35355That installs a local version of the PR into your bun-35355 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #33694. That PR was a full rewrite of |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-token fix with a targeted regression test.
What was reviewed
- Confirmed
FileHandle.read'soffsetis the write index within the view, so passingview.byteOffsetdouble-counted the subarray offset;0is correct. - Default-reader path unaffected (
autoAllocateChunkSizeallocates views withbyteOffset === 0), so no behavior change there. - Test uses
tempDir/await using, assertsbyteOffset/byteLength/content on the returned view, and would throwERR_OUT_OF_RANGEwithout the fix.
Extended reasoning...
Overview
One-line change in src/js/node/fs.promises.ts: the readableWebStream() pull handler now calls handle.read(view, 0, view.byteLength) instead of handle.read(view, view.byteOffset, view.byteLength). A new test in test/js/node/fs/fs.test.ts exercises a BYOB reader with a Uint8Array(new ArrayBuffer(1000), 500, 100) subarray view.
Correctness
FileHandle#read(buffer, offset, length) treats offset as the index within the passed view at which to begin writing. The typed-array view's underlying data pointer already accounts for its own byteOffset into the backing ArrayBuffer, so passing view.byteOffset as offset shifts the write target a second time and trips the offset + length <= view.byteLength bounds check whenever byteOffset > 0. Writing at index 0 of the view is the correct semantics for a BYOB request. The default (non-BYOB) path is unaffected because autoAllocateChunkSize always produces a fresh view with byteOffset === 0. The PR notes Node has the identical bug upstream; the semantics are unambiguous, so diverging from a Node bug is appropriate here.
Security risks
None. This is a read-side offset correction inside a bounds-checked path; no new inputs, no relaxed validation.
Level of scrutiny
Low. Single-token change to a JS builtin, semantics are clear from FileHandle.read's own implementation in the same file (validateInteger(offset, "offset", 0) followed by read(fd, buffer, offset, length, position)). No native/memory-safety code touched.
Other factors
The test is placed alongside the existing FileHandle#readableWebStream test, follows harness conventions (tempDir, await using, Buffer.alloc(n, fill).toString()), and asserts exact values (byteOffset === 500, byteLength === 100, content equals 100 'X's). It fails without the fix (throws ERR_OUT_OF_RANGE), satisfying the fails-for-the-right-reason requirement. PR description reports the full fs.test.ts suite and both Node parallel filehandle tests remain green.
There was a problem hiding this comment.
LGTM — one-line fix passing 0 instead of view.byteOffset to handle.read() in the BYOB pull handler.
What was reviewed:
- Confirmed
FileHandle.read'soffsetparam is the write index within the view;view.byteOffsetdouble-counted the backing-buffer offset for subarray views. - Default-reader path unaffected (autoAllocateChunkSize views always have
byteOffset === 0);respond(bytesRead)still correct since the write starts at view index 0. - Test uses
tempDir+await using, asserts exact bytes/offset/length, and would throwERR_OUT_OF_RANGEon the old code — cleanup traced throughclose→onFileHandleClose→ondoneso the stream's ref is released.
Extended reasoning...
Overview
Single-character semantic fix in src/js/node/fs.promises.ts: the readableWebStream() pull handler was calling handle.read(view, view.byteOffset, view.byteLength). FileHandle.read's second argument is the offset within the passed view to start writing at, but view.byteOffset is the view's offset into its backing ArrayBuffer. For a BYOB reader that supplies a subarray (new Uint8Array(buf, 500, 100)), this passed offset=500, length=100 into a 100-byte view and tripped the native ERR_OUT_OF_RANGE bounds check. Changing to 0 writes at the start of the view, which already maps to the correct byte in the underlying buffer. A new test in test/js/node/fs/fs.test.ts exercises exactly this case.
Security risks
None. Pure JS, no new inputs, no bounds relaxed — the native read still validates offset + length <= view.byteLength.
Level of scrutiny
Low. The diff is a one-token change whose semantics are unambiguous once you read FileHandle.read a few lines up in the same file: offset is validated as an integer ≥ 0 and passed straight to the native read(fd, buffer, offset, length, position), which indexes into the typed-array view. The default-reader path was never broken because autoAllocateChunkSize always allocates a fresh view with byteOffset === 0, so view.byteOffset and 0 were coincidentally equal there. controller.byobRequest.respond(bytesRead) remains correct because bytes are written starting at index 0 of the request view.
Other factors
- Test follows harness conventions (
tempDir,await using fh,Buffer.alloc(n, fill)), asserts exact byte content and the returned view'sbyteOffset/byteLength, and reads 100 bytes from a 200-byte regular file so short reads aren't a flake vector. - Traced cleanup:
reader.releaseLock()thenawait using fh→close()emits'close'→onFileHandleClose→ondone()→kUnref, so the stream's extra ref is released and the fd closes. - PR description notes Node v26.3.0 has the same bug upstream; deviating from Node here is correct because the semantics of
FileHandle.read's offset are documented and the current behavior is a hard failure, not a compat quirk. - Duplicate-PR bot flagged #33694; author clarified that #34505 already merged the rewrite with this bug intact and #33694 is stale — this is the targeted follow-up on main.
- No prior human or claude[bot] reviews; nothing outstanding.
|
The diff here is green: the new The remaining CI red on build 79261 is unrelated to this one-token JS builtin change:
Ready for review. |
1 participant
What does this PR do?
FileHandle.readableWebStream()'spull()handler callshandle.read(view, offset, length)whereoffsetis the index withinviewat which to start writing. It was passingview.byteOffset, which is the view's offset within its backingArrayBuffer. For any BYOB reader that supplies a subarray view (nonzerobyteOffset), this double-counts the offset and throwsERR_OUT_OF_RANGEbecauseoffset + length > view.byteLength.Repro
Fix
Pass
0as the offset:handle.read(view, 0, view.byteLength). The native read writes at index 0 of the typed-array view, which already corresponds toview.byteOffsetin the underlying buffer.Node currently has the same bug (v26.3.0,
lib/internal/fs/promises.js:362), but the semantics here are unambiguous:controller.byobRequest.viewis the consumer's view, andFileHandle.read'soffsetis the write index within that view. The default reader path was unaffected only becauseautoAllocateChunkSizealways creates a view withbyteOffset === 0.How did you verify your code works?
New test in
test/js/node/fs/fs.test.tspasses aUint8Array(new ArrayBuffer(1000), 500, 100)to a BYOB reader and asserts the result lands atbyteOffset: 500with the correct bytes. ThrowsERR_OUT_OF_RANGEwithout the fix, passes with it. Fullfs.test.ts(430 pass) andtest-filehandle-readablestream.js/test-filehandle-autoclose.mjsremain green.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts