Bug 58058 – Replace WKStringGetCharactersPtr() with WKStringGetCharacters()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeff Miller]

Replace WKStringGetCharactersPtr() with WKStringGetCharacters()

[Jeff Miller]

Created attachment 88659 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 88659 [details]
Patch

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

I should be encouraging you to write API tests for the new API you're adding. They're pretty easy to write; just add some code to Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp.

> Source/WebKit2/Shared/WebString.h:68
> +        ASSERT(buffer);
> +        if (!buffer)
> +            return 0;

I don't think we're normally this defensive in the WK2 API.

> Source/WebKit2/Shared/WebString.h:72
> +        size_t lengthInBytes = m_string.length() * sizeof(UChar);
> +        if (bufferSize > lengthInBytes)
> +            bufferSize = lengthInBytes;
> +        memcpy(buffer, m_string.characters(), bufferSize);

This will cause a buffer overrun if bufferSize is less than lengthInBytes.

> Source/WebKit2/Shared/API/c/WKString.cpp:55
> +size_t WKStringGetCharacters(WKStringRef stringRef, WKChar* buffer, size_t bufferSize)

It's surprising that this API takes a number bytes to copy rather than a number of characters.

[Adam Roben (:aroben)]

Comment on attachment 88659 [details]
Patch

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

>> Source/WebKit2/Shared/WebString.h:72
>> +        memcpy(buffer, m_string.characters(), bufferSize);
> 
> This will cause a buffer overrun if bufferSize is less than lengthInBytes.

Maybe this was intentional? Perhaps the caller is required to pass in a large enough bufferSize?

[Adam Roben (:aroben)]

Comment on attachment 88659 [details]
Patch

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

>>> Source/WebKit2/Shared/WebString.h:72
>>> +        memcpy(buffer, m_string.characters(), bufferSize);
>> 
>> This will cause a buffer overrun if bufferSize is less than lengthInBytes.
> 
> Maybe this was intentional? Perhaps the caller is required to pass in a large enough bufferSize?

Jeff politely pointed out to me that there is in fact no overrun here.

[Jeff Miller]

Created attachment 88670 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 88670 [details]
Patch

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

> Source/WebKit2/Shared/WebString.h:68
> +        if (bufferLength > length)
> +            bufferLength = length;

You could use std::min here.

> Source/WebKit2/Shared/API/c/WKString.cpp:58
> +    return (toImpl(stringRef)->getCharacters(static_cast<UChar*>(buffer), bufferLength));

I'm a little surprised you don't need reinterpret_cast here.

> Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:60
> +    maxSize = WKStringGetLength(string);
> +    TEST_ASSERT(maxSize == 5);
> +
> +    WKChar* uniBuffer = new WKChar[maxSize];
> +    actualSize = WKStringGetCharacters(string, uniBuffer, maxSize);
> +    TEST_ASSERT(actualSize == 5);
> +    
> +    WKChar helloBuffer[] = { 'h', 'e', 'l', 'l', 'o' };
> +    TEST_ASSERT(!memcmp(uniBuffer, helloBuffer, 10));
> +    
> +    delete[] uniBuffer;
> +    

Might be good to have tests that pass in buffers that are larger and smaller than needed.

[Jeff Miller]

(In reply to comment #6)
> (From update of attachment 88670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88670&action=review
> 
> > Source/WebKit2/Shared/WebString.h:68
> > +        if (bufferLength > length)
> > +            bufferLength = length;
> 
> You could use std::min here.

Will do.

> 
> > Source/WebKit2/Shared/API/c/WKString.cpp:58
> > +    return (toImpl(stringRef)->getCharacters(static_cast<UChar*>(buffer), bufferLength));
> 
> I'm a little surprised you don't need reinterpret_cast here.

Yeah, but it works.  My typical MO is to try static_cast first, and use reinterpret_cast if the compiler complains. :)

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:60
> > +    maxSize = WKStringGetLength(string);
> > +    TEST_ASSERT(maxSize == 5);
> > +
> > +    WKChar* uniBuffer = new WKChar[maxSize];
> > +    actualSize = WKStringGetCharacters(string, uniBuffer, maxSize);
> > +    TEST_ASSERT(actualSize == 5);
> > +    
> > +    WKChar helloBuffer[] = { 'h', 'e', 'l', 'l', 'o' };
> > +    TEST_ASSERT(!memcmp(uniBuffer, helloBuffer, 10));
> > +    
> > +    delete[] uniBuffer;
> > +    
> 
> Might be good to have tests that pass in buffers that are larger and smaller than needed.

Good idea, I'll add those.

[Jeff Miller]

Committed r83198: <http://trac.webkit.org/changeset/83198>

[WebKit Review Bot]

http://trac.webkit.org/changeset/83198 might have broken SnowLeopard Intel Release (Build)