Closed
Bug 182067
Opened 22 years ago
Closed 22 years ago
data:[parameters],<NULL data segement> crashes
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.3beta
People
(Reporter: Biesinger, Assigned: nisheeth_mozilla)
References
()
Details
(Keywords: crash, testcase, topembed+)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
harishd
:
review+
jst
:
superreview+
asa
:
approval1.3b+
|
Details | Diff | Splinter Review |
Stack Signature nsReadingIterator::normalize_forward c00f2499 Email Address cbiesinger@web.de Product ID MozillaTrunk Build ID 2002112008 Trigger Time 2002-11-26 11:47:46 Platform Win32 Operating System Windows 98 4.10 build 67766446 Module XPCOM.DLL URL visited data:text/xml, User Comments loaded url mentioned above Trigger Reason Access violation Source File Name ../../dist/include/string\nsStringIterator.h Trigger Line No. 380 Stack Trace nsReadingIterator::normalize_forward [../../dist/include/string\nsStringIterator.h, line 380] nsReadingIterator::advance [../../dist/include/string\nsStringIterator.h, line 180] copy_string [../../dist/include/string\nsAlgorithm.h, line 95] Distance [c:/builds/seamonkey/mozilla/string/src/nsReadableUtils.cpp, line 100] nsScanner::RewindToMark [c:/builds/seamonkey/mozilla/htmlparser/src/nsScanner.cpp, line 263] nsParser::Tokenize [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 2530] nsParser::ResumeParse [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 1752] nsParser::OnDataAvailable [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 2390] nsDocumentOpenInfo::OnDataAvailable [c:/builds/seamonkey/mozilla/uriloader/base/nsURILoader.cpp, line 246] nsDataChannel::OnDataAvailable [c:/builds/seamonkey/mozilla/netwerk/protocol/data/src/nsDataChannel.cpp, line 575] nsOnDataAvailableEvent0::HandleEvent [c:/builds/seamonkey/mozilla/netwerk/base/src/nsAsyncStreamListener.cpp, line 432] nsStreamListenerEvent0::HandlePLEvent [c:/builds/seamonkey/mozilla/netwerk/base/src/nsAsyncStreamListener.cpp, line 122] PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 645] PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 578] _md_TimerProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 931] KERNEL32.DLL + 0x2317 (0xbff72317) (that's TB14416065 if you care)
I'm using 11/26 TRUNK build and it doesn't crash. I get the following xml error: XML Parsing Error: no element found Location: data:text/xml, Line Number 1, Column 1: ^
Comment 3•22 years ago
|
||
You may have to reload it to get the crash, but it will happen...
Comment 4•22 years ago
|
||
I'm using 11/25 trunk and it does crash. Some details: (gdb) frame 0 #0 0x40ec5a68 in initScan (encodingTable=0x40ef6bc8, enc=0x86e8724, state=0, ptr=0xd00ff08 <Address 0xd00ff08 out of bounds>, end=0x0, nextTokPtr=0xbfffefb0) (note the value of "ptr") (gdb) frame 5 #5 0x40e816ab in nsExpatDriver::ParseBuffer (this=0x87c7228, aBuffer=0xd00ff08 <Address 0xd00ff08 out of bounds>, aLength=4076798200, aIsFinal=0) Note the value of aBuffer (gdb) frame 6 #6 0x40e81a68 in nsExpatDriver::ConsumeToken (this=0x87c7228, aScanner=@0x88352b8, aFlushTokens=@0xbffff094) (gdb) p aScanner.mCurrentPosition $29 = {mFragment = {mStart = 0x0, mEnd = 0x0, mFragmentIdentifier = 0x0}, mPosition = 0xd00ff08, mOwningString = 0x2e003408} (gdb) p aScanner.mCurrentPosition.mPosition $30 = (PRUnichar *) 0xd00ff08 (gdb) p *aScanner.mCurrentPosition.mPosition Cannot access memory at address 0xd00ff08 Oops. So the problem is that the scanner never initializes mCurrentPosition and mEndPosition (which looks similarly bogus) in any of its constructors. The whole thing is triggered by: (gdb) frame 9 #9 0x40e9a6f0 in nsParser::OnDataAvailable (this=0x87acb38, request=0x8817d50, aContext=0x0, pIStream=0x87ce690, sourceOffset=0, aLength=0) It looks like relying on the scanner to always get a decent Append() call to init those values is not a good idea. I suspect just adding some initializers to the constructors would do a world of good. That said, the crash will not happen if you just happen to have that uninitialized memory pointing to something that you can access. Reloading a few times on that url will generally crash. ;)
Severity: normal → critical
Comment 5•22 years ago
|
||
Oh, apparently data:text/html, data:text/css, data:text/plain, etc all hang (makes sense if the iterators are not initialized and we end up with them in reverse order so we can't break out of one of those while (start != end) loops till we overflow 32 bits.
Can we get this bug targeted? Since this is an easy crash to invoke, we should fix it asap.
Comment 7•22 years ago
|
||
note that this is a way of triggering access to random memory from a web page..... I don't think it's controllable, hence I don't think it's exploitable. But it's not something we should leave lying about.
Flags: blocking1.3b?
Tentatively targeting for 1.3beta.
Target Milestone: --- → mozilla1.3beta
Comment 10•22 years ago
|
||
(playing w/ Chimera while watching the news...) Chimera crashes on data:, as well as the example provided. (I don't know why Mozilla only crashes reliably on the URL example, because the two URL's have the same meaning).
Comment 11•22 years ago
|
||
Chimera is off the 1.0 branch. I bet 1.0 crashes on both too... (and the crash is not in the tokenizer).
I'd hate to ship 1.3beta with this.
Flags: blocking1.3b? → blocking1.3b+
Comment 13•22 years ago
|
||
discussed in edt. Plussing. harishd, please target appropriate milestone.
Comment 14•22 years ago
|
||
+testcase, cleaned up summary to be more accurate This bug needs to pass three sample URL's to be marked fixed. data:, data:text/plain, data:;base64, http://www.mozilla.org/quality/networking/testing/datatests.html Also, if the fixer could specify what was breaking (is it simply the data segment being NULL?) then there might be other specific data: URLs that should be tested as well.
Keywords: testcase
Summary: crash loading data:text/xml, → data:[parameters],<NULL data segement> crashes
Assignee | ||
Comment 16•22 years ago
|
||
Ok, this patch fixes the problem. I tested multiple reloads of all the different types of data urls mentioned on this bug.
Assignee | ||
Comment 17•22 years ago
|
||
Comment on attachment 113073 [details] [diff] [review] First attempt at fix The problem, as bzbarsky pointed out in an earlier comment, was that the three iterators in the scanner aren't initialized in two out of the three constructors. This patch initializes those iterators but I'm not too happy about adding a new nsString member to the scanner. Do you guys have any thoughts on how to do the initialization another way? The problem is that mSlidingBuffer is a null pointer at scanner construction time so I can't use its BeginReading method to initialize the constructors.
Attachment #113073 -
Flags: superreview?(heikki)
Attachment #113073 -
Flags: review?(harishd)
Assignee | ||
Comment 18•22 years ago
|
||
Adding jst to the cc list. Johnny, do you have any thoughts on my question in comment 17?
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Comment 19•22 years ago
|
||
Comment on attachment 113073 [details] [diff] [review] First attempt at fix >Index: nsScanner.cpp >=================================================================== >RCS file: /cvsroot/mozilla/htmlparser/src/nsScanner.cpp,v >retrieving revision 3.116 >diff -u -r3.116 nsScanner.cpp >--- nsScanner.cpp 3 Sep 2002 22:23:17 -0000 3.116 >+++ nsScanner.cpp 30 Jan 2003 04:53:33 -0000 >@@ -138,6 +138,9 @@ > MOZ_COUNT_CTOR(nsScanner); > > mSlidingBuffer = nsnull; >+ mInitIteratorString.BeginReading(mCurrentPosition); // Do this to initialize the iterator. How about NS_LITERAL_STRING("").BeginReading(mCurrentPosition). This should avoid the extra member ( mInitIteratorString ). no?
Comment 20•22 years ago
|
||
NS_LITERAL_STRING("").BeginReading(mCurrentPosition) would leave mCurrentPosition refering to data in a temporary string (which may or may not have a static buffer, depending on what NS_LITERAL_STRING() macro does on different platforms. If you could make mCurrentPosition refere to data to a static empty string, couldn't you? I bet there are some laying around already in the parser (and if bug 183373 was fixed, there'd be a generic mechanism for getting at one), so you could use those...
Comment 21•22 years ago
|
||
>mCurrentPosition refering to data in a temporary string (which may or may not
>have a static buffer, depending on what NS_LITERAL_STRING() macro does on
>different platforms.
Yep, ignore my stupid suggestion :-<
Assignee | ||
Comment 22•22 years ago
|
||
OK, I no longer add a new nsString member. I use mFilename.BeginReading() to init the iterators. mFilename is initialized right before I use it and is guaranteed not to change during the lifetime of the scanner (its only assigned once in the scanner's ctor).
Attachment #113073 -
Attachment is obsolete: true
Assignee | ||
Comment 23•22 years ago
|
||
Comment on attachment 113142 [details] [diff] [review] Patch v0.2 please see comment 22
Attachment #113142 -
Flags: superreview?(heikki)
Attachment #113142 -
Flags: review?(harishd)
Comment 24•22 years ago
|
||
Comment on attachment 113142 [details] [diff] [review] Patch v0.2 r=harishd
Attachment #113142 -
Flags: review?(harishd) → review+
Assignee | ||
Updated•22 years ago
|
Attachment #113142 -
Flags: superreview?(heikki) → superreview?(jst)
Comment 25•22 years ago
|
||
Comment on attachment 113142 [details] [diff] [review] Patch v0.2 sr=jst
Attachment #113142 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 26•22 years ago
|
||
Comment on attachment 113142 [details] [diff] [review] Patch v0.2 This patch fixes the crash/hang when a data: url with a null data segment is typed into the url bar. This bug is on the list of 1.3 beta blockers. The problem was that we weren't initializing the reading iterator members of the scanner. Now we do. I've tested the patch and run precheckin tests on Linux and Windows. The patch has been reviewed by harishd and super reviewed by jst. Please grant approval for checkin to the trunk.
Attachment #113142 -
Flags: approval1.3b?
Comment 27•22 years ago
|
||
Comment on attachment 113142 [details] [diff] [review] Patch v0.2 a=asa (on behalf of drivers) for checkin to 1.3beta.
Attachment #113142 -
Flags: approval1.3b? → approval1.3b+
Assignee | ||
Comment 28•22 years ago
|
||
Checked into trunk...
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
Attachment #113073 -
Flags: superreview?(heikki)
Attachment #113073 -
Flags: review?(harishd)
Comment 29•22 years ago
|
||
VERIFIED: Mozilla 1.4a, all plats. This is covered in my data: tests... http://www.mozilla.org/quality/networking/testing/datatests.html I think this working in 1.3f and 1.3b too...
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•