fix: support buffers greater than `2^32` bytes in length in `Buffer.concat` and `Buffer.copy` by duncpro · Pull Request #55492 · nodejs/node · GitHub

This pull request...

Adds support for Buffer.copy with lengths greater than or equal to 2^32 bytes.
Adds support for Buffer.concat where the result buffer has length greater than or equal to 2^32 bytes.
Improves the docs for Buffer.copy.
- They did not mention the behavior when the source slice cannot fit into the target slice.
- They did not mention the behavior when sourceEnd exceeds the total number of bytes in the source buffer.
Refactors implementation of Buffer.copy and Buffer.concat.
- Adds asserts verifying its impossible to copy bytes from outside the source buffer.
- Adds asserts verifying it's impossible to copy bytes anywhere except the target buffer.

Hi! Can you please amend your commit message to follow the guidelines of this repository? It needs to be under 80 characters

Additionally, can you add a test to validate this behavior

I noticed that parallel/test-stream-readable-unpipe-resume is failing on Ubuntu CI, which is strange because it's not failing on my Mac. I'll investigate.

I noticed that parallel/test-stream-readable-unpipe-resume is failing on Ubuntu CI, which is strange because it's not failing on my Mac. I'll investigate.

No need, it's a flaky test. It's a known process deadlock that's being investigated.

@ronag I've requested your review because you were involved in the issue this closes, and you know a lot about buffers that I don't

Don't merge this yet. It's possible to read bytes from outside the source buffer into the target buffer.
The current implementation _copyActual does not check for negatives in source and target.

For example...

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;

   return [source, target, targetStart, sourceStart, nb];
}

let src = Buffer.alloc(50);
let dest = Buffer.alloc(100);
const [_, __, ___, sourceStart, copy_len] = _copyActual(src, dest, 0, -10, 0); 
console.log([sourceStart, copy_len]); // [ -10, 10 ]

Its probably better here not to trust the caller of _copyActual and to guard the call to _copy with explicit checks.
Our choice is to either throw an error, or to silently clamp the bounds. I prefer the former.

Resolved by: 3c18a13

@nodejs/buffer PTAL ☝️

I've updated _copyActual to guard _copy (native binding here) against negative numbers, so there is no risk of potentially allowing out of bounds access in the future.

The expected behavior for an out of bounds copy is actually specified by test/parallel/test-buffer-copy.js.

At the latest commit 3c18a13, test/parallel/test-buffer-copy.js is passing (along with everything else).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.41%. Comparing base (7bc3e16) to head (5ca703f).
⚠️ Report is 3089 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #55492    +/-   ##
========================================
  Coverage   88.41%   88.41%            
========================================
  Files         653      654     +1     
  Lines      187435   187575   +140     
  Branches    36077    36081     +4     
========================================
+ Hits       165714   165851   +137     
+ Misses      14960    14954     -6     
- Partials     6761     6770     +9

Files with missing lines	Coverage Δ
lib/buffer.js	100.00% <100.00%> (ø)
src/node_buffer.cc	70.36% <100.00%> (+0.10%)	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Not sure what do about code coverage issue. It seems to me that the range check should stay in as preventative measure.

No need to worry about codecov, it's not 100% accurate

I think we could just make these asserts. The perf impact is negligable...

https://github.com/nodejs/node/blob/209289ceb27c660c065210308784debc58863c25/lib/buffer.js#L271-L275

Turns out the issue also occurs in Buffer.copy. See my comment on the issue thread.

What does this mean?

Shouldn't an assert be used instead?

If not, IMO a comment isn't needed at all

I agree it is redundant. This comment has been removed as of commit 5ca703f.

Suggested change

      
            // Test its possible to concat buffers where the total length of the result in bytes
          
            // is greater than 2^32.
          
            // Ref: ...

Comment is replaced with a reference to the issue as of commit 5ca703f.

Ditto

Comment is replaced with a reference to the issue as of commit 5ca703f.

@redyetidev This is no longer WIP unless you have anymore concerns or suggestions.

CI: https://ci.nodejs.org/job/node-test-pull-request/63349/

I believe CI is failing because the runner is exhausting its memory.

We do some pretty large allocations in the unit tests.

node/test/parallel/test-buffer-copy.js

Lines 239 to 245 in 5ca703f

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

node/test/parallel/test-buffer-concat.js

Lines 103 to 113 in 5ca703f

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

Perhaps the tests should be skipped?

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

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.

Ah. This is breaking

This works intentionally since #54088:

> Buffer.of(1, 2, 3, 4).copy(x = new Uint16Array(4)), x
Uint16Array(4) [ 513, 1027, 0, 0 ]

