[webkit-reviews] review denied: [Bug 211142] U_STRING_NOT_TERMINATED_WARNING ICU must be handled when using the output buffer as a C string : [Attachment 397904] patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied  review:
Bug 211142: U_STRING_NOT_TERMINATED_WARNING ICU must be handled when using the
output buffer as a C string
https://bugs.webkit.org/show_bug.cgi?id=211142

Attachment 397904: patch

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




--- Comment #19 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 397904
  --> https://bugs.webkit.org/attachment.cgi?id=397904
patch

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

>> Source/JavaScriptCore/runtime/IntlObject.cpp:154
>> +	    return String(buffer.data(), length);
> 
> This is wrong.  convertICULocaleToBCP47LanguageTag() is only called from
inside std::call_once guarded blocks i.e. it's meant to produce immortal
singleton strings.  Before I changed it to be
StringImpl::createStaticStringImpl(), we had UAFs due to a user of this string
freeing it.

As Mark and I discussed on Slack, the actual issue here isn't being called from
call_once, but the fact that the resulting string may ref/deref from more than
one thread at once.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:334
>> +	if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING ||
parsedLength != static_cast<int32_t>(input.length()))
> 
> How can U_STRING_NOT_TERMINATED_WARNING happen here? Seems like we can just
leave this out or simply assert it doesn’t happen. We are using the length it
returned, so there is no chance it will happen.

My thinking here was just ICU failing for some reason, but this would be a
weird way for it to fail.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:346
>> +#endif
> 
> Alternate implementation that does the same thing:
> 
>     ASSERT(intermediate.contains('\0'));

That'd be too easy!

>> Source/WTF/wtf/text/LineBreakIteratorPoolICU.h:78
>> +	    if (needsToGrowToProduceCString(status)) {
> 
> This doesn’t seem right. I don’t see code depending on scratchBuffer being
null terminated.

Yeah it's not needed. I got confused with the AtomString::fromUTF8 version that
doesn't take "length" that ends up calling strlen.