374244 - splitText() overflow causes Minefield to crash [@ nsBlockFrame::AppendFrames]

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[lsg-mtso]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070315 Minefield/3.0a3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070315 Minefield/3.0a3pre

splitText() appears to have a buffer overflow when presented with long string arguments.  This has been verified on the latest versions of Minefield on Win32.

FOund w/Javascript introspection fuzzer.  Calling splitText() directly doesn't have same results -- likely requires other dom elements to be manipulated prior to the splitText call.  See attached script that has been tweaked to only trigger this flaw.  Expect to respond to error popups for bad protocol 2-3 times prior to crash.

See win32 talkback output attached.



Reproducible: Always

Steps to Reproduce:
1. Go to above url.
2. click ok when it says its starting
3. click ok in response to bad protocol dialogs (unrelated to vuln)

Actual Results:  
Browser crashes.  EIP appears to be null.

Expected Results:  
Browser chugs along...

see attachments for test code + talkback.  test code has *only* been tested when served from a webserver.

Comment 1

[lsg-mtso]

Attachment: talkback output

talkback output from crash

Comment 2

[lsg-mtso]

Attachment: repro script

repro script.  expect to click ok to proto error dialog a couple of times prior to crash

Comment 3

[Martijn Wargers (dead)]

The repro script testcase only crashes for me when I test it locally, after I've clicked 2 or 3 alerts away.

Reporter, thanks for attaching the talkback output, but only a talkback ID is useful, normally.
I get this Talkback ID: TB30306141E
nsTextFragment::CharAt  [mozilla/dist/include/content/nstextfragment.h, line 184]
nsBlockFrame::AppendFrames  [mozilla/layout/generic/nsblockframe.cpp, line 4561]
nsCSSFrameConstructor::AppendFrames  [mozilla/layout/base/nscssframeconstructor.cpp, line 7855]
nsCSSFrameConstructor::ContentAppended  [mozilla/layout/base/nscssframeconstructor.cpp, line 8589]

I can't reproduce the crash on branch.
It's crashing in layout, so I'm reassigning this to layout.

I get a regression range between 2006-09-21 and 2006-09-22:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-21+03&maxdate=2006-09-22+07&cvsroot=%2Fcvsroot
Regression from bug 312963 or bug 350137?  

Comment 4

[chris hofmann]

roc, can you have a look?

Comment 5

[Samuel Sidler (old account; do not CC)]

roc, have you had a chance to look at this yet?

Comment 6

[Jesse Ruderman]

WFM using Mac trunk debug and a local copy of the repro script.

Comment 7

[Jesse Ruderman]

Oops, I don't think I was testing properly.  I was using a fresh profile, which allowed the script to resize the window, so I didn't see the third alert and thought it was done.

I got way too many alerts, so I uncommented the 
  x != "location" &&
line and I'm trying again.

Comment 8

[Jesse Ruderman]

I also replaced the XMLHttpRequest logging with calls to dump() so I could see what was going on cheaply :)

After running for about 10 minutes, it allocated nearly 4GB of memory, failed three times to allocate 16MB chunks, and crashed.  This leaves me wondering (1) why it ran out of memory and (2) why it crashed dereferencing the bogus address 0x5fc3e000 rather than with a null deref.  More soon.

Comment 9

[Jesse Ruderman]

Attachment: tail of console output + crash log

Crashed within nsTextFragment::AppendTo, suggesting that nsTextFragment doesn't handle OOM well.

Comment 10

[Jesse Ruderman]

Martijn or lsg-mtso, do you still see a non-OOM crash here?  I'm going to file a separate bug for the(?) OOM crash.

Comment 11

[Jesse Ruderman]

I filed bug 383426 with a testcase that triggers what looks like a buffer overflow in OOM situations.

Comment 12

[Damon Sicore (:damons)]

Poke.   

Roc, any news on this?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jesse has a patch in bug 383426 for the security-sensitive part of this bug. I don't think any other work has to be done here.

Comment 14

[Jesse Ruderman]

I'd mark this as WFM if I had tested on the same platform as the reporter.

Comment 15

[Damon Sicore (:damons)]

So, is this WFM?

Comment 16

[Jesse Ruderman]

This needs to be retested on Windows XP, preferably by the reporter (lsg-mtso), before it can be marked as WFM.

Comment 17

[Daniel Veditz [:dveditz]]

I crash immediately on the testcase in WinXP on the trunk. Couldn't figure out which breakpad report was mine, but in a trunk build I get the assertion

WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file c:/dev/fftrunk/mozilla/content/xbl/src/nsXBLService.cpp, line 121
###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file c:/dev/fftrunk/mozilla/layout/base/../../content/base/src\nsTextFragment.h, line 184

it looks like a null deref (plus offset mIndex which might be controllable by the content):

 	nsTextFragment::CharAt() Line 185	C++
 	nsTextFrame::HasTerminalNewline() Line 5841	C++
 	ShouldPutNextSiblingOnNewLine() Line 4657	C++
 	nsBlockFrame::AddFrames() Line 4743	C++
