url: fix port overflow checking by rmisev · Pull Request #15794 · nodejs/node

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

@@ -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

              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

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 <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

Uh oh!

url: fix port overflow checking#15794

url: fix port overflow checking#15794
rmisev wants to merge 5 commits into
nodejs:masterfrom
rmisev:url-port-overflow-check

rmisev commented Oct 6, 2017 •

edited

Loading

Uh oh!

mscdex commented Oct 6, 2017

Uh oh!

rmisev commented Oct 6, 2017

Uh oh!

addaleax Oct 6, 2017

Uh oh!

rmisev Oct 6, 2017

Uh oh!

TimothyGu Oct 7, 2017

Uh oh!

rmisev Oct 8, 2017

Uh oh!

joyeecheung commented Oct 8, 2017

Uh oh!

addaleax commented Oct 8, 2017

Uh oh!

joyeecheung Oct 9, 2017

Uh oh!

rmisev Oct 9, 2017

Uh oh!

joyeecheung commented Oct 10, 2017

Uh oh!

TimothyGu commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

rmisev commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Oct 6, 2017

Uh oh!

rmisev commented Oct 6, 2017

Uh oh!

addaleax Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

rmisev Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

rmisev Oct 8, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 8, 2017

Uh oh!

addaleax commented Oct 8, 2017

Uh oh!

joyeecheung Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

rmisev Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 10, 2017

Uh oh!

TimothyGu commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rmisev commented Oct 6, 2017 •

edited

Loading