10075 - Unterminated PRUnichar* -> segfault in nsString::Append()

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Kenneth Herron]

Crash in apprunner during startup before any window appeared.  Everything's
built with xlib, but I don't think that's relevant.  Apprunner printed:

[...]
ProfileName : mozProfile
ProfileDir  : /home/kherron/.mozilla/mozProfile
nsDeviceContextXlib::Init(dpy=0x811d808  screen=0x8146d08  visual=0x8164408
depth=16)
Using '/home/kherron/moz/dmalloc-xlib/dist/bin' as the resource: base
Reading file...
Reading file...Done
Reading file...


Oh no!  ./apprunner just dumped a core file.
[etc.  Start gdb]
#0  0x401b235a in nsCRT::strlen (s=0x837d002) at
../../../mozilla/xpcom/ds/nsCRT.cpp:261
261	    while (*s++ != 0) {
(gdb) p len
$1 = 6140
(gdb) bt
#0  0x401b235a in nsCRT::strlen (s=0x837d002) at
../../../mozilla/xpcom/ds/nsCRT.cpp:261
#1  0x401c168e in nsString::Append (this=0xbfffec30, aString=0x837a008,
aCount=37)
    at ../../../mozilla/xpcom/ds/nsString2.cpp:1006
#2  0x408a349a in RDFContentSinkImpl::FlushText (this=0x833e188,
aCreateTextNode=1,
    aDidFlush=0x0) at ../../../../mozilla/rdf/base/src/nsRDFContentSink.cpp:808
#3  0x408a2998 in RDFContentSinkImpl::CloseContainer (this=0x833e188,
aNode=@0xbfffed68)
    at ../../../../mozilla/rdf/base/src/nsRDFContentSink.cpp:536
#4  0x402f055e in CWellFormedDTD::HandleToken (this=0x836f7c8, aToken=0x8374788,
    aParser=0x8345408) at
../../../mozilla/htmlparser/src/nsWellFormedDTD.cpp:514
#5  0x402efaa0 in CWellFormedDTD::BuildModel (this=0x836f7c8, aParser=0x8345408,
    aTokenizer=0x8370a88, anObserver=0x0, aSink=0x833e188)
    at ../../../mozilla/htmlparser/src/nsWellFormedDTD.cpp:252
#6  0x402e82dc in nsParser::BuildModel (this=0x8345408)
    at ../../../mozilla/htmlparser/src/nsParser.cpp:932
#7  0x402e81a6 in nsParser::ResumeParse (this=0x8345408, aDefaultDTD=0x0,
aIsFinalChunk=0)
    at ../../../mozilla/htmlparser/src/nsParser.cpp:879
[and so on]...

Snooping around gets:

#0  0x401b235a in nsCRT::strlen (s=0x837d002) at
../../../mozilla/xpcom/ds/nsCRT.cpp:261
261	    while (*s++ != 0) {
(gdb) p len
$14 = 6140
(gdb) up
#1  0x401c168e in nsString::Append (this=0xbfffec30, aString=0x837a008,
aCount=37)
    at ../../../mozilla/xpcom/ds/nsString2.cpp:1006
1006	    temp.mLength=nsCRT::strlen(aString);
(gdb) p aCount
$15 = 37
(gdb) p aString[36]
$16 = 47
(gdb) p aString[37]
$17 = 50629
(gdb)

It appears that nsCRT::strlen() requires its argument to be 0-terminated, which
means nsString::Append() requires its first arg to be 0-terminated, but the
arg isn't in fact 0-terminated.

I've also had this happen in viewer where the text being manipulated was the
html for example 0.  Unfortunately I didn't save the evidence, but I managed
to determine that during a step in which the text was converted from a char
string to a PRUnichar string, the PRUnichar version wasn't being 0-terminated.

Comment 1

[Suresh Duddi (gone)]

Rick would this be you.

Comment 2

[rickg]

Hmm -- I'm greatful for the bug, but there are lots of conflicting details here.
For example, the flushtext call in RDF doesn't pass a count, so the count arg is
-1, which is then computed to via strlen. So it's odd that 37 would come in a
count, when len = 6140 in strlen.

More importantly, I could really use more data on which URL's you saw this on.
When I run apprunner I don't get any of these behaviors -- and the code in
question is completely XP.

Comment 3

[rickg]

One other thing -- the comments regarding the null terminated strings makes me
curious. Char*'s and PRUnichar*'s are always assumed to be null terminated.
Providing a count merely allows you to copy fewer chars than the full length of
the char* or PRUnichar*.

Comment 4

[rickg]

Just to be safe -- I've added just a tad more error checking to str functions.
In particular, I now guard against appending more chars than are actually in the
char* or PRUnichar*. Hope this helps. If you can provide more error data I'd be
greatful.

Comment 5

[Kenneth Herron]

Sorry, you're right; I left some things out.  I'm experimenting with a malloc
debugging library called dmalloc (<http://www.dmalloc.com/>); I didn't mention
this earlier because I wasn't sure it was relevant, but with further work it's
much easier to produce the crash if I turn on some of dmalloc's options.  It's
also rather random, but I find that if enable dmalloc and start running the
program, I get one of these crashes pretty quickly.

In any event, I've reproduced the viewer crash and it's a little clearer
what is going on.  Please see the attachment.  When the crash occurred, I had
just selected sample #1 from the samples built into viewer, which loads
bin/res/samples/test1.html.  This file is 2126 bytes long and the last two
characters in it are ">\n".  You can see bits of the HTML in the backtrace;
the code is in the process of loading the file into a char buf, converting it
to a PRUnichar buf, and appending it to another (blank I think) string.

At stack level 0, we can see the immediate cause of the crash is that strlen()
has gone 4096 bytes without seeing a NUL byte and walked off the end of valid
memory.  At stack level 1, we can see that the string passed to strlen()
is 2126 unichars long but not NUL-terminated.  62 is '>', 10 is '\n', and
56937 is uninitialized memory.  At stack level 2 is nsScanner::Append.  If
you examine this code, it converts a char* into a PRUnichar*, but the resulting
PRUnichar* is never NUL-terminated.  The buffer doesn't even have room
for the NUL; it's allocated exactly large enough for the text.

I didn't (and still don't) know enough to say whether nsString::Append is
wrong for assuming its arg would be NUL-terminated, or whether these callers
are just using it incorrectly.

Comment 6

[Kenneth Herron]

Attachment: Viewer crash gdb session

Comment 7

[Daniel Bratell]

I get this in a debug build on Windows NT too, so I set OS to all. The stack I
had at the crash moment is included below. What I don't understand is why the
argument to strlen is different than the argument to Append. It should be the
same as far as I can tell.

nsCRT::strlen(const unsigned short * 0x00e50000) line 261 + 5 bytes
nsString::Append(const unsigned short * 0x00e4f5c0, int 1240) line 1006 + 9
bytes
nsScanner::Append(const char * 0x00e566a0, unsigned int 1240) line 256
nsParser::OnDataAvailable(nsParser * const 0x022d52e4, nsIURI * 0x02245b50,
nsIInputStream * 0x022d1990, unsigned int 1240) line 1142
nsDocumentBindInfo::OnDataAvailable(nsDocumentBindInfo * const 0x022454e0,
nsIURI * 0x02245b50, nsIInputStream * 0x022d1990, unsigned int 1240) line 2023 +
24 bytes
OnDataAvailableProxyEvent::HandleEvent(OnDataAvailableProxyEvent * const
0x022c7130) line 634
StreamListenerProxyEvent::HandlePLEvent(PLEvent * 0x022c7134) line 473 + 12
bytes
PL_HandleEvent(PLEvent * 0x022c7134) line 509 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00cab850) line 470 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x000f020c, unsigned int 49481, unsigned int 0,
long 13285456) line 932 + 9 bytes
USER32! 77e71820()
00cab850()

Comment 8

[Daniel Bratell]

Looking closer at the stack dump I see that it might not be the same crash.
The one I'm seeing also appears after the window has appeared and while the
page is being rendered. Sometimes it doesn't apperar at the first page
visited but the second. The content of the page dosen't seem to matter.

I can open a new bug if you want to.

Comment 9

[rickg]

I've added more argument condition to the nsStr class. If the real problem was
there, then this should fix it.

Comment 10

[Daniel Bratell]

It still crashes for me with these changes. I have opened a new bug (10104) for
my case.

Comment 11

[Kenneth Herron]

I'm still crashing too.  The change to nsString::Append() doesn't address the
problem; the crash is occurring in nsCRT::strlen(), and the modification is
after that.  nsCRT::strlen() is crashing because its argument isn't
NUL-terminated, so it'll keep going until it sees a NUL PRUnichar or until it
reaches the end of valid memory.

My suspicion is that other coders are assuming that, since this particular
version of nsString::Append() takes a length argument, it's just going to
append that many characters and doesn't require the string be terminated.
Is there any API documentation detailed enough to support or refute this?
I haven't seen any.

I'm going to try reporting these as bugs with whatever's constructing these
strings and hope we don't get into fingerpointing.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a818afc2e74e
[components] Workflow to sync strings (https://github.com/mozilla-mobile/android-components/pull/10075)