[webkit-reviews] review granted: [Bug 180688] Fix possible out-of-bounds read in protocolIsInHTTPFamily : [Attachment 329070] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 180688: Fix possible out-of-bounds read in protocolIsInHTTPFamily
https://bugs.webkit.org/show_bug.cgi?id=180688

Attachment 329070: Patch

https://bugs.webkit.org/attachment.cgi?id=329070&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 329070
  --> https://bugs.webkit.org/attachment.cgi?id=329070
Patch

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

> Source/WebCore/platform/URL.cpp:877
> +    return url.length() > 4

We could cache url.length() in a local to avoid dereferencing it twice. Maybe
the compiler is smart enough?

For your consideration I suggest using >= 5 as it tends to be easier to reason
about when verifying bounds. The compiler should be smart enough to optimize
this to > 4 if such a micro-optimization is considered a win on a particular
ISA.

> Source/WebCore/platform/URL.cpp:882
> +	   && (url[4] == ':' || (isASCIIAlphaCaselessEqual(url[4], 's') &&
url.length() > 5 && url[5] == ':'));

For similar reasons I suggest changing > 5 to >= 6.

> Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:218
> +    EXPECT_FALSE(protocolIsInHTTPFamily("abc"));

I suggest that we also check the empty string, a null string, and a string with
a single character.