buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles - #34273
Conversation
… Buffer needles Buffer.prototype.lastIndexOf/indexOf with a Buffer needle and a ucs2/utf16le encoding was truncating the haystack to an even byte length before wrapping a negative byteOffset. On an odd-length haystack this makes byteOffset === -buf.length land one byte before the start and return -1 for data that is present. Node's IndexOfBuffer wraps negative offsets against the raw byte length and only floors to 16-bit units when searching. IndexOfString, by contrast, does truncate before the wrap; the previous code applied the string behavior to both paths. Move the search-end even-truncation into computeIndexOfRange so the empty-needle clamp also matches Node, and pass the raw byte length for Buffer needles.
WalkthroughChangesUTF-16 index range handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:27 PM PT - Jul 15th, 2026
❌ @robobun, your commit a3fa562 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34273That installs a local version of the PR into your bun-34273 --bun |
There was a problem hiding this comment.
Beyond the inline finding, I also checked: the indexOfString path is unaffected (it still passes the even-truncated haystackLength, so the new in-helper searchEnd &= ~1 is idempotent there), and the new empty-needle assertions (h3.lastIndexOf(empty, 3|undefined, 'ucs2') === 2) match Node's IndexOfBuffer — the raw-wrapped offset clamps against the even searchEnd.
Extended reasoning...
The confirmed regression in forward indexOf(Buffer, oddWrappedOffset, 'ucs2') is covered by the inline comment. Separately, I verified that indexOfString behavior is preserved (even-truncated haystackLength → & ~1 on searchEnd is a no-op) and that the new test's empty-Buffer-needle expectations on the odd-length haystack are Node-correct rather than locking in a divergence. indexOfNumber passes isUTF16=false and is untouched.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/js/node/buffer.test.js`:
- Around line 2436-2445: Replace the encoding loop surrounding the eight Buffer
search assertions with a describe.each() parameterized test over "ucs2" and
"utf16le". Pass the selected encoding into the test case and preserve all
existing assertions and expected results while providing separate reporting for
each encoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f48e2d86-9ae6-4170-a52e-71829c35ad00
📒 Files selected for processing (2)
src/jsc/bindings/JSBuffer.cpptest/js/node/buffer.test.js
There was a problem hiding this comment.
LGTM — thanks for the empirical Node check on the -2 offset case; my earlier read of IndexOfBuffer was wrong and the added lock-in assertions cover it.
Re-traced computeIndexOfRange with the new isUTF16 flag through all three callers (indexOfNumber unaffected, indexOfString unchanged since it already passed the even-floored length, indexOfBuffer now wraps against raw byteLength). Checked that the empty-needle clamp and the forward needleLength + offset > searchEnd early-exit both see the even-floored searchEnd, matching Node's ordering. Walked the new test assertions (h3/h5/h1, Buffer vs. string needle, empty needle) against the post-PR arithmetic and they all line up.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug in Buffer.prototype.indexOf/lastIndexOf with a Buffer needle and ucs2/utf16le encoding on odd-length haystacks. Previously indexOfBuffer floored byteLength to even before passing it to computeIndexOfRange, so a negative offset of exactly -byteLength wrapped to -1 and returned no-match for data that is present. Node's IndexOfBuffer wraps against the raw byte length and only floors to 16-bit units for the search itself; Node's IndexOfString does floor first. The fix threads an isUTF16 flag into computeIndexOfRange so searchEnd is even-floored there (used by both the empty-needle clamp and the forward-bound check), while indexOfBuffer now passes raw byteLength and indexOfString keeps the pre-floored length. indexOfNumber passes isUTF16=false and is unaffected. ~14 net lines in JSBuffer.cpp plus 54 lines of new tests.
Prior review
My earlier inline comment claimed h3.indexOf(n, -2, 'ucs2') should return 0 based on a misread of Node's source (I thought the forward-bound check used raw haystack_length). The author refuted this with a direct Node v26.3.0 run showing -1 and pointed to the actual line where Node floors search_end before that check. Commit 3d76c29 added expect(h3.indexOf(n, -2, enc)).toBe(-1) and the positive odd-offset case to lock the behavior in. I re-traced the post-PR path for that input and it returns -1 as Node does; the thread is correctly resolved.
Security risks
None. Pure integer offset arithmetic on already-bounds-checked buffer lengths; searchEnd is clamped to [0, haystackLength] before the & ~1, and the search helpers (indexOf16/lastIndexOf16) receive searchEnd ≤ byteLength. No new allocations, no user-controlled sizes flowing into pointer arithmetic beyond what was already there.
Level of scrutiny
Medium — native C++ offset arithmetic is easy to get subtly wrong (as my own earlier misread demonstrates), but the change is small, contained to three adjacent static functions, and the author ran a full differential sweep against Node v26.3.0 over haystack lengths 0..7 × needle lengths 0..4 × offsets -(hlen+2)..hlen+2 × both directions × both encoding names. That sweep plus the passing test-buffer-indexof.js port and 616 existing buffer.test.js cases is stronger evidence than manual tracing.
Other factors
All three call sites of computeIndexOfRange were updated for the new parameter. The new test covers the string-needle path explicitly to pin down that it stays on the even-truncated behavior (Node's documented divergence between IndexOfString and IndexOfBuffer). Evidence in the PR body shows the test fails on main (2 failures ASAN, 54 release) and passes with the fix. No CODEOWNERS entry for src/jsc/bindings/JSBuffer.cpp.
07be021 to
0fe7a90
Compare
|
The diff is green: The remaining CI failures are unrelated to this change:
None of these touch |
* upstream/main: (57 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (70 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ... # Conflicts: # test/js/bun/websocket/websocket-server.test.ts
…exOf Merge e66279d resolved conflicts by keeping the pre-fix uint32_t versions, reintroducing upstream bugs oven-sh#34273/oven-sh#34274 (odd-length ucs2 indexOf with Buffer needles, 2^32-byte write/toString truncation). JSBuffer.cpp has no OHOS-specific code, so restore the upstream file.
Reproduction
On an odd-length haystack,
byteOffset === -buf.lengthwrongly returns -1 for data that is present. The same applies toutf16le, toindexOfat certain offsets, and to the empty-needle clamp.Cause
indexOfBuffercomputedhaystackLength = byteLength & ~1for UTF-16 encodings and passed that tocomputeIndexOfRange, which wraps negative offsets aslength + offset. WithbyteLength = 3,-3 + (3 & ~1) = -1lands in the "before the start: no match" arm.Node's
IndexOfBufferuses the raw byte length forIndexOfOffsetand only floors to 16-bit units for the search itself. Node'sIndexOfString, by contrast, does truncate the haystack length to even beforeIndexOfOffset. Bun was applying the string-needle behavior to both paths.Fix
indexOfBuffernow passes the rawbyteLengthtocomputeIndexOfRange.indexOfStringkeeps the even-truncatedhaystackLength(matching Node).searchEnd &= ~1step moves insidecomputeIndexOfRange(via a newisUTF16flag) so the empty-needle clamp uses the even-truncated search end, again matching Node.Verification
bun bd test test/js/node/buffer.test.js(616 pass),test/js/node/buffer-indexOf-detach.test.ts(13 pass), andtest/js/node/test/parallel/test-buffer-indexof.jsall pass. A differential sweep over haystack lengths 0..7, needle lengths 0..4, offsets-(hlen+2)..hlen+2, both directions and both encoding names is byte-identical to Node v26.3.0.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file