Skip to content

Buffer.concat and Buffer.copy silently produce invalid results when the operation involves indices equal or greater than 2^32 #55422

Open
Open
Buffer.concat and Buffer.copy silently produce invalid results when the operation involves indices equal or greater than 2^32#55422
Labels
bufferIssues and PRs related to the buffer subsystem.confirmed-bugIssues and PRs for confirmed bugs.help wantedIssues that need assistance from volunteers or PRs that need help to proceed.regressionIssues related to regressions.

Description

@rotemdan
rotemdan
opened on Oct 17, 2024

Version

v22.9.0, v23.0.0

Platform

Windows 11 x64

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

Buffer

What steps will reproduce the bug?

const largeBuffer = Buffer.alloc(2 ** 32 + 5)
largeBuffer.fill(111)

const result = Buffer.concat([largeBuffer])
console.log(result)

How often does it reproduce? Is there a required condition?

Consistent in v22.9.0 and v23.0.0

What is the expected behavior? Why is that the expected behavior?

All bytes of the return buffer produced by Buffer.concat([largeBuffer]) should be identical to the source:

In this example:

111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, ....

What do you see instead?

In the returned buffer, first 5 bytes are 111, and all following ones are 0.

111, 111, 111, 111, 111, 0, 0, 0, 0, 0, 0, ....

The console.log(result) output looks like:

<Buffer 6f 6f 6f 6f 6f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 4294967251
 more bytes>

Additional information

No response

Activity

  1. targos
    added
    bufferIssues and PRs related to the buffer subsystem.
    on Oct 17, 2024
  2. rotemdan
    changed the title [-]`Buffer.concat` silently produces invalid output when its output size is greater than 4GB[/-] [+]`Buffer.concat` silently produces invalid output when its output size is greater than 4GiB[/+] on Oct 17, 2024
  3. avivkeller
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Oct 17, 2024

  4. rotemdan commented on Oct 17, 2024

    @rotemdan
    rotemdan
    on Oct 17, 2024
    Author

    My current workaround (tested to produce correct results with sizes greater than 4 GiB):

    export function concatBuffers(buffers: Buffer[]) {
	let totalLength = 0

	for (const buffer of buffers) {
		totalLength += buffer.length
	}

	const resultBuffer = Buffer.alloc(totalLength)

	if (totalLength === 0) {
		return resultBuffer
	}

	let writeOffset = 0

	for (const buffer of buffers) {
		resultBuffer.set(buffer, writeOffset)

		writeOffset += buffer.length
	}

	return resultBuffer
}

  5. avivkeller commented on Oct 17, 2024

    @avivkeller
    avivkeller
    on Oct 17, 2024
    Member

    The issue started in v22.7.0. I'll start bisecting. Maybe #54087?

  6. avivkeller
    added
    regressionIssues related to regressions.
    on Oct 17, 2024

  7. avivkeller commented on Oct 17, 2024

    @avivkeller
    avivkeller
    on Oct 17, 2024
    Member

    I've finished bisecting. This was indeed caused by #54087 cc @ronag.

    9f8f26eb2ff36f9352dd85643073af876b9d6b46 is the first bad commit
