test: fix test for buffer regression #649

[joyeecheung]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

pass a regexp to assert.throws()

[cjihrig]

If you check the entire error message, you can add ^ and $ to the regular expression. Also, since the regex is the same for all the checks, you could move it to a variable.

[joyeecheung]

Changes addressed

[cjihrig]

All, or at least some, of these should fit on one line now.

[joyeecheung]

Addressed.

A noob question: is it better that I amend the commits myself during the PR so you don't have to squash them when you land them, or is it better I just leave several commits in the PR so the review process won't get hidden in GitHub?

[cjihrig]

It's really a personal preference. Sometimes with large PRs it's easier for the reviewer(s) if the entire thing doesn't have to be re-reviewed after each amended change. It can be tricky though if a PR contains multiple logically divided commits and you want to amend the correct commit. For something of this size, I don't think it matters much.

[joyeecheung]

Got it, thanks for the explanation!

[cjihrig]

LGTM if it passes CI: https://ci.nodejs.org/job/node-test-pull-request/5173/

[addaleax]

CI looks like the error message is actually platform/machine-dependent here, locally I always get Array buffer allocation failed.

Maybe just setting len to an astronomically high value works here?

(And aside: Buffer.allocUnsafe(0x10000000000000000) returns an empty Buffer for me… which is somewhat unexpected?)

[joyeecheung]

I dug around a little bit and found out why Buffer.allocUnsafe(0x10000000000000000) return a empty buffer:

* thread #1: tid = 0x15c9bc, 0x0000000100464fe5 node`v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub(args=BuiltinArguments @ 0x00007fff5fbfdcf8, isolate=0x0000000106000000) + 1317 at builtins-arraybuffer.cc:52, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
  * frame #0: 0x0000000100464fe5 node`v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub(args=BuiltinArguments @ 0x00007fff5fbfdcf8, isolate=0x0000000106000000) + 1317 at builtins-arraybuffer.cc:52
    frame #1: 0x00000001004649ff node`v8::internal::Builtin_ArrayBufferConstructor_ConstructStub(args_length=5, args_object=0x00007fff5fbfddd0, isolate=0x0000000106000000) + 255 at builtins-arraybuffer.cc:26
    frame #3: 0x00000e1102909ec7 createUnsafeArrayBuffer(this=0x00002a4274304311:<undefined>, 0x000018207b7673b1:<Number: 18446744073709551616.000000>) at buffer.js:40:33 fn=0x00001e06fe02c9f9
    frame #4: 0x00000e1102953ce6 createUnsafeBuffer(this=0x00002a4274304311:<undefined>, 0x000018207b7673b1:<Number: 18446744073709551616.000000>) at buffer.js:36:28 fn=0x00001e06fe08ce91
    frame #5: 0x00000e110293cfb6 allocate(this=0x00002a4274304311:<undefined>, 0x000018207b7673b1:<Number: 18446744073709551616.000000>) at buffer.js:177:18 fn=0x00001e06fe06e741
    frame #11: 0x00000e11029376e1 tryModuleLoad(this=0x00002a4274304311:<undefined>, 0x00001e06fe012021:<Object: Module>, 0x00001e06fe0121e9:<String: "/Users/joyee/pro...">) at module.js:444:23 fn=0x00001e06fe0124d9
    frame #15: 0x00000e110287a96b startup(this=0x00002a4274304311:<undefined>) at bootstrap_node.js:1:10 fn=0x00001e06fe012679
    frame #17: 0x00000e110284aa83 <internal>
    frame #18: 0x00000e110282ac41 <entry>
    frame #19: 0x0000000100afe75e node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000106000000, is_construct=false, target=Handle<v8::internal::Object> @ 0x00007fff5fbfe4d0, receiver=Handle<v8::internal::Object> @ 0x00007fff5fbfe4c8, argc=1, args=0x00007fff5fbfe818, new_target=Handle<v8::internal::Object> @ 0x00007fff5fbfe4c0) + 1470 at execution.cc:141
    frame #20: 0x0000000100afe118 node`v8::internal::Execution::Call(isolate=0x0000000106000000, callable=Handle<v8::internal::Object> @ 0x00007fff5fbfe588, receiver=Handle<v8::internal::Object> @ 0x00007fff5fbfe580, argc=1, argv=0x00007fff5fbfe818) + 280 at execution.cc:178
    frame #21: 0x000000010034c8e5 node`v8::Function::Call(this=0x0000000106051d98, context=(val_ = 0x0000000106051de0), recv=(val_ = 0x0000000106000078), argc=1, argv=0x00007fff5fbfe818) + 773 at api.cc:4521
    frame #22: 0x000000010034ca61 node`v8::Function::Call(this=0x0000000106051d98, recv=(val_ = 0x0000000106000078), argc=1, argv=0x00007fff5fbfe818) + 113 at api.cc:4530
    frame #23: 0x000000010003182f node`node::LoadEnvironment(env=0x00007fff5fbfe9c8) + 703 at node.cc:3469
    frame #24: 0x0000000100037336 node`node::Start(isolate=0x0000000106000000, isolate_data=0x00007fff5fbff368, argc=2, argv=0x0000000103d00120, exec_argc=0, exec_argv=0x0000000103d00000) + 486 at node.cc:4432
    frame #25: 0x0000000100033e6c node`node::Start(event_loop=0x00000001021ebf90, argc=2, argv=0x0000000103d00120, exec_argc=0, exec_argv=0x0000000103d00000) + 524 at node.cc:4509
    frame #26: 0x0000000100033874 node`node::Start(argc=2, argv=0x0000000103d00120) + 388 at node.cc:4555
    frame #27: 0x000000010007cb3e node`main(argc=2, argv=0x00007fff5fbff828) + 94 at node_main.cc:58
    frame #28: 0x0000000100001534 node`start + 52

