309777 - double-free of CNavDTD leads to crash with some badly malformed HTML

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Download the stuff in bug 309467 and follow the steps there to reproduce it.
2)  Use the file I'm about to attach as the data file.

ACTUAL RESULTS:

Double-delete, with the following stack (gotten via MALLOC_CHECK_=2):

#0  0x00a127a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0xb7c5f7d5 in raise () from /lib/tls/libc.so.6
#2  0xb7c61149 in abort () from /lib/tls/libc.so.6
#3  0xb7c9e4b5 in free_check () from /lib/tls/libc.so.6
#4  0xb7c99de5 in free () from /lib/tls/libc.so.6
#5  0x00ac2cd3 in operator delete () from /usr/lib/libstdc++.so.5
#6  0xb5c34ca2 in ~CNavDTD (this=0x89a3ea0)
    at ../../../../mozilla/parser/htmlparser/src/CNavDTD.cpp:250
#7  0xb5c3454d in CNavDTD::Release (this=0x89a3ea0)
    at ../../../../mozilla/parser/htmlparser/src/CNavDTD.cpp:162
#8  0xb5c4db02 in CParserContext::~CParserContext ()
    at ../../../../mozilla/parser/htmlparser/src/nsScanner.cpp:54
#9  0xb5c44306 in ~nsParser (this=0x89989c8)
    at ../../../../mozilla/parser/htmlparser/src/nsParser.cpp:413
#10 0xb5c44623 in nsParser::Release (this=0x89989c8)
    at ../../../../mozilla/parser/htmlparser/src/nsParser.cpp:423
#11 0xb5ac63de in nsCOMPtr<nsIStreamListener>::~nsCOMPtr ()
    at ../../dist/include/necko/nsIRequestObserver.h:34
#12 0xb5ac3a9b in nsDocumentOpenInfo::OnStopRequest (this=0x8951200,
request=0x8950930, 
    aCtxt=0x0, aStatus=0) at ../../../mozilla/uriloader/base/nsURILoader.cpp:389
#13 0xb7759134 in nsFileChannel::OnStopRequest (this=0x8950900, req=0x8951488,
ctx=0x0, 
    status=0) at
../../../../../mozilla/netwerk/protocol/file/src/nsFileChannel.cpp:548
#14 0xb76c9ede in nsInputStreamPump::OnStateStop (this=0x8951488)
    at ../../../../mozilla/netwerk/base/src/nsInputStreamPump.cpp:506
#15 0xb76c97f5 in nsInputStreamPump::OnInputStreamReady (this=0x8951488, 
    stream=0x8951244) at
../../../../mozilla/netwerk/base/src/nsInputStreamPump.cpp:343
#16 0xb7f72695 in nsInputStreamReadyEvent::EventHandler (plevent=0x89515d4)
    at ../../../mozilla/xpcom/io/nsStreamUtils.cpp:119
#17 0xb7f96a30 in PL_HandleEvent (self=0x89515d4)
    at ../../../mozilla/xpcom/threads/plevent.c:688
#18 0xb7f968d1 in PL_ProcessPendingEvents (self=0x81fef10)
    at ../../../mozilla/xpcom/threads/plevent.c:623
#19 0xb7f99dea in nsEventQueueImpl::ProcessPendingEvents (this=0x81feec8)
    at ../../../mozilla/xpcom/threads/nsEventQueue.cpp:419

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: File to use

Let me know if you have trouble reproducing and I'll try to reduce the file to
bare minimum...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase, not reduced

Just loading that HTML gives me a double-delete

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: A little smaller, but not much. :(

Reducing this is kinda slow going.  :(

Comment 4

[Jesse Ruderman]

Loading the third testcase doesn't cause any odd console output (e.g. malloc errors) for me in a Mac trunk debug build.  So WFM if I'm understanding correctly.

bz, can you still reproduce (on Linux)?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I can definitely reproduce with a March 3 debug build.  Let me update and see whether that helps.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, still a problem in a build from this morning.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I can reproduce this on both Linux and Windows. I'm not convinced this is a double free, though.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed minimal patch

All other assignments to mXTags are protected by if statements. With this patch eMaxTags is no longer strictly necessary, but in the interests of keeping this patch small I decided to leave it in.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 214613 [details] [diff] [review]
Proposed minimal patch

r+sr=jst

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. Boris, can you confirm this really was the problem you were seeing?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, this fixes all three testcases for me.  Thanks!