61572 2011-05-26 15:51:43 -0700 KURL::protocolIs(const char* protocol) asserts in Debug builds with valid protocols 2011-06-21 09:08:58 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 ddkilzer aestes aestes darin joepeck oldest_to_newest 410950 0 ddkilzer 2011-05-26 15:51:43 -0700 * SUMMARY KURL::protocolIs(const char* protocol) throws an assertion in Debug builds when a (custom) protocol is compared that contains anything other than letters: ASSERTION FAILED: isASCIILower(lowercaseLetter) Source/WebCore/platform/KURL.cpp(63) : bool WebCore::isLetterMatchIgnoringCase(UChar, char) 1 _ZN7WebCoreL25isLetterMatchIgnoringCaseEtc 2 WebCore::KURL::protocolIs(char const*) const Note that Section 3.1 of RFC 3986 <http://tools.ietf.org/html/rfc3986#section-3.1> states that a scheme must start with a letter, and be composed of letters, digits, "+", "-" and "." characters. Thus this code that assumes that all characters in the scheme are letters is incorrect: // Do the comparison without making a new string object. for (int i = 0; i < m_schemeEnd; ++i) { if (!protocol[i] || !isLetterMatchIgnoringCase(m_string[i], protocol[i])) return false; } 410993 1 darin 2011-05-26 16:41:02 -0700 This is working as designed. The protocolIs function was designed to be used only with protocols that contain only letters. We can add the feature of checking for other protocols, and that will make it slower. We’re not assuming that all valid protocols contain entirely letters, just that all the ones we are using with this function are entirely letters. 411042 2 ddkilzer 2011-05-26 17:30:20 -0700 <rdar://problem/9510987> 422979 3 darin 2011-06-17 17:28:09 -0700 The issue is not just an assertion. The code actually works wrong for custom protocols with non-letters. When I originally wrote this we wanted it to be super-fast for protocols like "http" and "file" and it seemed the right tradeoff to only work with letters. If we want a slightly slower one that can work for any protocol, it's easy to change this into that. Or we could even have both, although I don't see an obvious way to automatically choose the faster one at compile time. The current function could be named fastAllLetterProtocolIs or something annoying like that. 422982 4 aestes 2011-06-17 17:32:30 -0700 *** Bug 62917 has been marked as a duplicate of this bug. *** 422986 5 darin 2011-06-17 17:35:46 -0700 (In reply to comment #3) > The current function could be named fastAllLetterProtocolIs or something annoying like that. Not sure we need a super-fast version, though. Please also see my comments in the patch in bug 62917. 424099 6 aestes 2011-06-20 17:07:04 -0700 (In reply to comment #3) > The issue is not just an assertion. The code actually works wrong for custom protocols with non-letters. > > When I originally wrote this we wanted it to be super-fast for protocols like "http" and "file" and it seemed the right tradeoff to only work with letters. If we want a slightly slower one that can work for any protocol, it's easy to change this into that. > > Or we could even have both, although I don't see an obvious way to automatically choose the faster one at compile time. > > The current function could be named fastAllLetterProtocolIs or something annoying like that. Since the valid non-letter scheme characters ('0'-'9', '+', '-', '.') all fall in the range [32, 63], ORing with 0x20 is a no-op. I think we can make a special version of isLetterMatchIgnoringCase() that works only for protocol characters. The check will be identical to isLetterMatchIgnoringCase() but will have a more permissive assertion. 424102 7 darin 2011-06-20 17:07:55 -0700 Good idea! 424110 8 97894 aestes 2011-06-20 17:22:29 -0700 Created attachment 97894 Patch 424141 9 97894 darin 2011-06-20 18:05:09 -0700 Comment on attachment 97894 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97894&action=review Is the isLetterMatchIgnoringCase function used anywhere else? If not, then we probably should remove it. > Source/WebCore/platform/KURL.cpp:249 > + ASSERT(isSchemeChar(character)); > + ASSERT(isASCIILower(schemeCharacter) || (!isASCIIUpper(schemeCharacter) && isSchemeChar(schemeCharacter))); > + return (character | 0x20) == schemeCharacter; I think we want another assertion: ASSERT(schemeCharacter & 0x20); 424175 10 aestes 2011-06-20 18:50:55 -0700 (In reply to comment #9) > (From update of attachment 97894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97894&action=review > > Is the isLetterMatchIgnoringCase function used anywhere else? If not, then we probably should remove it. It's used in numerous other places in KURL. > > > Source/WebCore/platform/KURL.cpp:249 > > + ASSERT(isSchemeChar(character)); > > + ASSERT(isASCIILower(schemeCharacter) || (!isASCIIUpper(schemeCharacter) && isSchemeChar(schemeCharacter))); > > + return (character | 0x20) == schemeCharacter; > > I think we want another assertion: > > ASSERT(schemeCharacter & 0x20); Agreed. Thanks for the review! 424281 11 aestes 2011-06-20 23:05:29 -0700 Committed r89336: <http://trac.webkit.org/changeset/89336> 424287 12 97894 aestes 2011-06-20 23:10:48 -0700 Comment on attachment 97894 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97894&action=review > Source/WebCore/platform/KURL.cpp:1788 > + if (!isSchemeCharacterMatchIgnoringCase(url[i], protocol[i])) I landed this without this change. Unlike the KURL member function, many strings are passed into the protocolIs() free function that are not normalized URLs and trigger assertions in isSchemeCharacterMatchIgnoringCase(). Of course the free function will trigger an assertion if the second argument isn't a letter-only protocol just like the member function did before this patch. I'm not sure how to handle this. It looks like some, but not all, call sites can be converted to call the KURL member function instead. 424489 13 darin 2011-06-21 09:08:58 -0700 I suggest making a slightly less optimized version for the case where we need it. I don’t know why I was so obsessed with using the | 0x20 trick. 97894 2011-06-20 17:22:29 -0700 2011-06-20 23:10:48 -0700 Patch bug-61572-20110620172228.patch text/plain 3084 aestes U3VidmVyc2lvbiBSZXZpc2lvbjogODkzMTIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMTExODk2MjdhODQ5NjNl NmRjYTAyYjZmZTQzZDEzZjE0Y2FkYWFiLi5kNWVhY2YwMmQ5NTQ0NDA0YzM5YmExMmNhZDI0YzFh OTFiZjNjYmQ3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMgQEAKKzIwMTEtMDYtMjAgIEFuZHkg RXN0ZXMgIDxhZXN0ZXNAYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEtVUkw6OnByb3RvY29sSXMoY29uc3QgY2hhciogcHJvdG9jb2wp IGFzc2VydHMgaW4gRGVidWcgYnVpbGRzIHdpdGgKKyAgICAgICAgdmFsaWQgcHJvdG9jb2xzCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MTU3MgorCisg ICAgICAgIE5vIG5ldyB0ZXN0cy4gTm8gY29kZSBjdXJyZW50bHkgY2FsbHMgcHJvdG9jb2xJcygp IHdpdGggYSBwcm90b2NvbCB0aGF0CisgICAgICAgIGNvbnRhaW5zIGEgbm9uLWxldHRlciBjaGFy YWN0ZXIuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9LVVJMLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OmlzU2NoZW1lQ2hhcmFjdGVyTWF0Y2hJZ25vcmluZ0Nhc2UpOiBBIGhlbHBlciBmdW5jdGlvbiB0 aGF0CisgICAgICAgIGNvbXBhcmVzIHR3byBjaGFyYWN0ZXJzIGlnbm9yaW5nIGNhc2UuIEl0IGFz c3VtZXMgKGFuZCBhc3NlcnRzKSB0aGF0CisgICAgICAgIGJvdGggY2hhcmFjdGVycyBhcmUgdmFs aWQgc2NoZW1lIGNoYXJhY3RlcnMsIGFuZCB0aGF0IGlmIHRoZSBzZWNvbmQKKyAgICAgICAgYXJn dW1lbnQgaXMgYSBsZXR0ZXIgdGhhdCBpdCBpcyBsb3dlcmNhc2UuCisgICAgICAgIChXZWJDb3Jl OjpLVVJMOjpwcm90b2NvbElzKTogQ2FsbCBpc1NjaGVtZUNoYXJhY3Rlck1hdGNoSWdub3JpbmdD YXNlKCkKKyAgICAgICAgaW5zdGVhZCBvZiBpc0xldHRlck1hdGNoSWdub3JpbmdDYXNlKCkuCisg ICAgICAgIChXZWJDb3JlOjpwcm90b2NvbElzKTogRGl0dG8uCisKIDIwMTEtMDYtMjAgIEFkYW0g QmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBFcmljIFNl aWRlbC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL0tVUkwuY3BwIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5jcHAKaW5kZXggODYxMWVkMTg5OWQwMTYyMWMwZWRj NDJiYjlkZDE2MzA5ZmFlMDRmZi4uMjkwMDQyYjVlM2Y2ZDlhZDdhNjA1M2VkODQ4MjllMDc3MTM2 M2RmNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5jcHAKQEAgLTI0MSw2ICsyNDEsMTMgQEAgc3Rh dGljIGlubGluZSBib29sIGlzSVB2NkNoYXIodW5zaWduZWQgY2hhciBjKSB7IHJldHVybiBjaGFy YWN0ZXJDbGFzc1RhYmxlW2NdICYKIHN0YXRpYyBpbmxpbmUgYm9vbCBpc1BhdGhTZWdtZW50RW5k Q2hhcihjaGFyIGMpIHsgcmV0dXJuIGNoYXJhY3RlckNsYXNzVGFibGVbc3RhdGljX2Nhc3Q8dW5z aWduZWQgY2hhcj4oYyldICYgUGF0aFNlZ21lbnRFbmRDaGFyOyB9CiBzdGF0aWMgaW5saW5lIGJv b2wgaXNQYXRoU2VnbWVudEVuZENoYXIoVUNoYXIgYykgeyByZXR1cm4gYyA8PSAweGZmICYmIChj aGFyYWN0ZXJDbGFzc1RhYmxlW2NdICYgUGF0aFNlZ21lbnRFbmRDaGFyKTsgfQogc3RhdGljIGlu bGluZSBib29sIGlzQmFkQ2hhcih1bnNpZ25lZCBjaGFyIGMpIHsgcmV0dXJuIGNoYXJhY3RlckNs YXNzVGFibGVbY10gJiBCYWRDaGFyOyB9CisgICAgCitzdGF0aWMgaW5saW5lIGJvb2wgaXNTY2hl bWVDaGFyYWN0ZXJNYXRjaElnbm9yaW5nQ2FzZShjaGFyIGNoYXJhY3RlciwgY2hhciBzY2hlbWVD aGFyYWN0ZXIpCit7CisgICAgQVNTRVJUKGlzU2NoZW1lQ2hhcihjaGFyYWN0ZXIpKTsKKyAgICBB U1NFUlQoaXNBU0NJSUxvd2VyKHNjaGVtZUNoYXJhY3RlcikgfHwgKCFpc0FTQ0lJVXBwZXIoc2No ZW1lQ2hhcmFjdGVyKSAmJiBpc1NjaGVtZUNoYXIoc2NoZW1lQ2hhcmFjdGVyKSkpOworICAgIHJl dHVybiAoY2hhcmFjdGVyIHwgMHgyMCkgPT0gc2NoZW1lQ2hhcmFjdGVyOworfQogCiBzdGF0aWMg aW5saW5lIGludCBoZXhEaWdpdFZhbHVlKFVDaGFyIGMpCiB7CkBAIC03MDIsNyArNzA5LDcgQEAg Ym9vbCBLVVJMOjpwcm90b2NvbElzKGNvbnN0IGNoYXIqIHByb3RvY29sKSBjb25zdAogCiAgICAg Ly8gRG8gdGhlIGNvbXBhcmlzb24gd2l0aG91dCBtYWtpbmcgYSBuZXcgc3RyaW5nIG9iamVjdC4K ICAgICBmb3IgKGludCBpID0gMDsgaSA8IG1fc2NoZW1lRW5kOyArK2kpIHsKLSAgICAgICAgaWYg KCFwcm90b2NvbFtpXSB8fCAhaXNMZXR0ZXJNYXRjaElnbm9yaW5nQ2FzZShtX3N0cmluZ1tpXSwg cHJvdG9jb2xbaV0pKQorICAgICAgICBpZiAoIXByb3RvY29sW2ldIHx8ICFpc1NjaGVtZUNoYXJh Y3Rlck1hdGNoSWdub3JpbmdDYXNlKG1fc3RyaW5nW2ldLCBwcm90b2NvbFtpXSkpCiAgICAgICAg ICAgICByZXR1cm4gZmFsc2U7CiAgICAgfQogICAgIHJldHVybiAhcHJvdG9jb2xbbV9zY2hlbWVF bmRdOyAvLyBXZSBzaG91bGQgaGF2ZSBjb25zdW1lZCBhbGwgY2hhcmFjdGVycyBpbiB0aGUgYXJn dW1lbnQuCkBAIC0xNzc4LDcgKzE3ODUsNyBAQCBib29sIHByb3RvY29sSXMoY29uc3QgU3RyaW5n JiB1cmwsIGNvbnN0IGNoYXIqIHByb3RvY29sKQogICAgIGZvciAoaW50IGkgPSAwOyA7ICsraSkg ewogICAgICAgICBpZiAoIXByb3RvY29sW2ldKQogICAgICAgICAgICAgcmV0dXJuIHVybFtpXSA9 PSAnOic7Ci0gICAgICAgIGlmICghaXNMZXR0ZXJNYXRjaElnbm9yaW5nQ2FzZSh1cmxbaV0sIHBy b3RvY29sW2ldKSkKKyAgICAgICAgaWYgKCFpc1NjaGVtZUNoYXJhY3Rlck1hdGNoSWdub3JpbmdD YXNlKHVybFtpXSwgcHJvdG9jb2xbaV0pKQogICAgICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAg IH0KIH0K