[robobun]

Problem

• Buffer.prototype.indexOf, lastIndexOf and includes coerce end before every other argument. With two invalid arguments, Bun throws a different error than Node v26.3.0. Example: Buffer.from("abcd").indexOf("a", 0, end, "bogus") where end.valueOf throws. Node throws ERR_UNKNOWN_ENCODING. Bun throws the error from end.valueOf.
• The cause is the indexOf helper (src/jsc/bindings/JSBuffer.cpp:1792). It calls endValue.toNumber() before the byteOffset coercion, the encoding lookup and the value type check.

Fix

• The new toIndexOfEnd helper coerces end in each search path (number, string, Uint8Array). The order is byteOffset, encoding, value type check, end, as in bidirectionalIndexOf in Node's lib/buffer.js.
• The buffer re-fetch still follows the last coercion. An end.valueOf that detaches or resizes a buffer stays safe.
• Verified: test/js/node/buffer.test.js (12 of 20 new test runs fail on bun 1.4.3, all pass here). Also buffer-indexOf-detach.test.ts, Node's test-buffer-indexof.js and test-buffer-includes.js.
• Self-reviewed: 8 concerns raised, 6 addressed. Rejected: a NaN end change (not an order bug, see Notes), and a move of the tests out of the Buffer.write loop (they stay with the other indexOf tests).

Background

• The three methods share one C++ helper, indexOf(...).
• toNumber() on an object runs user JavaScript (valueOf). That code can throw, or detach or resize an ArrayBuffer, so the position of each coercion is observable. refetchBufferState reads the buffer pointer and length again after the last coercion.
• Node never coerces end. Its native functions run CHECK(args[5]->IsNumber()), so an object end aborts the Node process. Bun coerces end with ToNumber where Node reads it.

Notes

Node reference (v26.3.0, unchanged on main): Buffer.prototype.indexOf and bidirectionalIndexOf in lib/buffer.js, and IndexOfString, IndexOfBuffer, SlowIndexOfNumber in src/node_buffer.cc.

Probe on node v26.3.0 against bun 1.4.3. T(n) is an object whose valueOf and toString throw Error(n). The result is the same for indexOf, lastIndexOf and includes. All rows match Node with this change:

arguments                                     node v26.3.0           bun 1.4.3
("a", T("off"), T("end"))                     Error off              Error end
(97, T("off"), T("end"))                      Error off              Error end
(new Uint8Array([97]), T("off"), T("end"))    Error off              Error end
(null, T("off"), T("end"))                    Error off              Error end
("a", 0, T("end"), T("enc"))                  Error enc              Error end
(new Uint8Array([97]), 0, T("end"), T("enc")) Error enc              Error end
(null, 0, T("end"), T("enc"))                 Error enc              Error end
("a", 0, T("end"), "bogus")                   ERR_UNKNOWN_ENCODING   Error end
("a", "bogus", T("end"))                      ERR_UNKNOWN_ENCODING   Error end
(null, 0, T("end"))                           ERR_INVALID_ARG_TYPE   Error end

Order of the first coercion call per argument, with a logging object in each position. Node ran in a child process because it aborts at the end step:

value type    node v26.3.0                      bun 1.4.3        this PR
number        off, abort (args[4]->IsNumber())  end, off         off, end
string        off, enc, abort (args[5])         end, off, enc    off, enc, end
Uint8Array    off, enc, abort (args[5])         end, off, enc    off, enc, end
null          off, enc, ERR_INVALID_ARG_TYPE    end, off, enc    off, enc

The new test re-reads the buffer after end is coerced guards the position of the coercion against the buffer re-fetch. With the coercion moved after refetchBufferState, that test fails (indexOf returns 0 from the freed haystack).

Three tests in test/js/node/buffer-indexOf-detach.test.ts passed their encoding object as the third argument. That position is end since Node added it, so node --test aborted on that file. They now pass the object as the fourth argument. The file passes under Node v26.3.0 again (13 of 13), as its header says. The new tests in buffer.test.js cover a detach from end.

Differences that this PR does not change:

• A NaN end means the whole buffer in Bun. In Node it reaches static_cast<int64_t>(NaN), which is undefined behavior, and the search range is empty. An Infinity end takes the same cast: the range is empty in Node on x64.
• Node captures the default end (this.length) before it coerces anything. If the byteOffset coercion grows a length-tracking view over a resizable ArrayBuffer, Node stops the search at the old length. Bun searches the new length.
• Node coerces a non-string encoding with encoding += '' (valueOf first). Bun uses ToString (toString first).

Other Buffer methods also differ from Node on the order of their checks. This PR does not change them. Two examples: buf.toString("bogus", 5, 2) returns "" in Node and throws ERR_UNKNOWN_ENCODING in Bun. buf.fill("6162", 0, 4, { toString: () => "hex" }) throws ERR_INVALID_ARG_TYPE in Node and fills in Bun.

PR #41498 edits refetchBufferState in the same helper. The two diffs do not touch the same lines.

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for one more day.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 10 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 0008b82a-978a-412f-a245-d79536f94c3e

📥 Commits

Reviewing files that changed from the base of the PR and between 26e7a4b and 307a79e.

📒 Files selected for processing (3)