>	nsBlockFrame::AppendFrames() Line 4592	C++
 	nsFrameManager::AppendFrames() Line 155	C++
 	nsCSSFrameConstructor::AppendFrames() Line 7840	C++
 	nsCSSFrameConstructor::ContentAppended() Line 8582	C++
 	PresShell::ContentAppended() Line 4431	C++
 	nsBindingManager::ContentAppended() Line 1246	C++
 	nsNodeUtils::ContentAppended() Line 110	C++
 	nsGenericElement::doInsertChildAt() Line 2627	C++
 	nsGenericElement::InsertChildAt() Line 2560	C++
 	nsGenericDOMDataNode::SplitText() Line 895	C++
 	nsTextNode::SplitText() Line 72	C++
 	NS_InvokeByIndex_P() Line 102	C++
 	XPCWrappedNative::CallMethod() Line 2240	C++
 	XPC_WN_CallMethod() Line 1467	C++
 	js_Invoke() Line 1306	C
 	js_Interpret() Line 3992	C
 	js_Execute() Line 1567	C
 	JS_EvaluateUCScriptForPrincipals() Line 4792	C
 	nsJSContext::EvaluateString() Line 1381	C++
 	nsScriptLoader::EvaluateScript() Line 607	C++
 	nsScriptLoader::ProcessRequest() Line 521	C++
 	nsScriptLoader::ProcessScriptElement() Line 486	C++
 	nsScriptElement::MaybeProcessScript() Line 200	C++
 	nsHTMLScriptElement::MaybeProcessScript() Line 534	C++
 	nsHTMLScriptElement::DoneAddingChildren() Line 480	C++
 	HTMLContentSink::ProcessSCRIPTEndTag() Line 3142	C++
 	SinkContext::CloseContainer() Line 1008	C++
 	HTMLContentSink::CloseContainer() Line 2405	C++
 	CNavDTD::CloseContainer() Line 2678	C++
 	CNavDTD::HandleEndToken() Line 1588	C++
 	CNavDTD::HandleToken() Line 703	C++
 	CNavDTD::BuildModel() Line 331	C++
 	nsParser::BuildModel() Line 1717	C++
 	nsParser::ResumeParse() Line 1594	C++
 	nsParser::OnDataAvailable() Line 2231	C++
 	nsDocumentOpenInfo::OnDataAvailable() Line 360	C++
 	nsStreamListenerTee::OnDataAvailable() Line 97	C++
 	nsHttpChannel::OnDataAvailable() Line 4388	C++
 	nsInputStreamPump::OnStateTransfer() Line 502	C++
 	nsInputStreamPump::OnInputStreamReady() Line 392	C++
 	nsInputStreamReadyEvent::Run() Line 111	C++
 	nsThread::ProcessNextEvent() Line 482	C++
 	NS_ProcessNextEvent_P() Line 227	C++
 	nsBaseAppShell::Run() Line 154	C++
 	nsAppStartup::Run() Line 171	C++
 	XRE_main() Line 2810	C++
 	main() Line 69	C++
 	mainCRTStartup() Line 398	C
 	7c816fd7	

Still a problem, but not the same stack as originally reported.

Comment 18

[Jesse Ruderman]

That assertion also shows up in bug 385426, but I don't think the testcase in this bug (which does exhaustive testing of DOM functions up to a certain depth) triggers it for the same reason.

If you modify the testcase to output exactly what it's doing right before it makes each call, and then paste the sequence back in, you should be able to make a reduced testcase pretty easily.

Comment 19

[Jesse Ruderman]

> Still a problem, but not the same stack as originally reported.

Your stack looks pretty similar to Martijn's in comment 3, actually.

Comment 20

[Damon Sicore (:damons)]

Is this something we can get QA to test/verify to get progress on this?

Comment 21

[Martijn Wargers (dead)]

It's still crashing for me, after clicking a few alerts away, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre

http://crash-stats.mozilla.com/report/index/ef782e26-6d3c-11dc-855f-001a4bd43ef6
0  	nsTextFragment::CharAt(int)  	 nsTextFragment.h:185
1 	HasTerminalNewline 	mozilla/layout/generic/nsTextFrameThebes.cpp:1054
2 	nsTextFrame::HasTerminalNewline() 	mozilla/layout/generic/nsTextFrameThebes.cpp:5824
3 	ShouldPutNextSiblingOnNewLine 	mozilla/layout/generic/nsBlockFrame.cpp:4756
4 	nsBlockFrame::AddFrames(nsIFrame*, nsIFrame*) 	mozilla/layout/generic/nsBlockFrame.cpp:4854
5 	nsBlockFrame::AppendFrames(nsIAtom*, nsIFrame*) 	mozilla/layout/generic/nsBlockFrame.cpp:4691
6 	nsCSSFrameConstructor::AppendFrames(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&, nsIFrame*) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:8039
7 	nsCSSFrameConstructor::ContentAppended(nsIContent*, int) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:8714
etc..

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On Mac, latest trunk, I don't crash. It's kinda hard to see what's going on because the window becomes super-wide, but alerts keep popping up (after a while I can't see them, but I can dismiss them with the keyboard) and eventually the page stops loading or doing anything. Then I can quit the browser. There are assertions about bad widths and stuff, and the window looks garbled, but nothing dangerous-looking.