Repository navigation
fix(ext/node): read byob views at offset 0 in FileHandle.readableWebStream - #19
Merged
Merged
Conversation
…tream
The byob pull passed the view's byteOffset as the read offset. That
offset is relative to the view, so a view that starts after byte 0 of
its ArrayBuffer failed with ERR_OUT_OF_RANGE ("It must be <= 0"). Pass
0, as Node does since nodejs/node#58842 (v26.10.0).
Add a unit test that reads with a byob reader into 100-byte DataViews at
increasing offsets of one ArrayBuffer. It fails before this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FileHandle.readableWebStream()with a byob reader failed when the reader read into a view with abyteOffsetgreater than 0. The first read worked. The next read threwRangeError [ERR_OUT_OF_RANGE]: The value of "length" is out of range. It must be <= 0. Received 100.Root cause. The
pullcallback inext/node/polyfills/internal/fs/handle.tspassed the view'sbyteOffsetas theoffsetargument ofFileHandle.read. That argument is relative to the view, not to the view'sArrayBuffer. Sofs.readcheckedbyteOffset + lengthagainstview.byteLength, and the check failed.Fix. Pass
0, as Node does since nodejs/node#58842 (commit 2c0ccc0067d, first in v26.10.0). TheTypedArrayPrototypeGetByteOffsetprimordial has no other user in the file, so this change removes its import.Node v24 and Node before v26.10.0 have the same defect. The fork now reads correctly on every Node lane. No test depends on the old error.
Tests
[node/fs filehandle.readableWebStream] byob reads into views at a byteOffset of one ArrayBuffer. It reads with a byob reader into 100-byteDataViews at increasing offsets of oneArrayBuffer, the same as the new block in Node'stest-filehandle-readablestream.js.RangeErrorabove.tests/unit_node/_fs/: 295 passed, 0 failed, 9 ignored.test/parallel/test-filehandle-readablestream.js, run through thenodeshim: passes. The build before the fix fails with the sameRangeError.dprint checkon the changed files andtools/lint.js --js: pass.Follow-up (not in this PR)
node_compatsuite is at 26.5.1, so its copy oftest-filehandle-readablestream.jsdoes not have the new block yet. Moving the suite to v26.10.0 adds it.