Skip to content
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.

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

Disallow invalid IPv4 in IPv6 parser #196

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Disallow invalid IPv4 in IPv6 parser #196

merged 1 commit into from
Jan 11, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 6, 2017

Fixes #195.

@annevk
Copy link
Member Author

annevk commented Jan 6, 2017

The reporter still needs to be acknowledged. Tests coming up.

@annevk
Copy link
Member Author

annevk commented Jan 6, 2017

Tests: web-platform-tests/wpt#4513.

@domenic
Copy link
Member

domenic commented Jan 6, 2017

This does not appear to give the correct results.

Given input ::1., there should be a failure, but following the new algorithm, there is none:

step 5: pointer = 2, piece pointer = compress pointer = 1

step 6 (main):

  • step 4: value = 1, pointer = 3, length = 1
  • step 6: c = "." => pointer = 2, ipv4 = true

step 10:

  • first round, c = "1"
    • step 3: value = 1, pointer = 3
    • step 4: no failure (c is "." now because pointer incremented in step 3)
    • new step "if dots seen is 3 and c is not the EOF code point, failure": no failure (c is ".", not EOF)
    • new step "if c is "." increase pointer and dots seen by one": pointer is now 4, so c = EOF, so loop exits

@annevk
Copy link
Member Author

annevk commented Jan 7, 2017

Thanks @domenic for the detailed run through. I pushed something that should definitely fail for EOF after ".". (Was hard to wait until Monday for this one.)

@annevk annevk requested a review from domenic January 9, 2017 08:42
@annevk annevk self-assigned this Jan 9, 2017
domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 9, 2017
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

All tests now pass!

domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 9, 2017
domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 9, 2017
@rmisev
Copy link
Member

rmisev commented Jan 10, 2017

Now for invalid IPv4 addresses with more than 3 dots, parser does unnecessary loop and calculations until reaches EOF code point.

I think additional check is required. I created two versions of fix here:

https://github.com/rmisev/url/commits/ipv4-in-ipv6-v1
https://github.com/rmisev/url/commits/ipv4-in-ipv6-v2

@annevk
Copy link
Member Author

annevk commented Jan 10, 2017

I think not failing early is fine. Or is there another test that we would fail with the proposed algorithm?

I like your numbers seen variant though. It seems kinda nice to base it on that.

@rmisev
Copy link
Member

rmisev commented Jan 10, 2017

I think the point of failure in your algorithm can be Integer overflow in step "4. Set piece to piece × 0x100 + value" (when invalid address has many dots). In some programming languages this can cause a run-time error.

So I preffer numbers seen variant.

@annevk
Copy link
Member Author

annevk commented Jan 10, 2017

That's a good point. @domenic mind reviewing this approach? Sorry about the churn.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Still works.

@annevk annevk merged commit a7ae1b8 into master Jan 11, 2017
@annevk annevk deleted the annevk/ipv4-in-ipv6 branch January 11, 2017 06:43
@annevk
Copy link
Member Author

annevk commented Jan 11, 2017

Thanks a lot for the patch @rmisev!

domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 12, 2017
rmisev added a commit to upa-url/upa that referenced this pull request May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants