From 4e0c06c5e8b4206cc30bcbafd64cf7baa80b01c4 Mon Sep 17 00:00:00 2001 From: robobun Date: Sun, 26 Apr 2026 04:08:16 +0000 Subject: [PATCH 1/4] test(buffer): add TOCTOU regression test for Buffer#copy and Buffer#fill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`. --- scripts/build/config.ts | 2 +- scripts/build/deps/webkit-version.ts | 11 + scripts/build/deps/webkit.ts | 8 +- test/js/node/buffer-copy-fill-detach.test.ts | 380 +++++++++++++++++++ 4 files changed, 397 insertions(+), 4 deletions(-) create mode 100644 scripts/build/deps/webkit-version.ts create mode 100644 test/js/node/buffer-copy-fill-detach.test.ts diff --git a/scripts/build/config.ts b/scripts/build/config.ts index fd527a26247..245be8add1e 100644 --- a/scripts/build/config.ts +++ b/scripts/build/config.ts @@ -11,7 +11,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, symlink import { homedir, arch as hostArch, platform as hostPlatform } from "node:os"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { NODEJS_ABI_VERSION, NODEJS_VERSION } from "./deps/nodejs-headers.ts"; -import { WEBKIT_VERSION } from "./deps/webkit.ts"; +import { WEBKIT_VERSION } from "./deps/webkit-version.ts"; import { assert, BuildError } from "./error.ts"; import { clangTargetArch } from "./tools.ts"; import { cyan, dim, green } from "./tty.ts"; diff --git a/scripts/build/deps/webkit-version.ts b/scripts/build/deps/webkit-version.ts new file mode 100644 index 00000000000..9be614f7aab --- /dev/null +++ b/scripts/build/deps/webkit-version.ts @@ -0,0 +1,11 @@ +/** + * WebKit commit pin. Lives in its own leaf module so `config.ts` can import + * the version without pulling in `webkit.ts` — which would transitively pull + * `flags.ts → config.ts`, a cycle that some bundlers/loaders evaluate in an + * order where the `webkit` Dependency export hits TDZ when `deps/index.ts` + * later dereferences it. + * + * Override via `--webkit-version=` to test a branch. + * From https://github.com/oven-sh/WebKit releases. + */ +export const WEBKIT_VERSION = "bdf6aab38a9c6f99df3fd1486406ab6b74180fbb"; diff --git a/scripts/build/deps/webkit.ts b/scripts/build/deps/webkit.ts index 4555a64dde6..a7ffe654d63 100644 --- a/scripts/build/deps/webkit.ts +++ b/scripts/build/deps/webkit.ts @@ -1,9 +1,11 @@ /** - * WebKit commit — determines prebuilt download URL + what to checkout - * for local mode. Override via `--webkit-version=` to test a branch. + * WebKit commit — re-exported from webkit-version.ts so the pin can be + * imported from modules that would otherwise cause a circular import + * through flags.ts → config.ts → webkit.ts. Override via + * `--webkit-version=` to test a branch. * From https://github.com/oven-sh/WebKit releases. */ -export const WEBKIT_VERSION = "bdf6aab38a9c6f99df3fd1486406ab6b74180fbb"; +export { WEBKIT_VERSION } from "./webkit-version.ts"; /** * WebKit (JavaScriptCore) — the JS engine. diff --git a/test/js/node/buffer-copy-fill-detach.test.ts b/test/js/node/buffer-copy-fill-detach.test.ts new file mode 100644 index 00000000000..f0d03206586 --- /dev/null +++ b/test/js/node/buffer-copy-fill-detach.test.ts @@ -0,0 +1,380 @@ +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// TOCTOU guards for Buffer#copy and Buffer#fill. Both functions coerce their +// numeric (and encoding) arguments via user-visible toNumber / toString / +// toInt32 / toPrimitive callbacks. Those callbacks can detach or resize the +// backing ArrayBuffer between the time length and pointer are captured and +// the time memmove / memset runs. Without the guard, a detach turns into a +// NULL-deref crash and a resize turns into an out-of-bounds read from the +// physical (still-mapped) allocation. +// +// Each case runs the PoC in a fresh subprocess via `bun -e`. If the current +// build lacks the fix, the subprocess segfaults (exit 139 / SIGSEGV) and +// the test fails with a readable error — rather than taking the in-process +// test runner down mid-run. The expected semantics match Node.js: copy() +// returns 0 / fill() returns the same Buffer when the target is no longer +// writable, and both clamp the range to the post-resize logical length. + +// stderr lines emitted by the runtime itself (ASAN / JSC banners) that don't +// indicate a test failure. Under bun bd, ASAN prints a WARNING on startup; +// anything else on stderr (Bun crash banner, sanitizer DEADLYSIGNAL dump, +// thrown exception text) is a real failure. +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 }; +} + +describe("Buffer.copy with detach / resize via valueOf", () => { + test("copy returns 0 when source is detached via sourceStart valueOf (crash repro)", async () => { + const { stdout, stderr, exitCode } = await runPoc(` + 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); + console.log(JSON.stringify({ copied, sourceByteLength: source.byteLength, t0: target[0], t500: target[500] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceByteLength: 0, t0: 0x00, t500: 0x00 }); + }); + + test("copy returns 0 when source is detached via targetStart valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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, 0, 1024); + console.log(JSON.stringify({ copied, t0: target[0] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 0, t0: 0x00 }); + }); + + test("copy returns 0 when source is detached via sourceEnd valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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); + console.log(JSON.stringify({ copied, t0: target[0], t500: target[500] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 0, t0: 0x00, t500: 0x00 }); + }); + + test("copy returns 0 when target is detached via sourceStart valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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); + console.log(JSON.stringify({ copied, targetByteLength: target.byteLength })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 0, targetByteLength: 0 }); + }); + + test("copy clamps to post-resize logical length when source is resized in sourceStart valueOf (OOB read repro)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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, 1024); + let oob = 0; + for (let i = 10; i < 1024; i++) if (target[i] !== 0x00) oob++; + console.log(JSON.stringify({ copied, sourceLength: source.length, t0: target[0], t9: target[9], oob })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + // copied == 10, only first 10 bytes written, no OOB + expect(JSON.parse(stdout)).toEqual({ copied: 10, sourceLength: 10, t0: 0xab, t9: 0xab, oob: 0 }); + }); + + test("copy clamps to post-resize length when source is resized in targetStart valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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, 0, 1024); + let oob = 0; + for (let i = 10; i < 1024; i++) if (target[i] !== 0x00) oob++; + console.log(JSON.stringify({ copied, t0: target[0], t9: target[9], oob })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 10, t0: 0xab, t9: 0xab, oob: 0 }); + }); + + test("copy clamps to post-resize length when source is resized in sourceEnd valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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); + let oob = 0; + for (let i = 10; i < 1024; i++) if (target[i] !== 0x00) oob++; + console.log(JSON.stringify({ copied, t0: target[0], t9: target[9], oob })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 10, t0: 0xab, t9: 0xab, oob: 0 }); + }); + + test("copy clamps to post-resize target length when target is resized via valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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, 1024); + console.log(JSON.stringify({ copied, targetLength: target.length })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 10, targetLength: 10 }); + }); + + test("copy still works correctly with a non-detaching valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const source = Buffer.from("hello world"); + const target = Buffer.alloc(11, 0); + const copied = source.copy(target, 0, { valueOf() { return 0; } }); + console.log(JSON.stringify({ copied, str: target.toString() })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 11, str: "hello world" }); + }); + + test("copy with plain integer arguments keeps working", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const b = Buffer.allocUnsafe(1024); + const c = Buffer.allocUnsafe(512); + b.fill(1); + c.fill(2); + const copied = b.copy(c, 0, 0, 512); + let mismatch = 0; + for (let i = 0; i < c.length; i++) if (c[i] !== b[i]) mismatch++; + console.log(JSON.stringify({ copied, mismatch })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 512, mismatch: 0 }); + }); +}); + +describe("Buffer.fill with detach / resize via valueOf", () => { + test("fill does not crash when buffer is detached via value valueOf (number branch, crash repro)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(100, 0xcc); + const sideEffect = { valueOf() { buf.buffer.transfer(0); return 0x42; } }; + const result = buf.fill(sideEffect); + console.log(JSON.stringify({ sameBuf: result === buf, byteLength: buf.byteLength })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, byteLength: 0 }); + }); + + test("fill does not crash when buffer is detached via value toString (via toInt32 number branch)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(100, 0xcc); + const sideEffect = { toString() { buf.buffer.transfer(0); return "42"; } }; + const result = buf.fill(sideEffect); + console.log(JSON.stringify({ sameBuf: result === buf, byteLength: buf.byteLength })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, byteLength: 0 }); + }); + + test("fill clamps to post-resize length when buffer is resized via value valueOf (number branch)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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); + let ok = result === buf && buf.length === 10; + for (let i = 0; i < 10; i++) if (buf[i] !== 0x42) ok = false; + console.log(JSON.stringify({ ok, length: buf.length, b0: buf[0], b9: buf[9] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ ok: true, length: 10, b0: 0x42, b9: 0x42 }); + }); + + test("fill still works correctly with a non-detaching valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(10, 0x00); + const result = buf.fill({ valueOf() { return 0x37; } }); + let ok = result === buf; + for (let i = 0; i < 10; i++) if (buf[i] !== 0x37) ok = false; + console.log(JSON.stringify({ ok })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ ok: true }); + }); + + test("fill with plain integer keeps working", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(10); + buf.fill(0xff); + let ok = true; + for (let i = 0; i < 10; i++) if (buf[i] !== 0xff) ok = false; + console.log(JSON.stringify({ ok })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ ok: true }); + }); +}); + +// 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", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(100, 0xcc); + const sideEffect = { toString() { buf.buffer.transfer(0); return "utf8"; } }; + const result = buf.fill("A", 0, 100, sideEffect); + console.log(JSON.stringify({ sameBuf: result === buf, byteLength: buf.byteLength })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, byteLength: 0 }); + }); + + test("fill(str, 0, end, {toString: resize}) clamps to post-resize length", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + 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); + let ok = buf.length === 10; + for (let i = 0; i < 10; i++) if (buf[i] !== 0x41) ok = false; + console.log(JSON.stringify({ ok, length: buf.length })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ ok: true, length: 10 }); + }); +}); + +// Argument evaluation order must match Node.js: a later argument's +// valueOf / coercion must not run when an earlier argument is already +// trivially invalid, and an empty write range must short-circuit before +// coercing `value` at all. +describe("Buffer.copy / fill argument evaluation order (Node-compat)", () => { + test("copy(target, -1, {valueOf:throws}) rejects targetStart before calling sourceStart valueOf", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const source = Buffer.alloc(10); + const target = Buffer.alloc(10); + let called = false; + try { + source.copy(target, -1, { valueOf() { called = true; throw new Error("boom"); } }); + console.log(JSON.stringify({ threw: false, called })); + } catch (e) { + console.log(JSON.stringify({ threw: true, code: e.code ?? null, called })); + } + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ threw: true, code: "ERR_OUT_OF_RANGE", called: false }); + }); + + test("fill({valueOf:throws}, 5, 3) returns buf without coercing value (empty range)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(10, 0x11); + let called = false; + const result = buf.fill({ valueOf() { called = true; throw new Error("boom"); } }, 5, 3); + console.log(JSON.stringify({ sameBuf: result === buf, called, b0: buf[0] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, called: false, b0: 0x11 }); + }); + + test("fill(emptyUint8Array, 5, 3) returns buf without ERR_INVALID_ARG_VALUE (empty range)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(10, 0x11); + const result = buf.fill(new Uint8Array(0), 5, 3); + console.log(JSON.stringify({ sameBuf: result === buf, b0: buf[0] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, b0: 0x11 }); + }); + + test("fill(detachedView, 5, 3) returns buf without TypeError (empty range)", async () => { + const { stderr, exitCode, stdout } = await runPoc(` + const ab = new ArrayBuffer(10); + const view = new Uint8Array(ab); + ab.transfer(0); + const buf = Buffer.alloc(10, 0x11); + const result = buf.fill(view, 5, 3); + console.log(JSON.stringify({ sameBuf: result === buf, b0: buf[0] })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ sameBuf: true, b0: 0x11 }); + }); + + test("copy: sourceStart primitive stays valid when sourceEnd valueOf shrinks source", async () => { + // sourceStart=100 is valid against original length 1024. sourceEnd's + // valueOf resizes to 50, then returns 200. Node bounds-checks + // sourceStart against the pre-sourceEnd-coercion length (1024, so + // passing) and the copy then computes 0 bytes. Bun must not throw + // ERR_OUT_OF_RANGE against the post-resize length (50). + const { stderr, exitCode, stdout } = await runPoc(` + const rab = new ArrayBuffer(1024, { maxByteLength: 1024 }); + const source = Buffer.from(rab); source.fill(0xab); + const target = Buffer.alloc(1024); + const copied = source.copy(target, 0, 100, { valueOf() { rab.resize(50); return 200; } }); + console.log(JSON.stringify({ copied, sourceLength: source.length })); + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ copied: 0, sourceLength: 50 }); + }); + + test("fill: invalid encoding throws ERR_UNKNOWN_ENCODING even when offset/end are out of range", async () => { + // Node validates encoding BEFORE validateNumber(offset/end), so when + // both are wrong the encoding error wins. Without this ordering my + // rewrite was throwing ERR_OUT_OF_RANGE instead. + const { stderr, exitCode, stdout } = await runPoc(` + const buf = Buffer.alloc(10); + try { buf.fill("a", 0, 11, "bogus"); console.log("no throw"); } + catch (e) { console.log(JSON.stringify({ code: e.code })); } + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ code: "ERR_UNKNOWN_ENCODING" }); + }); +}); From ef00380ce37f7fa733b07445264fbe4df27f2529 Mon Sep 17 00:00:00 2001 From: robobun Date: Sun, 26 Apr 2026 04:08:31 +0000 Subject: [PATCH 2/4] fix(buffer): prevent TOCTOU crash/OOB in Buffer#copy and Buffer#fill 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. --- src/bun.js/bindings/JSBuffer.cpp | 219 ++++++++++++++++++++++--------- 1 file changed, 157 insertions(+), 62 deletions(-) diff --git a/src/bun.js/bindings/JSBuffer.cpp b/src/bun.js/bindings/JSBuffer.cpp index 5d870dd1645..1e4f99d12cc 100644 --- a/src/bun.js/bindings/JSBuffer.cpp +++ b/src/bun.js/bindings/JSBuffer.cpp @@ -1168,49 +1168,78 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_copyBody(JSC::JSGlobalObjec return Bun::ERR::INVALID_ARG_TYPE(throwScope, lexicalGlobalObject, "target"_s, "Buffer or Uint8Array"_s, targetValue); } - auto sourceLength = source->byteLength(); - auto targetLength = target->byteLength(); - - size_t targetStart = 0; - if (targetStartValue.isUndefined()) { - } else { - double targetStartD = targetStartValue.isAnyInt() ? targetStartValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, targetStartValue, 0); + // Coerce each argument, then immediately bound-check against the + // buffer state AT THAT POINT — matches Node's lib/buffer.js evaluation + // order. Each coerce+check pair reads byteLength() fresh because an + // earlier argument's valueOf may have shrunk the source, and a + // sourceStart that was valid against the pre-coercion length must not + // be retroactively invalidated by a later sourceEnd's side effect + // (Node returns 0 in that case). After all coercions finish, a final + // byteLength() read clamps sourceEnd to the post-side-effect length + // so the memmove stays inside the current logical range, even if + // valueOf resized the buffer after its own argument was checked. + // + // toInteger() calls toNumber() which invokes user valueOf / + // Symbol.toPrimitive. Those callbacks can transfer() (detach → + // vector() returns nullptr) or resize() a resizable ArrayBuffer + // (pointer stays valid, logical length shrinks). The final clamp + // handles both: a detached buffer has byteLength 0 → sourceStart >= + // sourceEnd → we return 0 without touching the null vector. + double targetStartD = 0; + if (!targetStartValue.isUndefined()) { + targetStartD = targetStartValue.isAnyInt() ? targetStartValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, targetStartValue, 0); RETURN_IF_EXCEPTION(throwScope, {}); - if (targetStartD < 0) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "targetStart"_s, 0, targetLength, targetStartValue); - targetStart = static_cast(targetStartD); + if (targetStartD < 0) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "targetStart"_s, 0, target->byteLength(), targetStartValue); } - size_t sourceStart = 0; - if (sourceStartValue.isUndefined()) { - } else { - double sourceStartD = sourceStartValue.isAnyInt() ? sourceStartValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, sourceStartValue, 0); + double sourceStartD = 0; + if (!sourceStartValue.isUndefined()) { + sourceStartD = sourceStartValue.isAnyInt() ? sourceStartValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, sourceStartValue, 0); RETURN_IF_EXCEPTION(throwScope, {}); - if (sourceStartD < 0 || sourceStartD > sourceLength) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "sourceStart"_s, 0, sourceLength, sourceStartValue); - sourceStart = static_cast(sourceStartD); - } - - size_t sourceEnd = sourceLength; - if (sourceEndValue.isUndefined()) { - } else { - double sourceEndD = sourceEndValue.isAnyInt() ? sourceEndValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, sourceEndValue, 0); + // sourceStart is bound-checked against source.length as seen + // here — BEFORE the later sourceEnd coercion gets a chance to + // shrink the source. A primitive sourceStart valid against the + // original length must stay valid even if sourceEnd's valueOf + // resizes mid-call (Node behavior: the call then just copies 0). + auto sourceLengthAtCheck = source->byteLength(); + if (sourceStartD < 0 || sourceStartD > sourceLengthAtCheck) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "sourceStart"_s, 0, sourceLengthAtCheck, sourceStartValue); + } + + bool sourceEndGiven = !sourceEndValue.isUndefined(); + double sourceEndD = 0; + if (sourceEndGiven) { + sourceEndD = sourceEndValue.isAnyInt() ? sourceEndValue.asNumber() : toInteger(throwScope, lexicalGlobalObject, sourceEndValue, 0); RETURN_IF_EXCEPTION(throwScope, {}); - if (sourceEndD < 0) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "sourceEnd"_s, 0, sourceLength, sourceEndValue); - sourceEnd = static_cast(sourceEndD); + if (sourceEndD < 0) return Bun::ERR::OUT_OF_RANGE(throwScope, lexicalGlobalObject, "sourceEnd"_s, 0, source->byteLength(), sourceEndValue); } + // Single post-coercion read for the hot path. byteLength is 0 for a + // detached buffer so the range checks below naturally no-op a detach + // into "copy 0". + auto sourceLength = source->byteLength(); + auto targetLength = target->byteLength(); + + size_t targetStart = static_cast(targetStartD); + size_t sourceStart = static_cast(sourceStartD); + // If valueOf resized the source smaller, don't read past the new end + // even if the user passed a larger sourceEnd — that would bypass the + // JS-enforced resize boundary and leak hidden bytes into target. + size_t sourceEnd = sourceEndGiven ? std::min(static_cast(sourceEndD), sourceLength) : sourceLength; + if (targetStart >= targetLength || sourceStart >= sourceEnd) { return JSValue::encode(jsNumber(0)); } if (sourceEnd - sourceStart > targetLength - targetStart) - sourceEnd = sourceStart + targetLength - targetStart; - - ssize_t nb = sourceEnd - sourceStart; - auto sourceLen = sourceLength - sourceStart; - if (nb > sourceLen) nb = sourceLen; - - if (nb <= 0) return JSValue::encode(jsNumber(0)); - + sourceEnd = sourceStart + (targetLength - targetStart); + + // nb > 0 here: `sourceStart >= sourceEnd` and `targetStart >= + // targetLength` were both ruled out above, so the clamp on the + // preceding line assigns `sourceStart + (targetLength - targetStart)` + // with a strictly positive addend, keeping sourceEnd > sourceStart. + // vector() is only nullptr when byteLength == 0, which would have + // forced sourceStart >= sourceEnd and returned above. + size_t nb = sourceEnd - sourceStart; auto sourceStartPtr = reinterpret_cast(source->vector()) + sourceStart; auto targetStartPtr = reinterpret_cast(target->vector()) + targetStart; memmove(targetStartPtr, sourceStartPtr, nb); @@ -1264,6 +1293,19 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_fillBody(JSC::JSGlobalObjec } auto value = callFrame->uncheckedArgument(0); + // Capture byteLength up front for two orthogonal purposes: + // 1. The upper-bound argument to validateNumber(end) so `end > + // buf.length` throws ERR_OUT_OF_RANGE with Node's wording and + // against the length the caller saw (matches Node: parseEncoding + // may run a user toString before this check, but the Node- + // compat error message still uses the pre-call length). + // 2. The default for `end` when the caller omitted it. + // This read is pre-coercion; it's only ever compared against the + // user's raw number or used as a default. The actual write range is + // clamped against a single post-coercion byteLength read (after all + // observable side effects) right before the memset/memmove — THAT + // read is what keeps the write inside the current logical length + // even if valueOf detached or resized the buffer. const size_t limit = castedThis->byteLength(); size_t offset = 0; size_t end = limit; @@ -1294,13 +1336,28 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_fillBody(JSC::JSGlobalObjec endValue = jsUndefined(); } + // ── 1. Encoding parse (FIRST validation) ──────────────────────────── + // Node validates encoding before either `validateNumber` call, so + // `fill("a", 0, buf.length + 1, "bogus")` and `fill("a", -1, 0, + // "bogus")` throw ERR_UNKNOWN_ENCODING (not ERR_OUT_OF_RANGE) — the + // encoding error wins. parseEncoding is also the first + // user-JS-visible call: `toString` on an object encoding can detach + // or resize castedThis; the post-coercion clamp further down reads + // byteLength() once more to catch any such effect. if (!encodingValue.isUndefined() && value.isString()) { encoding = parseEncoding(scope, lexicalGlobalObject, encodingValue, true); RETURN_IF_EXCEPTION(scope, {}); } - // https://github.com/nodejs/node/blob/v22.9.0/lib/buffer.js#L1066-L1079 - // https://github.com/nodejs/node/blob/v22.9.0/lib/buffer.js#L122 + // ── 2. Pure offset / end coercion (no user JS) ────────────────────── + // validateNumber rejects non-numbers without coercion, and toLength + // on a number is a C++ conversion. parseEncoding above may have + // detached/resized the buffer, but the `limit` captured pre-coercion + // is still the correct Node-compat upper bound for ERR_OUT_OF_RANGE; + // the final write range is clamped against a separate post-coercion + // byteLength read further down. + // https://github.com/nodejs/node/blob/v22.9.0/lib/buffer.js#L1066-L1079 + // https://github.com/nodejs/node/blob/v22.9.0/lib/buffer.js#L122 if (!offsetValue.isUndefined()) { Bun::V::validateNumber(scope, lexicalGlobalObject, offsetValue, "offset"_s, jsNumber(0), jsNumber(Bun::Buffer::kMaxLength)); RETURN_IF_EXCEPTION(scope, {}); @@ -1311,40 +1368,83 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_fillBody(JSC::JSGlobalObjec RETURN_IF_EXCEPTION(scope, {}); end = endValue.toLength(lexicalGlobalObject); } + + // Node short-circuits empty/inverted ranges before coercing `value`, + // so a throwing valueOf / empty Uint8Array / detached view passed + // with an empty range stays a no-op. if (offset >= end) { RELEASE_AND_RETURN(scope, JSValue::encode(castedThis)); } + // ── 3. Value coercion per branch ──────────────────────────────────── + // toInt32 / toWTFString can invoke user valueOf / toString that + // detaches or resizes castedThis. Captures enough to do the write + // without touching `value` again. + WTF::String stringValue; + JSC::JSArrayBufferView* viewValue = nullptr; + size_t viewValueLength = 0; + uint8_t byteValue = 0; + enum { StringBranch, ViewBranch, ByteBranch } branch; + if (value.isString()) { - auto startPtr = castedThis->typedVector() + offset; - auto str_ = value.toWTFString(lexicalGlobalObject); + branch = StringBranch; + stringValue = value.toWTFString(lexicalGlobalObject); RETURN_IF_EXCEPTION(scope, {}); - ZigString str = Zig::toZigString(str_); - - if (str.len == 0) { - memset(startPtr, 0, end - offset); - } else if (!Bun__Buffer_fill(&str, startPtr, end - offset, encoding)) [[unlikely]] { - return Bun::ERR::INVALID_ARG_VALUE(scope, lexicalGlobalObject, "value"_s, value); - } } else if (auto* view = dynamicDowncast(value)) { - auto* startPtr = castedThis->typedVector() + offset; - auto* head = startPtr; - size_t remain = end - offset; - - if (view->isDetached()) [[unlikely]] { + branch = ViewBranch; + viewValue = view; + if (viewValue->isDetached()) [[unlikely]] { throwVMTypeError(lexicalGlobalObject, scope, "Uint8Array is detached"_s); return {}; } - - size_t length = view->byteLength(); - if (length == 0) [[unlikely]] { + // Single read of viewValue->byteLength() — used both for the + // empty check here and for the repeat length in the write loop. + // No further side effects can run before the write, so the value + // is stable. + viewValueLength = viewValue->byteLength(); + if (viewValueLength == 0) [[unlikely]] { scope.throwException(lexicalGlobalObject, createError(lexicalGlobalObject, Bun::ErrorCode::ERR_INVALID_ARG_VALUE, "Buffer cannot be empty"_s)); return {}; } + } else { + branch = ByteBranch; + byteValue = static_cast(value.toInt32(lexicalGlobalObject) & 0xFF); + RETURN_IF_EXCEPTION(scope, {}); + } + + // ── 4. Post-coercion clamp ────────────────────────────────────────── + // Read castedThis->byteLength() once here, after every observable + // side effect has run, and clamp the write range into it. This is + // what keeps the write inside the current logical length if valueOf + // shrank a resizable ArrayBuffer, and folds detach into a clean + // return (byteLength 0 → offset >= end → return below). + const size_t postLimit = castedThis->byteLength(); + if (offset > postLimit) offset = postLimit; + if (end > postLimit) end = postLimit; + if (offset >= end) { + RELEASE_AND_RETURN(scope, JSValue::encode(castedThis)); + } - length = std::min(length, remain); + // ── 5. Write. typedVector() is non-null here (postLimit > 0). ─────── + auto* startPtr = castedThis->typedVector() + offset; + size_t span = end - offset; + + switch (branch) { + case StringBranch: { + ZigString str = Zig::toZigString(stringValue); + if (str.len == 0) { + memset(startPtr, 0, span); + } else if (!Bun__Buffer_fill(&str, startPtr, span, encoding)) [[unlikely]] { + return Bun::ERR::INVALID_ARG_VALUE(scope, lexicalGlobalObject, "value"_s, value); + } + break; + } + case ViewBranch: { + auto* head = startPtr; + size_t remain = span; + size_t length = std::min(viewValueLength, remain); - memmove(head, view->vector(), length); + memmove(head, viewValue->vector(), length); remain -= length; head += length; while (remain >= length && length > 0) { @@ -1356,16 +1456,11 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_fillBody(JSC::JSGlobalObjec if (remain > 0) { memmove(head, startPtr, remain); } - } else { - auto value_ = value.toInt32(lexicalGlobalObject) & 0xFF; - RETURN_IF_EXCEPTION(scope, {}); - - auto value_uint8 = static_cast(value_); - RETURN_IF_EXCEPTION(scope, {}); - - auto startPtr = castedThis->typedVector() + offset; - auto endPtr = castedThis->typedVector() + end; - memset(startPtr, value_uint8, endPtr - startPtr); + break; + } + case ByteBranch: + memset(startPtr, byteValue, span); + break; } RELEASE_AND_RETURN(scope, JSValue::encode(castedThis)); From 24801f1adb3a55ef293fe91bb4e4f0542354d0b0 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sun, 26 Apr 2026 07:15:48 +0000 Subject: [PATCH 3/4] [autofix.ci] apply automated fixes --- src/bun.js/bindings/JSBuffer.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/bun.js/bindings/JSBuffer.cpp b/src/bun.js/bindings/JSBuffer.cpp index 1e4f99d12cc..539bb099719 100644 --- a/src/bun.js/bindings/JSBuffer.cpp +++ b/src/bun.js/bindings/JSBuffer.cpp @@ -1384,7 +1384,9 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_fillBody(JSC::JSGlobalObjec JSC::JSArrayBufferView* viewValue = nullptr; size_t viewValueLength = 0; uint8_t byteValue = 0; - enum { StringBranch, ViewBranch, ByteBranch } branch; + enum { StringBranch, + ViewBranch, + ByteBranch } branch; if (value.isString()) { branch = StringBranch; From 550acad3cac322f49bd0141cd7b00ddbd12489dc Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 26 Apr 2026 23:12:54 -0700 Subject: [PATCH 4/4] Drop scripts/build auto-fixes (superseded by #29771) 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. --- scripts/build/config.ts | 2 +- scripts/build/deps/webkit-version.ts | 11 ----------- scripts/build/deps/webkit.ts | 8 +++----- 3 files changed, 4 insertions(+), 17 deletions(-) delete mode 100644 scripts/build/deps/webkit-version.ts diff --git a/scripts/build/config.ts b/scripts/build/config.ts index 245be8add1e..fd527a26247 100644 --- a/scripts/build/config.ts +++ b/scripts/build/config.ts @@ -11,7 +11,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, symlink import { homedir, arch as hostArch, platform as hostPlatform } from "node:os"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { NODEJS_ABI_VERSION, NODEJS_VERSION } from "./deps/nodejs-headers.ts"; -import { WEBKIT_VERSION } from "./deps/webkit-version.ts"; +import { WEBKIT_VERSION } from "./deps/webkit.ts"; import { assert, BuildError } from "./error.ts"; import { clangTargetArch } from "./tools.ts"; import { cyan, dim, green } from "./tty.ts"; diff --git a/scripts/build/deps/webkit-version.ts b/scripts/build/deps/webkit-version.ts deleted file mode 100644 index 9be614f7aab..00000000000 --- a/scripts/build/deps/webkit-version.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * WebKit commit pin. Lives in its own leaf module so `config.ts` can import - * the version without pulling in `webkit.ts` — which would transitively pull - * `flags.ts → config.ts`, a cycle that some bundlers/loaders evaluate in an - * order where the `webkit` Dependency export hits TDZ when `deps/index.ts` - * later dereferences it. - * - * Override via `--webkit-version=` to test a branch. - * From https://github.com/oven-sh/WebKit releases. - */ -export const WEBKIT_VERSION = "bdf6aab38a9c6f99df3fd1486406ab6b74180fbb"; diff --git a/scripts/build/deps/webkit.ts b/scripts/build/deps/webkit.ts index a7ffe654d63..4555a64dde6 100644 --- a/scripts/build/deps/webkit.ts +++ b/scripts/build/deps/webkit.ts @@ -1,11 +1,9 @@ /** - * WebKit commit — re-exported from webkit-version.ts so the pin can be - * imported from modules that would otherwise cause a circular import - * through flags.ts → config.ts → webkit.ts. Override via - * `--webkit-version=` to test a branch. + * WebKit commit — determines prebuilt download URL + what to checkout + * for local mode. Override via `--webkit-version=` to test a branch. * From https://github.com/oven-sh/WebKit releases. */ -export { WEBKIT_VERSION } from "./webkit-version.ts"; +export const WEBKIT_VERSION = "bdf6aab38a9c6f99df3fd1486406ab6b74180fbb"; /** * WebKit (JavaScriptCore) — the JS engine.