From 4ab5e5a8fe7c745113cb58c76407220e79a7f3a9 Mon Sep 17 00:00:00 2001 From: Felipe Coelho Date: Sun, 8 Mar 2026 16:18:46 -0300 Subject: [PATCH] buffer: fix silent data corruption with indices >= 2^32 SlowCopy and FastCopy in node_buffer.cc used uint32_t for target_start, source_start, and to_copy parameters. When these values exceeded 2^32, they silently wrapped around, causing Buffer.copy and Buffer.concat to produce incorrect results with large buffers. Change CopyImpl parameters from uint32_t to size_t, SlowCopy to use IntegerValue (int64_t) instead of Uint32, and FastCopy to use uint64_t. Add a guard against negative int64_t values before casting to size_t to prevent potential out-of-bounds access. Note: other functions in node_buffer.cc (CopyArrayBuffer, StringWrite, FastByteLengthUtf8) have similar uint32_t limitations but are not addressed here to keep this change focused on the reported regression. Fixes: https://github.com/nodejs/node/issues/55422 --- src/node_buffer.cc | 43 ++++++++++++++------- test/pummel/test-buffer-copy-large.js | 54 +++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 test/pummel/test-buffer-copy-large.js diff --git a/src/node_buffer.cc b/src/node_buffer.cc index e40a21288ee79d..22897c99a2db30 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -587,9 +587,9 @@ void StringSlice(const FunctionCallbackInfo& args) { void CopyImpl(Local source_obj, Local target_obj, - const uint32_t target_start, - const uint32_t source_start, - const uint32_t to_copy) { + const size_t target_start, + const size_t source_start, + const size_t to_copy) { ArrayBufferViewContents source(source_obj); SPREAD_BUFFER_ARG(target_obj, target); @@ -598,29 +598,46 @@ void CopyImpl(Local source_obj, // Assume caller has properly validated args. void SlowCopy(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); Local source_obj = args[0]; Local target_obj = args[1]; - const uint32_t target_start = args[2].As()->Value(); - const uint32_t source_start = args[3].As()->Value(); - const uint32_t to_copy = args[4].As()->Value(); + int64_t target_start, source_start, to_copy; + if (!args[2]->IntegerValue(env->context()).To(&target_start) || + !args[3]->IntegerValue(env->context()).To(&source_start) || + !args[4]->IntegerValue(env->context()).To(&to_copy)) { + return; + } + + // Guard against negative values that would wrap to huge size_t. + if (target_start < 0 || source_start < 0 || to_copy < 0) { + return; + } - CopyImpl(source_obj, target_obj, target_start, source_start, to_copy); + CopyImpl(source_obj, + target_obj, + static_cast(target_start), + static_cast(source_start), + static_cast(to_copy)); - args.GetReturnValue().Set(to_copy); + args.GetReturnValue().Set(static_cast(to_copy)); } // Assume caller has properly validated args. -uint32_t FastCopy(Local receiver, +uint64_t FastCopy(Local receiver, Local source_obj, Local target_obj, - uint32_t target_start, - uint32_t source_start, - uint32_t to_copy, + uint64_t target_start, + uint64_t source_start, + uint64_t to_copy, // NOLINTNEXTLINE(runtime/references) FastApiCallbackOptions& options) { HandleScope scope(options.isolate); - CopyImpl(source_obj, target_obj, target_start, source_start, to_copy); + CopyImpl(source_obj, + target_obj, + static_cast(target_start), + static_cast(source_start), + static_cast(to_copy)); return to_copy; } diff --git a/test/pummel/test-buffer-copy-large.js b/test/pummel/test-buffer-copy-large.js new file mode 100644 index 00000000000000..33632ce0e02668 --- /dev/null +++ b/test/pummel/test-buffer-copy-large.js @@ -0,0 +1,54 @@ +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/55422 +// Buffer.copy and Buffer.concat silently produced incorrect results when +// indices >= 2^32 due to uint32_t overflow in the native SlowCopy path. + +const common = require('../common'); +const assert = require('assert'); + +// Cannot test on 32-bit platforms since buffers that large cannot exist. +common.skipIf32Bits(); + +const THRESHOLD = 2 ** 32; // 4 GiB + +// Allocate a large target buffer (just over 4 GiB). Skip if there is not +// enough memory available in the current environment (e.g. CI). +let target; +try { + target = Buffer.alloc(THRESHOLD + 10, 0); +} catch (e) { + if (e.code === 'ERR_MEMORY_ALLOCATION_FAILED' || + /Array buffer allocation failed/.test(e.message)) { + common.skip('insufficient memory for large buffer allocation'); + } + throw e; +} + +const source = Buffer.alloc(10, 111); + +// Test 1: Buffer.copy with targetStart >= 2^32 +// Copy only the first 5 bytes so _copyActual falls through to the native +// _copy (SlowCopy) instead of using TypedArrayPrototypeSet. +source.copy(target, THRESHOLD, 0, 5); + +assert.strictEqual(target[0], 0, 'position 0 must not have been overwritten'); +assert.strictEqual(target[THRESHOLD], 111, 'byte at THRESHOLD must be 111'); +assert.strictEqual(target[THRESHOLD + 4], 111, 'byte at THRESHOLD+4 must be 111'); +assert.strictEqual(target[THRESHOLD + 5], 0, 'byte at THRESHOLD+5 must be 0'); + +// Test 2: Buffer.copy at the 2^32 - 1 boundary (crossing the 32-bit edge) +target.fill(0); +source.copy(target, THRESHOLD - 2, 0, 5); +assert.strictEqual(target[THRESHOLD - 2], 111, 'byte at boundary start'); +assert.strictEqual(target[THRESHOLD + 2], 111, 'byte crossing boundary'); +assert.strictEqual(target[THRESHOLD + 3], 0, 'byte after copied range'); + +// Test 3: Buffer.concat producing a result with total length > 2^32. +// Note: concat uses TypedArrayPrototypeSet (V8), not the native _copy path +// fixed above. This test verifies the V8 path also handles large offsets. +const small = Buffer.alloc(2, 115); +const result = Buffer.concat([target, small]); +assert.strictEqual(result.length, THRESHOLD + 12); +assert.strictEqual(result[THRESHOLD + 10], 115, 'concat: byte from second buffer'); +assert.strictEqual(result[THRESHOLD + 11], 115, 'concat: second byte from second buffer');