269095 - Firefox (more precisely the gecko engine) crashed by the scanit browser crash test [@ SinkContext::Begin]

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Yuan Qi]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

I tested firefox 1.0 on the following browser crash test from scanit:
http://webtest.scanit.be/bcheck/mangle.php. It is based on mangleme by Michal
Zalewski: http://www.securityfocus.com/archive/1/378632. Firefox handled the
broken pages for a while, then crashed. I attached the crash inducing html file.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Yuan Qi]

Attachment: The crash inducing page

Comment 2

[Yuan Qi]

It is reported that Microsoft Internet Explorer does not crash with this test!
Gecko developers should look into this test, which will help gecko to become
more crash proof and allow it to handle malformed and nonstandard html better.

Comment 3

[Robert Strong (they/them - no direct email)]

I get a crash with Firefox and it hangs Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.8a5) Gecko/20041109.

Reporter - will you provide a simplified testcase?
http://www.mozilla.org/newlayout/bugathon.html#testcase

Comment 4

[Paul van Schayck]

It even crashes X here.

With this build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5)
Gecko/20041109 Firefox/1.0

Comment 5

[Bernd]

SinkContext::Begin(nsHTMLTag 0xcdcdcdcd, nsIHTMLContent * 0xcdcdcdcd, unsigned
int 0xcdcdcdcd, int 0xffffffff) line 1106 + 3 bytes
HTMLContentSink::BeginContext(HTMLContentSink * const 0x034ae090, int
0x00000003) line 2527
CNavDTD::HandleSavedTokens(int 0x00000003) line 2129
CNavDTD::HandleToken(CNavDTD * const 0x03667068, CToken * 0x035a4d18, nsIParser
* 0x0334e0e0) line 894 + 17 bytes
CNavDTD::HandleSavedTokens(int 0x00000003) line 2159 + 23 bytes
CNavDTD::HandleToken(CNavDTD * const 0x03667068, CToken * 0x03560888, nsIParser
* 0x0334e0e0) line 894 + 17 bytes
CNavDTD::BuildModel(CNavDTD * const 0x03667068, nsIParser * 0x0334e0e0,
nsITokenizer * 0x0343c640, nsITokenObserver * 0x00000000, nsIContentSink *
0x034ae090) line 471 + 17 bytes
nsParser::BuildModel(nsParser * const 0x0334e0e0) line 2027 + 31 bytes
nsParser::ResumeParse(int 0x00000001, int 0x00000001, int 0x00000001) line 1894
+ 11 bytes
nsParser::OnStopRequest(nsParser * const 0x0334e0e4, nsIRequest * 0x0336e800,
nsISupports * 0x00000000, unsigned int 0x00000000) line 2608 + 21 bytes
nsDocumentOpenInfo::OnStopRequest(nsDocumentOpenInfo * const 0x033fc2a8,
nsIRequest * 0x0336e800, nsISupports * 0x00000000, unsigned int 0x00000000) line 360
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x033db390,
nsIRequest * 0x0336e800, nsISupports * 0x00000000, unsigned int 0x00000000) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x0336e808, nsIRequest *
0x032cddb8, nsISupports * 0x00000000, unsigned int 0x00000000) line 3755
nsInputStreamPump::OnStateStop() line 505
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x032cddbc,
nsIAsyncInputStream * 0x02ff1cc8) line 341 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x034b7674) line 119
PL_HandleEvent(PLEvent * 0x034b7674) line 692 + 9 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ed4ff8) line 627 + 8 bytes
_md_EventReceiverProc(HWND__ * 0x0006029a, unsigned int 0x0000c11a, unsigned int
0x00000000, long 0x00ed4ff8) line 1433 + 8 bytes
USER32! 77d18709()
USER32! 77d187eb()
USER32! 77d189a5()
USER32! 77d189e8()
nsAppShell::Run(nsAppShell * const 0x00f9ac58) line 135
nsAppStartup::Run(nsAppStartup * const 0x00f9a9d8) line 221
main1(int 0x00000003, char * * 0x002a4230, nsISupports * 0x00edaee8) line 1321 +
31 bytes
main(int 0x00000003, char * * 0x002a4230) line 1799 + 34 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

This is a parser crash

mStack[0].mType = aNodeType;
  mStack[0].mContent = aRoot;
  mStack[0].mFlags = APPENDED;
  mStack[0].mNumFlushed = aNumFlushed;
  mStack[0].mInsertionPoint = aInsertionPoint;
