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

buffer: throw a consistent range error for length > kMaxLength #10152

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@joyeecheung
Member

joyeecheung commented Dec 6, 2016

  • Version: v8.0.0-pre, or master as of Dec.6
  • Platform: multiple platforms
  • Subsystem: buffer

Background

#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:

  1. RangeError: Invalid array buffer length
    • This one gets checked first in v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub, the stub for new ArrayBuffer
    • The check fails when the length is even larger than std::numeric_limits<size_t>::max(), and v8 can't convert this into a size_t
  2. 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 to realloc and alike has an overflow as size_t(i.g. length * 8 > std::numeric_limits<size_t>::max()). * Or it could fail in node::UncheckedRealloc-like functions, when the machine don't have enough memory and realloc-like calls fail. The limit here depends on the machine's current available memory.
  3. RangeError: Invalid typed array length
    • This gets checked the last because it happens when the ArrayBuffer is created and gets passed to new FastBuffer aka Uint8Array.
    • 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 just node::kMaxLength/buffer.kMaxLength), when the length is larger than that, this error will be thrown.

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:

buffer-length

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(#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 and std::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 to assert.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.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Dec 6, 2016

Member

Changing error messages is currently Semver-Major, this does sound like a good idea for the next major (Node 8) though.

Looking at it though it is possible that if the errors varied anyways we may be able to do this as a patch. I'll let others discuss. :D

Member

Fishrock123 commented Dec 6, 2016

Changing error messages is currently Semver-Major, this does sound like a good idea for the next major (Node 8) though.

Looking at it though it is possible that if the errors varied anyways we may be able to do this as a patch. I'll let others discuss. :D

Show outdated Hide outdated lib/buffer.js
@@ -115,6 +115,9 @@ function assertSize(size) {
err = new TypeError('"size" argument must be a number');
else if (size < 0)
err = new RangeError('"size" argument must not be negative');
else if (size > binding.kMaxLength)
err = new RangeError('"size" argument must not be larger ' +
'than 0x' + binding.kMaxLength.toString(16));

This comment has been minimized.

@mscdex

mscdex Dec 6, 2016

Contributor

Why hex? It seems like having a base-10 decimal number would be easier to read/understand?

@mscdex

mscdex Dec 6, 2016

Contributor

Why hex? It seems like having a base-10 decimal number would be easier to read/understand?

This comment has been minimized.

@joyeecheung

joyeecheung Dec 6, 2016

Member

Just trying to be close to the old behavior here(

throw new RangeError('Attempt to allocate Buffer larger than maximum ' +
). Not sure why it did that either. Personally I prefer a base-10 number too. Will change it that way.

@joyeecheung

joyeecheung Dec 6, 2016

Member

Just trying to be close to the old behavior here(

throw new RangeError('Attempt to allocate Buffer larger than maximum ' +
). Not sure why it did that either. Personally I prefer a base-10 number too. Will change it that way.

@joyeecheung joyeecheung referenced this pull request Dec 6, 2016

Closed

test: fix test for buffer regression #649 #9924

2 of 2 tasks complete
@addaleax

Looks good so far!
I’d probably like to be cautious and regard this as semver-major too, unless somebody else feels strongly?

@joyeecheung joyeecheung changed the title from buffer: return a consistent range error for length > kMaxLength to buffer: throw a consistent range error for length > kMaxLength Dec 6, 2016

@jasnell

jasnell approved these changes Dec 6, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 6, 2016

Member

It has to be semver-major given the additional throw.

Member

jasnell commented Dec 6, 2016

It has to be semver-major given the additional throw.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 6, 2016

Member

It has to be semver-major given the additional throw.

The throw always happen when length > kMaxLength, one way or another. This PR just makes it happen earlier in the JS land of Node instead of in the C++ land of V8, and produce a consistent error message. The semver-major-ness I believe come from the change of error messages, though that's very platform-dependent before this PR anyway.

(ref: CI results of #9924, as the second and third range error regexp added, more platforms went green)

If the error message is not a completely new one but one of the old ones instead(say, Invalid typed array length which #9924 originally expected), then this could be a patch if I understand correctly(but IMHO the new one is more friendly).

Member

joyeecheung commented Dec 6, 2016

It has to be semver-major given the additional throw.

The throw always happen when length > kMaxLength, one way or another. This PR just makes it happen earlier in the JS land of Node instead of in the C++ land of V8, and produce a consistent error message. The semver-major-ness I believe come from the change of error messages, though that's very platform-dependent before this PR anyway.

(ref: CI results of #9924, as the second and third range error regexp added, more platforms went green)

If the error message is not a completely new one but one of the old ones instead(say, Invalid typed array length which #9924 originally expected), then this could be a patch if I understand correctly(but IMHO the new one is more friendly).

@cjihrig

cjihrig approved these changes Dec 6, 2016

I don't think anything needs to be added to test/common.js, but the rest LGTM.

It should be semver major because the error message changes.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 7, 2016

Member

I don't think anything needs to be added to test/common.js, but the rest LGTM.

That is intended to be reused in other test-buffer-*. There are multiple tests that assert.throws without an accurate RegExp match, so I think putting this bit in test/common.js would be helpful for future test improvements. I could amend those tests here too, or just leave them to future code-and-learn sessions.

Member

joyeecheung commented Dec 7, 2016

I don't think anything needs to be added to test/common.js, but the rest LGTM.

That is intended to be reused in other test-buffer-*. There are multiple tests that assert.throws without an accurate RegExp match, so I think putting this bit in test/common.js would be helpful for future test improvements. I could amend those tests here too, or just leave them to future code-and-learn sessions.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 8, 2016

Member

I've updated the tests since #9924 has been merged. Can I get a CI run?

Member

joyeecheung commented Dec 8, 2016

I've updated the tests since #9924 has been merged. Can I get a CI run?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 13, 2016

Member

Ping. What is the status of this? Is there anything left I can do to get this landed?

Member

joyeecheung commented Dec 13, 2016

Ping. What is the status of this? Is there anything left I can do to get this landed?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 13, 2016

Member

This should be good to go except for the one tiny thing that the commit subject lines should ideally be no longer than 50 characters long. They can also be shortened by the person landing the PR, but since this is semver-major, there should be no hurry to get it into master.

Member

addaleax commented Dec 13, 2016

This should be good to go except for the one tiny thing that the commit subject lines should ideally be no longer than 50 characters long. They can also be shortened by the person landing the PR, but since this is semver-major, there should be no hurry to get it into master.

@addaleax addaleax added this to the 8.0.0 milestone Dec 13, 2016

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 13, 2016

Member

Thanks. I will squash the commits and make the subject line cleaner. Maybe it needs another CI?

Member

joyeecheung commented Dec 13, 2016

Thanks. I will squash the commits and make the subject line cleaner. Maybe it needs another CI?

buffer: consistent error for length > kMaxLength
- Always return the same error message(hopefully more informative)
  for buffer length > kMaxLength and avoid getting into V8 C++ land
  for unnecessary checks.
- Use accurate RegExp(reusable as `common.bufferMaxSizeMsg`)
  in tests for this error.
- Separate related tests from test-buffer-alloc.
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Dec 13, 2016

Member

Sorry for the spamming, I didn't realize a semver-major PR needs longer to land since this is my first semver-major PR :). I think I will need to add that into #10202 too.

Member

joyeecheung commented Dec 13, 2016

Sorry for the spamming, I didn't realize a semver-major PR needs longer to land since this is my first semver-major PR :). I think I will need to add that into #10202 too.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 13, 2016

Member

I will squash the commits and make the subject line cleaner. Maybe it needs another CI?

If you just squashed the commits, it should be fine. But here’s another CI anyway: https://ci.nodejs.org/job/node-test-commit/6618/

I didn't realize a semver-major PR needs longer to land

There’s no rule saying it needs longer to land, it’s just that it doesn’t really make a difference whether this lands two days earlier or later. :)

Member

addaleax commented Dec 13, 2016

I will squash the commits and make the subject line cleaner. Maybe it needs another CI?

If you just squashed the commits, it should be fine. But here’s another CI anyway: https://ci.nodejs.org/job/node-test-commit/6618/

I didn't realize a semver-major PR needs longer to land

There’s no rule saying it needs longer to land, it’s just that it doesn’t really make a difference whether this lands two days earlier or later. :)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 13, 2016

Member

Landed in 3d353c7, thanks!

Member

addaleax commented Dec 13, 2016

Landed in 3d353c7, thanks!

@addaleax addaleax closed this Dec 13, 2016

addaleax added a commit that referenced this pull request Dec 13, 2016

buffer: consistent error for length > kMaxLength
- Always return the same error message(hopefully more informative)
  for buffer length > kMaxLength and avoid getting into V8 C++ land
  for unnecessary checks.
- Use accurate RegExp(reusable as `common.bufferMaxSizeMsg`)
  in tests for this error.
- Separate related tests from test-buffer-alloc.

PR-URL: #10152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

AnnaMag added a commit to AnnaMag/node that referenced this pull request Dec 13, 2016

buffer: consistent error for length > kMaxLength
- Always return the same error message(hopefully more informative)
  for buffer length > kMaxLength and avoid getting into V8 C++ land
  for unnecessary checks.
- Use accurate RegExp(reusable as `common.bufferMaxSizeMsg`)
  in tests for this error.
- Separate related tests from test-buffer-alloc.

PR-URL: nodejs#10152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 15, 2016

Closed

doc: clarify the review and landing process #10202

2 of 2 tasks complete

@addaleax addaleax referenced this pull request Dec 22, 2016

Closed

test: fix test-buffer-slow #9809

2 of 2 tasks complete

@joyeecheung joyeecheung deleted the joyeecheung:consistent-buffer-range-error branch Jan 2, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

buffer: consistent error for length > kMaxLength
- Always return the same error message(hopefully more informative)
  for buffer length > kMaxLength and avoid getting into V8 C++ land
  for unnecessary checks.
- Use accurate RegExp(reusable as `common.bufferMaxSizeMsg`)
  in tests for this error.
- Separate related tests from test-buffer-alloc.

PR-URL: nodejs#10152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment