[webkit-reviews] review granted: [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 bugzilla-daemon at webkit.org
Tue Apr 28 17:04:00 PDT 2020


Darin Adler <darin at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for 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 #15 from Darin Adler <darin 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

Honestly I think needsToGrowToProduceBuffer doesn’t make things easier to read.
It’s mainly valuable as a record of the fact that you audited a given call
site. On the other hand, needsToGrowToProduceCString is definitely valuable.
But only in exactly one place. I think adding the header is overkill, but I
won’t stand in your way if you still like it.

> 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.

> Source/JavaScriptCore/runtime/IntlObject.cpp:346
> +#if ASSERT_ENABLED
> +    bool sawNullTerminator = false;
> +    for (char c : intermediate) {
> +	   if (c == '\0') {
> +	       sawNullTerminator = true;
> +	       break;
> +	   }
> +    }
> +    ASSERT(sawNullTerminator);
> +#endif

Alternate implementation that does the same thing:

    ASSERT(intermediate.contains('\0'));

> 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.

> Source/WTF/wtf/text/LineBreakIteratorPoolICU.h:79
>	       scratchBuffer.grow(lengthNeeded + 1);

Seems like this should just be lengthNeeded, not + 1.


More information about the webkit-reviews mailing list