Bug 162655 - URLParser should properly handle unexpected periods and overflows in IPv4 addresses
Summary: URLParser should properly handle unexpected periods and overflows in IPv4 add...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-27 19:04 PDT by Alex Christensen
Modified: 2016-09-28 14:56 PDT (History)
1 user (show)

See Also:


Attachments
Patch (2.91 KB, patch)
2016-09-27 19:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2016-09-28 10:01 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.20 KB, patch)
2016-09-28 13:13 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.14 KB, patch)
2016-09-28 14:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-09-27 19:04:38 PDT
URLParser should properly handle unexpected periods in IPv4 addresses
Comment 1 Alex Christensen 2016-09-27 19:05:33 PDT
Created attachment 290049 [details]
Patch
Comment 2 Alex Christensen 2016-09-28 10:01:03 PDT
Created attachment 290097 [details]
Patch
Comment 3 Geoffrey Garen 2016-09-28 12:41:01 PDT
/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.
Comment 4 Alex Christensen 2016-09-28 13:13:36 PDT
Created attachment 290109 [details]
Patch
Comment 5 Geoffrey Garen 2016-09-28 13:33:14 PDT
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.
Comment 6 Alex Christensen 2016-09-28 14:52:00 PDT
Created attachment 290124 [details]
Patch
Comment 7 Alex Christensen 2016-09-28 14:56:32 PDT
http://trac.webkit.org/changeset/206554