Summary: | REGRESSION (r253987): StringImpl::adopt(Vector&&) copies only half of the characters in the Vector when copying across malloc zones | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Fujii Hironori
2020-04-20 00:06:48 PDT
Or,
> return StringImpl::create(vector.data(), size);
(In reply to Fujii Hironori from comment #0) > StringImpl::adopt truncates the string by copying only half of the argument > Vector > > r253987 (Bug 186422) added the following line: > > > memcpy(stringImplBuffer.get(), vectorBuffer.get(), size); > > But it should be the following for UChar string. > > > memcpy(stringImplBuffer.get(), vectorBuffer.get(), size * sizeof(CharacterType)); StringImpl::copyCharacters(stringImplBuffer.get(), vectorBuffer.get(), size); (In reply to Darin Adler from comment #2) > StringImpl::copyCharacters(stringImplBuffer.get(), vectorBuffer.get(), size); Since the code is inside StringImpl: copyCharacters(stringImplBuffer.get(), vectorBuffer.get(), size); Should use copyCharacters instead of memcpy both in StringImpl::adopt and in StringImpl::replace. And in StringImpl::StringImpl. copyCharacters(const_cast<LChar*>(m_data8), characters.get(), length); Let's just not use memcpy, which ignores the types of the pointers, except in copyCharacters, which respects the types of the pointers. Hard part about this is adding an appropriate test case. Oops, nice catch! I think this is hard to test without manual one, because this code is used when malloc_zone breakdown debugging feature is enabled. Created attachment 396980 [details]
Patch
I did not add a test. Maybe someone else wants to pick up my patch and add a test? Created attachment 396982 [details]
Patch
Created attachment 396983 [details]
Patch
Comment on attachment 396983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396983&action=review > Source/WTF/wtf/text/StringImpl.h:1044 > + return create(vector.data(), vector.size()); Could have this do vector.clear() after creating the StringImpl, but I don’t think that’s necessary. Yusuke, as further cleanup maybe we could change the StringImpl(MallocPtr) constructor to not be a template, and only work with StringImplMalloc. Make it a caller responsibility to check. Is there any other caller? (In reply to Darin Adler from comment #13) > Yusuke, as further cleanup maybe we could change the StringImpl(MallocPtr) > constructor to not be a template, and only work with StringImplMalloc. Make > it a caller responsibility to check. Is there any other caller? Hmm, no it seems that it can be called with StringBufferMalloc, so I guess we'll leave it alone for now. Comment on attachment 396983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396983&action=review r=me >> Source/WTF/wtf/text/StringImpl.h:1044 >> + return create(vector.data(), vector.size()); > > Could have this do vector.clear() after creating the StringImpl, but I don’t think that’s necessary. Yeah, I think either is OK. Committed r260383: <https://trac.webkit.org/changeset/260383> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396983 [details]. |