Repository navigation
node:buffer: fix six Node compatibility divergences - #32784
Conversation
WalkthroughUTF-16LE byte-length sizing now uses replacement semantics for unpaired surrogates. Buffer swap methods throw a new invalid-buffer-size code. Buffer fill, BigInt offset checks, and UCS2/empty-buffer search behavior are updated, with matching Node-style tests. ChangesBuffer UTF-16 and validation changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:17 PM PT - Jun 26th, 2026
❌ @robobun, your commit b87f06f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32784That installs a local version of the PR into your bun-32784 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Regarding the duplicate flag above: #32383 is not a duplicate, and it does not change They are related, though: the Left a note on #32383 with the details so a maintainer can decide which shape to land. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The ErrorCode.ts/ErrorCode.rs append-only fix in 497ae87 looks correct (both tables now end with ERR_INVALID_BUFFER_SIZE at index 328, COUNT=329). Deferring to a human for the rest — six independent behavioral changes across C++/Rust/FFI in hot Buffer paths, plus the overlap with #32383, warrant maintainer eyes.
Extended reasoning...
Overview
This PR fixes six separate node:buffer Node-compat divergences, touching 8 files across three languages: C++ (JSBuffer.cpp — fill, indexOf/lastIndexOf, swap16/32/64, validateOffsetBigInt64), Rust (bun_core/lib.rs, simdutf_sys/simdutf.rs, runtime/webcore/encoding.rs, jsc/ErrorCode.rs), the simdutf C++ FFI shim, the ErrorCode.ts codegen source, and ~130 lines of new tests in buffer.test.js.
Since my previous review, commit 497ae87 addressed the blocking issue I raised: ERR_INVALID_BUFFER_SIZE is now appended to the tail of ErrorCode.ts (not inserted alphabetically), and src/jsc/ErrorCode.rs was updated in lockstep — discriminant 328, COUNT bumped to 329, the ERR_-prefixed alias added, and the CODE_STR table extended. The Rust↔C++ index alignment is preserved.
Security risks
None identified. The changes are Node-compat behavioral corrections (error codes/messages, search alignment, length computation). The byteLength fix only ever increases the reported length, so it cannot introduce under-allocation; the PR description notes the encoder write path was already bounds-checked. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
This warrants human review despite no bugs found this round. Reasons: (1) six logically independent fixes bundled together, each with its own edge cases; (2) element_length_utf16_into_utf8 is a shared helper feeding both Buffer.byteLength and TextEncoder — its semantics change globally; (3) the indexOf dispatcher removes four byteLength == 0 early-returns, relying on computeIndexOfRange and the per-type helpers to handle empty haystacks safely (including with possibly-null typedVector); (4) lastIndexOf16 reinterprets uint8_t* as uint16_t* (mirrors existing indexOf16, but worth a glance for alignment assumptions); (5) the fill() change swaps validateNumber+toLength for validateInteger with an out-param, which changes coercion semantics for non-number inputs too.
Other factors
Test coverage is thorough — every fix has a dedicated test block verified against Node v26.3.0, and the PR description reports the relevant upstream Node test files and Bun's encoding suite still pass. There is an acknowledged overlap with #32383 (same root cause, different fix shape) that a maintainer should resolve before landing either.
- Buffer.byteLength(s, "utf8") under-counted strings containing unpaired
surrogates: it used simdutf's non-validating UTF-16 length, which
charges every surrogate code unit 2 bytes, while the encoder writes
U+FFFD (3 bytes). So byteLength disagreed with Buffer.from(s).length,
an under-allocation footgun for code that sizes a buffer with one and
writes with the other. Expose simdutf's
utf8_length_from_utf16le_with_replacement and use it in
element_length_utf16_into_utf8 so the length helper matches the
encoder for ill-formed UTF-16.
- Buffer#lastIndexOf(value, offset, "utf16le") did a raw byte search.
Node searches in uint16_t units (a match can only start on an even
byte offset); the forward indexOf already did. Add a uint16 backward
search and route the ucs2 + last case through it.
- An empty haystack returned -1 before the empty-needle logic could run,
so Buffer.alloc(0).indexOf(Buffer.alloc(0)) was -1 instead of 0.
Remove the short-circuits; computeIndexOfRange already handles both
the empty haystack and the empty needle correctly.
- Buffer#fill() validated offset and end with validateNumber (no integer
check) and then truncated them with toLength, so fill("a", 1.5)
silently filled from 1. Node uses validateOffset (= validateInteger)
and throws ERR_OUT_OF_RANGE "It must be an integer".
- Buffer#swap16/32/64 threw with code ERR_OUT_OF_RANGE. Node uses
ERR_INVALID_BUFFER_SIZE, which did not exist in ErrorCode.ts; add it.
- writeBig[U]Int64LE/BE with a negative offset threw
ERR_BUFFER_OUT_OF_BOUNDS. Node's boundsError reports it as
ERR_OUT_OF_RANGE ">= 0 and <= max", the same as a too-large offset.
The `errors` array in ErrorCode.ts is index-aligned with the hand-maintained Rust mirror in src/jsc/ErrorCode.rs, and the C++ ErrorCode enum plus its errors[] table are regenerated from array position. Inserting the new code alphabetically shifted every code past the insertion point by one, so any error thrown from Rust with a discriminant at or above it surfaced with the previous entry's constructor, code, and name. For example, a Rust-thrown ERR_ZLIB_INITIALIZATION_FAILED became a TypeError with code ERR_WORKER_UNSUPPORTED_OPERATION, which is what made test-zlib-brotli.js and test-zlib-zstd.js fail in CI. Move the entry to the documented append-only tail and register it in the Rust mirror: the discriminant const, the ERR_* alias, the CODE_STR table, and COUNT.
497ae87 to
8d02ea4
Compare
…tale comments validateOffsetBigInt64's integer-ness check used std::fmod, which is NaN for +-Infinity, so those reported "an integer" where Node's boundsError (whose test is Math.floor(value) !== value) reports the ">= 0 and <= max" range. Use std::floor to match, and range-check the double BEFORE truncating so a non-finite value never reaches truncateDoubleToUint64; the now-redundant post-truncation check is folded into it. Also update two comments in fillBody that still named validateNumber.
refetchBufferState's doc said a detached buffer is treated as an empty one (both returning -1). This PR decoupled those: a detached haystack still returns -1 but an empty one is handled by computeIndexOfRange and can return 0 for an empty needle, so the equivalence no longer holds. Also delete the leftover unpaired-surrogate TODO above the Latin-1 byteLength helper. It was a misplaced duplicate of the UTF-16 one this PR resolved; surrogates do not exist in Latin-1 input.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/JSBuffer.cpp (2)
1320-1333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the expanded comment blocks.
These updated comments exceed the repo’s 3-line limit; keep only the durable invariant and move detailed Node-trace/history into the PR description. As per coding guidelines, “Keep code comments to 3 lines max.”
Also applies to: 1364-1385, 1704-1707, 2689-2701
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/JSBuffer.cpp` around lines 1320 - 1333, The comment in JSBuffer-related code is too long and exceeds the 3-line limit; trim the expanded explanation down to the durable invariant only and move the Node-trace/history details out of the source. Update the affected comment block near the byteLength handling in JSBuffer.cpp, and apply the same condensation to the other listed comment blocks so they remain concise while preserving the key behavior note.Source: Coding guidelines
1560-1571: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid
uint16_tspans over Buffer bytes.Both UCS2 helpers cast
typedVector()toconst uint16_t*; that assumes 2-byte alignment for arbitrary Buffer storage and is UB on targets that require aligned access. Keep the even-byte search rule, but compare bytes or realign before forming the span. Same fix applies toindexOf16.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/JSBuffer.cpp` around lines 1560 - 1571, The UCS2 search helpers in JSBuffer, including lastIndexOf16 and indexOf16, are creating std::span<const uint16_t> views directly from Buffer bytes, which can violate alignment on some targets. Update these helpers to preserve the even-byte search behavior without casting typedVector()/byte storage to uint16_t pointers; instead compare as bytes or copy/realign before forming any 16-bit view so the search works safely for arbitrary Buffer memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/bindings/JSBuffer.cpp`:
- Around line 1320-1333: The comment in JSBuffer-related code is too long and
exceeds the 3-line limit; trim the expanded explanation down to the durable
invariant only and move the Node-trace/history details out of the source. Update
the affected comment block near the byteLength handling in JSBuffer.cpp, and
apply the same condensation to the other listed comment blocks so they remain
concise while preserving the key behavior note.
- Around line 1560-1571: The UCS2 search helpers in JSBuffer, including
lastIndexOf16 and indexOf16, are creating std::span<const uint16_t> views
directly from Buffer bytes, which can violate alignment on some targets. Update
these helpers to preserve the even-byte search behavior without casting
typedVector()/byte storage to uint16_t pointers; instead compare as bytes or
copy/realign before forming any 16-bit view so the search works safely for
arbitrary Buffer memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fedbf851-9f12-4158-a98b-f9e99b95a6b9
📒 Files selected for processing (3)
src/jsc/bindings/JSBuffer.cppsrc/runtime/webcore/encoding.rstest/js/node/buffer.test.js
💤 Files with no reviewable changes (1)
- src/runtime/webcore/encoding.rs
|
Responding to CodeRabbit's two "outside diff range" findings (these did not create inline threads, so replying here). Comment length. The 4-line comment I added at the
Happy to make both helpers alignment-safe (e.g. |
count16 and count16_z still used simdutf's non-validating UTF-16 length, and their comments claimed it over-estimates for WTF-16. The opposite is true: it charges a lone surrogate 2 bytes, while append16's fallback (to_utf8_alloc) writes U+FFFD, 3 bytes. So the count -> allocate -> append16 contract under-reserved by 1 byte per unpaired surrogate and append16's bounds check returned None (not memory-unsafe, just wrong). Route both through element_length_utf16_into_utf8, which this PR already corrected, and fix the two comments. Requested by @Jarred-Sumner in review.
…ffer The `byteLength < 8` check ran before the offset was examined at all, but Node's checkBounds validates the offset's type first, and its boundsError tests integer-ness before `length < 0`. So on a too-short buffer Bun reported ERR_BUFFER_OUT_OF_BOUNDS for a non-number or fractional offset where Node reports ERR_INVALID_ARG_TYPE (a TypeError) or ERR_OUT_OF_RANGE "an integer". Validate the offset fully first, then the buffer length, then the range. This also merges the int32 and double paths (the separate int32 range check was duplicated) and keeps the too-short buffer ahead of the range message, which Node also does.
|
CI status for whoever reviews this: on build 65193 278 of 280 jobs pass and there are zero test-failure annotations on the whole build. The only failures are two shards of the That identical 120-second artifact-download timeout has hit that one lane on every build of both of my open PRs today (65110, 65116, 65130, 65147, 65158, 65193), always before any test executes, so it is a macOS CI infrastructure issue and unrelated to this change. The The lanes this change actually affects are all green, including the |
Six
node:bufferbehaviors that disagree with Node, found by comparing Bun and Node output over the same inputs. All are exercised by new or strengthened tests intest/js/node/buffer.test.js; every expected value was verified againstnode v26.3.0.Buffer.byteLengthunder-counts unpaired surrogatesBun__encoding__byteLengthUTF16AsUTF8(which carried a// TODO(@190n) handle unpaired surrogates) went throughelement_length_utf16_into_utf8, which called simdutf's non-validatingutf8_length_from_utf16le. That function charges every code unit in0xD800..0xDFFFtwo bytes, half of an assumed 4-byte pair. The encoder replaces each unpaired surrogate with U+FFFD (three bytes), sobyteLengthdisagreed withBuffer.from(s).length. That is an under-allocation footgun for the commonBuffer.allocUnsafe(Buffer.byteLength(s))+buf.write(s)pattern (the write is bounds-checked, so the result is truncation, not a memory error).simdutf already ships the correct function,
utf8_length_from_utf16le_with_replacement(SIMD-accelerated;.countis documented to be valid even when the error field isSURROGATE). It just was not exposed through the FFI shim. This adds the binding and pointselement_length_utf16_into_utf8at it. The new value is always greater than or equal to the old one and exactly matches the encoder's output, so every existing caller that used it as an allocation size or an expected-written length is either unchanged (valid UTF-16) or corrected (lone surrogates). As a side effect,TextEncoder.encodeno longer falls back to a second allocation for lone-surrogate input.lastIndexOfwithutf16ledoes a raw byte searchNode searches in
uint16_tunits, so a UCS2 match can only start on an even byte offset. Bun's forwardindexOf/includesalready do this viaindexOf16, but inindexOfString/indexOfBufferthelastbranch was taken before theucs2branch, so backward searches always fell through to the byte-levelstd::find_end. This adds alastIndexOf16mirroringindexOf16and moves theucs2branch outward.Empty haystack short-circuits before the empty-needle logic
computeIndexOfRangealready implements Node's semantics here (an empty needle is found at the clamped byteOffset; a non-empty needle in an empty haystack is -1), butif (byteLength == 0) return -1;in the dispatcher and three per-type re-checks ran first. They are removed, so the shared helper is the single source of truth. The byteOffset coercion (which can run uservalueOf) is also no longer skipped for an empty buffer, matching Node.fill()silently truncates a fractional offsetoffsetandendwent throughvalidateNumber(which has no integer check) and were then truncated withtoLength. Node routes both throughvalidateOffset, which isvalidateInteger.Bun::V::validateIntegeralready exists and is used the same way bycompare()in the same file.swap16/swap32/swap64throw the wrongcodeThe message was already correct, but
throwNodeRangeErrorhardcodesERR_OUT_OF_RANGE. Node usesERR_INVALID_BUFFER_SIZE(aRangeError), which did not exist inErrorCode.ts; it is added there.writeBig[U]Int64LE/BEwith a negative offset throws the wrong codeNode's
boundsErrorputs a negative offset in the same>= 0 and <= maxbucket as a too-large one;ERR_BUFFER_OUT_OF_BOUNDSis reserved for a buffer that is too short to hold 8 bytes at all.validateOffsetBigInt64's two negative branches now issue the sameOUT_OF_RANGEthe too-large path already used, and the integer-ness check moves ahead of the sign check so that-1.5reportsan integer, also matching Node.During review this grew into a full reordering of
validateOffsetBigInt64to match Node'svalidateNumberthenboundsErrorsequence exactly: the offset's type is checked first, then its integer-ness (usingMath.floor(value) !== valuesemantics, soNaNand fractions reportan integerwhile+-Infinitydo not), then the too-short buffer, then the range. This makesBuffer.alloc(7).writeBigInt64LE(1n, "x")aTypeErrorERR_INVALID_ARG_TYPEandBuffer.alloc(7).writeBigInt64LE(1n, 1.5)anERR_OUT_OF_RANGE, as in Node, instead ofERR_BUFFER_OUT_OF_BOUNDSfor both. Every combination is in the test block and verified against Node v26.3.0.Verification
On the released Bun the 8 new or strengthened test blocks all fail (16 failures across
buffer.test.js's two parameterized suites; the other 501 tests are unchanged). With this change:bun bd test test/js/node/buffer.test.js: 516 pass, 0 failtest-buffer-{indexof,fill,swap,bytelength,bigint64}.jsall still passbun bd test test/js/web/encoding/: 2433 pass, 0 fail (element_length_utf16_into_utf8also feedsTextEncoder)bun bd test test/js/node/buffer-utf16.test.ts test/js/node/buffer-indexOf-detach.test.ts test/js/node/buffer-copy-fill-detach.test.ts test/js/node/buffer-compare-bounds.test.ts test/js/node/buffer-concat.test.ts: 64 pass, 0 failRelated
#32383 works around the same
element_length_utf16_into_utf8under-count atTextEncoder's call site (it caused a doubleUint8Arrayallocation perencode()for large lone-surrogate input). The root-cause fix here resolves that too: verified with 20encode()calls on a 100,000-unit string containing a lone surrogate going from 40 liveUint8Arrays to 20, with no change toTextEncoder.rs.