-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,8 @@ const { | |
| inspect: utilInspect, | ||
| } = require('internal/util/inspect'); | ||
|
|
||
| const assert = require('internal/assert'); | ||
|
|
||
| const { | ||
| codes: { | ||
| ERR_BUFFER_OUT_OF_BOUNDS, | ||
|
|
@@ -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); | ||
|
|
@@ -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)); | ||
| // 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); | ||
|
Comment on lines
+269
to
+272
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also likely fix #59985 |
||
|
|
||
| const copyLength = sourceEnd - sourceStart; | ||
| const targetCapacity = target.byteLength - targetStart; | ||
| assert(copyLength <= targetCapacity); | ||
|
|
||
| _copy(source, target, targetStart, sourceStart, copyLength); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _copy is slower than set but #60399 would fix that |
||
| return copyLength; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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') ]; | ||
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
| } | ||
There was a problem hiding this comment.
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
copyImplcall site.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 thatsourceandtargetneed to beUint8Arrayexplicit.It indicates to any future/new maintainers needing to call
_copyActualwhat the invariants are. They don't have to guess at the invariants, or deduce the preconditions by exploring all the call-sites of_copyActual.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. This is breaking
This works intentionally since #54088:
And it can't be reverted without a semver-major
#54088 just missed to add tests, #60399 will add them