Bug 58058

Summary: Replace WKStringGetCharactersPtr() with WKStringGetCharacters()
Product: WebKit Reporter: Jeff Miller <jeffm>
Component: New BugsAssignee: Jeff Miller <jeffm>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch aroben: review+

Description Jeff Miller 2011-04-07 10:27:16 PDT
Replace WKStringGetCharactersPtr() with WKStringGetCharacters()
Comment 1 Jeff Miller 2011-04-07 10:31:36 PDT
Created attachment 88659 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-04-07 10:40:44 PDT
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.
Comment 3 Adam Roben (:aroben) 2011-04-07 10:44:16 PDT
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?
Comment 4 Adam Roben (:aroben) 2011-04-07 10:52:59 PDT
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.
Comment 5 Jeff Miller 2011-04-07 12:04:42 PDT
Created attachment 88670 [details]
Patch
Comment 6 Adam Roben (:aroben) 2011-04-07 12:07:26 PDT
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.
Comment 7 Jeff Miller 2011-04-07 12:11:09 PDT
(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.
Comment 8 Jeff Miller 2011-04-07 12:49:27 PDT
Committed r83198: <http://trac.webkit.org/changeset/83198>
Comment 9 WebKit Review Bot 2011-04-07 13:15:02 PDT
http://trac.webkit.org/changeset/83198 might have broken SnowLeopard Intel Release (Build)