-
Notifications
You must be signed in to change notification settings - Fork 141
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
Conversation
The reporter still needs to be acknowledged. Tests coming up. |
Tests: web-platform-tests/wpt#4513. |
This does not appear to give the correct results. Given input step 5: pointer = 2, piece pointer = compress pointer = 1 step 6 (main):
step 10:
|
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.) |
There was a problem hiding this 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!
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 |
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. |
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. |
That's a good point. @domenic mind reviewing this approach? Sorry about the churn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still works.
938bc10
to
ecc79fc
Compare
Thanks a lot for the patch @rmisev! |
Fixes #195.