test: read proper inspector message size by bzoz · Pull Request #14596 · nodejs/node

bzoz · 2017-08-02T13:30:09Z

Fix a bug when messages bigger than 64kb where incorrectly parsed by the inspector-helper.
See #14507 (comment)

Fixes: #14507

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

/cc @eugeneo @nodejs/v8-inspector

Fix a bug when messages bigger than 64kb where incorrectly parsed by the inspector-helper. Fixes: nodejs#14507

cjihrig · 2017-08-02T13:50:47Z

test/inspector/inspector-helper.js

    bodyOffset = 4;
  } else if (dataLen === 127) {
-    dataLen = buffer.readUInt32BE(2);
+    dataLen = buffer.readUIntBE(2, 8);


Note: the docs for buf.readUIntBE() state that the second argument Must satisfy: 0 < byteLength <= 6

lpinca · 2017-08-02T13:57:43Z

test/inspector/inspector-helper.js

    bodyOffset = 4;
  } else if (dataLen === 127) {
-    dataLen = buffer.readUInt32BE(2);
+    dataLen = buffer.readUIntBE(2, 8);


Hmm, byteLength can only go up to 6 for a total of 48 bits.
Should we split the read into two chunks, 4 bytes each? Like

dataLen = buffer.readUInt32BE(2); if (dataLen > Math.pow(2, 53 - 32) - 1) { assert.fail('Frame size is bigger than `Number.MAX_SAFE_INTEGER`'); } dataLen += buffer.readUInt32BE(6);

I think it will be easier to assert that buffer[1] and buffer[2] equal 0

If I understand correctly, the max value for a UInt32 is 4294967295, vastly lower than Number.MAX_SAFE_INTEGER, so the if check is not needed.

@Trott yes but dataLen === 127 means that the frame size is a 64 bit int.

I guess it's safe to assume that frames are not bigger than 2 ** 32 -1 😄 (I'm not sure how they are generated). In that case we can ignore the first 4 bytes:

dataLen = buffer.readUInt32BE(6);

bzoz · 2017-08-02T14:32:33Z

I've added a test for max message size, PTAL

lpinca · 2017-08-02T14:39:12Z

test/inspector/inspector-helper.js

  } else if (dataLen === 127) {
-    dataLen = buffer.readUInt32BE(2);
+    if (buffer[2] !== 0 || buffer[3] !== 0)
+      assert.fail('Inspector message to big');


Nit: typo "to" -> "too"

This check can be condensed into a single...

assert(buffer[2] === 0 && buffer[3] === 0, 'Inspector message too big');

Could you add a reference to https://tools.ietf.org/html/rfc6455#section-5.2

Trott · 2017-08-02T14:47:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9451/

Only failure is build/infra related. CI is effectively green.

refack

LGTM % nits

refack · 2017-08-02T16:34:54Z

test/inspector/inspector-helper.js

  } else if (dataLen === 127) {
-    dataLen = buffer.readUInt32BE(2);
+    if (buffer[2] !== 0 || buffer[3] !== 0)
+      assert.fail('Inspector message to big');


Could you add a reference to https://tools.ietf.org/html/rfc6455#section-5.2

Trott · 2017-08-03T17:44:48Z

Given the frequency that the test is failing on CI, I'd like to land this now-ish. Anyone else feel the same?

bzoz · 2017-08-04T11:14:28Z

I've fixed the spelling, changed assert per @jasnell suggestion and added @refack link, PTAL

bzoz · 2017-08-04T11:15:58Z

CI (its still "pending", I think this is the correct one): https://ci.nodejs.org/job/node-test-pull-request/9483/

refack · 2017-08-04T15:04:06Z

last CI cancelled.
new CI: https://ci.nodejs.org/job/node-test-pull-request/9483/

Trott · 2017-08-04T17:46:51Z

Only failure in CI is a known flaky unrelated to this. CI is effectively green. Landing...

Fix a bug when messages bigger than 64kb where incorrectly parsed by the inspector-helper. PR-URL: nodejs#14596 Fixes: nodejs#14507 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2017-08-04T17:48:28Z

Landed in 2dc09f6. 🎉

refack · 2017-08-04T19:07:32Z

AIX fail is a known flake and is unrelated #14599 -
https://ci.nodejs.org/job/node-test-commit-aix/7643/nodes=aix61-ppc64/

Fix a bug when messages bigger than 64kb where incorrectly parsed by the inspector-helper. PR-URL: #14596 Fixes: #14507 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test: read proper inspector message size

993636a

Fix a bug when messages bigger than 64kb where incorrectly parsed by the inspector-helper. Fixes: nodejs#14507

bzoz added the test Issues and PRs related to the tests. label Aug 2, 2017

bzoz requested a review from eugeneo August 2, 2017 13:30

nodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Aug 2, 2017

bzoz mentioned this pull request Aug 2, 2017

investigate flaky test-inspector-stop-profile-after-done on Windows #14507

Closed

cjihrig reviewed Aug 2, 2017

View reviewed changes

lpinca reviewed Aug 2, 2017

View reviewed changes

fixup: limit max message size

3c886e5

lpinca approved these changes Aug 2, 2017

View reviewed changes

lpinca reviewed Aug 2, 2017

View reviewed changes

eugeneo approved these changes Aug 2, 2017

View reviewed changes

refack approved these changes Aug 2, 2017

View reviewed changes

kfarnung approved these changes Aug 2, 2017

View reviewed changes

cjihrig approved these changes Aug 3, 2017

View reviewed changes

fixup: assert, spelling and added link

ebb912a

refack approved these changes Aug 4, 2017

View reviewed changes

jasnell approved these changes Aug 4, 2017

View reviewed changes

Trott closed this Aug 4, 2017

MylesBorins mentioned this pull request Aug 8, 2017

v8.3.0 proposal #14594

Merged

addaleax mentioned this pull request Aug 13, 2017

v8.4.0 proposal #14811

Merged

MylesBorins added the land-on-v6.x label Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

Uh oh!

Conversation

bzoz commented Aug 2, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 3, 2017

Uh oh!

bzoz commented Aug 4, 2017

Uh oh!

bzoz commented Aug 4, 2017

Uh oh!

refack commented Aug 4, 2017

Uh oh!

Trott commented Aug 4, 2017

Uh oh!

Trott commented Aug 4, 2017

Uh oh!

refack commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lpinca Aug 2, 2017 •

edited

Loading

Trott commented Aug 2, 2017 •

edited

Loading

refack commented Aug 4, 2017 •

edited

Loading