Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

url: disallow invalid IPv4 in IPv6 parser #12315

Closed
wants to merge 1 commit into from
Closed

url: disallow invalid IPv4 in IPv6 parser #12315

wants to merge 1 commit into from

Conversation

watilde
Copy link
Contributor

@watilde watilde commented Apr 10, 2017

Fixes: #10655.

Checklist
  • make -j4 test
  • tests are included
Affected core subsystem(s)

url

@nodejs-github-bot 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 Apr 10, 2017
@watilde
Copy link
Contributor Author

watilde commented Apr 10, 2017 •
edited
Loading

ci: https://ci.nodejs.org/job/node-test-pull-request/7307/ (canceled)
ci: https://ci.nodejs.org/job/node-test-pull-request/7310/

addaleax
addaleax reviewed Apr 10, 2017
View reviewed changes
src/node_url.cc Outdated
while (ch != kEOL) {
value = 0xffffffff;
if (numbers_seen > 0) {
if (ch == '.' && 4 > numbers_seen) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe numbers_seen < 4 is a bit more intuitive than 4 > numbers_seen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I've updated it :)

@watilde watilde force-pushed the feature/ipv4-in-ipv6 branch from 37de626 to 521926a Compare April 10, 2017 20:43
@TimothyGu TimothyGu self-requested a review April 11, 2017 03:09
bnoordhuis
bnoordhuis reviewed Apr 11, 2017
View reviewed changes
src/node_url.cc Outdated
pointer++;
ch = pointer < end ? pointer[0] : kEOL;
if (value > 255)
goto end;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was just reading the spec from the top to the bottom. The order comes from it, but yeah it's better to not touch for the performance. I will update it :)

src/node_url.cc Outdated
ch = pointer < end ? pointer[0] : kEOL;
}
if (dots == 3 && ch != kEOL)
if (ch == kEOL && numbers_seen != 4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by the if (numbers_seen > 0) { check at the start of the loop, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could happen if the numbers_seen is increased in the loop in the loop that the top loop can't detect: https://github.com/watilde/node/blob/521926ae2f502759c5fc752c82a2661a3dbf419e/src/node_url.cc#L179

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you mean but in that case it can be moved to right after the loop, right? And the ch == kEOL clause can be dropped because that's implied by while (ch != kEOL).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right! I just got what you meant of right after the loop. I will update and let's wait for the spec update at whatwg/url#292. Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec was updated at whatwg/url#292.

@rmisev rmisev mentioned this pull request Apr 12, 2017
Merged
@watilde watilde force-pushed the feature/ipv4-in-ipv6 branch from 521926a to a5786ac Compare April 12, 2017 18:41
@watilde
Copy link
Contributor Author

watilde commented Apr 14, 2017

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

@watilde
Copy link
Contributor Author

watilde commented Apr 14, 2017

Landed in 1b99d8f. Thanks!

@watilde watilde closed this Apr 14, 2017
@watilde watilde deleted the feature/ipv4-in-ipv6 branch April 14, 2017 17:12
watilde added a commit that referenced this pull request Apr 14, 2017
@watilde
url: disallow invalid IPv4 in IPv6 parser
1b99d8f 
Fixes: #10655
PR-URL: #12315
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@TimothyGu TimothyGu mentioned this pull request Apr 19, 2017
Merged
4 tasks
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@baby636 baby636 mentioned this pull request Jun 22, 2022
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@addaleax addaleax addaleax left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@watilde @bnoordhuis @jasnell @addaleax @TimothyGu @nodejs-github-bot