Skip to content

url: fix port overflow checking #15794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rmisev wants to merge 5 commits into from
Closed

url: fix port overflow checking #15794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d871ee3
url: fix port overflow checking
rmisev Oct 2, 2017
de78f25
url: port overflow tests
rmisev Oct 6, 2017
cbde589
change port type to unsigned
rmisev Oct 6, 2017
3d0d00c
move the port overflow check into the for-loop head
rmisev Oct 8, 2017
5d3cc27
replace the hard-coded failure tests count with a variable
rmisev Oct 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,10 +1597,11 @@ void URL::Parse(const char* input,
ch == '#' ||
special_back_slash) {
if (buffer.size() > 0) {
int port = 0;
for (size_t i = 0; i < buffer.size(); i++)
unsigned port = 0;
// the condition port <= 0xffff prevents integer overflow
for (size_t i = 0; port <= 0xffff && i < buffer.size(); i++)
port = port * 10 + buffer[i] - '0';
if (port < 0 || port > 0xffff) {
if (port > 0xffff) {
// TODO(TimothyGu): This hack is currently needed for the host
// setter since it needs access to hostname if it is valid, and
// if the FAILED flag is set the entire response to JS layer
Expand All @@ -1611,7 +1612,8 @@ void URL::Parse(const char* input,
url->flags |= URL_FLAGS_FAILED;
return;
}
url->port = NormalizePort(url->scheme, port);
// the port is valid
url->port = NormalizePort(url->scheme, static_cast<int>(port));
buffer.clear();
} else if (has_state_override) {
// TODO(TimothyGu): Similar case as above.
Expand Down
20 changes: 19 additions & 1 deletion test/fixtures/url-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

/* The following tests are copied from WPT. Modifications to them should be
upstreamed first. Refs:
https://github.com/w3c/web-platform-tests/blob/5d149f0/url/urltestdata.json
https://github.com/w3c/web-platform-tests/blob/11757f1/url/urltestdata.json
License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html
*/
module.exports =
Expand Down Expand Up @@ -5811,6 +5811,24 @@ module.exports =
"base": "about:blank",
"failure": true
},
"Port overflow (2^32 + 81)",
{
"input": "http://f:4294967377/c",
"base": "http://example.org/",
"failure": true
},
"Port overflow (2^64 + 81)",
{
"input": "http://f:18446744073709551697/c",
"base": "http://example.org/",
"failure": true
},
"Port overflow (2^128 + 81)",
{
"input": "http://f:340282366920938463463374607431768211537/c",
"base": "http://example.org/",
"failure": true
},
"# Non-special-URL path tests",
{
"input": "sc://ñ",
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-whatwg-url-parsing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }, failureTests.length);

for (const test of failureTests) {
assert.throws(
Expand Down