46462 - basic_nsAWritableString<char>::Cut assumes that memcpys are done front to back

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Daniel Bratell]

basic_nsAWritableString<char>::Cut assumes that memcpys are done front to back 
which mustn't be true. It works fine as long as memcpy work from lower addresses 
to higher, but will break completely if it is implemented differently. 


String before Cut: ....XXXX-------   (Cutting away the XXXX part)
String after  Cut: ....-------
                           ^^^ overlap

This fell out of bug 40027 because Purify warns when it encounters these 
overlapping memcpy:s.

Comment 1

[Daniel Bratell]

I don't really now what to do here. The fix seems to use memmove instead of 
memcpy, or in this case nsCharTraits<char>::move instead of 
nsCharTraits<char>::copy but the caller isn't Cut itself. Instead the call 
chain goes through copy_string and two write functions. 

nsCharTraits<char>::copy(char *,char const*,UINT
nsWritingIterator<char>::write(char const*,UINT)
nsCharSinkTraits<nsWritingIterator<char>>::write(nsWritingIterator<char>&,char 
const*,UINT)
copy_string(nsReadingIterator<char>,nsReadingIterator<char>,nsWritingIterator<ch
ar>)
basic_nsAWritableString<char>::Cut(UINT,UINT)

There is a copy_string_backward which I guess is supposed to do the opposite of 
copy_string so copy_string is apparently supposed to copy forward but it isn't 
guaranteed to do that. I'm tempted to just change nsWritingIterator<char>::write 
to use move instead of copy, but I don't know if there are any drawbacks with 
using memmove? Performance?

(Reassigning to scc and cc:ing waterson who seems to be the ones who know about 
strings in mozilla)

Comment 2

[Scott Collins]

I actually just changing the |copy|s to |move|s in the char traits classes is the 
most reasonable answer for the current architecture (which doesn't differentiate 
between the cases that are moving things within a string, and writing into it, or 
copying out from/to an external source).

Comment 3

[Scott Collins]

Attachment: change appropriate |copy|s to |move|s

Comment 4

[Scott Collins]

We should measure; if the performance penalty is unacceptable, then we should fix 
the architecture to differentiate intra vs. inter string character moving

Comment 5

[Scott Collins]

there are actually a couple more |copy|s that must become |move|s in 
"nsAWritableString.h", I already got that patch approved by waterson along with 
some other changes in that file

Comment 6

[Scott Collins]

the changes are checked in.  As I said earlier: we'll want to profile this to see 
if |move| introduces a non-negligible performance impact, and if so, separate the 
code path for moves between strings versus within a string

Comment 7

[Tracy Walker [:tracy]]

please verify

Comment 8

[leger]

Marking Verified.  Please reopen if problem reoccurs.