Summary: | Replace WKStringGetCharactersPtr() with WKStringGetCharacters() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Miller <jeffm> | ||||||
Component: | New Bugs | Assignee: | 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
Jeff Miller
2011-04-07 10:27:16 PDT
Created attachment 88659 [details]
Patch
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 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 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. Created attachment 88670 [details]
Patch
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. (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. Committed r83198: <http://trac.webkit.org/changeset/83198> http://trac.webkit.org/changeset/83198 might have broken SnowLeopard Intel Release (Build) |