commit 9f8f26eb2ff36f9352dd85643073af876b9d6b46 (HEAD)
Author: Robert Nagy <ronagy@icloud.com>
Date:   Fri Aug 2 11:19:41 2024 +0200

    buffer: use native copy impl
    
    PR-URL: https://github.com/nodejs/node/pull/54087
    Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
    Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
    Reviewed-By: Daniel Lemire <daniel@lemire.me>

 benchmark/buffers/buffer-copy.js |  6 ------
 lib/buffer.js                    | 11 ++++++-----
 src/node_buffer.cc               | 56 +++++++++++++++++++++++++++-----------------------------
 src/node_external_reference.h    |  9 +++++++++
 4 files changed, 42 insertions(+), 40 deletions(-)

  8. ronag commented on Oct 21, 2024

    @ronag
    ronag
    on Oct 21, 2024
    Member

    Anyone care to open a PR? I think this could be a simple case of just switching to .set(srcBuffer) (instead of using native methods) in the case where the total length exceeds e.g. 2 GB.

  9. ronag
    added
    help wantedIssues that need assistance from volunteers or PRs that need help to proceed.
    on Oct 21, 2024

  10. duncpro commented on Oct 21, 2024

    @duncpro
    duncpro
    on Oct 21, 2024
    Contributor

    I reproduced this on macOS.

    @ronag I'd like to try and tackle this one.

  11. MrJithil commented on Oct 21, 2024

    @MrJithil
    MrJithil
    on Oct 21, 2024
    Member

    I reproduced this on macOS.

    @ronag I'd like to try and tackle this one.

    good luck.

  12. rotemdan commented on Oct 21, 2024

    @rotemdan
    rotemdan
    on Oct 21, 2024
    Author

    This call to _copy is possibly the reason:

    function _copyActual(source, target, targetStart, sourceStart, sourceEnd) {
  if (sourceEnd - sourceStart > target.byteLength - targetStart)
    sourceEnd = sourceStart + target.byteLength - targetStart;

  let nb = sourceEnd - sourceStart;
  const sourceLen = source.byteLength - sourceStart;
  if (nb > sourceLen)
    nb = sourceLen;

  if (nb <= 0)
    return 0;

  _copy(source, target, targetStart, sourceStart, nb); // <--

  return nb;
}

    _copy is imported from some sort of internal binding.

    const {
  byteLengthUtf8,
  compare: _compare,
  compareOffset,
  copy: _copy, // <--
  fill: bindingFill,
  isAscii: bindingIsAscii,
  isUtf8: bindingIsUtf8,
  indexOfBuffer,
  indexOfNumber,
  indexOfString,
  swap16: _swap16,
  swap32: _swap32,
  swap64: _swap64,
  kMaxLength,
  kStringMaxLength,
  atob: _atob,
  btoa: _btoa,
} = internalBinding('buffer');

    A thorough solution is to ensure this method correctly handles large array sizes, or fails.

    Just working around it by falling back to TypedArray.set, would leave the possibility of a future issue if some other code calls _copy.

  13. duncpro commented on Oct 21, 2024

    @duncpro
    duncpro
    on Oct 21, 2024
    Contributor

    So the root cause of this problem is 32-bit integer overflow in SlowCopy in node_buffer.cc here.

    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();

    Apparently Uint32Value performs a wrapping conversion. So that's why in the example below the target buffer only gets filled with 5 bytes.

    const largeBuffer = Buffer.alloc(2 ** 32 + 5)
largeBuffer.fill(111)

const result = Buffer.concat([largeBuffer])
console.log(result); // 6f 6f 6f 6f 6f 00 00 00 ...
                     // 1  2  3  4  5

    Simply replacing Uint32Value with IntegerValue will fix this barring edge cases I've yet to fully consider.

  14. rotemdan commented on Oct 21, 2024

    @rotemdan
    rotemdan
    on Oct 21, 2024
    Author

    I'm not sure what exactly the binding refers to, but I found a candidate method in the C++ code (at node/src/node_buffer.cc) that treats all arguments as Uint32:

    // Assume caller has properly validated args.
