Summary: | URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Alex Christensen
2016-10-05 17:37:50 PDT
Created attachment 290766 [details]
Patch
Comment on attachment 290766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290766&action=review r=me > Source/WebCore/platform/URLParser.cpp:2340 > + if (!piece && *iterator == '0') > + leadingZeros = true; I think you can remove the (!piece && digitCount != 1) check below and the digitCount variable if you did: if (!piece && *iterator == '0') { if (leadingZeros) return Nullopt; leadingZeros = true; } > Source/WebCore/platform/URLParser.cpp:2363 > + address = address * 256 + piece.value(); Style: This is one of the few times I think "<< 8" is clearer than "* 256" to see that you're composing a 32-bit integer in 8bit chunks. > Source/WebCore/platform/URLParser.cpp:2376 > + if (!iterator.atEnd()) > + return Nullopt; This seems redundant given the above check, however, it might be clearer to keep the above check and assert the fact here. > Source/WebCore/platform/URLParser.cpp:2416 > + if (Optional<IPv4Address> ipv4Address = parseIPv4AddressInsideIPv6(c)) { Do you have to rewind your iterator? Or does passing this in copy an iterator in such a way that it doesn't affect the iterator it was copied from? Comment on attachment 290766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290766&action=review >> Source/WebCore/platform/URLParser.cpp:2340 >> + leadingZeros = true; > > I think you can remove the (!piece && digitCount != 1) check below and the digitCount variable if you did: > if (!piece && *iterator == '0') { > if (leadingZeros) return Nullopt; > leadingZeros = true; > } Actually, you could probably remove more checks by just checking if '.' follows a leading zero since nothing else is valid after that. (In reply to comment #3) > Comment on attachment 290766 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290766&action=review > > >> Source/WebCore/platform/URLParser.cpp:2340 > >> + leadingZeros = true; > > > > I think you can remove the (!piece && digitCount != 1) check below and the digitCount variable if you did: > > if (!piece && *iterator == '0') { > > if (leadingZeros) return Nullopt; > > leadingZeros = true; > > } > > Actually, you could probably remove more checks by just checking if '.' > follows a leading zero since nothing else is valid after that. I chose your first suggestion. Finding out if a '0' is the first '0' or if it's in the middle of a number like 127.0.0.101 (which I will also add as a test) is complicated, and it's not intuitive why anything other than the end or a '.' after a '0' is invalid, but only if it's the first '0'. (In reply to comment #2) > > Source/WebCore/platform/URLParser.cpp:2416 > > + if (Optional<IPv4Address> ipv4Address = parseIPv4AddressInsideIPv6(c)) { > > Do you have to rewind your iterator? Or does passing this in copy an > iterator in such a way that it doesn't affect the iterator it was copied > from? This calls the CodePointIterator copy constructor, so c isn't changed. Calls to parseIPv4PieceInsideIPv6 pass by reference. |