Last Comment Bug 335047 - Firefox 1.5.0.2 topcrash [@ nsExpatDriver::ParseBuffer]
: Firefox 1.5.0.2 topcrash [@ nsExpatDriver::ParseBuffer]
Status: RESOLVED FIXED
[sg:low][need testcase]
: crash, topcrash, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Peter Van der Beken [:peterv]
: Ashish Bhatt
Mentors:
http://talkback-public.mozilla.org/se...
Depends on: 360936 361180
Blocks: 314660 357852
  Show dependency treegraph
 
Reported: 2006-04-21 22:59 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2006-12-22 10:59 PST (History)
10 users (show)
mconnor: blocking1.9+
mconnor: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.23 KB, patch)
2006-08-13 15:30 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
v1.1 (4.49 KB, patch)
2006-11-01 13:01 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
bzbarsky: superreview+
mconnor: approval1.8.0.9+
mconnor: approval1.8.1.1+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-04-21 22:59:29 PDT
Crashes at nsExpatDriver::ParseBuffer are one of the close-to-the-top crashes in Firefox 1.5.0.2.  Unfortunately, talkback doesn't provide very much useful information; see the above URL for most of what it does have.  Pretty much all the crashes are on Windows, so it is possible they're showing up under another signature on other platforms.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-04-21 23:04:50 PDT
This may be related to bug 314660, although I don't see how.
Comment 2 Axel Hecht 2006-04-22 02:03:55 PDT
Peter should have fixed this in bug 320375 for 1.8.0.1, I would think.
At least that's the obvious candidate in the logic involved.
Comment 3 Jesse Ruderman 2006-04-22 16:46:46 PDT
I skimmed the URLs and comments on Talkback.  They're all over the place, but these things each appeared at least twice:

* Downloading files
* Updating or installing Roboform
* Logging into various web sites
* Out of memory
* http://www.orb.com
* http://www.runescape.com
* http://www.gazzetta.it - reading articles
Comment 4 Peter Van der Beken [:peterv] 2006-06-22 09:04:48 PDT
I hit an assertion in nsExpatDriver on a branch build that might be pointing to this problem. I didn't crash though.
Comment 5 Peter Van der Beken [:peterv] 2006-08-13 15:23:10 PDT
The assertion I'm hitting is the one at http://lxr.mozilla.org/mozilla1.8/source/parser/htmlparser/src/nsExpatDriver.cpp#994 which means XML_GetCurrentByteIndex is broken. That means we'll walk off the end of the buffer (similar as bug 320375), marking security sensitive because of that. I have a fix, unfortunately we won't be able to test this on trunk (the Expat code is completely different there).
Comment 6 Peter Van der Beken [:peterv] 2006-08-13 15:30:00 PDT
Created attachment 233511 [details] [diff] [review]
v1

XML_GetCurrentByteIndex returns |parseEndByteIndex - (parseEndPtr - eventPtr);|. I've just made sure that we always set eventPtr to after the processed content that made us block, set parseEndPtr to eventPtr (the difference between the two is only relevant inside a handler, we call it outside the handlers) and to update parseEndByteIndex correctly. In the doContent section for empty elements, we call the startElementHandler and if that blocks we don't call the endElementHandler. That's a bad situation, since resuming will either not call the endElementHandler or call the startElementHandler again. Fortunatly we only block when closing elements I think, right?
Comment 7 Boris Zbarsky [:bz] (TPAC) 2006-08-13 15:52:25 PDT
Comment on attachment 233511 [details] [diff] [review]
v1

Peter, this is basically what you showed me back in late June, right?
Comment 8 Peter Van der Beken [:peterv] 2006-08-14 11:08:37 PDT
(In reply to comment #7)
> Peter, this is basically what you showed me back in late June, right?

The patch is slightly different, but in spirit it's the same. I just made sure I understood what the various member variables represent and in that sense it is more correct.
Comment 9 Peter Van der Beken [:peterv] 2006-11-01 13:01:25 PST
Created attachment 244349 [details] [diff] [review]
v1.1

This makes sure to update the parser position always after creating PIs (we were missing that in two spots). That also fixes bug 357852.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2006-11-01 18:46:21 PST
Comment on attachment 244349 [details] [diff] [review]
v1.1

Looks good.
Comment 11 Peter Van der Beken [:peterv] 2006-11-02 04:04:38 PST
Comment on attachment 244349 [details] [diff] [review]
v1.1

This fixes a minor security issue (reading past the end of a buffer). It is a little risky, since we can't test this patch on the trunk. I also can't reproduce this bug anymore, but when I could the patch did fix it. It also fixes bug 357852.
Comment 12 Mike Connor [:mconnor] 2006-11-07 06:39:42 PST
Comment on attachment 244349 [details] [diff] [review]
v1.1

a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin

risk seems iffy, but an exploitable topcrash makes it worth getting in ASAP IMO
Comment 13 chris hofmann 2006-11-16 13:41:44 PST
looks like this might have caused a regression in parsing errors for comments.  cc'ing mgalli who getting a test case put together for bug 360936
Comment 14 Jay Patel [:jay] 2006-11-28 16:27:53 PST
v.fixed on 1.8.0 and 1.8.1 branches based on Talkback data.  No new crashes since the few that were reported with 1.5.0.8 and 2.0.  If this crash comes back, we can  reopen, but for now it looks like this crash is gone.

If anyone has a testcase or easy way to reproduce, please add a testcase.
Comment 15 Daniel Veditz [:dveditz] 2006-12-11 21:48:13 PST
At a quick glance this appears to be reading past the end of the buffer, not writing past it. Is it really sg:critical? The similar bug 320375 was marked sg:low.
Comment 16 Peter Van der Beken [:peterv] 2006-12-12 13:59:50 PST
Yeah, I think sg:low makes more sense.

Note You need to log in before you can comment on or make changes to this bug.