Skip to content

fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy #55492

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions doc/api/buffer.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
53 changes: 38 additions & 15 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ const {
inspect: utilInspect,
} = require('internal/util/inspect');

const assert = require('internal/assert');

const {
codes: {
ERR_BUFFER_OUT_OF_BOUNDS,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert here is a bit redundant given the checks that are made in the copyImpl call site.

Copy link
Contributor Author

@duncpro duncpro Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this assert is useful since it makes _copyActual's invariant that source and target need to be Uint8Array explicit.

It indicates to any future/new maintainers needing to call _copyActual what the invariants are. They don't have to guess at the invariants, or deduce the preconditions by exploring all the call-sites of _copyActual.

// 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;
}

/**
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,9 @@ void SlowCopy(const FunctionCallbackInfo<Value>& args) {
ArrayBufferViewContents<char> source(args[0]);
SPREAD_BUFFER_ARG(args[1].As<Object>(), 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);
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-buffer-concat.js
Original file line number Diff line number Diff line change
Expand Up @@ -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') ];
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please consider expanding the comment a bit here with a short summary of what is being tested for so folks don't have to necessarily follow the link to get the gist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is better to have the description of the test inlined.

However I was under the impression that the policy was to use refs?

See #55492 (comment) where I actually removed the explanation at the request of another maintainer.

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);
}
9 changes: 9 additions & 0 deletions test/parallel/test-buffer-copy.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

require('../common');
const { kMaxLength } = require('buffer');
const assert = require('assert');

const b = Buffer.allocUnsafe(1024);
Expand Down Expand Up @@ -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);
}
Loading