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+

Description Alex Christensen 2016-10-05 17:37:50 PDT
URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address
Comment 1 Alex Christensen 2016-10-05 17:41:29 PDT
Created attachment 290766 [details]
Patch
Comment 2 Saam Barati 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?
Comment 3 Saam Barati 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.
Comment 4 Alex Christensen 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'.
Comment 5 Alex Christensen 2016-10-05 19:37:52 PDT
http://trac.webkit.org/changeset/206842
Comment 6 Alex Christensen 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.