Here ArrayBufferConstructor_ConstructStub gets a 0 as byte_length because there is a cast error triggered by floating number precision problems in TryNumberToSize:

// // 1.8446744073709552E+19, that is, 0x10000000000000000 with possibly some floating number inaccuracy
double value = HeapNumber::cast(number)->value();  
// on my machine the limit is 18446744073709551615, just a bit less than 18446744073709552000 = 0x10000000000000000
if (value >= 0 && value <= std::numeric_limits<size_t>::max()) {  
  *result = static_cast<size_t>(value);  // 0
  return true;
} else {
  return false;
}

If you add one more 0 to the number, then you will hit the std::numeric_limits<size_t>::max() here and return false, then get a Invalid array buffer length thrown by this stub.

[joyeecheung]

And for that Array buffer allocation failed error, I think on your machine new ArrayBuffer fails in ArrayBufferConstructor_ConstructStub in the first place and it never gets a chance to enter new FastBuffer and throw the Invalid typed array length.

The failed call path should be JSArrayBuffer::SetupAllocatingData -> ArrayBufferAllocator::Allocate -> node::UncheckedMalloc -> node::UncheckedRealloc, and it's possibly caused by a failed MultiplyWithOverflowCheck or realloc, so it's platform specific.

The Invalid typed array length as expected in this PR is thrown by v8::internal::Runtime_TypedArrayInitialize:

  if (length > static_cast<unsigned>(Smi::kMaxValue)) {
    THROW_NEW_ERROR_RETURN_FAILURE(
        isolate, NewRangeError(MessageTemplate::kInvalidTypedArrayLength));
  }

And I believe the reason is the same as explained in typedarray.js:

  if (length > %_MaxSmi()) {
    // Note: this is not per spec, but rather a constraint of our current
    // representation (which uses smi's).
    throw %make_range_error(kInvalidTypedArrayLength);
  }

[joyeecheung]

So regarding to the limit of buffer length, we have two types of error:

1. Array buffer allocation failed caused by overflows of the length that gets passed to realloc or whatever (that should be the limit of size_t, or std::numeric_limits<size_t>::max()), The checks are performed by node::MultiplyWithOverflowCheck and the errors are eventually thrown by v8::internal::Builtin_Impl_ArrayBufferConstructor_ConstructStub. This gets checked first because it happends in new ArrayBuffer.
2. Invalid typed array length caused by V8's incapability of handling a length larger than what a SMI can handle(that should be the limit of Smi::kMaxValue). The checks are performed by v8::internal::Runtime_TypedArrayInitialize and thrown there. This gets checked later because it happens when the ArrayBuffer is created and gets passed to new FastBuffer aka Uint8Array.

Sometimes the huge length we set for this test is larger than the size_t limit on the machine, so you get a Array buffer allocation failed. Sometimes it's smaller than that, but larger than Smi::kMaxValue(2147483647 or 2^31 - 1 on 64-bit machines), then we will get Invalid typed array length.

So, the results really depends on the size_t limit on the machine that run this test. There are 4 solutions for this that I can think of:

