test: fix test for buffer regression #649 #9924

joyeecheung · 2016-12-01T16:35:33Z

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 · 2016-12-04T06:09:00Z

test/parallel/test-buffer-regression-649.js

-assert.throws(() => Buffer.allocUnsafe(len).toString('utf8'));
-assert.throws(() => Buffer.allocUnsafeSlow(len).toString('utf8'));
+assert.throws(() => Buffer(len).toString('utf8'),
+              /Invalid typed array length/);


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 · 2016-12-04T16:05:36Z

Changes addressed

- pass a regexp to assert.throws()

cjihrig · 2016-12-04T16:07:43Z

test/parallel/test-buffer-regression-649.js

-assert.throws(() => Buffer.allocUnsafeSlow(len).toString('utf8'));
+const lenLimitMsg = /^RangeError: Invalid typed array length$/;
+assert.throws(() => Buffer(len).toString('utf8'),
+              lenLimitMsg);


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

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?

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.

Got it, thanks for the explanation!

cjihrig

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

addaleax · 2016-12-04T22:09:52Z

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 · 2016-12-05T10:37:27Z

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 · 2016-12-05T10:58:14Z

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 · 2016-12-05T11:31:00Z

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

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

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.
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.
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.
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 · 2016-12-05T16:38:10Z

@nodejs/buffer @nodejs/v8

ofrobots · 2016-12-05T17:23:55Z

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

joyeecheung · 2016-12-05T19:00:36Z

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 · 2016-12-05T19:43:16Z

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 · 2016-12-05T19:48:42Z

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 · 2016-12-06T14:20:20Z

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 · 2016-12-06T17:38:52Z

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 · 2016-12-06T18:27:47Z

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 · 2016-12-06T18:31:13Z

@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 · 2016-12-06T18:35:56Z

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

addaleax · 2016-12-06T18:44:58Z

Sure :)

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

pass a regexp to assert.throws() PR-URL: #9924 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 1, 2016

imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016

cjihrig requested changes Dec 4, 2016

View reviewed changes

joyeecheung force-pushed the test-regression-649 branch from d4eef26 to e94a667 Compare December 4, 2016 16:04

test: fix test for buffer regression nodejs#649

d6a4e3d

- pass a regexp to assert.throws()

joyeecheung force-pushed the test-regression-649 branch from e94a667 to d6a4e3d Compare December 4, 2016 16:07

cjihrig reviewed Dec 4, 2016

View reviewed changes

test: squash line for buffer regression nodejs#649 test

1277c6a

cjihrig approved these changes Dec 4, 2016

View reviewed changes

addaleax approved these changes Dec 6, 2016

View reviewed changes

joyeecheung mentioned this pull request Dec 6, 2016

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

Closed

test: loose the error message regexp for platform differences

c679654

joyeecheung force-pushed the test-regression-649 branch from 16f7e21 to c679654 Compare December 6, 2016 18:26

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

addaleax mentioned this pull request Dec 22, 2016

test: fix test-buffer-slow #9809

Closed

2 tasks

This was referenced Jul 6, 2023

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#23

Open

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#57

Open

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#77

Open

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#147

Open

aliscco mentioned this pull request Jul 10, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#628

Open

leonardoadame mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#199

Open

This was referenced Nov 29, 2023

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#785

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#799

Open

leonardoadame mentioned this pull request Nov 30, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#281

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1050

Open

leonardoadame mentioned this pull request Dec 5, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#539

Open

aliscco mentioned this pull request Dec 8, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1158

Open

This was referenced Dec 10, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#577

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 leonardoadame/node#578

Open

This was referenced Dec 10, 2023

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1165

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1167

Open

leonardoadame mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#581

Open

aliscco mentioned this pull request Jan 1, 2024

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1182

Open

aliscco mentioned this pull request Jan 31, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1205

Open

leonardoadame mentioned this pull request Feb 12, 2024

[Snyk] Fix for 3 vulnerabilities leonardoadame/node#684

Open

leonardoadame mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#831

Open

aliscco mentioned this pull request May 15, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1499

Open

leonardoadame mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 leonardoadame/node#963

Open

This was referenced May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1578

Open

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1581

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1601

Open

leonardoadame mentioned this pull request May 19, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#998

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test for buffer regression #649 #9924

test: fix test for buffer regression #649 #9924

joyeecheung commented Dec 1, 2016

cjihrig Dec 4, 2016

joyeecheung commented Dec 4, 2016

cjihrig Dec 4, 2016 •

edited

Loading

joyeecheung Dec 4, 2016

cjihrig Dec 4, 2016

joyeecheung Dec 4, 2016

cjihrig left a comment

addaleax commented Dec 4, 2016

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

jasnell commented Dec 5, 2016

ofrobots commented Dec 5, 2016

joyeecheung commented Dec 5, 2016

addaleax commented Dec 5, 2016

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016

addaleax left a comment

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016 •

edited

Loading

test: fix test for buffer regression #649 #9924

test: fix test for buffer regression #649 #9924

Conversation

joyeecheung commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

cjihrig Dec 4, 2016

Choose a reason for hiding this comment

joyeecheung commented Dec 4, 2016

cjihrig Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

joyeecheung Dec 4, 2016

Choose a reason for hiding this comment

cjihrig Dec 4, 2016

Choose a reason for hiding this comment

joyeecheung Dec 4, 2016

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

addaleax commented Dec 4, 2016

joyeecheung commented Dec 5, 2016 • edited Loading

joyeecheung commented Dec 5, 2016 • edited Loading

joyeecheung commented Dec 5, 2016 • edited Loading

jasnell commented Dec 5, 2016

ofrobots commented Dec 5, 2016

joyeecheung commented Dec 5, 2016

addaleax commented Dec 5, 2016

joyeecheung commented Dec 5, 2016 • edited Loading

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016

addaleax left a comment

Choose a reason for hiding this comment

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016

joyeecheung commented Dec 6, 2016

addaleax commented Dec 6, 2016 • edited Loading

cjihrig Dec 4, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

joyeecheung commented Dec 5, 2016 •

edited

Loading

addaleax commented Dec 6, 2016 •

edited

Loading