[webkit-reviews] review denied: [Bug 88936] StringImpl::characters can return NULL for an empty string : [Attachment 147419] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at google.com>'s request for review:
Bug 88936: StringImpl::characters can return NULL for an empty string
https://bugs.webkit.org/show_bug.cgi?id=88936

Attachment 147419: Patch
https://bugs.webkit.org/attachment.cgi?id=147419&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=147419&action=review


> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:101
> +    // The ICU functions have the property where they assume that a null
pointer means an invalid string
> +    // (and therefore won't do the comparison). A null pointer could come
about here if an empty string
> +    // was allocated with a malloc() implementation that returns null on a
zero-sized malloc (which is
> +    // valid according to C99 section 7.20.3). Therefore, we have to change
any valid null pointers before
> +    // passing them to ICU.

Comment is much too long. Should say something more like this:

    // ICU does not allow null pointers for empty strings, but we do.

> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:106
> +    if (!lhs && !lhsLength)
> +	 lhs = (const UChar*)"";
> +    if (!rhs && !rhsLength)
> +	 rhs = (const UChar*)"";

This is wrong. You can’t just cast the pointer to an empty C string to a UChar*
and expect it to work. That will read off the end of the buffer.