Bug 162655 – URLParser should properly handle unexpected periods and overflows in IPv4 addresses

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

URLParser should properly handle unexpected periods in IPv4 addresses

[Alex Christensen]

Created attachment 290049 [details]
Patch

[Alex Christensen]

Created attachment 290097 [details]
Patch

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:2076:24: error: implicit conversion loses integer precision: 'unsigned long long' to 'IPv4Address' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
    IPv4Address ipv4 = items.takeLast();
                ~~~~   ^~~~~~~~~~~~~~~~
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:2076:24: error: implicit conversion loses integer precision: 'unsigned long long' to 'IPv4Address' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
    IPv4Address ipv4 = items.takeLast();
                ~~~~   ^~~~~~~~~~~~~~~~
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:2347:28: note: in instantiation of function template specialization 'WebCore::URLParser::parseIPv4Host<unsigned char>' requested here
        if (auto address = parseIPv4Host(CodePointIterator<CharacterType>(hostIterator, iterator))) {
                           ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:1326:26: note: in instantiation of function template specialization 'WebCore::URLParser::parseHostAndPort<unsigned char>' requested here
                    if (!parseHostAndPort(CodePointIterator<CharacterType>(authorityOrHostBegin, c))) {
                         ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:1026:9: note: in instantiation of function template specialization 'WebCore::URLParser::parse<unsigned char>' requested here
        parse(input.characters8(), input.length(), base, encoding);
        ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:2076:24: error: implicit conversion loses integer precision: 'unsigned long long' to 'IPv4Address' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
    IPv4Address ipv4 = items.takeLast();
                ~~~~   ^~~~~~~~~~~~~~~~
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:2347:28: note: in instantiation of function template specialization 'WebCore::URLParser::parseIPv4Host<unsigned short>' requested here
        if (auto address = parseIPv4Host(CodePointIterator<CharacterType>(hostIterator, iterator))) {
                           ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:1326:26: note: in instantiation of function template specialization 'WebCore::URLParser::parseHostAndPort<unsigned short>' requested here
                    if (!parseHostAndPort(CodePointIterator<CharacterType>(authorityOrHostBegin, c))) {
                         ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/URLParser.cpp:1029:9: note: in instantiation of function template specialization 'WebCore::URLParser::parse<unsigned short>' requested here
        parse(input.characters16(), input.length(), base, encoding);
        ^
3 errors generated.

[Alex Christensen]

Created attachment 290109 [details]
Patch

[Geoffrey Garen]

Comment on attachment 290109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290109&action=review

r=me

> Source/WebCore/platform/URLParser.cpp:1973
> +    uint64_t value = 0;

I feel like this code would read more naturally using our Checked<T> class.

That would look like this:

    Checked<uint32_t, RecordOverflow> value;

    return value.unsafeGet();

    if (UNLIKELY(value.hasOverflowed()))

It's clever to use a 64bit value to check for overflow of a 32bit value, but a little obtuse, and a little suboptimal on 32bit systems.

Also, the Checked<T> class will automatically ensure the "ASSERT before return" semantics you want without extra code, which is nice.

[Alex Christensen]

Created attachment 290124 [details]
Patch

[Alex Christensen]

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