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

duncpro · 2024-10-22T15:49:23Z

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.

avivkeller · 2024-10-22T16:06:22Z

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

avivkeller · 2024-10-22T16:07:00Z

Additionally, can you add a test to validate this behavior

duncpro · 2024-10-22T17:32:57Z

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.

avivkeller · 2024-10-22T17:33:44Z

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.

test/parallel/test-buffer-concat.js

avivkeller · 2024-10-22T17:54:33Z

@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

test/parallel/test-buffer-concat.js

duncpro · 2024-10-22T20:42:17Z

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

avivkeller · 2024-10-22T21:26:08Z

@nodejs/buffer PTAL ☝️

duncpro · 2024-10-22T22:01:45Z

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 · 2024-10-22T23:54:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (7bc3e16) to head (5ca703f).
Report is 56 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

duncpro · 2024-10-23T00:50:45Z

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

node/lib/buffer.js

Line 247 in 0bc0b81

throw new ERR_BUFFER_OUT_OF_BOUNDS('expected 0 <= sourceStart <= sourceEnd' +
node/lib/buffer.js

Line 255 in 0bc0b81

throw new ERR_BUFFER_OUT_OF_BOUNDS('expected 0 <= targetStart <= target.byteLength' +

avivkeller · 2024-10-23T00:57:44Z

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

ronag · 2024-10-23T07:33:46Z

lib/buffer.js


  if (targetStart >= target.byteLength || sourceStart >= sourceEnd)
    return 0;

+  // Guaranteed 0 <= sourceStart < sourceEnd (NO GUARANTEE <= source.byteLnegth)
+  // Guaranteed 0 <= targetStart < target.byteLength here


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

ronag · 2024-10-23T07:33:51Z

lib/buffer.js

  return _copyActual(source, target, targetStart, sourceStart, sourceEnd);
 }

+// Assumes `source`, `target` are real buffers.
+// Assumes 0 <= sourceStart <= sourceEnd
+// Assumes 0 <= targetStart <= target.byteLength


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

node/lib/buffer.js

Lines 271 to 275 in 209289c

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

rotemdan · 2024-10-23T08:52:22Z

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

avivkeller · 2024-10-24T15:59:12Z

lib/buffer.js

@@ -218,6 +221,7 @@ function copyImpl(source, target, targetStart, sourceStart, sourceEnd) {
    if (targetStart < 0)
      throw new ERR_OUT_OF_RANGE('targetStart', '>= 0', targetStart);
  }
+  // Guaranteed 0 <= targetStart


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.

avivkeller · 2024-10-24T16:03:18Z

test/parallel/test-buffer-concat.js

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


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.

avivkeller · 2024-10-24T16:03:24Z

test/parallel/test-buffer-copy.js

@@ -234,3 +235,12 @@ assert.deepStrictEqual(c, b.slice(0, c.length));
  // No copying took place:
  assert.deepStrictEqual(c.toString(), 'C'.repeat(c.length));
 }
+
+
+// Test its possible to copy buffer with length > 2^32


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

duncpro · 2024-10-30T07:34:23Z

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

nodejs-github-bot · 2024-10-30T09:02:15Z

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

duncpro · 2024-11-05T23:38:55Z

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?

jasnell · 2024-11-05T23:39:23Z

lib/buffer.js

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


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.

jasnell · 2024-11-05T23:40:51Z

test/parallel/test-buffer-concat.js

@@ -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


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.

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

test/parallel/test-buffer-concat.js Outdated Show resolved Hide resolved

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

avivkeller reviewed Oct 22, 2024

View reviewed changes

test/parallel/test-buffer-concat.js Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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` #55492

duncpro commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024

avivkeller commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024 •

edited

Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

duncpro commented Oct 23, 2024 •

edited

Loading

avivkeller commented Oct 23, 2024

ronag Oct 23, 2024

ronag Oct 23, 2024

duncpro Oct 24, 2024

rotemdan commented Oct 23, 2024

avivkeller Oct 24, 2024

duncpro Oct 25, 2024

avivkeller Oct 24, 2024

duncpro Oct 25, 2024

avivkeller Oct 24, 2024

duncpro Oct 25, 2024

duncpro commented Oct 30, 2024

nodejs-github-bot commented Oct 30, 2024

duncpro commented Nov 5, 2024 •

edited

Loading

jasnell Nov 5, 2024

duncpro Nov 6, 2024 •

edited

Loading

jasnell Nov 5, 2024

duncpro Nov 6, 2024

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

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

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

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

Are you sure you want to change the base?

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

Conversation

duncpro commented Oct 22, 2024 • edited Loading

avivkeller commented Oct 22, 2024 • edited Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024

avivkeller commented Oct 22, 2024 • edited Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024 • edited Loading

avivkeller commented Oct 22, 2024

duncpro commented Oct 22, 2024 • edited Loading

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

duncpro commented Oct 23, 2024 • edited Loading

avivkeller commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotemdan commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncpro commented Oct 30, 2024

nodejs-github-bot commented Oct 30, 2024

duncpro commented Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

duncpro Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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` #55492

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