Bug 162991

Summary: URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch saam: review+

Alex Christensen
Reported 2016-10-05 17:37:50 PDT
URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address
Attachments
Patch (15.13 KB, patch)
2016-10-05 17:41 PDT, Alex Christensen
saam: review+
Alex Christensen
Comment 1 2016-10-05 17:41:29 PDT
Saam Barati
Comment 2 2016-10-05 18:38:17 PDT
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?
Saam Barati
Comment 3 2016-10-05 18:43:08 PDT
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.
Alex Christensen
Comment 4 2016-10-05 19:36:05 PDT
(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'.
Alex Christensen
Comment 5 2016-10-05 19:37:52 PDT
Alex Christensen
Comment 6 2016-10-05 19:39:08 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.