===>  NS_ADDREF(aRoot);
  mStackPos = 1;
  mTextLength = 0;
aRoot is 0xcdcdcdcd

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: reduced testcase

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Note that I don't always actually crash if the testcase isn't the first page I
load - instead, the browser hangs.  To crash, I have to run "mozilla.exe
https://bugzilla.mozilla.org/attachment.cgi?id=165566&action=view"

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

This is really complicated, and it's my first real experience with table stuff
in CNavDTD to boot. Basically, what's happening is that we're getting nested
HandleSavedToken() calls. Since we simply pass indexes from the DTD to the
content sink, when we end up in this sort of nested call, the indexes are off
(i.e., we've pushed stuff onto our stack that the content sink doesn't know
about) so when the sink tries to use the DTD's indexes in the nested call,
they're invalid and we crash.

I need to investigate more to see what we *should* be doing in this case. I
think we definately need to disallow going back into HandleSavedTokens(), but
I'm not sure yet.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: this fixes the crash

So, this patch makes us not crash, but I'm not sure I'm happy about it.
Basically, we refuse to deal with (new) misplaced content if we're already
dealing with misplaced content. Then, we need to make sure that we actually
handled all of the content (and don't drop any on the floor). For reference,
here is the content model created from the reduced testcase:

html@03A01AE8 refcount=3<
  head@03A01C20 refcount=2<
  >
  frameset@03A5D628 refcount=3<
    Text@03A5D690 refcount=1<\n>
  >
  Text@03A608E0 refcount=1<\n\n>
  map@03A61580 refcount=2<
    Text@03A61628 refcount=1<\n>
    form@03A61758 refcount=1<
      Text@03A61890 refcount=1<\n>
      table@03A66D68 refcount=1<
	Text@03A3B960 refcount=1<\n>
      >
      param@03A59F68 refcount=1<
      >
      rtable@03A5A128 _moz-userdefined="" refcount=1<
      >
      table@03A66960 refcount=1<
	Text@03A3B760 refcount=1<\n>
      >
    >
  >
>

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: another idea

I like this idea better. I don't think the other patch allows more tokens into
the misplaced content stack if we were already processing misplaced content.
This does that.

My one question is whether or not the assertion in the comment in
CNavDTD::DidBuildModel is valid (i.e., that mContentTopIndex really can contain
all of the misplaced contents).

Note: this patch passes the parser regression tests.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 165610 [details] [diff] [review]
another idea

jst, can you give this r= (and/or have any thoughts on this)?

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 165610 [details] [diff] [review]
another idea

This deals with the maybe impossible case of nesting into this code with
misplaced content that document.write()'s out more misplaced content etc? Looks
like it does, so r=jst

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 165610 [details] [diff] [review]
another idea

Looking for sr=.

Comment 14

[Yuan Qi]

Remember that gecko might not just crash on this particular page, this crash 
test shows that gecko has some general problems when dealing with malformed 
html. If might be worth it to request the source code of the .php page.

Comment 15

[Yuan Qi]

I mean the source code of the crash test page.

Comment 16

[Boris Zbarsky [:bzbarsky]]

There is no such thing as "general problems".  There are specific problems
caused by not anticipating certain types of input.  Rest assured  that Blake's
patch here fixes not just this testcase but also various variants of it...

Comment 17

[rbs]

Comment on attachment 165610 [details] [diff] [review]
another idea

> My one question is whether or not the assertion in the comment

The comment puzzled me as well. 

sr=rbs

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Checked in with some minor comment tweaks.
Checking in src/CNavDTD.cpp;
/cvsroot/mozilla/parser/htmlparser/src/CNavDTD.cpp,v  <--  CNavDTD.cpp
new revision: 3.440; previous revision: 3.439
done

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED for me with build 2004-11-18-05 on Windows XP.

Comment 20

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060213 Firefox/1.0.8, no crash or hang with reduced testcase.

Comment 21

[Tracy Walker [:tracy]]

verified with:
Windows:
Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Firefox/1.0.8
Macintosh:
Moz - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060214 Firefox/1.0.8
Fx - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060214 Firefox/1.0.8
Linux
Moz - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214
Fx -  Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214
Firefox/1.0.8

Comment 22

[Bob Clary [:bc] (inactive)]

parser/htmlparser/tests/crashtests/269095-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3