1. Fix the buffer module: The buffer module can just check for the (usually smaller) limit Smi::kMaxValue and throw an Invalid typed array length right away so we can get a consistent error message(maybe check this first in the UncheckedRealloc-like functions before they check for limits of size_t, since it's usually smaller that that). But since this limit is just an implementation detail of V8 and not in the ES spec, this might get tricky.
2. Just fix this test, the hard way: We can set a lower length in this test and make sure it's Smi::kMaxValue < length < std::numeric_limits<size_t>::max() everywhere, so we still get a consistent Invalid typed array length for this test. But I don't even know if std::numeric_limits<size_t>::max() > Smi::kMaxValue is always true on every platform though. If these limits are the same, then we will still get a Array buffer allocation failed since that get's thrown first.
3. Just fix this test, the easy way: We can loose the test a bit and just test if the error message is one of those two.
4. Just fix this test, the hardest way, We can test for all different combinations of limits in these different platforms. That can be tricky.

And I think that empty buffer returned by Buffer.allocUnsafe(0x10000000000000000) is a V8 bug? If HeapNumber just can't be accurate about these Non-SMI large numbers and need to return a double, then V8 needs to test if there is a cast error in TryNumberToSize, possibly like this:

diff --git a/deps/v8/src/conversions-inl.h b/deps/v8/src/conversions-inl.h
index 427a67d..b78410e 100644
--- a/deps/v8/src/conversions-inl.h
+++ b/deps/v8/src/conversions-inl.h
@@ -156,6 +156,9 @@ bool TryNumberToSize(Object* number, size_t* result) {
     double value = HeapNumber::cast(number)->value();
     if (value >= 0 && value <= std::numeric_limits<size_t>::max()) {
       *result = static_cast<size_t>(value);
+      if (value > 0 && *result == 0) {
+        return false;  // cast error
+      }
       return true;
     } else {
       return false;

Noob question: should I open an issue on V8's issue tracker(and possibly subtmit a patch) or..?

[jasnell]

@nodejs/buffer @nodejs/v8

[ofrobots]

Guide on reporting / fixing V8 bugs: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#unfixed-upstream-bugs

[joyeecheung]

Verified that the bug exists in v8/node/vee-eight-lkgr and V8 master. Opened a bug https://bugs.chromium.org/p/v8/issues/detail?id=5712 and submitted a patch.

[addaleax]

Thanks for the in-depth explanation of what’s going on here! I think I like the third option („We can loose the test a bit and just test if the error message is one of those two.”) but maybe that’s just me wanting to go down the path of least resistance.

[joyeecheung]

Update: I looked into the C++ side of Buffer and it seems we already (kinda) do Option 1 described in my previous comments(#9924 (comment)) for Buffer::New, and this was the old behavior at least in #657.

MaybeLocal<Object> New(Environment* env, size_t length) {
  EscapableHandleScope scope(env->isolate());

  // V8 currently only allows a maximum Typed Array index of max Smi.
  if (length > kMaxLength) {
    return Local<Object>();
  }
...
}

Somehow the kMaxLength check gets lost in the JS side of code. I can open a new issue and a new PR if my understanding about this is correct.

[joyeecheung]

I looked around a little bit more and believe for this PR I should just go with option 3 and loose the test a bit, like what test-buffer-slow does, to make CI happy and try to get this landed. This way we can at least end up with a more accurate error message.

Regarding how the errors for a large length should look like, I think this deserves a separate issue. There are many tests that touch this but still don't have an accurate error message match. I'll do a write up about this there.

[addaleax]

Somehow the kMaxLength check gets lost in the JS side of code. I can open a new issue and a new PR if my understanding about this is correct.

Riiight… should have thought of that. We don’t use any of our own C++ code when allocating Buffer instances in JS (anymore?), it’s all handled inside the standard Uint8Array constructor.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/6482/

[joyeecheung]

I think the CI failed because I missed out another type of range error(as mentioned in #10152).

Not sure if we are going that direction, should this PR be closed? I'll amend this PR to cover the other range error anyway.

[addaleax]

@joyeecheung If you manage to tweak the regexp here so that CI passes, I don’t think you need to close the PR here :)

[joyeecheung]

OK I amended the regexp here. Can you poke the CI for me again? Thanks :)

[addaleax]

Sure :)

CI: https://ci.nodejs.org/job/node-test-commit/6484/ (edit: green!)