Skip to content

fix(buffer): prevent TOCTOU crash/OOB in Buffer#copy and Buffer#fill via valueOf #29731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 4 commits into from
Apr 27, 2026
+539 −62
Merged

fix(buffer): prevent TOCTOU crash/OOB in Buffer#copy and Buffer#fill via valueOf #29731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e0c06c
test(buffer): add TOCTOU regression test for Buffer#copy and Buffer#fill
robobun Apr 26, 2026
ef00380
fix(buffer): prevent TOCTOU crash/OOB in Buffer#copy and Buffer#fill …
robobun Apr 26, 2026
24801f1
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 26, 2026
550acad
Drop scripts/build auto-fixes (superseded by #29771)
Jarred-Sumner Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
221 changes: 159 additions & 62 deletions src/bun.js/bindings/JSBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t>(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<size_t>(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<size_t>(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<size_t>(targetStartD);
size_t sourceStart = static_cast<size_t>(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<size_t>(static_cast<size_t>(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<unsigned char*>(source->vector()) + sourceStart;
auto targetStartPtr = reinterpret_cast<unsigned char*>(target->vector()) + targetStart;
memmove(targetStartPtr, sourceStartPtr, nb);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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, {});
Expand All @@ -1311,40 +1368,85 @@ 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<JSC::JSArrayBufferView>(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<uint8_t>(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<size_t>(viewValueLength, remain);

memmove(head, view->vector(), length);
memmove(head, viewValue->vector(), length);
remain -= length;
head += length;
while (remain >= length && length > 0) {
Expand All @@ -1356,16 +1458,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<uint8_t>(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));
Expand Down
Loading
Loading