buffer: throw a consistent range error for length > kMaxLength
- Version: v8.0.0-pre, or master as of Dec.6
- Platform: multiple platforms
- Subsystem: buffer
Background
https://github.com/nodejs/node/pull/9924
When trying to make the error messages thrown for buffer length exceeding the maximum limit, CI failed on some platforms and succeeded on others. As @addaleax pointed out the error could be Array buffer allocation failed
on some machines, while being Invalid typed array length
on others.
const len = 1422561062959;
const lenLimitMsg = new RegExp('^RangeError: (Invalid typed array length' +
'|Array buffer allocation failed)$');
assert.throws(() => Buffer(len).toString('utf8'), lenLimitMsg);
Why this happens
When the buffer allocation methods receive a relatively large length as the sole argument, they all go to createUnsafeBuffer
eventually.
function createUnsafeBuffer(size) {
return new FastBuffer(createUnsafeArrayBuffer(size));
}
function createUnsafeArrayBuffer(size) {
zeroFill[0] = 0;
try {
return new ArrayBuffer(size);
} finally {
zeroFill[0] = 1;
}
}
And when the length is extrodinarily large, this could throw three types of errors:
-
RangeError: Invalid array buffer length
- This one gets checked first in
v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub
, the stub fornew ArrayBuffer
- The check fails when the length is even larger than
std::numeric_limits<size_t>::max()
, and v8 can't convert this into asize_t
- This one gets checked first in
-
RangeError: Array buffer allocation failed
- This one gets checked later in
v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub
- The check could fail in
node::MultiplyWithOverflowCheck
, when the byte length(buffer length * 8) that will get passed torealloc
and alike has an overflow assize_t
(i.g.length * 8 > std::numeric_limits<size_t>::max()
). * Or it could fail innode::UncheckedRealloc
-like functions, when the machine don't have enough memory andrealloc
-like calls fail. The limit here depends on the machine's current available memory.
- This one gets checked later in
-
RangeError: Invalid typed array length
- This gets checked the last because it happens when the
ArrayBuffer
is created and gets passed tonew FastBuffer
akaUint8Array
. - The checks are performed in
v8::internal::Runtime_TypedArrayInitialize
and thrown there. Since V8 can't handle a typed array with length larger than what a SMI can handle(v8::Smi::kMaxValue
, or justnode::kMaxLength
/buffer.kMaxLength
), when the length is larger than that, this error will be thrown.
- This gets checked the last because it happens when the
This is what happens on a 64-bit mac, where std::numeric_limits<size_t>::max()
is 2^64-1
and v8::Smi::kMaxValue
is 2^32-1
:
But on other platforms, the limits can vary, hence the error message is platform-specific.
What the docs say
Basically the API docs for all the allocation methods say:
The
size
must be less than or equal to the value of [buffer.kMaxLength
]. Otherwise, a [RangeError
] is thrown.
What the C++ methods do
node::Buffer::New
and node::Buffer::Copy
do:
if (length > kMaxLength) {
return Local<Object>();
}
What is the old behavior
Before the new buffer implementation came along(https://github.com/nodejs/node/pull/1825), a manual checking is performed and throw a range error in the JS land of Node(as of https://github.com/nodejs/node/blob/9cd44bb2b683446531306bbcff8739fc3526d02c/lib/buffer.js)
// Note: cannot use `length < kMaxLength` here because that fails when
// length is NaN (which is otherwise coerced to zero.)
if (length >= kMaxLength) {
throw new RangeError('Attempt to allocate Buffer larger than maximum ' +
'size: 0x' + kMaxLength.toString(16) + ' bytes');
}
What is the issue
- When the length is larger than
v8::Smi::kMaxValue
, the allocation would fail, but we need to get into the C++ land of V8 and go through a lot of steps to throw an error out. This could have been avoided by doing a fast check in the JS land first, as the old implementation did. - The error message is platform-specific when a large length is passed to the allocation methods. The value of
v8::Smi::kMaxValue
andstd::numeric_limits<size_t>::max()
could be very different on different platforms, so the tests can not test for a consistent error message. But as I understand we are trying to add exact regexp match toassert.throws
in tests, so we need to do a lot of/^RangeError: (Invalid typed array length|Array buffer allocation failed|Invalid array buffer length)$/
. That looks weird. - These different error messages could be confusing to Node.js beginners. A consistent error message like the one in the old implementation is much friendlier.
Possible fix
Currently when a large length is passed into the allocation methods, the call path looks like this:
Buffer ->
Buffer.allocUnsafe ->
assertSize
allocate ->
createUnsafeBuffer ->
createUnsafeArrayBuffer ->
new ArrayBuffer
FastBuffer
SlowBuffer ->
// no assertSize here
createUnsafeBuffer ->
createUnsafeArrayBuffer ->
new ArrayBuffer
FastBuffer
Buffer.alloc ->
assertSize
createUnsafeBuffer ->
createUnsafeArrayBuffer ->
new ArrayBuffer
FastBuffer
FastBuffer
Buffer.allocUnsafeSlow ->
assertSize
createUnsafeBuffer ->
createUnsafeArrayBuffer ->
new ArrayBuffer
FastBuffer
So I believe the most suitable place for this check should be in assertSize
, and ifassertSize
is added to SlowBuffer()
, all allocations will get checked before they go into new ArrayBuffer
and enter V8 C++ land.
I've only fixed the test that fails on my machine as of now. If @nodejs/buffer think this approach is plausible then I will try to fix more tests with matching regexps.