Repository navigation
fix(ext/node): convert Buffer string write arguments once - #33
Merged
Merged
Conversation
`buf.asciiWrite()`, `buf.latin1Write()`, and `buf.utf8Write()` compared the raw `offset` and `length` values and then used them again, so a `valueOf()` method ran more than once. The methods also did not do the native steps of Node's StringWrite(): a NaN offset was not treated as 0, fractional values were not truncated, `utf8Write()` with a zero length wrote the full string, and string-number arguments gave wrong results. A shared stringWrite() helper now converts each argument once, as Node.js 24.21 and later do (nodejs/node#65043). It then truncates each value and clamps the length to the bytes that remain, as the native write does. It reads the byte length again after conversion, because user code can resize or detach the buffer. The bounds checks differ between Node.js releases, so a new BufferStringWriteBoundsPolicy lets an embedder select the contract: - TruncateThenClamp: Node.js 20 and 22.0 through 22.8. - CheckThenClamp: Node.js 22.9 and later 22.x (nodejs/node#54310, nodejs/node#54524). - CheckThenReject: Node.js 23 and later (nodejs/node#54588). This is the default.
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
buf.asciiWrite(),buf.latin1Write(), andbuf.utf8Write()compared the rawoffsetandlengthvalues and then used them again. Thus avalueOf()method ran more than once. Node.js 24.21 and 26.8 convert each argument once (nodejs/node#65043). Thetest-buffer-write.jsfixture of those releases asserts this.The methods also skipped the native steps of Node's
StringWrite(). These bugs occurred on all Node.js versions:utf8Write('abcdef', 0, 0)buf.write('abc', 0, 0)asciiWrite('abcdef', NaN)utf8Write('abcdef', '1', '2')Change
stringWrite()helper converts each argument once. It truncates each value and clamps the length, as the native write does. It reads the byte length again after conversion, because user code can resize or detach the buffer.BufferStringWriteBoundsPolicylets an embedder select the contract:TruncateThenClamp: Node.js 20 and 22.0 through 22.8. A negative value givesERR_OUT_OF_RANGE, and a long length is clamped.CheckThenClamp: Node.js 22.9 and later 22.x (buffer: optimize writing short strings nodejs/node#54310, buffer: truncate instead of throw when writing beyond buffer nodejs/node#54524). A long length is clamped.CheckThenReject: Node.js 23 and later (buffer: throw when writing beyond buffer nodejs/node#54588). A long length givesERR_BUFFER_OUT_OF_BOUNDS. This is the default inworker.rs,web_worker.rs, andsnapshot_info.rs.hexIndexOutOfRange()is nowindexOutOfRange(), because the write path uses it too.Verification
unit_node buffer_test: pass. The two new tests pass. Their expected values match Node.js 26.7, except for the singlevalueOf()call and the BigInt case, which are the #65043 contract.node_compat test-buffer-write: 6 of 6 pass.cargo test -p deno_node --lib policy: 6 of 6 pass, including the new policy test.cargo fmt --check, dprint, andtools/lint.js --js: clean.test-buffer-write.jsfixtures from Nimbus: all assertions pass.