[webkit-reviews] review granted: [Bug 192853] stringProtoFuncRepeatCharacter overflow is not caught with 16-bit character times 2**30 : [Attachment 357741] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted  review:
Bug 192853: stringProtoFuncRepeatCharacter overflow is not caught with 16-bit
character times 2**30
https://bugs.webkit.org/show_bug.cgi?id=192853

Attachment 357741: Patch

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




--- Comment #8 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 357741
  --> https://bugs.webkit.org/attachment.cgi?id=357741
Patch

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

>>>> Source/WTF/wtf/text/StringImpl.cpp:196
>>>> +	  if (length > std::min(static_cast<size_t>(MaxLength),
(std::numeric_limits<unsigned>::max() - sizeof(StringImpl)) /
sizeof(CharacterType)))
>>> 
>>> Would it be possible to use a template function like this?
>>> 
>>>	template<typename CharacterType>
>>>	constexpr size_t maxUtf8Length() { return
std::min(static_cast<size_t>(MaxLength), (std::numeric_limits<unsigned>::max()
- sizeof(StringImpl)) / sizeof(CharacterType)); }
>>> 
>>> constexpr would be nice, but if that doesn't work, just change it to inline
instead.  This allows you to define this limit in one place instead of 3.
>> 
>> I forgot: this should be a static method.
> 
> Wait a minute.  I don't get this.  Why take the min of MaxLength and
(std::numeric_limits<unsigned>::max() - sizeof(StringImpl))?

OK, I get it:
1. The actual bytes allocated is allocationSize<CharacterType>(length) which
adds the sizeof(StringImpl) to length * sizeof(CharacterType).
2. For an 8bit char, the limit we'll use is always MaxLength.
3. For a 16-bit char, the limit can't be MaxLength because at MaxLength, length
* sizeof(CharacterType) is already equal to UINT_MAX.  Hence, adding
sizeof(StringImpl) would overflow length.

This also means that my choice of name for the method maxUtf8Length() is wrong.
 This has nothing to do with utf8 strings.  Can you do use a method instead:

    template<typename CharacterType>
    static constexpr unsigned maxInternalLength()
    {
	// In order to not overflow the unsigned length, the check for
(std::numeric_limits<unsigned>::max() - sizeof(StringImpl)) is needed when
sizeof(CharacterType) == 2.
	return std::min(static_cast<size_t>(MaxLength),
(std::numeric_limits<unsigned>::max() - sizeof(StringImpl)) /
sizeof(CharacterType));
    }

Sorry for the r- noise earlier.