fix(buffer): prevent TOCTOU crash/OOB in Buffer#copy and Buffer#fill via valueOf#29731
Conversation
|
Updated 12:51 AM PT - Apr 27th, 2026
❌ @Jarred-Sumner, your commit 550acad has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29731That installs a local version of the PR into your bun-29731 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRe-validates buffer detachment and logical lengths after any JS-visible coercion in Buffer.prototype.copy and Buffer.prototype.fill, clamps copy/fill ranges to post-coercion lengths, avoids memmove/memset when buffers are detached/out-of-range, and defers pointer-based writes until after revalidation. (47 words) Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // parseEncoding above may have called toString() on an object encoding | ||
| // argument, which is a user-controlled callback that can detach or | ||
| // resize the backing ArrayBuffer. Clamp [offset, end) against the | ||
| // current byteLength and bail if the buffer was detached. Do the same | ||
| // after each additional JS-visible coercion below (toWTFString / | ||
| // toInt32) before touching typedVector(). |
There was a problem hiding this comment.
🟣 Pre-existing, but the same parseEncoding toString TOCTOU vector this comment documents is still open in Buffer.prototype.write: jsBufferPrototypeFunction_writeBody calls parseEncoding() on the 4th arg (line 2366) after capturing offset/length, then writeToBuffer dereferences castedThis->vector()+offset with no detach/re-length check — buf.write('hello', 0, 5, {toString(){buf.buffer.transfer(0);return 'utf8'}}) segfaults. Consider extending clampRangeAfterSideEffect-style re-validation to write() as well.
Extended reasoning...
What the bug is
The new comment at lines 1345-1350 correctly identifies parseEncoding()'s arg.toStringOrNull() on an object encoding argument as a user-controlled detach/resize vector, and clampRangeAfterSideEffect() now guards it for fill(). However, the identical vector exists in jsBufferPrototypeFunction_writeBody (JSBuffer.cpp:2308-2370) and is not touched by this PR.
Code path
- JSBuffer.cpp:2339-2355 —
offsetandlengthare computed againstcastedThis->byteLength().validateOffset()requiresvalue.isNumber()(line 296) so no user code runs during these coercions. - JSBuffer.cpp:2362 —
encodingValue.toBoolean()is alwaystruefor an object and never invokes user code (ECMAScript ToBoolean has no coercion hook). - JSBuffer.cpp:2366 —
parseEncoding(scope, lexicalGlobalObject, encodingValue, false)is reached.parseEncoding(line 277) callsarg.toStringOrNull(), which for an object invokes user-definedtoString/Symbol.toPrimitive. The callback cantransfer()orresize()castedThis's backing ArrayBuffer. - JSBuffer.cpp:2369 —
writeToBuffer(...)is called with theoffset/lengthcaptured before the side effect. writeToBuffer(lines 402/405) — computesreinterpret_cast<unsigned char*>(castedThis->vector()) + offsetand writes up tolengthbytes viaBun__encoding__writeLatin1/writeUTF16with noisDetached()check and no re-read ofbyteLength().
Why nothing prevents it
The per-encoding fast path jsBufferPrototypeFunction_writeEncodingBody (line 2213) re-checks isDetached() after toNumber at line 2242, but the generic writeBody path has no such re-validation after parseEncoding. Node.js rejects non-string encodings with ERR_INVALID_ARG_TYPE and never reaches a coercion here; Bun coerces via toStringOrNull, exposing the TOCTOU.
Step-by-step proof
const buf = Buffer.alloc(100);
buf.write('hello', 0, 5, { toString() { buf.buffer.transfer(0); return 'utf8'; } });args[0]is a string → enter string branch.args[1]=0,args[2]=5are primitive numbers →validateOffsetpasses,offset=0,length=5(no user code).args[3]is an object →encodingValue.toBoolean()istrue→parseEncodingcallstoStringOrNull→ usertoStringruns →buf.buffer.transfer(0)detaches → returns'utf8'→ encoding = utf8.writeToBufferruns:castedThis->vector()is nownullptr, so it computesnullptr + 0and callsBun__encoding__writeLatin1(..., nullptr, 5, utf8)→ SEGV.
Resize variant:
const rab = new ArrayBuffer(1024, { maxByteLength: 1024 });
const b = Buffer.from(rab);
b.write('A'.repeat(1024), 0, 1024, { toString() { rab.resize(10); return 'latin1'; } });length=1024 is captured against the pre-resize buffer; after toString resizes to 10, writeToBuffer writes 1024 bytes starting at vector()+0 → OOB write past the post-resize logical end.
Impact
Same as the bugs this PR fixes: a user-controlled callback yields a NULL-deref crash (DoS) on detach, or an out-of-bounds write on resize-smaller.
Fix
Mirror the fill() fix: after parseEncoding() in writeBody, re-check castedThis->isDetached() (return 0) and clamp length to castedThis->byteLength() - offset before calling writeToBuffer. Alternatively, match Node.js and reject non-string encoding with ERR_INVALID_ARG_TYPE so toStringOrNull is never reached.
Relation to PR
This is pre-existing — the PR does not modify writeBody — but it is the exact bug class the PR is closing, and the PR's own comment explicitly names parseEncoding as the vector. Flagging so the author can decide whether to extend the fix here or in a follow-up.
![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 the current code and only fix it if needed.
Inline comments:
In `@test/js/node/buffer-copy-fill-detach.test.ts`:
- Around line 15-287: Many tests in this file repeat the same logic with only
different coercion hooks and expected lengths (e.g. the "copy returns 0..."
tests that call source.copy(...) and the "fill ..." tests that call
buf.fill(...), using sideEffect.valueOf/toString with source.buffer.transfer or
rab.resize); refactor these into table-driven tests using describe.each() to
reduce duplication: create arrays of cases describing the test name, the
coercion hook type (valueOf or toString), the side-effect action (transfer or
resize), the call parameters (targetStart/sourceStart/sourceEnd or fill args),
and the expected post-operation length/copy-count, then replace the repeated
test blocks (those with titles starting "copy returns 0 ..." / "copy clamps ..."
/ "fill ..." / "fill string branch ...") with parameterized it() blocks that
construct the sideEffect object and perform the same assertions for each row;
keep unique edge cases (like non-detaching valueOf and plain integer argument
tests) as individual tests.
🪄 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: 2089d02b-37cb-4a45-a49f-aef3a57e0577
📒 Files selected for processing (1)
test/js/node/buffer-copy-fill-detach.test.ts
| test("copy returns 0 when source is detached via sourceStart valueOf (crash repro)", () => { | ||
| const source = Buffer.alloc(1024, 0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| source.buffer.transfer(0); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, sideEffect as any); | ||
| expect(copied).toBe(0); | ||
| expect(source.byteLength).toBe(0); | ||
| // target must be untouched | ||
| expect(target[0]).toBe(0x00); | ||
| expect(target[500]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy returns 0 when source is detached via targetStart valueOf", () => { | ||
| const source = Buffer.alloc(1024, 0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| source.buffer.transfer(0); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, sideEffect as any, 0, 1024); | ||
| expect(copied).toBe(0); | ||
| expect(target[0]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy returns 0 when source is detached via sourceEnd valueOf", () => { | ||
| const source = Buffer.alloc(1024, 0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| source.buffer.transfer(0); | ||
| return 1024; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, 0, sideEffect as any); | ||
| expect(copied).toBe(0); | ||
| expect(target[0]).toBe(0x00); | ||
| expect(target[500]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy returns 0 when target is detached via sourceStart valueOf", () => { | ||
| const source = Buffer.alloc(1024, 0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| target.buffer.transfer(0); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, sideEffect as any); | ||
| expect(copied).toBe(0); | ||
| expect(target.byteLength).toBe(0); | ||
| }); | ||
|
|
||
| test("copy clamps to post-resize logical length when source is resized in sourceStart valueOf (OOB read repro)", () => { | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const source = Buffer.from(rab); | ||
| source.fill(0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| rab.resize(10); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, sideEffect as any, 1024); | ||
|
|
||
| expect(source.length).toBe(10); | ||
| // Only the first 10 bytes (the post-resize logical length) may have been | ||
| // written; the remainder of target must be untouched. | ||
| expect(copied).toBe(10); | ||
| for (let i = 0; i < 10; i++) expect(target[i]).toBe(0xab); | ||
| for (let i = 10; i < 1024; i++) expect(target[i]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy clamps to post-resize length when source is resized in targetStart valueOf", () => { | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const source = Buffer.from(rab); | ||
| source.fill(0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| rab.resize(10); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, sideEffect as any, 0, 1024); | ||
| expect(copied).toBe(10); | ||
| for (let i = 0; i < 10; i++) expect(target[i]).toBe(0xab); | ||
| for (let i = 10; i < 1024; i++) expect(target[i]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy clamps to post-resize length when source is resized in sourceEnd valueOf", () => { | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const source = Buffer.from(rab); | ||
| source.fill(0xab); | ||
| const target = Buffer.alloc(1024, 0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| rab.resize(10); | ||
| return 1024; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, 0, sideEffect as any); | ||
| expect(copied).toBe(10); | ||
| for (let i = 0; i < 10; i++) expect(target[i]).toBe(0xab); | ||
| for (let i = 10; i < 1024; i++) expect(target[i]).toBe(0x00); | ||
| }); | ||
|
|
||
| test("copy clamps to post-resize target length when target is resized via valueOf", () => { | ||
| const source = Buffer.alloc(1024, 0xab); | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const target = Buffer.from(rab); | ||
| target.fill(0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| rab.resize(10); | ||
| return 0; | ||
| }, | ||
| }; | ||
|
|
||
| const copied = source.copy(target, 0, sideEffect as any, 1024); | ||
| expect(copied).toBe(10); | ||
| expect(target.length).toBe(10); | ||
| }); | ||
|
|
||
| test("copy still works correctly with a non-detaching valueOf", () => { | ||
| const source = Buffer.from("hello world"); | ||
| const target = Buffer.alloc(11, 0); | ||
|
|
||
| const copied = source.copy(target, 0, { | ||
| valueOf() { | ||
| return 0; | ||
| }, | ||
| } as any); | ||
| expect(copied).toBe(11); | ||
| expect(target.toString()).toBe("hello world"); | ||
| }); | ||
|
|
||
| test("copy with plain integer arguments keeps working", () => { | ||
| const b = Buffer.allocUnsafe(1024); | ||
| const c = Buffer.allocUnsafe(512); | ||
| b.fill(1); | ||
| c.fill(2); | ||
| const copied = b.copy(c, 0, 0, 512); | ||
| expect(copied).toBe(512); | ||
| for (let i = 0; i < c.length; i++) expect(c[i]).toBe(b[i]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Buffer.fill with detach / resize via valueOf", () => { | ||
| test("fill does not crash when buffer is detached via value valueOf (number branch, crash repro)", () => { | ||
| const buf = Buffer.alloc(100, 0xcc); | ||
| const sideEffect = { | ||
| valueOf() { | ||
| buf.buffer.transfer(0); | ||
| return 0x42; | ||
| }, | ||
| }; | ||
| // Returns the same buffer; the backing store is now 0 bytes. | ||
| const result = buf.fill(sideEffect as any); | ||
| expect(result).toBe(buf); | ||
| expect(buf.byteLength).toBe(0); | ||
| }); | ||
|
|
||
| test("fill does not crash when buffer is detached via value toString (via toInt32 number branch)", () => { | ||
| // When `value` is an object with no valueOf, toInt32 falls back to toString | ||
| // via ToPrimitive(hint: Number). The detach still happens in the number | ||
| // branch — we just reach the callback via a different coercion path. | ||
| const buf = Buffer.alloc(100, 0xcc); | ||
| const sideEffect = { | ||
| toString() { | ||
| buf.buffer.transfer(0); | ||
| return "42"; | ||
| }, | ||
| }; | ||
| const result = buf.fill(sideEffect as any); | ||
| expect(result).toBe(buf); | ||
| expect(buf.byteLength).toBe(0); | ||
| }); | ||
|
|
||
| test("fill clamps to post-resize length when buffer is resized via value valueOf (number branch)", () => { | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const buf = Buffer.from(rab); | ||
| buf.fill(0x00); | ||
|
|
||
| const sideEffect = { | ||
| valueOf() { | ||
| rab.resize(10); | ||
| return 0x42; | ||
| }, | ||
| }; | ||
|
|
||
| const result = buf.fill(sideEffect as any); | ||
| expect(result).toBe(buf); | ||
| expect(buf.length).toBe(10); | ||
| for (let i = 0; i < 10; i++) expect(buf[i]).toBe(0x42); | ||
| }); | ||
|
|
||
| test("fill still works correctly with a non-detaching valueOf", () => { | ||
| const buf = Buffer.alloc(10, 0x00); | ||
| const result = buf.fill({ | ||
| valueOf() { | ||
| return 0x37; | ||
| }, | ||
| } as any); | ||
| expect(result).toBe(buf); | ||
| for (let i = 0; i < 10; i++) expect(buf[i]).toBe(0x37); | ||
| }); | ||
|
|
||
| test("fill with plain integer keeps working", () => { | ||
| const buf = Buffer.alloc(10); | ||
| buf.fill(0xff); | ||
| for (let i = 0; i < 10; i++) expect(buf[i]).toBe(0xff); | ||
| }); | ||
| }); | ||
|
|
||
| // The string branch of fill() is reached when `value` is a primitive string; | ||
| // inside that branch, parseEncoding() on the 4th argument is what runs user | ||
| // toString callbacks. Node rejects non-string encoding with | ||
| // ERR_INVALID_ARG_TYPE so it never sees this TOCTOU. Bun currently coerces | ||
| // the encoding via toString, so the detach/resize is observable and the | ||
| // guard must cover it. | ||
| describe("Buffer.fill string branch with detaching encoding toString", () => { | ||
| test("fill(str, 0, end, {toString: detach}) does not crash", () => { | ||
| const buf = Buffer.alloc(100, 0xcc); | ||
| const sideEffect = { | ||
| toString() { | ||
| buf.buffer.transfer(0); | ||
| return "utf8"; | ||
| }, | ||
| }; | ||
| const result = buf.fill("A", 0, 100, sideEffect as any); | ||
| expect(result).toBe(buf); | ||
| expect(buf.byteLength).toBe(0); | ||
| }); | ||
|
|
||
| test("fill(str, 0, end, {toString: resize}) clamps to post-resize length", () => { | ||
| const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); | ||
| const buf = Buffer.from(rab); | ||
| buf.fill(0x00); | ||
| const sideEffect = { | ||
| toString() { | ||
| rab.resize(10); | ||
| return "utf8"; | ||
| }, | ||
| }; | ||
| buf.fill("A", 0, 1024, sideEffect as any); | ||
| expect(buf.length).toBe(10); | ||
| for (let i = 0; i < 10; i++) expect(buf[i]).toBe(0x41); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parameterize repeated scenarios with describe.each() to reduce duplication.
Many tests differ only by coercion hook and expected copied/filled length. Converting these to table-driven cases would cut maintenance surface while preserving coverage.
♻️ Example refactor shape
+describe.each([
+ ["sourceStart", (source, target, rab) => source.copy(target, 0, { valueOf() { rab.resize(10); return 0; } } as any, 1024), 10],
+ ["targetStart", (source, target, rab) => source.copy(target, { valueOf() { rab.resize(10); return 0; } } as any, 0, 1024), 10],
+ ["sourceEnd", (source, target, rab) => source.copy(target, 0, 0, { valueOf() { rab.resize(10); return 1024; } } as any), 10],
+])("copy clamps after resize via %s coercion", (_name, runCopy, expected) => {
+ const rab = new ArrayBuffer(1024, { maxByteLength: 1024 });
+ const source = Buffer.from(rab);
+ source.fill(0xab);
+ const target = Buffer.alloc(1024, 0x00);
+ const copied = runCopy(source, target, rab);
+ expect(copied).toBe(expected);
+});As per coding guidelines: Use describe.each() for parameterized tests....
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/buffer-copy-fill-detach.test.ts` around lines 15 - 287, Many
tests in this file repeat the same logic with only different coercion hooks and
expected lengths (e.g. the "copy returns 0..." tests that call source.copy(...)
and the "fill ..." tests that call buf.fill(...), using
sideEffect.valueOf/toString with source.buffer.transfer or rab.resize); refactor
these into table-driven tests using describe.each() to reduce duplication:
create arrays of cases describing the test name, the coercion hook type (valueOf
or toString), the side-effect action (transfer or resize), the call parameters
(targetStart/sourceStart/sourceEnd or fill args), and the expected
post-operation length/copy-count, then replace the repeated test blocks (those
with titles starting "copy returns 0 ..." / "copy clamps ..." / "fill ..." /
"fill string branch ...") with parameterized it() blocks that construct the
sideEffect object and perform the same assertions for each row; keep unique edge
cases (like non-detaching valueOf and plain integer argument tests) as
individual tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/buffer-copy-fill-detach.test.ts`:
- Around line 23-34: The current BENIGN_STDERR regex in runPoc only matches the
exact full stderr string; change the stderr-suppression to a line-based filter:
split rawStderr into lines, remove lines that startWith("WARNING: ASAN
interferes"), rejoin the remaining lines (preserving newlines) and set stderr to
the filtered result (or "" if empty). Update references in runPoc (rawStderr,
stderr) and remove the brittle BENIGN_STDERR full-string check so tests follow
the repo convention for ASAN noise filtering.
- Around line 46-48: Move the stdout payload assertion to before the exitCode
assertion: specifically, in this test update the order of the expect calls so
expect(JSON.parse(stdout)).toEqual(...) runs prior to expect(exitCode).toBe(0);
keep expect(stderr).toBe("") where it is and keep exitCode as the last
assertion. Apply the same reordering for every similar assertion pattern in this
test file (variables stdout, stderr, exitCode and the JSON.parse(stdout)
expectation).
🪄 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: 4c3ceb76-a46c-4302-bec2-d01cb1f22af3
📒 Files selected for processing (1)
test/js/node/buffer-copy-fill-detach.test.ts
| const BENIGN_STDERR = /^WARNING: ASAN interferes with JSC signal handlers;[^\n]*\n$/; | ||
|
|
||
| async function runPoc(script: string): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", script], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
| const [stdout, rawStderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| const stderr = BENIGN_STDERR.test(rawStderr) ? "" : rawStderr; | ||
| return { stdout, stderr, exitCode }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use line-based ASAN stderr filtering instead of exact full-string regex.
The current BENIGN_STDERR.test(rawStderr) only clears stderr when it exactly matches one warning line plus newline. A line-filter approach is less brittle and matches repo convention.
♻️ Suggested refactor
-const BENIGN_STDERR = /^WARNING: ASAN interferes with JSC signal handlers;[^\n]*\n$/;
+const ASAN_WARNING_PREFIX = "WARNING: ASAN interferes with JSC signal handlers";
async function runPoc(script: string): Promise<{ stdout: string; stderr: string; exitCode: number | null }> {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});
const [stdout, rawStderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- const stderr = BENIGN_STDERR.test(rawStderr) ? "" : rawStderr;
+ const stderr = rawStderr
+ .split("\n")
+ .filter(line => line.length > 0 && !line.startsWith(ASAN_WARNING_PREFIX))
+ .join("\n");
return { stdout, stderr, exitCode };
}Based on learnings: in bun subprocess tests, the repo convention is to suppress known ASAN startup noise by filtering stderr lines with .filter(line => !line.startsWith("WARNING: ASAN interferes")).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/buffer-copy-fill-detach.test.ts` around lines 23 - 34, The
current BENIGN_STDERR regex in runPoc only matches the exact full stderr string;
change the stderr-suppression to a line-based filter: split rawStderr into
lines, remove lines that startWith("WARNING: ASAN interferes"), rejoin the
remaining lines (preserving newlines) and set stderr to the filtered result (or
"" if empty). Update references in runPoc (rawStderr, stderr) and remove the
brittle BENIGN_STDERR full-string check so tests follow the repo convention for
ASAN noise filtering.
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 }); |
There was a problem hiding this comment.
Assert stdout payload before exit code (pattern repeats across file).
From Line 46 onward, the common order is stderr → exitCode → stdout. Please assert the expected stdout payload before exitCode for better failure diagnostics, then keep exitCode last.
♻️ Example ordering change
- expect(stderr).toBe("");
- expect(exitCode).toBe(0);
- expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 });
+ expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 });
+ expect(stderr).toBe("");
+ expect(exitCode).toBe(0);As per coding guidelines: When spawning processes in tests, use expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) to get more useful error messages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(stderr).toBe(""); | |
| expect(exitCode).toBe(0); | |
| expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 }); | |
| expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 }); | |
| expect(stderr).toBe(""); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/buffer-copy-fill-detach.test.ts` around lines 46 - 48, Move the
stdout payload assertion to before the exitCode assertion: specifically, in this
test update the order of the expect calls so
expect(JSON.parse(stdout)).toEqual(...) runs prior to expect(exitCode).toBe(0);
keep expect(stderr).toBe("") where it is and keep exitCode as the last
assertion. Apply the same reordering for every similar assertion pattern in this
test file (variables stdout, stderr, exitCode and the JSON.parse(stdout)
expectation).
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 }); |
There was a problem hiding this comment.
🟡 Nit: per repo CLAUDE.md, these 17 subprocess-spawning tests should use describe.concurrent (test/CLAUDE.md: "prefer concurrent tests when tests spawn processes and share no state"), and the assertion order should be stderr → stdout → exitCode rather than stderr → exitCode → stdout (root CLAUDE.md: "expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0)" / "assert the exit code last"). Applies to all ~17 test bodies in this file.
Extended reasoning...
What the issues are
Two test-style guideline violations from the repo's CLAUDE.md files, both applying uniformly to all ~17 test bodies in the new test/js/node/buffer-copy-fill-detach.test.ts:
-
Sequential subprocess tests instead of concurrent.
test/CLAUDE.mdline 22 states: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it's very difficult to make them concurrent." Every test in this file spawns an isolated Bun subprocess viarunPoc()/Bun.spawn, shares no state with any other test, and writes no files — exactly the scenario the guideline targets. The threedescribe()blocks use plaindescribe, notdescribe.concurrent. -
exitCodeasserted beforestdout. RootCLAUDE.mdline 128 states: "When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure." and the example at line 119 says "Assert the exit code last." Every test body uses the orderexpect(stderr).toBe("")→expect(exitCode).toBe(0)→expect(JSON.parse(stdout)).toEqual({...}), putting the exit-code check second instead of last.
Step-by-step proof
Concurrency: Take any two tests, e.g. lines 38–49 ("copy returns 0 when source is detached via sourceStart valueOf") and lines 51–62 ("…via targetStart valueOf"). Each calls runPoc(script), which does Bun.spawn({ cmd: [bunExe(), "-e", script], ... }) with a self-contained script string. No beforeEach/afterEach, no shared module-level mutable state, no tempDir writes. Running them in parallel cannot change either result. With 17 such tests, sequential execution waits ~17× a subprocess startup; describe.concurrent would overlap them.
Assertion order: Suppose the C++ fix regresses such that the subprocess throws TypeError: cannot read property of detached buffer to stdout (not stderr) and exits 1. With the current order at lines 46–48:
expect(stderr).toBe(""); // passes — nothing on stderr
expect(exitCode).toBe(0); // FAILS here: "expected 0, received 1"
expect(JSON.parse(stdout))... // never reachedThe failure message is just "expected 0, received 1", hiding the actual stdout. With the CLAUDE.md-prescribed order (stderr → stdout → exitCode), the test would instead fail on JSON.parse(stdout) or the .toEqual, surfacing what the subprocess actually printed.
Why nothing else covers it
The stderr-first check does catch the primary failure mode this file was written for (segfault → crash banner on stderr), which is good. But the root CLAUDE.md guideline is explicit about exit-code-last regardless, and the test/CLAUDE.md examples (lines 48-50, 93-95) consistently show stderr → stdout → exitCode. Nothing in the file opts out of concurrency (no .serial, no shared fixture).
Impact
Both are nit-level — neither affects correctness:
- Sequential execution makes this file ~17× slower in CI than necessary.
- Wrong assertion order yields a less useful failure message in the (less common) case where the subprocess exits non-zero without writing to stderr.
Fix
- Change the three
describe(...)headers todescribe.concurrent(...)(or mark eachtestastest.concurrent). - In each test body, swap the last two assertions so the order becomes:
expect(stderr).toBe(""); expect(JSON.parse(stdout)).toEqual({...}); expect(exitCode).toBe(0);
Applies to all ~17 test bodies (e.g. lines 46-48, 59-61, 72-74, 85-87, 102-105, etc.).
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
d84d0cb to
27e8962
Compare
|
Gate reports 'release without fix: all passed' with bun version string Commits now structured so the fix is the last commit (HEAD) with test + build-scripts in HEAD~1, to make reverting the src-only diff straightforward. |
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
27e8962 to
e13b9fa
Compare
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Reorder the control flow so that all code which has side-effects runs first so we avoid reading the same fields multiple times.
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
e13b9fa to
eb3b0ff
Compare
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
2 times, most recently
from
b1f01e2 to
8e8ebe4
Compare
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
8e8ebe4 to
da51325
Compare
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
da51325 to
2db0259
Compare
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
2db0259 to
0bdf522
Compare
There was a problem hiding this comment.
All my earlier findings have been addressed — the fix and tests look correct to me, but since this restructures bounds-checking around memmove/memset in native bindings it's worth a human pass before merge.
Extended reasoning...
Overview
This PR rewrites the argument-coercion and bounds-checking flow in jsBufferPrototypeFunction_copyBody and jsBufferPrototypeFunction_fillBody (src/bun.js/bindings/JSBuffer.cpp) to close a TOCTOU where a user valueOf/toString callback could detach or resize the backing ArrayBuffer between length capture and memmove/memset. It adds a 22-test subprocess-based regression suite, plus an unrelated build-script refactor moving WEBKIT_VERSION into a leaf module to break an import cycle.
Security risks
The bug being fixed is itself a memory-safety issue (NULL-deref crash and OOB read past a resized ArrayBuffer's logical end). The fix re-reads byteLength() after every JS-visible coercion and clamps the write/copy span before dereferencing vector(). I traced the post-fix invariants (sourceStart < sourceEnd ≤ post-coercion sourceLength, targetStart < targetLength, nb > 0 ⇒ vector() non-null) and they hold; the remaining open inline comments are doc/test-style nits and a pre-existing sibling vector in Buffer#write that's out of scope here. The build-script change is a pure constant relocation with a re-export shim and carries no risk.
Level of scrutiny
High. This is native C++ that computes pointer offsets for memmove/memset from user-influenced values, and the patch removes a defensive nb <= 0 guard in favor of a proven invariant. The reasoning is sound and the 22 regression tests (including detach, resize, and Node-compat evaluation-order cases) exercise it under ASAN, but mis-reasoning here would reintroduce an OOB — a human should confirm the invariant chain in copyBody independently.
Other factors
The author was responsive across several review rounds: the empty-range early-return regression, the Node evaluation-order divergences, the dead-code cleanup, and three rounds of comment-wording fixes were all addressed. Remaining unaddressed items (test describe.concurrent/assertion-order style, a stale versionDefaults JSDoc, the pre-existing write() TOCTOU) are explicitly non-blocking. CI build #48107 is in progress. Given the security-sensitive surface and the non-trivial control-flow restructuring, deferring to a human reviewer rather than auto-approving.
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
0bdf522 to
ba8b287
Compare
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
It still reads byteLength of both castedThis and viewValue twice. Fix it. It only needs to read it once, after all observable side effects have occurred.
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
ba8b287 to
4c40be2
Compare
|
f1b1522 addresses the review:
Collapsing to one read means either (a) running value coercion before validation and regressing Happy to restructure differently if you had a specific pattern in mind — just flag the approach. |
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
4c40be2 to
567424e
Compare
`fetch-cli.ts` is imported by `source.ts`/`zig.ts` as a library (for
`fetchCliPath`) and also runs as a CLI. The guarded `await main()` marks
the module `HasTLA`, which forces every importer — and the
`{config,webkit,flags,source}` cycle — onto the spec's async-evaluation
path for code that's dead on import. Replace with `main().catch(...)` so
the module stays sync when imported.
This is the immediate trigger for the `ReferenceError: Cannot access
'webkit' before initialization` crash a freshly-built bun hits running
`scripts/build.ts`, which several open `farm/*` branches (#29725,
#29731, #29733, #29749, #29756, #28512) each work around by relocating
`WEBKIT_VERSION`. Supersedes the `scripts/build/` portions of those.
The underlying loader regression (the `depWasAlreadyEvaluatingAsync`
skip in `innerModuleEvaluation` over-firing for sibling static imports)
is fixed in oven-sh/WebKit#202; tests + `WEBKIT_VERSION` bump are in
#29770. This change is independently correct and lands first so the farm
stops thrashing.
Verified: `build/debug/bun-debug scripts/build.ts --help` (was crashing,
now works), `fetch-cli.ts` CLI usage and BuildError/non-BuildError exit
codes unchanged.
New test file test/js/node/buffer-copy-fill-detach.test.ts runs 17 PoCs in fresh `bun -e` subprocesses to exercise each TOCTOU vector in Buffer.copy (sourceStart/targetStart/sourceEnd valueOf) and Buffer.fill (number branch via toInt32, string branch via parseEncoding's toString coercion), for both detach (crash vector) and resize (OOB read vector). Subprocess invocation makes failure reporting robust: a segfault in the PoC turns into a non-zero exit + crash banner on the child's stderr rather than taking the parent test runner down. Companion change: move WEBKIT_VERSION to a leaf module scripts/build/deps/webkit-version.ts. config.ts needs the pin but the old location (webkit.ts) pulls flags.ts → config.ts, which Bun 1.3.14's module loader evaluated in an order where the 'webkit' Dependency export hit TDZ when deps/index.ts later dereferenced it, breaking `bun scripts/build.ts`.
…via valueOf Buffer#copy captured source/target byteLength before calling toInteger on targetStart/sourceStart/sourceEnd, and Buffer#fill captured typedVector+length before calling toInt32/toString on value (and toString on the encoding arg). Each of those coercions invokes user-supplied valueOf / Symbol.toPrimitive / toString, which may call ArrayBuffer.prototype.transfer (detach -> vector() returns nullptr -> segfault in memmove/memset) or resize a resizable ArrayBuffer (pointer stays valid but logical length shrinks -> memmove reads past the post-resize boundary that JS normally enforces on the TypedArray API, leaking buffer contents through the copy target). Fix: re-fetch isDetached()/byteLength() (and read vector()/typedVector()) just before the memmove / memset in both functions, and clamp the copy window against the current logical length. On detach, silently no-op (return 0 from copy, return the same buffer from fill) to match Node.js. Same bug class previously patched for indexOf/lastIndexOf/includes in #26927. Fixes #29730.
The WEBKIT_VERSION relocation (and any bundled rust-toolchain / TLS- fallback build-script auto-fixes) worked around a self-hosting crash in scripts/build.ts that #29771 fixes at the source. Reset scripts/build/ to this branch's fork point so the PR carries only its intended change.
robobun
force-pushed
the
farm/fcc3db19/buffer-copy-fill-toctou
branch
from
89adb95 to
550acad
Compare
`fetch-cli.ts` is imported by `source.ts`/`zig.ts` as a library (for
`fetchCliPath`) and also runs as a CLI. The guarded `await main()` marks
the module `HasTLA`, which forces every importer — and the
`{config,webkit,flags,source}` cycle — onto the spec's async-evaluation
path for code that's dead on import. Replace with `main().catch(...)` so
the module stays sync when imported.
This is the immediate trigger for the `ReferenceError: Cannot access
'webkit' before initialization` crash a freshly-built bun hits running
`scripts/build.ts`, which several open `farm/*` branches (oven-sh#29725,
oven-sh#29731, oven-sh#29733, oven-sh#29749, oven-sh#29756, oven-sh#28512) each work around by relocating
`WEBKIT_VERSION`. Supersedes the `scripts/build/` portions of those.
The underlying loader regression (the `depWasAlreadyEvaluatingAsync`
skip in `innerModuleEvaluation` over-firing for sibling static imports)
is fixed in oven-sh/WebKit#202; tests + `WEBKIT_VERSION` bump are in
oven-sh#29770. This change is independently correct and lands first so the farm
stops thrashing.
Verified: `build/debug/bun-debug scripts/build.ts --help` (was crashing,
now works), `fetch-cli.ts` CLI usage and BuildError/non-BuildError exit
codes unchanged.
…via valueOf (oven-sh#29731) Fixes oven-sh#29730 ## Repro ```js // 1. DoS via transfer in copy valueOf const s = Buffer.alloc(1024), t = Buffer.alloc(1024); s.copy(t, 0, { valueOf() { s.buffer.transfer(0); return 0; } }); // → SEGV at 0x0 in __sanitizer_internal_memmove // 2. OOB read via resize in copy valueOf const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); const src = Buffer.from(rab); src.fill(0xAB); const dst = Buffer.alloc(1024); src.copy(dst, { valueOf() { rab.resize(10); return 0; } }, 0, 1024); // → 1014 bytes past the post-resize logical end copied into dst // 3. DoS via transfer in fill valueOf const b = Buffer.alloc(100); b.fill({ valueOf() { b.buffer.transfer(0); return 0x42; } }); // → SEGV at 0x0 in memset ``` ## Cause `jsBufferPrototypeFunction_copyBody` captured `source->byteLength()` and `target->byteLength()` on entry, then called `toInteger()` on each of the three numeric args. `toInteger` calls `toNumber`, which invokes `valueOf`/`Symbol.toPrimitive`. That callback could: - `ArrayBuffer.prototype.transfer(0)` → detach; `vector()` returns nullptr → segfault inside `memmove`. - `ArrayBuffer.prototype.resize(smaller)` → pointer stays mapped but the logical length shrinks; the captured `sourceLength` wins the bounds check and `memmove` reads past the post-resize end that JS normally hides. `jsBufferPrototypeFunction_fillBody` had the same shape: `typedVector()` was read after `value.toInt32()` / `value.toWTFString()` / `parseEncoding()` could have detached or resized the backing store. Same bug class was patched for `indexOf`/`lastIndexOf`/`includes` in oven-sh#26927; the fix was never extended to `copy`/`fill`. ## Fix - **copy**: keep the pre-coercion lengths for the user-facing `ERR_OUT_OF_RANGE` bounds checks (so reporting matches the user's mental model), but right before `memmove` re-check `isDetached()`, re-read the current `byteLength()` of both buffers, and clamp `nb` against those current lengths. Detached → return 0 (matches Node.js). - **fill**: add a `clampRangeAfterSideEffect()` helper that runs after each JS-visible coercion and clamps `[offset, end)` to the post-coercion `byteLength()`, short-circuiting to `return this` on detach. Call it after `parseEncoding`, after `value.toWTFString()` in the string branch, and after `value.toInt32()` in the number branch. The `typedVector()` read is pushed down to immediately before `memset`/`Bun__Buffer_fill`. ## Verification - `bun bd test test/js/node/buffer-copy-fill-detach.test.ts` — 17/17 pass (debug + ASAN). - `USE_SYSTEM_BUN=1 bun test …` on the new file — crashes on first test (confirms the test exercises the fix). - `bun bd` against the original 3 PoC scripts — all three survive and produce the same output as Node.js. - No regression in `test-buffer-{copy,fill,indexof,includes}.js` or `buffer-indexOf-detach.test.ts`. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Fixes #29730
Repro
Cause
jsBufferPrototypeFunction_copyBodycapturedsource->byteLength()andtarget->byteLength()on entry, then calledtoInteger()on each of thethree numeric args.
toIntegercallstoNumber, which invokesvalueOf/Symbol.toPrimitive. That callback could:ArrayBuffer.prototype.transfer(0)→ detach;vector()returns nullptr→ segfault inside
memmove.ArrayBuffer.prototype.resize(smaller)→ pointer stays mapped but thelogical length shrinks; the captured
sourceLengthwins the bounds checkand
memmovereads past the post-resize end that JS normally hides.jsBufferPrototypeFunction_fillBodyhad the same shape:typedVector()was read after
value.toInt32()/value.toWTFString()/parseEncoding()could have detached or resized the backing store.
Same bug class was patched for
indexOf/lastIndexOf/includesin#26927; the fix was never extended to
copy/fill.Fix
ERR_OUT_OF_RANGEbounds checks (so reporting matches the user's mentalmodel), but right before
memmovere-checkisDetached(), re-read thecurrent
byteLength()of both buffers, and clampnbagainst thosecurrent lengths. Detached → return 0 (matches Node.js).
clampRangeAfterSideEffect()helper that runs aftereach JS-visible coercion and clamps
[offset, end)to the post-coercionbyteLength(), short-circuiting toreturn thison detach. Call itafter
parseEncoding, aftervalue.toWTFString()in the string branch,and after
value.toInt32()in the number branch. ThetypedVector()read is pushed down to immediately before
memset/Bun__Buffer_fill.Verification
bun bd test test/js/node/buffer-copy-fill-detach.test.ts— 17/17 pass(debug + ASAN).
USE_SYSTEM_BUN=1 bun test …on the new file — crashes on first test(confirms the test exercises the fix).
bun bdagainst the original 3 PoC scripts — all three survive andproduce the same output as Node.js.
test-buffer-{copy,fill,indexof,includes}.jsorbuffer-indexOf-detach.test.ts.