Buffer: match Node semantics for raw <enc>Slice / <enc>Write bindings - #33530
Conversation
The raw Buffer.prototype.<enc>Slice / <enc>Write prototype methods were routed through the strict validator used by the documented toString() / write() wrappers, so calls that succeed on Node threw on Bun: buf.hexSlice(pastEnd) -> "" on Node, RangeError on Bun buf.hexWrite(str, off, hugeLen) -> clamped on Node, RangeError on Bun Mirror Node's node_buffer.cc bindings instead: - StringSlice short-circuits to "" when start >= end, before the end <= length range check. - base64/base64url/hex/ucs2/utf16le Write clamp length to the space left and report a negative index as ERR_OUT_OF_RANGE. utf8/latin1/ ascii Write stay on the strict wrapper Node keeps for them. - parseArrayIndex follows v8::Value::IntegerValue (NaN -> 0, saturating) instead of truncateDoubleToInt64, which mapped NaN/Infinity to INT64_MIN and made them throw. Also drop the length/offset assertions in jsBufferToString: a valueOf() that shrinks a resizable ArrayBuffer during index coercion leaves the snapshotted range stale, which aborted debug builds. The existing clamp below already handles it.
|
Warning Review limit reached
Next review available in: 39 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)
WalkthroughThis PR updates ChangesBuffer slice/write parity tests
Sequence Diagram(s)Not applicable; changes are limited to test assertions without new sequential runtime interactions. Estimated code review effort: 2/5 (Low-Medium) Related issues: None provided. Related PRs: None provided. Suggested labels: test, buffer, node-compat Suggested reviewers: None provided. 🐰 A hop, a skip, through slices tight, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The raw Buffer.prototype.<enc>Slice / <enc>Write prototype methods were routed through the strict validator used by the documented toString() / write() wrappers, so calls that succeed on Node threw on Bun: buf.hexSlice(pastEnd) -> "" on Node, RangeError on Bun buf.hexWrite(str, off, hugeLen) -> clamped on Node, RangeError on Bun Mirror Node's node_buffer.cc bindings instead: - StringSlice short-circuits to "" when start >= end, before the end <= length range check. - base64/base64url/hex/ucs2/utf16le Write clamp length to the space left and report a negative index as ERR_OUT_OF_RANGE. utf8/latin1/ ascii Write stay on the strict wrapper Node keeps for them. - parseArrayIndex follows v8::Value::IntegerValue (NaN -> 0, saturating) instead of truncateDoubleToInt64, which mapped NaN/Infinity to INT64_MIN and made them throw. Also drop the length/offset assertions in jsBufferToString: a valueOf() that shrinks a resizable ArrayBuffer during index coercion leaves the snapshotted range stale, which aborted debug builds. The existing clamp below already handles it.
|
Good catch on the first thread: the Also applied the stderr-surfacing suggestion on the resizable-shrink spawn test so an abort diagnostic lands in the diff on regression. |
There was a problem hiding this comment.
No issues found on this pass now that the C++ is in — but this adds new bounds/detach-recheck logic in JSBuffer.cpp and loosens validation on a hot memory-write path, so it's worth a human set of eyes on the StringWriteWithEncoding clamping and the removed jsBufferToString assertions.
Extended reasoning...
Overview
This PR aligns Bun's raw Buffer.prototype.<enc>Slice / <enc>Write bindings with Node's semantics. In src/jsc/bindings/JSBuffer.cpp it: (1) makes SliceWithEncoding return "" on start >= end before the range check, (2) adds a new ~55-line StringWriteWithEncoding template (clamping length to remaining space, re-checking detach after each coercion) and rewires utf16le/base64/base64url/hex Write to it, (3) replaces truncateDoubleToInt64 in parseArrayIndex with a saturating toIntegerValue (NaN→0, ±Inf saturate), (4) removes three debug ASSERTs in jsBufferToString in favor of the existing runtime clamp, and (5) tweaks the strict wrapper's default-length computation when offset is NaN. test/js/node/buffer.test.js gains ~200 lines of parity tests and updates two latin1Slice assertions that encoded the old (wrong) throwing behavior.
My previous review flagged that the C++ was missing entirely; the author pushed it in 3b85955 and applied the stderr-surfacing nit. Both prior threads are resolved. The bug-hunting pass on the current revision found nothing.
Security risks
The new StringWriteWithEncoding does user-controlled offset/length arithmetic feeding into writeToBuffer (which writes into the backing store at offset for up to maxLength bytes). The clamping looks correct — offset > byteLength throws, then maxLength = min(byteLength - offset, maxLength) after re-reading byteLength post-coercion — and detach is re-checked after each parseArrayIndex. The size_t→uint32_t narrowing into writeToBuffer is pre-existing (shared with the strict path). Removing the jsBufferToString assertions is justified by the immediately-following clamp, but it does mean a future caller that skips the clamp loses the debug tripwire. Nothing jumps out as exploitable, but this is precisely the class of code (bounds arithmetic on adversarial input, re-entrancy via valueOf(), resizable-buffer shrink) that CLAUDE.md flags as the most-blocked review category.
Level of scrutiny
High. Buffer write paths are among the hottest and most memory-safety-sensitive code in the runtime, and this change loosens validation (calls that previously threw now write). The implementation is careful and well-commented against Node source lines, and the test coverage is thorough (per-encoding matrix, NaN/Infinity/negative, resizable-shrink-during-coercion spawn test, wrapper-unchanged regression checks). But new C++ bounds logic in JSBuffer.cpp is not something I should wave through without a human reviewer.
Other factors
Test coverage is strong and the author verified byte-for-byte against Node v26.3.0. No CODEOWNERS entry matches this path. CI build #69269 was still running at last timeline update. The parseArrayIndex signature change dropped the per-call-site error message in favor of a fixed "Index out of range" — that matches Node but is a user-visible message change a human might want to confirm.
… ran JS The offset/length coercions only run user-overridable code (valueOf / Symbol.toPrimitive / toString) for object arguments. Gate the isDetached re-check on that so the common path (string + numeric args) skips it, and re-read byteLength only after a coercion that could have resized the view.
|
CI status: the diff is green on its own terms, the red lanes are unrelated flake. This PR touches only Across the last two CI runs the only red lane was
The set changing run-to-run with no overlap to the changed files is the signature of pre-existing flake on that lane. I pushed one |
2 participants
What
The raw
Buffer.prototype.<enc>Slice/<enc>Writeprototype methods (hexSlice,utf8Write,ucs2Write,base64Write, ...) are Node's undocumented but long-standing raw C++ bindings, which Bun also exposes. Bun routed them through the strict argument validator used by the documentedtoString()/write()wrappers, so calls that succeed on Node threw on Bun.The documented wrappers (
buf.toString(enc, start, end),buf.write(str, offset, length, enc)) already matched Node and are unchanged. This affects code that calls the raw methods directly, which a few performance-sensitive libraries do.Cause
Two divergences from Node's
node_buffer.cc:StringSlicereturns""whenstart >= end, before theend <= lengthrange check. Bun did the range check first, so anystartpast the end threw.base64/base64url/hex/ucs2Writeare still Node's raw binding, which clampslengthto the space left in the buffer. Node only movedutf8/latin1/asciiWriteonto a strict JS wrapper that throwsERR_BUFFER_OUT_OF_BOUNDS; Bun applied that wrapper's strictness to all seven.Separately,
parseArrayIndexused WTF'struncateDoubleToInt64, which mapsNaNandInfinitytoINT64_MIN. That made them look negative and throw, where Node'sv8::Value::IntegerValuetreatsNaNas 0 and saturates at the int64 bounds.Fix
In
src/jsc/bindings/JSBuffer.cpp:SliceWithEncodingshort-circuits to""onstart >= endbefore the range check, matching Node'sStringSlice.StringWriteWithEncoding, Node's clampingStringWrite: clamplengthtobyteLength - offset, return 0 when no room, report a negative offset/length asERR_OUT_OF_RANGE("Index out of range").base64/base64url/hex/ucs2/utf16leWriteuse it;utf8/latin1/asciikeep the strict wrapper.parseArrayIndexnow followsv8::Value::IntegerValue(NaN-> 0, saturating).While verifying hostile coercions, found that a
valueOf()which shrinks a resizableArrayBufferduring index coercion trips a debug assertion injsBufferToString(callers snapshotbyteLengthbefore the coercion runs user JS). The documentedtoString()wrapper hits it too. The clamp right below the assertions already handles the stale range correctly in release, so the assertions were wrong; removed them.Verification
Every expectation in the new tests was checked byte-for-byte against a real Node v26.3.0 binary (including pulling Node's own
internal/buffer.jssource out of the binary to confirm which encodings use which path). New tests intest/js/node/buffer.test.jsfail on unfixed Bun and pass with the fix;latin1Sliceassertions that encoded the old throwing behavior were updated to Node's actual output. The fullbuffer.test.js(611 pass) and alltest-buffer-*.jsNode-ported suites pass.