182067 - data:[parameters],<NULL data segement> crashes

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Stack Signature  nsReadingIterator::normalize_forward c00f2499
Email Address [e-mail]
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)

Comment 1

[Michael Hendy (Hendikins)]

Happening on Linux, Setting OS/All

Comment 2

[harishd]

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

[Michael Hendy (Hendikins)]

You may have to reload it to get the crash, but it will happen...

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[benc]

Can we get this bug targeted? Since this is an easy crash to invoke, we should
fix it asap.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Tentatively targeting for 1.3beta.

Comment 9

[benc]

(tb16026013z)

if you need more...

shouldn't this bug be ASSIGNED?

Comment 10

[benc]

(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

[Boris Zbarsky [:bzbarsky]]

Chimera is off the 1.0 branch.  I bet 1.0 crashes on both too... (and the crash
is not in the tokenizer).

Comment 12

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

I'd hate to ship 1.3beta with this.

Comment 13

[Michael Buckland]

discussed in edt.  Plussing.  harishd, please target appropriate milestone.

Comment 14

[benc]

+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.

Comment 15

[Nisheeth Ranjan]

taking...

Comment 16

[Nisheeth Ranjan]

Attachment: First attempt at fix

Ok, this patch fixes the problem.  I tested multiple reloads of all the
different types of data urls mentioned on this bug.

Comment 17

[Nisheeth Ranjan]

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.

Comment 18

[Nisheeth Ranjan]

Adding jst to the cc list.  Johnny, do you have any thoughts on my question in
comment 17?

Comment 19

[harishd]

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

[Johnny Stenback (:jst)]

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

[harishd]

>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 :-<

Comment 22

[Nisheeth Ranjan]

Attachment: Patch v0.2

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).

Comment 23

[Nisheeth Ranjan]

Comment on attachment 113142 [details] [diff] [review]
Patch v0.2

please see comment 22

Comment 24

[harishd]

Comment on attachment 113142 [details] [diff] [review]
Patch v0.2

r=harishd

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 113142 [details] [diff] [review]
Patch v0.2

sr=jst

Comment 26

[Nisheeth Ranjan]

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.

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 113142 [details] [diff] [review]
Patch v0.2

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 28

[Nisheeth Ranjan]

Checked into trunk...

Comment 29

[benc]

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...