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

VERIFIED FIXED

Status

()

Core
XPCOM
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Daniel Bratell, Assigned: Scott Collins)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
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)
Assignee: rayw → scc
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

18 years ago
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).
(Assignee)

Comment 3

18 years ago
Created attachment 12544 [details] [diff] [review]
change appropriate |copy|s to |move|s
(Assignee)

Comment 4

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

Comment 5

18 years ago
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
(Assignee)

Comment 6

18 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 7

18 years ago
please verify

Comment 8

18 years ago
Marking Verified.  Please reopen if problem reoccurs.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.