diff --git a/src/bun.js/bindings/JSBuffer.cpp b/src/bun.js/bindings/JSBuffer.cpp index 2cfe9247be..69c35e4180 100644 --- a/src/bun.js/bindings/JSBuffer.cpp +++ b/src/bun.js/bindings/JSBuffer.cpp @@ -1985,12 +1985,15 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_SliceWithEncoding(JSC::JSGl template static JSC::EncodedJSValue jsBufferPrototypeFunction_writeEncodingBody(JSC::VM& vm, JSC::JSGlobalObject* lexicalGlobalObject, JSArrayBufferView* castedThis, JSString* str, JSValue offsetValue, JSValue lengthValue) { - size_t byteLength = castedThis->byteLength(); auto scope = DECLARE_THROW_SCOPE(vm); double offset; - double length; + double length = 0; + bool lengthWasUndefined = lengthValue.isUndefined(); + // Convert offset and length to numbers BEFORE caching byteLength, + // as toNumber can call arbitrary JS (via Symbol.toPrimitive) which + // could detach the buffer or cause GC. if (offsetValue.isUndefined()) { offset = 0; } else if (offsetValue.isNumber()) { @@ -1999,23 +2002,52 @@ static JSC::EncodedJSValue jsBufferPrototypeFunction_writeEncodingBody(JSC::VM& offset = offsetValue.toNumber(lexicalGlobalObject); RETURN_IF_EXCEPTION(scope, {}); } - if (lengthValue.isUndefined()) { - length = byteLength - offset; - } else if (lengthValue.isNumber()) { - length = lengthValue.asNumber(); - } else { - length = lengthValue.toNumber(lexicalGlobalObject); - RETURN_IF_EXCEPTION(scope, {}); + if (!lengthWasUndefined) { + if (lengthValue.isNumber()) { + length = lengthValue.asNumber(); + } else { + length = lengthValue.toNumber(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, {}); + } } - if (offset < 0 || offset > byteLength) { + // Re-check if detached after potential JS execution + if (castedThis->isDetached()) [[unlikely]] { + throwTypeError(lexicalGlobalObject, scope, "ArrayBufferView is detached"_s); + return {}; + } + + // Now safe to cache byteLength after all JS calls + size_t byteLength = castedThis->byteLength(); + + // Node.js JS wrapper checks: if (offset < 0 || offset > this.byteLength) + // When offset is NaN, both comparisons return false, so no error is thrown. + // We need to match this behavior exactly. + bool offsetWasNaN = std::isnan(offset); + if (!offsetWasNaN && (offset < 0 || offset > byteLength)) { return Bun::ERR::BUFFER_OUT_OF_BOUNDS(scope, lexicalGlobalObject, "offset"); } - if (length < 0 || length > byteLength - offset) { - return Bun::ERR::BUFFER_OUT_OF_BOUNDS(scope, lexicalGlobalObject, "length"); + // Convert NaN offset to 0 for actual use (matching V8's IntegerValue behavior) + size_t safeOffset = offsetWasNaN ? 0 : static_cast(offset); + + // Calculate max_length + size_t maxLength; + if (lengthWasUndefined) { + maxLength = byteLength - safeOffset; + } else { + // Node.js JS wrapper checks: if (length < 0 || length > this.byteLength - offset) + // When offset is NaN, (byteLength - offset) is NaN, so (length > NaN) is false. + // This means the check passes even for large lengths when offset is NaN. + if (!offsetWasNaN && (length < 0 || length > byteLength - offset)) { + return Bun::ERR::BUFFER_OUT_OF_BOUNDS(scope, lexicalGlobalObject, "length"); + } + // Convert NaN length to 0, negative to 0 (for NaN offset case) + int64_t intLength = (std::isnan(length) || length < 0) ? 0 : static_cast(length); + // Clamp to available buffer space + maxLength = std::min(byteLength - safeOffset, static_cast(intLength)); } - RELEASE_AND_RETURN(scope, writeToBuffer(lexicalGlobalObject, castedThis, str, offset, length, encoding)); + RELEASE_AND_RETURN(scope, writeToBuffer(lexicalGlobalObject, castedThis, str, safeOffset, maxLength, encoding)); } template diff --git a/test/js/node/buffer.test.js b/test/js/node/buffer.test.js index ba74951816..d216ef1a75 100644 --- a/test/js/node/buffer.test.js +++ b/test/js/node/buffer.test.js @@ -3127,3 +3127,50 @@ describe("ERR_BUFFER_OUT_OF_BOUNDS", () => { } } }); + +describe("*Write methods with NaN/invalid offset and length", () => { + // Regression test: NaN offset/length values must be handled safely. + // NaN offset should be treated as 0, and length should be clamped to buffer size. + // This matches Node.js behavior where V8's IntegerValue converts NaN to 0. + const writeMethods = [ + "utf8Write", + "utf16leWrite", + "latin1Write", + "asciiWrite", + "base64Write", + "base64urlWrite", + "hexWrite", + ]; + + for (const method of writeMethods) { + it(`${method} should handle NaN offset and custom length via ToNumber coercion`, () => { + // F1 is a function - ToNumber(F1) returns NaN, which should be treated as 0 + function F1() { + if (!new.target) { + throw "must be called with new"; + } + } + // C3 is a class constructor with Symbol.toPrimitive - ToNumber(C3) returns 215 + class C3 {} + C3[Symbol.toPrimitive] = function () { + return 215; + }; + const buf = Buffer.from("string"); + // F1 as offset -> NaN -> 0, C3 as length -> 215 -> clamped to buf.length + // This should NOT crash, and should write to the buffer starting at offset 0 + const result = buf[method]("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", F1, C3); + // Result should be clamped to buffer size + expect(result).toBeLessThanOrEqual(buf.length); + }); + + it(`${method} should throw on length larger than available buffer space`, () => { + const buf = Buffer.from("string"); + // Length 1000 with valid offset 0 should throw ERR_BUFFER_OUT_OF_BOUNDS + expect(() => buf[method]("test".repeat(100), 0, 1000)).toThrow( + expect.objectContaining({ + code: "ERR_BUFFER_OUT_OF_BOUNDS", + }), + ); + }); + } +});