node:buffer: default lastIndexOf(value, encoding) to search from the end by alii · Pull Request #31507 · oven-sh/bun

alii · 2026-05-28T13:04:15Z

Buffer.prototype.lastIndexOf(value, encoding) — the documented 2-arg overload where byteOffset is omitted and the second positional argument is the encoding — searched from offset 0 instead of from the end, so it returned -1 for substrings that are actually present (or the wrong index). Broken for every encoding passed as the 2nd arg (utf8/latin1/hex/utf16le/base64).

JSBuffer.cpp hardcoded byteOffsetD = 0 in the byteOffsetValue.isString() branch, which defeated the direction-aware default a few lines below (if (isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength). Setting it to NaN lets that default apply: lastIndexOf now searches backward from byteLength (matching Node), while forward indexOf/includes (which default to 0 anyway) are unchanged, as is the 3-arg lastIndexOf(value, byteOffset, encoding) form.

Adds tests covering lastIndexOf(value, encoding) across utf8/latin1/hex/utf16le/base64. (This matches Node — Bun 1.3.14 had the same bug, so it's not a port regression.)

alii · 2026-05-28T13:04:18Z

@robobun adopt

robobun · 2026-05-28T13:04:28Z

^{Updated 9:05 PM PT - May 28th, 2026}

❌ @robobun, your commit b94ca91 has 1 failures in Build #58777 (All Failures):

test/js/web/streams/streams-leak.test.ts - code 1 on 🐧 13 x64-baseline

🧪 To try this PR locally:

bunx bun-pr 31507

That installs a local version of the PR into your bun-31507 executable, so you can run:

bun-31507 --bun

robobun · 2026-05-28T13:05:39Z

✅ Adopted and verified.

Repro (release build): Buffer.from("ello hello hello").lastIndexOf("hello", "utf8") returned -1 (Node gives 11); utf16le/hex/base64 were likewise wrong. The lastIndexOf(value, encoding) overload searched from offset 0 instead of from the end.

Cause: the byteOffsetValue.isString() branch hardcoded byteOffsetD = 0, pre-empting the direction-aware default (isnan(byteOffsetD) → dir ? 0 : byteLength). Setting it to NaN lets that default apply — lastIndexOf searches backward from byteLength; forward indexOf/includes stay 0-based; the 3-arg form is untouched.

Node parity: machine-verified identical to Node across a 19-case matrix (all encodings, present/absent/empty needles, forward + 3-arg forms).

Verification: new test fails-before / passes-after; full buffer.test.js (503) green in both debug and release; Node-suite test-buffer-indexof.js + test-buffer-includes.js pass with the fix.

coderabbitai · 2026-05-28T13:08:23Z

Actionable comments posted: 0

coderabbitai · 2026-05-28T13:08:26Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9df8aeef-03a4-4589-9d51-cfbb2f575ff3

📥 Commits

Reviewing files that changed from the base of the PR and between 6940cc8 and 54a2ace.

📒 Files selected for processing (1)

test/js/node/buffer.test.js

Walkthrough

This PR fixes the default byte offset behavior for Buffer.prototype.lastIndexOf(value, encoding) when the second argument is an encoding string with no explicit byteOffset. The implementation sets byteOffsetD to NaN to trigger direction-dependent defaults, and a new test validates the fix across multiple encodings while confirming related methods retain their expected behavior.

Changes

Default byte offset for lastIndexOf(value, encoding) overload

Layer / File(s)	Summary
lastIndexOf(value, encoding) byte offset fix and validation `src/jsc/bindings/JSBuffer.cpp`, `test/js/node/buffer.test.js`	JSBuffer.cpp sets `byteOffsetD = NaN` when the second argument is an encoding string, routing the later `std::isnan(byteOffsetD)` branch for direction-based defaults. Test validates `lastIndexOf(value, encoding)` searches from buffer end across utf8, latin1, binary, utf16le, ucs2, hex, and base64 encodings, and confirms `indexOf` and `includes` overloads maintain existing semantics.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and clearly summarizes the main change: fixing Buffer.lastIndexOf to search from the end when encoding is provided as the second argument.
Description check	✅ Passed	The description covers what the PR does and how it was verified through testing and Node parity checks. Both required template sections are addressed substantively.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude

LGTM — one-line fix that lets the existing direction-aware NaN default apply, with tests across encodings.

Extended reasoning...

Overview

This PR fixes Buffer.prototype.lastIndexOf(value, encoding) — the 2-arg overload where the second argument is an encoding string rather than a byte offset. Previously, JSBuffer.cpp hardcoded byteOffsetD = 0 in this branch, which caused lastIndexOf to search forward from offset 0 instead of backward from the end. The fix changes that to quiet_NaN(), which falls through to the existing direction-aware default at line 1664 (if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength). For indexOf/includes (dir=true) the default remains 0, so forward search is unchanged; for lastIndexOf (dir=false) it now correctly defaults to byteLength.

Security risks

None. This is a pure Node.js-compat correctness fix in the offset-defaulting logic. No new user input parsing, no buffer bounds changes, no allocation changes. The detachment/re-fetch handling that surrounds this code is untouched.

Level of scrutiny

Low. A single-line change from a literal 0 to NaN, where the downstream NaN handling already exists and is correct. I verified the surrounding code: dir = !last at line 1621 and the NaN default at line 1664 match the PR description exactly. The 3-arg form (lastIndexOf(value, byteOffset, encoding)) goes through the else branch and is unaffected.

Other factors

The new tests in buffer.test.js cover utf8/latin1/binary/utf16le/ucs2/hex/base64 for the fixed overload, and explicitly assert that forward indexOf/includes and the explicit-offset 3-arg form still behave correctly. No bugs were found by the bug hunting system, and there are no outstanding reviewer comments.

alii · 2026-05-28T13:28:37Z

@robobun confirm the behaviour is the same as Node.js here. Also can you find a test from Node.js' test suite and copy it into this PR with no changes to the file? It's fine if there is no test suite, but it's good to get these things included if there are passing tests

robobun · 2026-05-28T13:40:35Z

@alii

1. Node.js parity — confirmed identical. I ran a 19-case matrix through Node 25 and this build and the output is byte-for-byte equal (utf8/latin1/binary/ascii/utf16le/ucs2/hex/base64/base64url, present + absent + single-char + empty needles, plus the forward indexOf/includes, the 3-arg lastIndexOf(value, offset, encoding) form, and number needles):

Buffer.from("ello hello hello").lastIndexOf("hello", "utf8")   → 11   (was -1)
                                .lastIndexOf("hello", "latin1") → 11
Buffer.from("ello hello hello","utf16le").lastIndexOf("hello","ucs2") → 22
Buffer.from("aabbccaabbcc","hex").lastIndexOf("aabb","hex")    → 3
Buffer.from("Zm9vYmFyZm9v","base64").lastIndexOf("Zm9v","base64") → 6

2. Node test suite. The relevant upstream files — test/parallel/test-buffer-indexof.js and test-buffer-includes.js — are already in the repo (test/js/node/test/parallel/), are auto-discovered by the runner, and run in CI. Both pass with this fix (exit 0). I re-confirmed them against this build.

I could not drop in the latest upstream copies verbatim, for two reasons unrelated to this PR:

Bun's ERR_INVALID_ARG_TYPE message is worded differently (…must be of type number, string, Buffer, or Uint8Array vs Node's …must be one of type number or string or an instance of…), so those assert.throws lines fail.
Bun doesn't implement the 3-arg length-limiting overload indexOf(value, byteOffset, end) (it reads arg 3 as an encoding → ERR_UNKNOWN_ENCODING), which is a ~90-line block at the end of the upstream file.

The repo copies are already adapted for exactly those two differences, which is why they diverge from upstream.

Worth flagging: upstream Node's own test-buffer-indexof.js has no test for the 2-arg lastIndexOf(value, encoding) case where the needle is present — all nine of its lastIndexOf(str, encoding) assertions expect -1, which the buggy code also returned. So copying it verbatim would not have guarded this regression. That's why the added test in buffer.test.js (asserting the positive indices above) is the one that actually fails-before / passes-after.

claude

LGTM — minimal one-line fix that routes through the existing direction-aware default, with solid test coverage.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (the indexOf helper backing Buffer.prototype.{indexOf,lastIndexOf,includes}): when the second positional argument is a string (i.e., the (value, encoding) overload), byteOffsetD is now set to NaN instead of 0. A few lines later, the existing if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; applies the correct direction-aware default — 0 for forward indexOf/includes, byteLength for lastIndexOf. The PR also adds a focused test block in test/js/node/buffer.test.js covering utf8/latin1/binary/utf16le/ucs2/hex/base64, plus regression checks for the forward and 3-arg forms.

Security risks

None. This is a pure read-only search-offset default in the Node Buffer compat layer. No memory bounds, allocation, encoding parsing, or user-controlled pointer arithmetic is altered — the only thing that changes is the starting offset fed into the existing (already bounds-clamped) search routines.

Level of scrutiny

Low. The change is one token on one line, and its effect is fully determined by the immediately-following isnan branch in the same function, which I verified in the current source. Forward indexOf/includes are provably unchanged (they already defaulted to 0 and still do via dir ? 0 : ...); the 3-arg form never enters this branch. The fix matches Node's documented semantics, and robobun machine-verified parity against Node 25 across a 19-case matrix.

Other factors

Bug hunter found no issues.
The new test fails-before/passes-after; the in-repo Node ports test-buffer-indexof.js and test-buffer-includes.js continue to pass; CI is green per the build comment.
The maintainer's follow-up request (Node parity confirmation + upstream test inclusion) was answered with a clear explanation of why the upstream test files are already present and why a verbatim re-import isn't applicable — nothing remains outstanding.
No CODEOWNERS entries cover the touched paths.

claude

LGTM — one-line fix routes the encoding-as-2nd-arg case through the existing direction-aware NaN default, with thorough test coverage.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (the shared indexOf helper used by Buffer.prototype.indexOf/lastIndexOf/includes). When the second argument is a string (i.e., the (value, encoding) overload with no explicit byteOffset), byteOffsetD is now set to NaN instead of hardcoded 0. A few lines below, the existing if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; already handles direction correctly — the old 0 was simply pre-empting it. A new test block in test/js/node/buffer.test.js exercises the fix across utf8/latin1/binary/utf16le/ucs2/hex/base64 and asserts that forward indexOf/includes and the 3-arg form are unchanged.

Security risks

None. This is pure search-offset arithmetic in an already bounds-checked buffer search routine. No new user input paths, no allocation changes, no detachment-window changes (the isString() branch doesn't call into JS, so the existing detachment refetch logic is unaffected).

Level of scrutiny

Low. The fix is one line, the surrounding code already contained the correct logic (the NaN → direction-aware default at line 1664), and the change merely stops short-circuiting it. The forward-search path (dir=true) resolves NaN back to 0, so indexOf/includes behavior is provably identical to before. Only lastIndexOf changes, and only in the documented direction.

Other factors

Node.js parity was machine-verified across a 19-case matrix per the PR thread.
The new test fails-before/passes-after and covers the regression that upstream Node's own test-buffer-indexof.js does not (all of Node's 2-arg lastIndexOf(str, enc) cases assert -1).
The reviewer's request (confirm Node parity, check for upstream tests) was answered in detail; no outstanding review comments remain.
CI is green including ASAN.

claude

LGTM — one-line fix that routes the 2-arg lastIndexOf(value, encoding) overload through the existing direction-aware default, with regression tests across all encodings.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (the shared indexOf helper used by Buffer.prototype.indexOf/lastIndexOf/includes): when the second argument is a string (i.e., it's the encoding, not a byteOffset), byteOffsetD is now set to NaN instead of 0. This lets the existing if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; line apply the correct direction-aware default. A new test block in test/js/node/buffer.test.js covers utf8/latin1/binary/utf16le/ucs2/hex/base64 and confirms the forward indexOf/includes and 3-arg overloads are unaffected.

Security risks

None. This is pure search-offset arithmetic with no parsing, allocation, auth, or external input handling changes. The NaN value is consumed two lines later by the existing isnan check and never propagates further.

Level of scrutiny

Low. The code change is one line and the surrounding logic already handled NaN correctly (it's exactly what toNumber(undefined) produces in the else branch for indexOf(value) with no second arg). For indexOf/includes (dir=true), NaN → 0 preserves the old behavior; for lastIndexOf (dir=false), NaN → byteLength fixes the bug to match Node. The 3-arg form doesn't enter this branch at all.

Other factors

robobun verified Node.js parity across a 19-case matrix and confirmed test-buffer-indexof.js / test-buffer-includes.js from Node's suite (already vendored in-repo) pass.
The reviewer's request (confirm Node parity, check for upstream tests) was addressed with a detailed response; no outstanding review comments remain.
The new test fails-before / passes-after, and the full buffer.test.js suite stays green.
Bug-hunting system found no issues.

alii · 2026-05-28T14:17:15Z

@robobun okay, we'll merge this and then do a followup later fixing the remaining parity problems

robobun · 2026-05-28T14:18:40Z

Sounds good 👍 Ready to merge — the fix is verified fails-before/passes-after in both debug (ASAN) and release builds.

Heads up: the red status currently shown on the commit was a one-off darwin-14-x64 runner hiccup (exit status -1) on the prior build — the buffer tests it covers don't run on that lane anyway. I kicked a fresh build (#58634) so the board clears to green.

claude

LGTM — one-line fix that lets the existing direction-aware NaN default apply, with thorough test coverage.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (the shared indexOf helper backing Buffer.prototype.{indexOf,lastIndexOf,includes}). When the second positional argument is a string (i.e. the (value, encoding) overload with no byteOffset), byteOffsetD is now set to NaN instead of 0, so the existing if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; line a few lines below correctly defaults lastIndexOf to search from the end while leaving forward indexOf/includes at 0. A new test block in test/js/node/buffer.test.js covers utf8/latin1/binary/utf16le/ucs2/hex/base64 plus regression checks for the forward and 3-arg forms.

Security risks

None. This is pure search-offset arithmetic on an already-bounds-checked buffer; no new user input parsing, no allocation, no auth/crypto/permissions surface. The change cannot introduce OOB access — it only alters the starting offset fed into the existing search routines, which already clamp.

Level of scrutiny

Low. The diff is one constant changed from 0 to quiet_NaN(), and the surrounding code at line 1664 was visibly written to handle exactly this case — the old hardcoded 0 was just defeating it. I read the surrounding ~60 lines and confirmed dir = !last, so the NaN branch yields byteLength for lastIndexOf and 0 for indexOf, matching Node semantics. The 3-arg form and non-string byteOffset paths go through the else branch and are unaffected.

Other factors

The bug-hunting system found nothing. Node.js parity was machine-verified across a 19-case matrix per the PR thread, the new test fails-before/passes-after, the full buffer.test.js suite plus the ported Node test-buffer-indexof.js/test-buffer-includes.js pass, and the maintainer has already signaled intent to merge. The change is small, mechanical, and well-covered.

Jarred-Sumner

Add a test that verifies the behavior of buffer.indexOf is not changed.

… fix

robobun · 2026-05-28T20:42:55Z

Done — added indexOf(value, encoding) is unchanged by the lastIndexOf fix in test/js/node/buffer.test.js (b8b6ddc).

It asserts Buffer.indexOf/includes still return the first match (0-based) across utf8/latin1/binary/ascii/utf16le/ucs2/hex/base64, plus the 3-arg indexOf(value, byteOffset, encoding) and the non-string-2nd-arg (byteOffset) coercion cases. The buffer repeats "hello" (indexOf → 0, lastIndexOf → 12) so any regression that searched from the end would be caught.

Verified it passes both with and without the one-line fix — i.e. forward indexOf behavior is provably untouched — while the lastIndexOf test still fails-before / passes-after.

Jarred-Sumner

What about when an offset is included

claude

LGTM — one-line fix routing the encoding-as-2nd-arg case through the existing direction-aware NaN default, with thorough fails-before/passes-after test coverage.

Extended reasoning...

Overview

Single-character semantic change in src/jsc/bindings/JSBuffer.cpp: when the 2nd argument to indexOf/lastIndexOf/includes is a string (i.e. it's the encoding, not a byteOffset), byteOffsetD is now set to NaN instead of hardcoded 0. Three lines below, the existing if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; branch then picks the correct direction-dependent default — 0 for forward search (unchanged) and byteLength for lastIndexOf (the fix). The rest of the diff is two new test blocks in test/js/node/buffer.test.js covering both the fixed lastIndexOf(value, encoding) overload and regression guards proving indexOf/includes are unaffected.

Security risks

None. This is pure search-offset arithmetic in the Node Buffer compat layer; no memory access, allocation, auth, or external I/O changes. The buffer pointer/length re-fetch logic and detachment guards are untouched.

Level of scrutiny

Low. The fix is a one-token change whose effect is fully determined by the immediately-following isnan branch, which I verified in the current source (JSBuffer.cpp:1664). Forward search keeps defaulting to 0, so indexOf/includes are provably unchanged; the 3-arg form and non-string 2nd-arg paths go through the else branch and are untouched. Node parity was machine-verified across a 19-case encoding matrix per the PR thread.

Other factors

The bug hunter found nothing. Tests are comprehensive (utf8/latin1/binary/ascii/utf16le/ucs2/hex/base64, present + absent needles, 3-arg form, non-string offset coercion) and were confirmed fails-before/passes-after in debug+release. The maintainer has already indicated intent to merge. No outstanding review comments.

robobun · 2026-05-28T20:53:39Z

Covered in 54a2ace — indexOf/lastIndexOf with an explicit byteOffset are unchanged by the fix.

When a numeric byteOffset is supplied (with or without a trailing encoding), both methods take the non-string branch — the one the fix doesn't touch — so behavior is identical. The test exercises a repeated-needle buffer ("hello" at 0 and 12) so the offset actually selects which match is returned:

indexOf(value, offset) — positive, 0, past-the-end (-1), and negative (counts from end)
indexOf(value, offset, encoding) — utf8/latin1
lastIndexOf(value, offset) — positive, 0, -1, and past-the-end (clamped)
lastIndexOf(value, offset, encoding) — utf8
utf16le/ucs2 with a byte offset (both directions)

Every assertion matches Node 25, and I verified this test passes both with and without the one-line fix — i.e. the explicit-offset paths are provably untouched.

coderabbitai · 2026-05-28T20:58:01Z

Actionable comments posted: 0

claude

LGTM — one-line fix routing the encoding-as-2nd-arg case through the existing direction-aware default, with thorough Node-parity tests.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (in the shared indexOf helper used by Buffer.prototype.indexOf/lastIndexOf/includes): when the second positional argument is a string (i.e., it's the encoding, not a byteOffset), byteOffsetD is now set to NaN instead of hardcoded 0. A few lines later, if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength; already implements the direction-aware default — so lastIndexOf now correctly starts from the end while indexOf/includes still resolve to 0. The other ~100 lines are pure test additions in test/js/node/buffer.test.js covering the fixed overload across all encodings, plus regression guards confirming indexOf, includes, and the explicit-byteOffset forms are unchanged.

Security risks

None. This is a read-only search-offset default in a Buffer index lookup. No allocation, no bounds computation, no user-controlled pointer arithmetic is altered — the existing offset normalization (indexOfOffset) and detachment re-fetch logic are untouched. The change cannot widen the search range beyond what byteLength already allows.

Level of scrutiny

Low. The C++ change is one line whose correctness is mechanically verifiable against the isnan branch six lines below, and the forward path is provably a no-op (NaN → dir ? 0 : … → 0). The tests are straightforward expect(...).toBe(...) assertions that match Node 25's documented and observed behavior. Neither file is covered by CODEOWNERS.

Other factors

The maintainer has already indicated intent to merge. robobun verified fails-before/passes-after in debug (ASAN) and release, byte-for-byte parity with Node across a 19-case matrix, and that the upstream test-buffer-indexof.js/test-buffer-includes.js (already in-tree) pass. The bug-hunting system found no issues, and there are no outstanding reviewer comments.

claude

LGTM — one-line fix routing the encoding-as-2nd-arg case through the existing direction-aware NaN default, with thorough Node-parity tests.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSBuffer.cpp (indexOf helper, line ~1635): when the second positional argument to Buffer.prototype.{indexOf,lastIndexOf,includes} is a string (i.e. an encoding rather than a byteOffset), byteOffsetD is now set to quiet_NaN() instead of 0. This lets the pre-existing direction-aware default at line 1664 (if (std::isnan(byteOffsetD)) byteOffsetD = dir ? 0 : byteLength) apply, so lastIndexOf(value, encoding) searches from the end (matching Node) while indexOf/includes remain 0-based. The rest of the diff is three new test blocks in test/js/node/buffer.test.js covering the fix across utf8/latin1/binary/utf16le/ucs2/hex/base64 and asserting that forward indexOf/includes and explicit-byteOffset forms are unchanged.

Security risks

None. This is a pure search-offset default in Buffer's indexOf family — no auth, crypto, permissions, memory allocation, or pointer arithmetic is touched. The detachment-safety and re-fetch logic in the surrounding code is unaffected.

Level of scrutiny

Low. The production change is one constant (0 → NaN) feeding into an existing, already-tested branch. I verified in the source that the std::isnan(byteOffsetD) check at line 1664 exists and resolves to byteLength when dir is false (lastIndexOf), and to 0 when dir is true (indexOf/includes) — so forward search semantics are provably preserved. No CODEOWNERS apply to either file.

Other factors

The bug-hunting system found no issues. robobun verified Node 25 parity across a 19-case matrix, fails-before/passes-after on the new tests, and that the full buffer.test.js suite plus Node's test-buffer-indexof.js/test-buffer-includes.js pass. The maintainer has already indicated intent to merge. The added regression tests are well-constructed (repeated needles so direction errors would surface) and explicitly guard the unchanged paths.

node:buffer: default lastIndexOf(value, encoding) to search from the end

6940cc8