Repository navigation
buffer: match Node's Buffer.prototype.fill offset and end argument handling - #33033
Conversation
…ndling
Buffer.prototype.fill reinterprets a string `offset` or `end` argument
as the encoding regardless of the fill value's type, and always
validates `end`. Node's _fill (lib/buffer.js) only performs that
reinterpretation when the fill value is itself a string, and only reads
`end` once `offset` is present. Eight argument shapes diverge:
buf.fill(0, "1", 3) // bun fills [0,3); node ERR_INVALID_ARG_TYPE
buf.fill(0, 1, "3") // bun fills [1,len); node ERR_INVALID_ARG_TYPE
buf.fill(0, undefined, "3") // bun ERR_INVALID_ARG_TYPE; node fills all
buf.fill(0, undefined, 3) // bun fills [0,3); node fills all
buf.fill("b", "utf8", 3) // bun fills [0,3); node fills all
Gate the string-offset/end to encoding reinterpretation on
value.isString(), discard `end` in the offset-slot branch to match
Node's `end = buf.length` reset, and nest the `end` validation inside
the offset-present branch so it is never read without an `offset`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesBuffer.fill Argument Handling Fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:25 PM PT - Jun 29th, 2026
❌ @robobun, your commit ba95354 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33033That installs a local version of the PR into your bun-33033 --bun |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Add tests that run in Node.js and Bun that verify t he output is the same.
The new fixture enumerates 34 Buffer.prototype.fill argument shapes and prints the resulting bytes or the thrown error class and code. buffer.test.js spawns it under nodeExe() and bunExe() and requires the two outputs to be byte-identical. Node 24.3.0 (the version the CI image pins) and 26.3.0 agree on every line.
|
Done in 274affb.
With this PR the outputs match exactly. Against the unfixed build, 14 of the 34 lines differ, so the test catches every shape this PR changes. I also confirmed the fixture's Node output is identical between v24.3.0 (the version the CI test image pins in Fixture output (Node v24.3.0 == Node v26.3.0 == Bun with this PR) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/jsc/bindings/JSBuffer.cpp`:
- Around line 1356-1367: The JSBuffer::_fill argument rewrite is incorrectly
treating an omitted offset as if it were the encoding slot, which clobbers the
explicit 4th-argument encoding. Update the string-value handling so only a
string offset is reassigned to encoding; when offsetValue is undefined, leave
encodingValue untouched and only clear endValue, preserving calls like fill with
an explicit encoding in the 4th argument.
🪄 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: 533618be-85b0-4813-a4d0-476b337d55bb
📒 Files selected for processing (3)
src/jsc/bindings/JSBuffer.cpptest/js/node/buffer-fill-args-fixture.jstest/js/node/buffer.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two more argument-count shapes in fill()'s intake diverge from Node:
Buffer.alloc(3, 0xaa).fill() // bun no-op <aa aa aa>; node zero-fills
Buffer.alloc(5, 0xaa).fill(0, 1, 3, "utf8", "x")
// bun fills [0, 5); node fills [1, 3)
The zero-argument early return skipped Node's path, where the undefined
value reaches the numeric branch and coerces to 0. The argument switch
had no case for five or more arguments, so every slot degraded to
undefined. Use the bounds-checked callFrame->argument(n) for all four
slots instead: it returns undefined for an absent argument and ignores
trailing ones, matching Node's plain-JS signature.
Node's normalizeEncoding tests `enc == null`, so an explicit null
encoding is the same as an absent one and defaults to utf8. Bun's
fill() and Buffer.alloc() gated on isUndefined(), so null reached
parseEncoding and threw:
Buffer.alloc(5, 0xaa).fill("a", 1, 3, null) // bun ERR_INVALID_ARG_TYPE
// node <aa 61 61 aa aa>
Buffer.alloc(3, "a", null) // bun ERR_INVALID_ARG_TYPE
// node <61 61 61>
buf.toString(null) is unchanged: Node routes it through getEncodingOps,
which does throw ERR_UNKNOWN_ENCODING for a null encoding, so the null
handling belongs at these two call sites and not inside parseEncoding.
Node's normalizeEncoding also returns utf8 for an empty string (the
slowCases default branch), so the treat-as-absent set is exactly
undefined, null, and "". The fill() and Buffer.alloc() encoding gates
only skipped null and undefined, so "" reached parseEncoding and threw:
Buffer.alloc(5, 0xaa).fill("a", 1, 3, "") // bun ERR_UNKNOWN_ENCODING
// node <aa 61 61 aa aa>
Buffer.alloc(5, 0xaa).fill("a", "") // bun ERR_UNKNOWN_ENCODING
// node <61 61 61 61 61>
Buffer.alloc(3, "a", "") // bun ERR_UNKNOWN_ENCODING
// node <61 61 61>
Extract the two gates' shared predicate into isAbsentEncoding, which now
covers all three values. parseEncoding is unchanged: buf.toString("")
and buf.toString(null) must keep throwing ERR_UNKNOWN_ENCODING, because
Node routes toString through getEncodingOps, which rejects both.
Buffer.from("ab", "") has the same symptom but through a different Node
helper (fromString treats any non-string or empty-string encoding as
utf8, and Bun already matches it for every non-string value); its gate
is outside this file's fill/alloc path and is left for its own change.
There was a problem hiding this comment.
All four of my earlier findings are addressed and the 48-case Node/Bun differential is a strong correctness backstop — nothing further from me. Deferring only because this rewrites fill's C++ argument intake in JSC bindings, which is worth a human glance even when the tests are this thorough.
Extended reasoning...
Overview
This PR rewrites the argument intake of jsBufferPrototypeFunction_fillBody in src/jsc/bindings/JSBuffer.cpp to match Node's Buffer.prototype.fill semantics for offset/end/encoding handling. It also adds a small isAbsentEncoding helper (shared with Buffer.alloc's encoding gate), a 48-case differential fixture (test/js/node/buffer-fill-args-fixture.js) that is run under both Node and Bun and required to be byte-identical, and an 11-test describe block in buffer.test.js.
The C++ changes are: drop the zero-argument early return in favor of bounds-checked callFrame->argument(0); replace the fallthrough switch on argumentCount() with three argument(n) reads (the established idiom from writeBody); gate the string-offset/end → encoding reinterpretation on value.isString() and discard end in the offset-slot branch; nest end validation inside the offset-present branch; and route the encoding-presence check through isAbsentEncoding (undefined / null / "").
Security risks
Low. Buffer.prototype.fill is memory-adjacent, but this PR does not touch the bounds enforcement (validateInteger on offset and end), the post-coercion detach/resize clamp, or the actual write loop — only the JS-value classification that feeds those validators. The move from uncheckedArgument to argument is strictly safer. isAbsentEncoding is a pure predicate on a JSValue with no side effects. I see no new path that could widen the written range or bypass the existing offset/end checks.
Level of scrutiny
Medium. This is hand-written C++ in JSC bindings on a hot, user-facing Buffer method. The change is self-contained and conservative, and the differential test is unusually strong evidence of correctness, but rewriting argument-intake control flow in this file is the kind of change a maintainer typically wants to eyeball once — particularly the interaction with the existing detach/resize hardening that the surrounding comments reference.
Other factors
- The bug-hunting system found nothing on the current revision.
- I left four inline findings on earlier revisions (0-arg / ≥5-arg handling,
nullencoding,""encoding, boxed-Stringencoding); the first three were folded in with tests, and the fourth was correctly deferred to a follow-up and documented in the PR body's "Deliberately excluded" section, since fixing it requires changingparseEncodingand the TOCTOU-hardening suite together. All four threads are resolved. - CodeRabbit's one finding was incorrect (it would have made Bun diverge from Node) and was withdrawn after the author pinned the contract with extra fixture cases.
- The two CI failures (
terminal.test.tson macOS x64,test-net-connect-memleak.json Linux) are unrelated flakes — neither touches Buffer. - No CODEOWNERS entry covers
src/jsc/bindings/JSBuffer.cpp.
Given "when in doubt, do NOT approve" and that this is a non-mechanical C++ change in core runtime code, I'm deferring rather than shadow-approving, even though I have no outstanding concerns.
|
CI status, for whoever reviews this: the diff is green, and the three remaining red lanes are unrelated to it. Build 66658 is a clean retrigger of the full pipeline at the final commit: 281 lanes passed, 3 failed. Every failure is in a file this PR does not touch.
Review state: CodeRabbit reports no actionable comments on the final revision; the Claude Code Review's closing summary has nothing outstanding and defers only for a human look at the C++; all five review threads are resolved. I will not push further retriggers. This is ready for a maintainer. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
this gets the encoding as a string twice, consolidate to return an std::optional<JSString*>?
isAbsentEncoding checked the value with asString() and then parseEncoding coerced the same value again with toStringOrNull(). Replace it with resolveEncodingString, which returns std::optional<JSString*>: nullopt for Node's absent set (undefined, null, the primitive empty string) and otherwise the encoding resolved to a JSString exactly once. parseEncoding gains an overload taking that JSString directly, and the existing JSValue overload (used by toString and write) delegates to it unchanged. No behavior change: the 48-shape Node/Bun differential, the full buffer.test.js suite, and the detaching-encoding-toString TOCTOU suite all pass unchanged.
|
Done in b721d06.
No behavior change: the 48-shape Node/Bun differential fixture is still byte-identical to Node, |
There was a problem hiding this comment.
Nothing outstanding from my side on the final revision — all four threads I raised are addressed and the differential fixture pins them — but this reworks argument intake in the C++ Buffer.fill/Buffer.alloc bindings (with the TOCTOU-sensitive encoding-coercion point moved into a new helper), so a maintainer should sign off.
Extended reasoning...
Overview
The PR rewrites the argument-intake logic of jsBufferPrototypeFunction_fillBody in src/jsc/bindings/JSBuffer.cpp to match Node's _fill semantics exactly: gating the string-offset/end → encoding reinterpretation on value.isString(), nesting end validation inside the offset-present branch, replacing the argument-count switch with bounds-checked callFrame->argument(n), and routing the encoding gate (in both fill and Buffer.alloc) through a new resolveEncodingString helper that treats undefined/null/"" as absent. parseEncoding is split into a JSString* overload plus a thin JSValue wrapper so the encoding is stringified once. Tests add a 48-shape Node-vs-Bun differential fixture and 11 targeted unit tests.
Security risks
Buffer.fill and Buffer.alloc write to raw memory, and the encoding argument's toStringOrNull is a user-JS re-entrancy point that can detach or resize the underlying ArrayBuffer mid-call. The PR moves that call from inside parseEncoding to resolveEncodingString, but at the same control-flow point (before offset/end validation, before the post-coercion byteLength() clamp that bounds the actual write). The existing buffer-copy-fill-detach.test.ts TOCTOU suite is reported passing, and the in-source comments documenting the clamp are preserved. I see no new OOB-write or use-after-detach surface introduced — the offset/end values still flow through validateInteger and the downstream clamp — but this is exactly the kind of change where a second pair of eyes on the C++ is worthwhile.
Level of scrutiny
Medium-high. This is not a mechanical or config change: it restructures control flow in a memory-writing native binding, adds a new helper with a non-trivial contract (std::optional<JSString*> where nullopt means absent-encoding, and the non-primitive path can throw), and the final commit (b721d06) was a refactor landed after the last round of review discussion. The change is narrowly scoped, exhaustively commented, and pinned by a differential fixture against real Node, which substantially de-risks it — but "argument parsing for Buffer.fill" is upstream of memset bounds, so it warrants a maintainer's read rather than a bot approval.
Other factors
All five review threads (one CodeRabbit, four of mine) are resolved; each fold-in landed with fixture coverage and an explicit unit test. The two deliberately-excluded adjacent divergences are documented in the PR body with rationale. CI on the final commit is green on every lane that touches buffer.test.js; the three red lanes are unrelated (musl GC timing, Windows flaky init/napi). No CODEOWNERS entry covers this path. The bug-hunting pass on b721d06 found nothing.
|
Final CI status for the reviewed commit (b721d06, the
The two Windows flakes from the previous build did not recur this time. Nothing here is actionable from the diff, and I have already used one retrigger, so I will not push more. Ready for a maintainer. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
shouldn't it be toString instead of toStringOrNull? sot that it throws the exception
…oding Both JSC variants throw the pending exception; toStringOrNull only returns null instead of the empty string on the throwing path (see the comment on JSValue::toStringOrNull in JSCJSValue.h). With toString, an engaged std::optional<JSString*> can never hold a null pointer. Add a test pinning that an exception thrown from an object encoding's toString propagates out of fill() and Buffer.alloc().
|
Done in ba95354: both call sites ( One note for accuracy: the exception was already being thrown. In JSC both variants throw; they differ only in what they return on the throwing path. From inline JSString* toString(JSGlobalObject*) const; // On exception, this returns the empty string.
inline JSString* toStringOrNull(JSGlobalObject*) const; // On exception, this returns null, to make exception checks faster.They share Buffer.alloc(4).fill("a", 0, 4, { toString() { throw new Error("boom"); } });
// already threw "boom"
No behavior change otherwise: the 48-shape Node differential, the full |
There was a problem hiding this comment.
All earlier feedback is addressed and the final resolveEncodingString refactor + toString switch look correct, but this is a non-trivial rewrite of fill()'s argument intake in the JSC C++ bindings (with TOCTOU/detachment-adjacent paths), so it warrants a maintainer's sign-off.
Extended reasoning...
Overview
This PR rewrites jsBufferPrototypeFunction_fillBody's argument intake in src/jsc/bindings/JSBuffer.cpp to match Node's Buffer.prototype.fill semantics exactly: gating the string-offset/end → encoding reinterpretation on value.isString(), nesting end validation under the offset-present branch, replacing the argument-count switch and zero-arg early return with bounds-checked callFrame->argument(n), and routing the encoding gate (in both fill and Buffer.alloc) through a new resolveEncodingString helper that treats undefined/null/"" as absent. parseEncoding is split into a JSString* overload plus a JSValue wrapper. Backed by a 48-case Node/Bun differential fixture and 11 new unit tests.
Security risks
No new attack surface. The change is pure argument-validation reshuffling; the actual memset/memmove and post-coercion byte-length clamp are untouched. The detachment/TOCTOU-hardening comments and the buffer-copy-fill-detach.test.ts suite still pass unchanged. The toStringOrNull → toString switch in the final commit avoids a latent engaged-optional-holding-nullptr hazard, which is a hardening, not a regression.
Level of scrutiny
Medium-high. Buffer.prototype.fill is a hot, memory-writing path in the JSC bindings, and the function this PR rewrites carries explicit TOCTOU comments about user-JS re-entrancy via toString. The change is well-scoped and thoroughly tested (differential against real Node on every CI run), but it is not a mechanical or obvious change — it went through eight commits absorbing four review findings, and the final two commits (b721d06, ba95354) introduced a new helper and refactored the encoding-resolution flow after my last inline comment.
Other factors
All five inline review threads (one CodeRabbit, four from me) are resolved; the author verified each fix against Node v24.3.0 and v26.3.0 and pinned them in the fixture. CI is green on every lane that ran tests; the three failing lanes are unrelated (musl GC timing, darwin artifact-download infra, Windows flakes). The bug-hunting system found nothing on the final revision. Two adjacent pre-existing divergences (Buffer.from("ab", "") and boxed-String encodings) are deliberately deferred and documented in the PR body. No CODEOWNERS coverage on the touched files. Given the C++ surface and the iterative review history, a maintainer should sign off rather than a bot.
|
CI for ba95354 (the
The |
Buffer.prototype.filldiverges from Node on several argument shapes. Most of them silently produce a wrong fill range (no error), and a few throw where Node does not.Reproduction
The first case is the one a caller is most likely to hit by accident:
buf.fill(0, "1", 3)looks like a typo'd offset and Node rejects it, but Bun quietly treats"1"as an encoding, drops it (the value is not a string), and fills[0, 3).Cause
jsBufferPrototypeFunction_fillBodyunconditionally moves a stringoffset(or, failing that, a stringend) into the encoding slot, and always validatesend. In Node's_fill(lib/buffer.js) that reinterpretation lives insideif (typeof value === 'string'), the offset-slot branch also resetsend = buf.length, andendis only validated onceoffsetis present:For a non-string value the string stays put and
validateOffsetrejects it withERR_INVALID_ARG_TYPE; Bun consumed it as a (then-unused) encoding instead. And because Bun validatedendeven with nooffset,fill(0, undefined, "3")threw where Node accepts.Three more pre-existing shapes in the same intake were flagged in review and folded in:
fill()with no arguments took an early return instead of Node's path, where theundefinedvalue reaches the numeric branch and coerces to 0.switchhad cases for 2, 3, and 4 arguments but none for 5 or more, so a stray extra argument made every slot degrade toundefinedand the whole buffer got filled.normalizeEncodingtreats exactlyundefined,null, and""as an absent encoding (utf8). Bun's encoding gate only skippedundefined, sonulland""reachedparseEncodingand threw.Buffer.alloc(size, fill, encoding)has the identical gate, so both now go through one sharedresolveEncodingStringhelper.Fix
Mirror Node's rules in
jsBufferPrototypeFunction_fillBody:value.isString(), and discardendin the offset-slot branch (Node'send = buf.length);endvalidation inside the offset-present branch soendis never read without anoffset;switchand the zero-argument early return with the bounds-checkedcallFrame->argument(n)(the idiomjsBufferPrototypeFunction_writeBodyalready uses), which returnsundefinedfor an absent argument and ignores trailing ones;fill()andBuffer.alloc()through a newresolveEncodingStringhelper coveringundefined,null, and"".parseEncodingitself is unchanged on purpose:buf.toString(null)andbuf.toString("")must stayERR_UNKNOWN_ENCODING, because Node routestoStringthroughgetEncodingOps, which rejects both.The error-ordering contract is unchanged: for a string value, a bad encoding still wins over a bad range (
fill("a", -1, 0, "bogus")is stillERR_UNKNOWN_ENCODING).Deliberately excluded
Two adjacent divergences turned up while mapping this. Each is real, pre-existing, and left for its own change rather than half-fixed here:
Buffer.from("ab", "")throwsERR_UNKNOWN_ENCODINGwhere Node returns<61 62>. Node'sfromStringtreats any non-string or empty-string encoding as utf8 there, and Bun already matches it for every non-string value (null,5,false,{}); only""diverges. Its gate lives outside this file's fill/alloc path.toStringyields a valid encoding name is honored by Bun but ignored or rejected by Node:fill("ab", 0, 4, new String("utf16le"))is<61 62 61 62 aa>(utf8) in Node and<61 00 62 00 aa>(utf16le) in Bun, andfill("ab", 0, 4, {toString: () => "hex"})throwsERR_INVALID_ARG_TYPEin Node but is honored in Bun. Bun'sparseEncodingcoerces its argument viatoStringOrNullbefore any type check. Changing that touches six callers, and the coercion is load-bearing for the TOCTOU-hardening suite intest/js/node/buffer-copy-fill-detach.test.ts, which deliberately documents this divergence and detaches the buffer from inside the encoding object'stoStringto provefillsurvives. That behavior and that safety suite have to change together.Verification
test/js/node/buffer-fill-args-fixture.jsenumerates 48fill()argument shapes and prints the resulting bytes or the thrown error class + code for each.buffer.test.jsspawns the fixture undernodeExe()andbunExe()and requires the two outputs to be byte-identical. With this PR they match exactly; without thesrc/change they diverge on every shape the PR targets. The fixture's Node output is identical between v24.3.0 (the version the CI image pins) and v26.3.0.test/js/node/buffer.test.js: all existing tests pass; the newBuffer.fill offset/end argument handlingdescribe adds 11 tests, of which 8 fail on the unfixed build (the other 3 are controls pinning behavior that already matched Node).test-buffer-fill.js,test-buffer-alloc.js,test-buffer-write.js,test-buffer-write-fast.jsall pass.test/js/node/buffer-copy-fill-detach.test.ts(detach-during-fill and detach-from-encoding-toString) passes unchanged.Fixture output (Node v24.3.0 == Node v26.3.0 == Bun with this PR)