From 5ca703f01e7aa41eef7e3c30ac821303293cd019 Mon Sep 17 00:00:00 2001 From: Duncan Proctor Date: Tue, 22 Oct 2024 11:24:28 -0400 Subject: [PATCH] buffer: support copy/concat for >= 2^32 bytes --- doc/api/buffer.md | 5 +++ lib/buffer.js | 53 +++++++++++++++++++++-------- src/node_buffer.cc | 6 ++-- test/parallel/test-buffer-concat.js | 13 +++++++ test/parallel/test-buffer-copy.js | 9 +++++ 5 files changed, 68 insertions(+), 18 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index d72e8720c688fa..cfe34a240df1ed 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -1752,6 +1752,11 @@ added: v0.1.90 Copies data from a region of `buf` to a region in `target`, even if the `target` memory region overlaps with `buf`. +If `sourceEnd` exceeds `buf.bytesLength`, only the bytes up to +`buf.bytesLength` will be copied. + +The source slice is truncated if it does not fit into the target slice. + [`TypedArray.prototype.set()`][] performs the same operation, and is available for all TypedArrays, including Node.js `Buffer`s, although it takes different function arguments. diff --git a/lib/buffer.js b/lib/buffer.js index 88625299bced5e..f8993e3f28a93e 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -100,6 +100,8 @@ const { inspect: utilInspect, } = require('internal/util/inspect'); +const assert = require('internal/assert'); + const { codes: { ERR_BUFFER_OUT_OF_BOUNDS, @@ -205,6 +207,7 @@ function toInteger(n, defaultVal) { return defaultVal; } + function copyImpl(source, target, targetStart, sourceStart, sourceEnd) { if (!ArrayBufferIsView(source)) throw new ERR_INVALID_ARG_TYPE('source', ['Buffer', 'Uint8Array'], source); @@ -235,27 +238,45 @@ function copyImpl(source, target, targetStart, sourceStart, sourceEnd) { throw new ERR_OUT_OF_RANGE('sourceEnd', '>= 0', sourceEnd); } + // Clamp sourceEnd to be inbounds of source buffer. + // This is expected behavior and not to throw. + sourceEnd = MathMin(sourceEnd, source.byteLength); + if (targetStart >= target.byteLength || sourceStart >= sourceEnd) return 0; - return _copyActual(source, target, targetStart, sourceStart, sourceEnd); -} - -function _copyActual(source, target, targetStart, sourceStart, sourceEnd) { - if (sourceEnd - sourceStart > target.byteLength - targetStart) - sourceEnd = sourceStart + target.byteLength - targetStart; + // Assert source slice in bounds of source buffer. + // 0 <= sourceStart < sourceEnd <= source.byteLength + assert(0 <= sourceStart && sourceStart < sourceEnd && sourceEnd <= source.byteLength); - let nb = sourceEnd - sourceStart; - const sourceLen = source.byteLength - sourceStart; - if (nb > sourceLen) - nb = sourceLen; + // Assert target slice in bounds of target buffer. + // 0 <= targetStart < target.byteLength + assert(0 <= targetStart && targetStart < target.byteLength); - if (nb <= 0) - return 0; + // Truncate source so that its length doesn't exceed targets. + // This is the expected behavior, not to throw. + const copyLength = MathMin(sourceEnd - sourceStart, target.byteLength - targetStart); + sourceEnd = sourceStart + copyLength; - _copy(source, target, targetStart, sourceStart, nb); + return _copyActual(source, target, targetStart, sourceStart, sourceStart + copyLength); +} - return nb; +// Safely performs native copy from valid source slice to valid target slice. +// - The source slice is not clamped to fit into the target slice. If it won't fit, this throws. +// - If either the source or target slice are out of bounds, this throws. +function _copyActual(source, target, targetStart, sourceStart, sourceEnd) { + assert(isUint8Array(source) && isUint8Array(target)); + // Enforce: 0 <= sourceStart <= sourceEnd <= source.byteLength + assert(0 <= sourceStart && sourceStart <= sourceEnd && sourceEnd <= source.byteLength); + // Enforce: 0 <= targetStart<= target.byteLength + assert(0 <= targetStart && targetStart <= target.byteLength); + + const copyLength = sourceEnd - sourceStart; + const targetCapacity = target.byteLength - targetStart; + assert(copyLength <= targetCapacity); + + _copy(source, target, targetStart, sourceStart, copyLength); + return copyLength; } /** @@ -602,7 +623,9 @@ Buffer.concat = function concat(list, length) { throw new ERR_INVALID_ARG_TYPE( `list[${i}]`, ['Buffer', 'Uint8Array'], list[i]); } - pos += _copyActual(buf, buffer, pos, 0, buf.length); + + const copyLength = MathMin(buf.length, buffer.length - pos); + pos += _copyActual(buf, buffer, pos, 0, copyLength); } // Note: `length` is always equal to `buffer.length` at this point diff --git a/src/node_buffer.cc b/src/node_buffer.cc index cd51d9acf9540d..95cd24f6a6c3b4 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -572,9 +572,9 @@ void SlowCopy(const FunctionCallbackInfo& args) { ArrayBufferViewContents source(args[0]); SPREAD_BUFFER_ARG(args[1].As(), target); - const auto target_start = args[2]->Uint32Value(env->context()).ToChecked(); - const auto source_start = args[3]->Uint32Value(env->context()).ToChecked(); - const auto to_copy = args[4]->Uint32Value(env->context()).ToChecked(); + const auto target_start = args[2]->IntegerValue(env->context()).ToChecked(); + const auto source_start = args[3]->IntegerValue(env->context()).ToChecked(); + const auto to_copy = args[4]->IntegerValue(env->context()).ToChecked(); memmove(target_data + target_start, source.data() + source_start, to_copy); args.GetReturnValue().Set(to_copy); diff --git a/test/parallel/test-buffer-concat.js b/test/parallel/test-buffer-concat.js index 2e7541fb58b063..10027c0546d7a8 100644 --- a/test/parallel/test-buffer-concat.js +++ b/test/parallel/test-buffer-concat.js @@ -22,6 +22,7 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); +const { kMaxLength } = require('buffer'); const zero = []; const one = [ Buffer.from('asdf') ]; @@ -98,3 +99,15 @@ assert.deepStrictEqual( assert.deepStrictEqual(Buffer.concat([new Uint8Array([0x41, 0x42]), new Uint8Array([0x43, 0x44])]), Buffer.from('ABCD')); + +// Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812 +if (2 ** 32 + 1 <= kMaxLength) { + const a = Buffer.alloc(2 ** 31, 0); + const b = Buffer.alloc(2 ** 31, 1); + const c = Buffer.alloc(1, 2); + const destin = Buffer.concat([a, b, c]); + + assert.deepStrictEqual(destin.subarray(0, 2 ** 31), a); + assert.deepStrictEqual(destin.subarray(2 ** 31, 2 ** 32), b); + assert.deepStrictEqual(destin.subarray(2 ** 32), c); +} diff --git a/test/parallel/test-buffer-copy.js b/test/parallel/test-buffer-copy.js index fef20578cfd396..2938ea68c20ca8 100644 --- a/test/parallel/test-buffer-copy.js +++ b/test/parallel/test-buffer-copy.js @@ -1,6 +1,7 @@ 'use strict'; require('../common'); +const { kMaxLength } = require('buffer'); const assert = require('assert'); const b = Buffer.allocUnsafe(1024); @@ -234,3 +235,11 @@ assert.deepStrictEqual(c, b.slice(0, c.length)); // No copying took place: assert.deepStrictEqual(c.toString(), 'C'.repeat(c.length)); } + +// Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812 +if (2 ** 32 + 1 <= kMaxLength) { + const src = Buffer.alloc(2 ** 32 + 1, 1); + const dst = Buffer.alloc(2 ** 32 + 1, 2); + src.copy(dst); + assert.deepStrictEqual(src, dst); +}