Bug 192853 – WTF::String and StringImpl overflow MaxLength

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

<rdar://problem/45726906>

[Tadeu Zagallo]

Created attachment 357678 [details]
Patch

[Tadeu Zagallo]

Created attachment 357741 [details]
Patch

[Keith Miller]

Comment on attachment 357741 [details]
Patch

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

r=me

> Source/WTF/wtf/text/StringImpl.h:575
> +template<typename CharacterType> size_t reverseFindLineTerminator(const CharacterType*, unsigned length, unsigned index = StringImpl::MaxLength);
> +template<typename CharacterType> size_t reverseFind(const CharacterType*, unsigned length, CharacterType matchCharacter, unsigned index = StringImpl::MaxLength);
> +size_t reverseFind(const UChar*, unsigned length, LChar matchCharacter, unsigned index = StringImpl::MaxLength);
> +size_t reverseFind(const LChar*, unsigned length, UChar matchCharacter, unsigned index = StringImpl::MaxLength);

Nit: You don't need the "StringImpl::" here

[Mark Lam]

Comment on attachment 357741 [details]
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.

> Source/WTF/wtf/text/StringImpl.cpp:225
> +    if (length > std::min(static_cast<size_t>(MaxLength), (std::numeric_limits<unsigned>::max() - sizeof(StringImpl)) / sizeof(CharacterType)))

Use maxUtf8Length<CharacterType>() instead.

> Source/WTF/wtf/text/StringImpl.h:957
> +    if (length > std::min(static_cast<size_t>(MaxLength), (std::numeric_limits<unsigned>::max() - sizeof(StringImpl)) / sizeof(CharacterType))) {

Use maxUtf8Length<CharacterType> instead.

[Mark Lam]

Comment on attachment 357741 [details]
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.

[Mark Lam]

Comment on attachment 357741 [details]
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))?

[Mark Lam]

Comment on attachment 357741 [details]
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.

[Tadeu Zagallo]

Created attachment 357811 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 357811 [details]
Patch for landing

Clearing flags on attachment: 357811

Committed r239439: <https://trac.webkit.org/changeset/239439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.