void SlowCopy(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(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();

  memmove(target_data + target_start, source.data() + source_start, to_copy);
  args.GetReturnValue().Set(to_copy);
}

    Regardless on whether it's the method used in the binding, using Uint32Value to extract the arguments doesn't seem right.

    This method follows, also taking in uint32_ts:

    uint32_t FastCopy(Local<Value> receiver,
                  const v8::FastApiTypedArray<uint8_t>& source,
                  const v8::FastApiTypedArray<uint8_t>& target,
                  uint32_t target_start,
                  uint32_t source_start,
                  uint32_t to_copy) {
  uint8_t* source_data;
  CHECK(source.getStorageIfAligned(&source_data));

  uint8_t* target_data;
  CHECK(target.getStorageIfAligned(&target_data));

  memmove(target_data + target_start, source_data + source_start, to_copy);

  return to_copy;
}

  15. duncpro commented on Oct 21, 2024

    @duncpro
    duncpro
    on Oct 21, 2024
    Contributor

    @rotemdan this is correct

  16. 10 remaining items

  17. danielalanbates
    added a commit that references this issue on Mar 9, 2026

    buffer: fix Buffer.copy for large offsets >= 2^32

    6ac2ab5
  18. ronag
    added a commit that references this issue on Jun 10, 2026

    buffer: optimize Buffer.prototype.copy

    da189fb
  19. nodejs-github-bot
    added a commit that references this issue on Jun 12, 2026

    buffer: optimize Buffer.prototype.copy

    4383f67
  20. ronag
    added a commit that references this issue on Jun 13, 2026

    buffer: speed up Buffer.prototype.copy via view-level copy

    afe87da
  21. aduh95
    added a commit that references this issue on Jun 18, 2026

    buffer: optimize Buffer.prototype.copy

    509cd1b

  22. LeonxLJX commented on Sep 1, 2026

    @LeonxLJX
    LeonxLJX
    on Sep 1, 2026 · Hidden as low-quality

  23. aron-intframe commented on Sep 16, 2026

    @aron-intframe
    aron-intframe
    on Sep 16, 2026

    Measured on linux-x64 release binaries. big = Buffer.alloc(2**32 + 16, 111) with marker bytes 1..16 written past the 4 GiB boundary, then partial copy() calls that cannot take the TypedArrayPrototypeSet whole-buffer fast path in _copyActual:

    v22.23.2 {'D_ok': False, 'E_ok': False, 'F2_ok': False, 'G_ok': True}
v24.21.0 {'D_ok': False, 'E_ok': False, 'F2_ok': False, 'G_ok': True}
v26.3.1  {'D_ok': False, 'E_ok': False, 'F2_ok': False, 'G_ok': True}
v26.4.0  {'D_ok': True,  'E_ok': True,  'F2_ok': True,  'G_ok': True}
v26.9.0  same as v26.4.0
    • D big.copy(dst, 0, 2**32 + 1, 2**32 + 11): v22/v24 fill dst with 6f6f6f… (source offset wrapped to 1); v26.4+ give 02030405060708090a0b.
    • E big.copy(dst, 0, 1, 2**32 + 16): v22/v24 return 4294967311 but only 15 bytes are written (dst.indexOf(0) === 15).
    • F2 small.copy(dst, 2**32 + 2, 1): v22/v24 write at offset 2 instead of 2**32 + 2.
    • G (the concat case above) passes everywhere now; SlowCopy on v22.x/v24.x still reads the offsets as Uint32.

    So this is fixed on 26.x since v26.4.0 via #63828, but that PR is labeled dont-land-on-v22.x / dont-land-on-v24.x (needs the new V8 CopyArrayBufferBytes API), so both LTS lines still silently corrupt partial copies past 4 GiB.

    Is a v22.x/v24.x-only change to SlowCopy/FastCopy (read offsets as doubles into size_t, no V8 change) acceptable, or should this be closed as fixed in 26.4.0? I can open that PR if wanted. Only checked linux-x64.

  24. aron-intframe
    added a commit that references this issue on Oct 7, 2026

    buffer: optimize Buffer.prototype.copy

    cfe1936
  25. aduh95
    added a commit that references this issue on Oct 7, 2026

    buffer: optimize Buffer.prototype.copy

    875d896
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bufferIssues and PRs related to the buffer subsystem.confirmed-bugIssues and PRs for confirmed bugs.help wantedIssues that need assistance from volunteers or PRs that need help to proceed.regressionIssues related to regressions.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions