URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

URLParser should parse IPv4 addresses as the last two pieces of an IPv6 address

[Alex Christensen]

Created attachment 290766 [details]
Patch

[Saam Barati]

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 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]

(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]

http://trac.webkit.org/changeset/206842

[Alex Christensen]

(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.