And it can't be reverted without a semver-major

#54088 just missed to add tests, #60399 will add them

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.

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.

What's the status of this?
It looks like this PR got blocked because of the new tests are not functioning on CI?
Does it really need those new tests to get landed or could they be a separate PR?

This should also likely fix #59985

_copy is slower than set but #60399 would fix that

Ah. This is breaking

This works intentionally since #54088:

> Buffer.of(1, 2, 3, 4).copy(x = new Uint16Array(4)), x
Uint16Array(4) [ 513, 1027, 0, 0 ]

And it can't be reverted without a semver-major

#54088 just missed to add tests, #60399 will add them

@dunpro Please rebase and rectify the concerns

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 22, 2024

duncpro force-pushed the slow-copy-u64 branch from 03d27e2 to d15c192 Compare October 22, 2024 16:01

duncpro force-pushed the slow-copy-u64 branch from d15c192 to cd0a59a Compare October 22, 2024 16:08

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 22, 2024

avivkeller reviewed Oct 22, 2024

View reviewed changes

Comment thread test/parallel/test-buffer-concat.js Outdated

avivkeller requested a review from ronag October 22, 2024 17:54

avivkeller reviewed Oct 22, 2024

View reviewed changes

Comment thread test/parallel/test-buffer-concat.js Outdated

ronag approved these changes Oct 22, 2024

View reviewed changes

avivkeller added the wip Issues and PRs that are still a work in progress. label Oct 22, 2024

ronag reviewed Oct 23, 2024

View reviewed changes

duncpro changed the title fix: support concatenating buffers larger than 4GB on 64-bit systems fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

duncpro changed the title fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy fix: support buffers greater than $2^32$ bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

duncpro changed the title fix: support buffers greater than $2^32$ bytes in length in Buffer.concat and Buffer.copy fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy Oct 24, 2024

avivkeller reviewed Oct 24, 2024

View reviewed changes

duncpro force-pushed the slow-copy-u64 branch from 21b9ce0 to 5ca703f Compare October 25, 2024 03:38

buffer: support copy/concat for >= 2^32 bytes

5ca703f

BridgeAR approved these changes Oct 28, 2024

View reviewed changes

BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. and removed wip Issues and PRs that are still a work in progress. labels Oct 30, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2024

jasnell reviewed Nov 5, 2024

View reviewed changes

ChALkeR reviewed Oct 31, 2025

View reviewed changes

ChALkeR requested changes Oct 31, 2025

View reviewed changes

duncpro closed this Jan 10, 2026

+              // - 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);

+              // - 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));

	// Test its possible to concat buffers where the total length of the result in bytes
	// is greater than 2^32.
	// Ref: ...

                                                     new Uint8Array([0x43, 0x44])]),
                                      Buffer.from('ABCD'));
+              // Ref: https://github.com/nodejs/node/issues/55422#issue-2594486812

+                const targetCapacity = target.byteLength - targetStart;
+                assert(copyLength <= targetCapacity);
+                _copy(source, target, targetStart, sourceStart, copyLength);

fix: support buffers greater than `2^32` bytes in length in `Buffer.concat` and `Buffer.copy` by duncpro · Pull Request #55492 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy#55492duncpro wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom duncpro:slow-copy-u64duncpro/node:slow-copy-u64Copy head branch name to clipboard

Conversation

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

duncpro commented Oct 22, 2024

Uh oh!

avivkeller commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

Uh oh!

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 22, 2024

Uh oh!

duncpro commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

duncpro commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rotemdan commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncpro commented Oct 30, 2024

Uh oh!

nodejs-github-bot commented Oct 30, 2024

Uh oh!

duncpro commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncpro Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fix: support buffers greater than 2^32 bytes in length in Buffer.concat and Buffer.copy#55492
duncpro wants to merge 1 commit into
nodejs:mainfrom
duncpro:slow-copy-u64

duncpro commented Oct 22, 2024 •
edited
Loading

avivkeller commented Oct 22, 2024 •
edited
Loading

avivkeller commented Oct 22, 2024 •
edited
Loading

duncpro commented Oct 22, 2024 •
edited
Loading

duncpro commented Oct 22, 2024 •
edited
Loading

codecov Bot commented Oct 22, 2024 •
edited
Loading

duncpro commented Oct 23, 2024 •
edited
Loading

duncpro commented Nov 5, 2024 •
edited
Loading

duncpro Nov 6, 2024 •
edited
Loading

ChALkeR Oct 31, 2025 •
edited
Loading

ChALkeR Oct 31, 2025 •
edited
Loading