• src/jsc/bindings/JSBuffer.cpp
• test/js/node/buffer-indexOf-detach.test.ts
• test/js/node/buffer.test.js

---

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

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the reentrancy shape of the three new toIndexOfEnd call sites in src/jsc/bindings/JSBuffer.cpp: in each path (number, string, Uint8Array) the end coercion is the last call that can run user JS before refetchBufferState, and toInt32 on an already-number value in the number path cannot enter JS. The helper's {} return on exception is never consumed since every caller checks the scope on the next line, and the ERR_UNKNOWN_ENCODING return at :1864 still fires before end is touched, matching Node.

Extended reasoning...

The diff is a small reordering inside the shared indexOf helper that backs indexOf, lastIndexOf and includes, so all three siblings are covered by construction. I traced each of the three value paths for any user-JS-reachable call between the new toIndexOfEnd and refetchBufferState and found none; the encoding toWTFString and the string toString both happen before end is coerced. The two inline findings concern divergences from Node that predate this PR (the entry-time byteLength default for lastIndexOf and ToString vs ToPrimitive for a non-string encoding), which the PR description also lists as intentionally unchanged, so I did not treat them as reasons to block on their own. I am not approving outright because this is JSC binding code where user callbacks can detach or resize the haystack, and a human should confirm the ordering choice against Node is the one they want to commit to.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟣 src/jsc/bindings/JSBuffer.cpp — Callers passing an object as the encoding whose valueOf and toString differ get a different encoding (or ERR_UNKNOWN_ENCODING) than Node. JSBuffer.cpp:1855 uses toWTFString, which is ToString and calls toString first; Node's getEncodingOps does encoding += '', which is ToPrimitive default and calls valueOf first. The PR's new order test uses logging objects whose valueOf and toString are the same function, so it cannot see the difference. Fix: coerce the encoding with toPrimitive(default) then ToString (or JSValue::toStringOrNull after toPrimitive) at this shared site so string and Uint8Array values match Node.

  Extended reasoning...

  Trace: buf = Buffer.from("6162", "hex") = [0x61, 0x62]. enc = { valueOf() { return "hex"; }, toString() { return "utf8"; } }. buf.indexOf("6162", 0, undefined, enc). Bun: line 1855 toWTFString -> toString -> "utf8" -> searches for the UTF-8 bytes of "6162" -> -1. Node: encoding += '' -> valueOf -> "hex" -> needle [0x61,0x62] -> 0. With toString returning "bogus", Bun throws ERR_UNKNOWN_ENCODING at 1864 while Node succeeds. The finder let it go as unchanged by the diff, which is true of the line, but the diff moves the end coercion to run after this call and adds a test that claims to pin the coercion order of the encoding argument; the test cannot fail on this divergence because the logging object aliases both methods. Population: any object with distinct valueOf/toString in the encoding slot (e.g. a wrapper class exposing valueOf). Remedy: use ToPrimitive(default)+ToString here; add a test with distinct valueOf/toString.

  Verification: pre-existing — triggers only when a caller passes a non-string object as encoding whose valueOf and toString return different values. Mechanism verified: /home/claude/bun/src/jsc/bindings/JSBuffer.cpp:1855 encodingString = encodingValue.toWTFString(lexicalGlobalObject); is ES ToString, i.e. ToPrimitive(hint String) → OrdinaryToPrimitive tries toString before valueOf. Node's…

[robobun]

Status: CI is green (Buildkite build 118433). The diff is ready for a maintainer.

How I reproduced it (Node v26.3.0 against bun 1.4.3):

const T = n => ({ valueOf() { throw new Error(n); }, toString() { throw new Error(n); } });
const b = () => Buffer.from("abcd");
b().indexOf("a", T("off"), T("end"));   // node: Error off             bun: Error end
b().indexOf("a", 0, T("end"), "bogus"); // node: ERR_UNKNOWN_ENCODING  bun: Error end
b().indexOf(null, 0, T("end"));         // node: ERR_INVALID_ARG_TYPE  bun: Error end

USE_SYSTEM_BUN=1 bun test test/js/node/buffer.test.js -t "argument order" fails 12 of 20 test runs. bun bd test test/js/node/buffer.test.js -t "argument order" passes all 20.

[robobun]

On the encoding finding: I left it out of this PR because it is not specific to indexOf. Node coerces an encoding with encoding += '' in getEncodingOps, and Bun uses ToString in every path that takes an encoding. With enc = { valueOf: () => "hex", toString: () => "utf8" }:

                                       node v26.3.0   bun (this PR)
Buffer.from("ab").toString(enc)        "6162"         "ab"
Buffer.byteLength("6162", enc)         2              4
Buffer.from("ab").indexOf("6162", 0, undefined, enc)   0   -1

A fix belongs in the shared encoding path, with tests for each API. The PR notes list it as a known difference. The order test uses one function for valueOf and toString on purpose. It checks the order between arguments, not the hint.

I replied to the lastIndexOf default start finding in its thread.

[robobun]

^{Updated 1:28 PM PT - Sep 19th, 2026}

✅ @robobun, your commit 307a79e3e30fcfc4ab6ffa17353c0dfe6a56509b passed in Build #118433! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43522

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

bun-43522 --bun