test: read proper inspector message size #14596

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

@@ -68,7 +68,7 @@ function parseWSFrame(buffer, handler) {
    dataLen = buffer.readUInt16BE(2);
    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

@@ -68,7 +68,7 @@ function parseWSFrame(buffer, handler) {
    dataLen = buffer.readUInt16BE(2);
    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

@@ -68,7 +68,9 @@ function parseWSFrame(buffer, handler) {
    dataLen = buffer.readUInt16BE(2);
    bodyOffset = 4;
  } 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

@@ -68,7 +68,9 @@ function parseWSFrame(buffer, handler) {
    dataLen = buffer.readUInt16BE(2);
    bodyOffset = 4;
  } 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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: read proper inspector message size #14596

test: read proper inspector message size #14596

bzoz commented Aug 2, 2017

cjihrig Aug 2, 2017

lpinca Aug 2, 2017

bzoz Aug 2, 2017

Trott Aug 2, 2017

lpinca Aug 2, 2017

lpinca Aug 2, 2017 •

edited

Loading

bzoz commented Aug 2, 2017

lpinca Aug 2, 2017

jasnell Aug 2, 2017

refack Aug 2, 2017

Trott commented Aug 2, 2017 •

edited

Loading

refack left a comment

refack Aug 2, 2017

Trott commented Aug 3, 2017

bzoz commented Aug 4, 2017

bzoz commented Aug 4, 2017

refack commented Aug 4, 2017

Trott commented Aug 4, 2017

Trott commented Aug 4, 2017

refack commented Aug 4, 2017 •

edited

Loading

test: read proper inspector message size #14596

test: read proper inspector message size #14596

Conversation

bzoz commented Aug 2, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

bzoz commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 2, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 3, 2017

bzoz commented Aug 4, 2017

bzoz commented Aug 4, 2017

refack commented Aug 4, 2017

Trott commented Aug 4, 2017

Trott commented Aug 4, 2017

refack commented Aug 4, 2017 • edited Loading

lpinca Aug 2, 2017 •

edited

Loading

Trott commented Aug 2, 2017 •

edited

Loading

refack commented Aug 4, 2017 •

edited

Loading