Bug 210736 – REGRESSION (r253987): StringImpl::adopt(Vector&&) copies only half of the characters in the Vector when copying across malloc zones

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Fujii Hironori]

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));

[Fujii Hironori]

Or,

> return StringImpl::create(vector.data(), size);

[Darin Adler]

(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);

[Darin Adler]

(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);

[Darin Adler]

Should use copyCharacters instead of memcpy both in StringImpl::adopt and in StringImpl::replace.

[Darin Adler]

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.

[Darin Adler]

Hard part about this is adding an appropriate test case.

[Yusuke Suzuki]

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.

[Darin Adler]

Created attachment 396980 [details]
Patch

[Darin Adler]

I did not add a test. Maybe someone else wants to pick up my patch and add a test?

[Darin Adler]

Created attachment 396982 [details]
Patch

[Darin Adler]

Created attachment 396983 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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?

[Darin Adler]

(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.

[Yusuke Suzuki]

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.

[EWS]

Committed r260383: <https://trac.webkit.org/changeset/260383>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396983 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62069530>