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. 11 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

  26. 61465 commented on Oct 14, 2026

    @61465
    61465
    on Oct 14, 2026

    Picking this up — proposed LTS-only fix path, asking for a sign-off before coding

    Hi — reading @aron-intframe's 2026-09-16 note and the five earlier attempts (#55492, #61914, #62157, #62500, #62942) that all expired to stale-bot without a technical rejection. Rather than open PR #6 cold, I'd rather confirm the direction so this one makes it through LTS review+CI.

    Scope. Main is fine since v26.4.0 (via #63828, @ronag). This proposal only targets v22.x and v24.x — the two LTS lines where silent data corruption past 4 GiB still ships. #63828 was marked dont-land-on-v22.x / dont-land-on-v24.x because it uses the new V8 CopyArrayBufferBytes API, so the LTS lines need an independent, V8-ABI-neutral patch.

    Current state on both LTS branches (verified against src/node_buffer.cc HEAD of v22.x and v24.x just now):

    // v24.x src/node_buffer.cc:600-622
void CopyImpl(Local<Value> source_obj,
              Local<Value> target_obj,
              const uint32_t target_start,
              const uint32_t source_start,
              const uint32_t to_copy) {
  ArrayBufferViewContents<char> source(source_obj);
  SPREAD_BUFFER_ARG(target_obj, target);
  memmove(target_data + target_start, source.data() + source_start, to_copy);
}

void SlowCopy(const FunctionCallbackInfo<Value>& args) {
  Local<Value> source_obj = args[0];
  Local<Value> target_obj = args[1];
  const uint32_t target_start = args[2].As<Uint32>()->Value();   // ← 32-bit wrap
  const uint32_t source_start = args[3].As<Uint32>()->Value();   // ← 32-bit wrap
  const uint32_t to_copy      = args[4].As<Uint32>()->Value();   // ← 32-bit wrap
  CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);
  args.GetReturnValue().Set(to_copy);
}

    (On v22.x the shape is identical — args[2..4]->Uint32Value(env->context()).To(&target_start) at lines 574–576.)

    Proposed LTS-only fix (no V8 ABI change, no new API required):

    1. Widen CopyImpl params from uint32_t to size_t.
    2. In SlowCopy, read the three offsets as IntegerValue(ctx) (which gives int64_t on every V8 version) and bound-check < 0 + fit-in-size_t before assigning to size_t. Uint32::Value() is replaced by this safer reader.
    3. Keep FastCopy's V8 signature untouched (uint32_t params) — the eager fallback lives in _copyActual (lib/internal/buffer.js): before calling the _copy binding, if to_copy > 0xFFFFFFFF or either offset is > 0xFFFFFFFF, route to a new slow-path-only binding (e.g. _copyLarge) that is not registered via SetFastMethod. V8 fast API keeps its invariants, the sub-4-GiB hot path is untouched.
    4. SlowCopy's return value: args.GetReturnValue().Set(uint32_t) still works for the common case; for to_copy > UINT32_MAX, use Set(static_cast<double>(to_copy)) since partial-copy semantics cap by surviving buffer length and we stay well under Number.MAX_SAFE_INTEGER (2^53).

    Test matrix (ported directly from @aron-intframe's 2026-09-16 scenarios so the test fails on current v22.x/v24.x HEAD and passes on the fix branch):

    Case Setup Expected after fix
    D big = Buffer.alloc(2**32 + 16, 111) + marker bytes 1..16; big.copy(dst, 0, 2**32 + 1, 2**32 + 11) dst starts 02 03 04 05 06 07 08 09 0a 0b
    E big.copy(dst, 0, 1, 2**32 + 16) return value equals bytes actually written (not 4294967311)
    F2 small.copy(dst, 2**32 + 2, 1) writes at offset 2**32 + 2, not 2
    G Buffer.concat([Buffer.alloc(2, 112), Buffer.alloc(2**32 + 5, 111), Buffer.alloc(2, 115)]).slice(-12) [0x6f × 10, 0x73, 0x73]

    Each lives behind common.enoughTestMem so CI without 4 GiB headroom skips cleanly, matching the pattern already in test/parallel/test-buffer-concat-4gb.js.

    Asking for direction before I open the PR:

    1. Is a v22.x/v24.x-only fix welcome, or does the LTS release-captain want this closed as "fixed on 26.4.0 — don't backport"? If (b), I'll close out and the issue can retire.
    2. FastCopy strategy: eager fallback-to-SlowCopy on to_copy > UINT32_MAX (my preference, zero V8-signature change), vs. a FastCopy with size_t params (requires one of V8's int64 fast-API variants if available on v22.x's V8 pin).
    3. Which branches first: v24.x then cherry to v22.x, or parallel PRs. I can carry CI on both.
    4. Work already in flight we should not duplicate.

    If any of (1a/b, 2, 3) is pre-decided, I'll open the PR against that direction with the test matrix + benchmark number showing no regression on the fast path for sub-4-GiB copies. Zero V8 version bump requested.

    cc @ChALkeR + @ronag — thanks for carrying this on 26.x and keeping the thread honest.
    cc @aron-intframe — this is your writeup; please pull me in if you'd rather drive the PR yourself. Credit goes to you + @duncpro, @danielalanbates, @Felipeness, @crawfordxx, @maruthang for the earlier attempts that mapped out the shape.

    🤖 Used Claude Code for the code-trace and comment drafting; all content signed off by me.

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