url: fix port overflow checking #15794

rmisev · 2017-10-06T06:12:57Z

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow.

In the current implementation a result can be incorrect if an integer overflow occurs. For example:

const URL = require('url').URL;
const url = new URL("http://example.com:4294967377/");
console.log(url.port);
// actual: 81
// expected: failure (TypeError)

Refs: web-platform-tests/wpt#7602

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

url

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow.

mscdex · 2017-10-06T06:48:06Z

Maybe only bother checking if the buffer length is > 4? I think that will avoid having to check for most common ports.

rmisev · 2017-10-06T08:50:22Z

@mscdex I think your proposal complicates code a bit, and a gain is minimal - only three comparisons less for 4-digit port :

    int port = 0;
    if (buffer.size() <= 4) {
      // port overflow will not occur
      for (size_t i = 0; i < buffer.size(); i++)
        port = port * 10 + buffer[i] - '0';
    } else {
      for (size_t i = 0; i < buffer.size(); i++) {
        port = port * 10 + buffer[i] - '0';
        // prevent integer overflow
        if (port > 0xffff) break;
      }
    }

What do you think?

Refs: web-platform-tests/wpt#7602

addaleax · 2017-10-06T14:16:40Z

src/node_url.cc

@@ -1598,9 +1598,12 @@ void URL::Parse(const char* input,
                   special_back_slash) {
          if (buffer.size() > 0) {
            int port = 0;


I think it might even get a little clearer now if you make this unsigned

Yes, makes sense. Changed.

TimothyGu · 2017-10-07T21:17:16Z

src/node_url.cc

              port = port * 10 + buffer[i] - '0';
-            if (port < 0 || port > 0xffff) {
+              // prevent integer overflow
+              if (port > 0xffff) break;


nit: the port > 0xffff could be moved into the for-loop head

Seems reasonable, moved.

joyeecheung · 2017-10-08T01:25:14Z

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

addaleax · 2017-10-08T18:50:21Z

There are CI failures like this:

not ok 1615 parallel/test-whatwg-url-parsing
  ---
  duration_ms: 0.213
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 110, actual 113.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/common/index.js:485:10)
        at Object.expectsError (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/common/index.js:710:27)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1610-x64/test/parallel/test-whatwg-url-parsing.js:28:30)
        at Module._compile (module.js:600:30)
        at Object.Module._extensions..js (module.js:611:10)
        at Module.load (module.js:521:32)
        at tryModuleLoad (module.js:484:12)
        at Function.Module._load (module.js:476:3)
        at Function.Module.runMain (module.js:641:10)

I guess that’s just a mismatched test counter that needs to be updated or something like that?

joyeecheung · 2017-10-09T14:54:53Z

test/parallel/test-whatwg-url-parsing.js

@@ -26,7 +26,7 @@ const failureTests = tests.filter((test) => test.failure).concat([
 ]);

 const expectedError = common.expectsError(
-  { code: 'ERR_INVALID_URL', type: TypeError }, 110);
+  { code: 'ERR_INVALID_URL', type: TypeError }, 113);


I think the counter here is just failureTests.length, there is no need to hard-code this

agree, done.

joyeecheung · 2017-10-10T11:49:39Z

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

TimothyGu · 2017-10-10T15:13:41Z

Landed in 92146e0.

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow. PR-URL: #15794 Refs: web-platform-tests/wpt#7602 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow. PR-URL: nodejs/node#15794 Refs: web-platform-tests/wpt#7602 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow. PR-URL: #15794 Refs: web-platform-tests/wpt#7602 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

url: fix port overflow checking

d871ee3

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Oct 6, 2017

rmisev mentioned this pull request Oct 6, 2017

URL: test port parsing when integer overflow can occur web-platform-tests/wpt#7602

Merged

url: port overflow tests

de78f25

Refs: web-platform-tests/wpt#7602

addaleax approved these changes Oct 6, 2017

View reviewed changes

change port type to unsigned

cbde589

watilde approved these changes Oct 7, 2017

View reviewed changes

TimothyGu approved these changes Oct 7, 2017

View reviewed changes

cjihrig approved these changes Oct 7, 2017

View reviewed changes

joyeecheung approved these changes Oct 8, 2017

View reviewed changes

move the port overflow check into the for-loop head

3d0d00c

TimothyGu approved these changes Oct 8, 2017

View reviewed changes

joyeecheung reviewed Oct 9, 2017

View reviewed changes

replace the hard-coded failure tests count with a variable

5d3cc27

rmisev force-pushed the url-port-overflow-check branch from f43d979 to 5d3cc27 Compare October 9, 2017 16:43

TimothyGu approved these changes Oct 9, 2017

View reviewed changes

joyeecheung approved these changes Oct 10, 2017

View reviewed changes

TimothyGu closed this Oct 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: fix port overflow checking #15794

url: fix port overflow checking #15794

rmisev commented Oct 6, 2017 •

edited

Loading

mscdex commented Oct 6, 2017

rmisev commented Oct 6, 2017

addaleax Oct 6, 2017

rmisev Oct 6, 2017

TimothyGu Oct 7, 2017

rmisev Oct 8, 2017

joyeecheung commented Oct 8, 2017

addaleax commented Oct 8, 2017

joyeecheung Oct 9, 2017

rmisev Oct 9, 2017

joyeecheung commented Oct 10, 2017

TimothyGu commented Oct 10, 2017

url: fix port overflow checking #15794

url: fix port overflow checking #15794

Conversation

rmisev commented Oct 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Oct 6, 2017

rmisev commented Oct 6, 2017

addaleax Oct 6, 2017

Choose a reason for hiding this comment

rmisev Oct 6, 2017

Choose a reason for hiding this comment

TimothyGu Oct 7, 2017

Choose a reason for hiding this comment

rmisev Oct 8, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 8, 2017

addaleax commented Oct 8, 2017

joyeecheung Oct 9, 2017

Choose a reason for hiding this comment

rmisev Oct 9, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 10, 2017

TimothyGu commented Oct 10, 2017

rmisev commented Oct 6, 2017 •

edited

Loading