40027 - nsCString::ToCString: Reads past internal buffer. Was: Overlapping memcpy causes Purify to warn

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Daniel Bratell]

I don't know if this is a bug or not, but when running with mozilla with Purify 
I get a lot of warnings:
[W] PAR: Overlapping block copy may produce unexpected results:... {892 
occurrences}

I don't know how to reproduce it (if it is a bug) but I have the top of a stack 
trace that might say something.

    memcpy         [memcpy.asm:109]

    nsCharTraits<char>::copy(char *,char const*,UINT) [nsCharTraits.h:282]

    nsWritingIterator<char>::write(char const*,UINT) [nsAWritableString.h:193]
    
nsCharSinkTraits<nsWritingIterator<char>>::write(nsWritingIterator<char>&,char 
const*,UINT) [nsCharTraits.h:586]
    
copy_string(nsReadingIterator<char>,nsReadingIterator<char>,nsWritingIterator<ch
ar>) [nsAlgorithm.h:72]
    
basic_nsAWritableString<char>::do_AssignFromReadable(basic_nsAReadableString<cha
r> const&) [nsAWritableString.h:432]

    basic_nsAWritableString<char>::do_AssignFromElementPtrLength(char 
const*,UINT) [nsAWritableString.h:492]

    basic_nsAWritableString<char>::Assign(char const*,UINT) 
[nsAWritableString.h:284]

    nsCString::ToCString(char *,UINT,UINT)const [nsString.cpp:697]

    nsIOService::GetProtocolHandler(char const*,nsIProtocolHandler * *) 
[nsIOService.cpp:122]

Comment 1

[Suresh Duddi (gone)]

Scott, can you analyze this.

Comment 2

[Scott Collins]

*** Bug 42601 has been marked as a duplicate of this bug. ***

Comment 3

[Daniel Bratell]

I tried to analyze an instance of this and I think I found the bug. Waterson, 
can you take a swift look at this, since (I guess) scc isn't available and the 
bug might be well worth fixing quickly? (The fix is at the bottom). 

This was my notes during debugging and I leave them here if I was wrong or if 
anyone else want to find out what I did.

*********** DEBUGGING NOTES / MAY BE SKIPPED ****************
In basic_nsAWritableString<CharT>::do_AssignFromReadable I have a string 
aReadable which contains the string 
component://netscape/network/protocol?name=about
on the adresses 0x13fa98 to 0x13fac8 (including the null char after the string). 
The fragment according to what BeginReading and EndReading returns is between 
0x13fa98 and 0x13fb03. Notice that it's larger than the string. 

BeginWriting returns a fragment that 0x13fadc and 0x13fb47. Notice that it 
overlaps the fragment in the source string! That must be the bug.

So where do the fragments come from? The write fragment comes from a stack char 
array of 108 chars with start address 0x13fadc and end adress 0x13fb47. That's 
consistant with what we already know. So how come the other (source/read 
fragment) points into that area?

The source originates from a nsCAutoString allocated directly after the char 
array. The nsCAutoString is constructed directly with the argument 
"component://netscape/network/protocol?name=". After that "about" is appended 
with the += operation and ToLowerCase is called. The string then contains a 
buffer (mBuffer) starting at 0x13fa98 and a capacity (according to the nsStr 
member) of 63. That would make the string end at 0x13fad6 which is well ahead of 
the char array. Shouldn't be any problem here. So why did EndReading say that it 
could read till 0x13fb03? We are closing in on the bug!

Looking at nsCString::ToCString, it first creates a buffer descriptor describing 
the char array where the string should end up, and then a nsCAutoString object 
that uses that buffer. Nothing strange here. But then something weird happens. 
This new String gets assigned a pointer to the old string 0x13fa98 with 107 as 
buffer length. But 107 is the length of _the_other_ buffer! This pointer is only 
pointing to an area 48/63 bytes large depending if you count the length of the 
string or the capacity of the buffer. I think we've found the bug!
*********** END DEBUGGING NOTES  ***************************


The fix would be to replace aBufLength-1 with PR_MIN(mLength, aBufLength-1). 
Everything seems to work fine and the purify warnings are gone.

Index: nsString.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsString.cpp,v
retrieving revision 3.125
diff -r3.125 nsString.cpp
692c692
<     temp.Assign(*this,aBufLength-1);
---
>     temp.Assign(*this, PR_MIN(mLength, aBufLength-1));


The same bug may be in nsString::ToCString but I'm a little too tired to look at 
that right now (time differences you know).

Since this could cause crashes (and maybe already does) because we sometimes 
read way beyond "our" memory and we do have a fix (if it's correct, which i 
think it is), I wan't to nominate this for nsbeta2.

Comment 4

[Chris Waterson]

Yep, looks good. r,a=waterson

Comment 5

[Daniel Bratell]

Do you want me to check it in?

Comment 6

[Chris Waterson]

yes, please!

Comment 7

[Daniel Bratell]

Fix checked in. I will leave the bug open so that nsString::ToString is looked 
at. I also think there may be some other place in the string code where purify 
warns about overlapping memcpy, but at least the massive amount of warnings are 
gone now.

Comment 8

[Daniel Bratell]

Well, nsString::ToCString has the same code but it uses a totally different way 
to copy the string and in that way there are code that corrects the count if you 
pass in a too large value. Nevertheless, I think nsString::ToCString should be 
fixed to pass in the correct value from the beginning. 

Proposed patch:

Index: nsString2.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsString2.cpp,v
retrieving revision 1.98
diff -r1.98 nsString2.cpp
787c787
<     nsStr::StrAssign(temp, *this, anOffset, aBufLength-1);
---
>     nsStr::StrAssign(temp, *this, anOffset, PR_MIN(mLength, aBufLength-1));

Waterson?

nsString2x (whatever that is) does it correctly and the rest of the overlapping 
memcpy:s is covered in bug 46462 so this should be the last fix before closing 
this bug.

Comment 9

[Chris Waterson]

r,a=waterson.

PDT: we should get this checked in on the nsbeta2 branch. It's a 
straight-forward "long read" bug that has the potential of introducing random 
crashes if the string's buffer has been allocated at the end of a page. This 
method is called fairly frequently, and the long read error has shown up 
repeatedly in Purify logs.

Comment 10

[Daniel Bratell]

Fixed.

The part that really could cause crashes was checked in yesterday, before the 
branching. For nsString2.cpp, there was sanity checking further down the call 
chain that corrected the parameter, but now we will send it in correctly from 
the beginning. 

Comment 11

[leger]

Marking Verified.