The default bug view has changed. See this FAQ.

Firefox 1.5.0.2 topcrash [@ nsExpatDriver::ParseBuffer]

RESOLVED FIXED

Status

()

Core
XML
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dbaron, Assigned: peterv)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, topcrash, verified1.8.0.9, verified1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.1 +
blocking1.8.0.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low][need testcase], crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Keywords: crash, topcrash
(Reporter)

Comment 1

11 years ago
This may be related to bug 314660, although I don't see how.

Comment 2

11 years ago
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

11 years ago
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
(Assignee)

Comment 4

11 years ago
I hit an assertion in nsExpatDriver on a branch build that might be pointing to this problem. I didn't crash though.
Assignee: xml → peterv
Blocks: 314660
(Assignee)

Comment 5

11 years ago
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).
Group: security
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
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?
Attachment #233511 - Flags: superreview?(bzbarsky)
Attachment #233511 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #233511 - Flags: review? → review?(bzbarsky)
Comment on attachment 233511 [details] [diff] [review]
v1

Peter, this is basically what you showed me back in late June, right?
Attachment #233511 - Flags: superreview?(bzbarsky)
Attachment #233511 - Flags: superreview+
Attachment #233511 - Flags: review?(bzbarsky)
Attachment #233511 - Flags: review+
(Assignee)

Comment 8

11 years ago
(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.
(Assignee)

Updated

11 years ago
Blocks: 357852
Flags: blocking1.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9+
Whiteboard: [sg:critical]
(Assignee)

Comment 9

11 years ago
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.
Attachment #233511 - Attachment is obsolete: true
Attachment #244349 - Flags: superreview?(bzbarsky)
Attachment #244349 - Flags: review?(bzbarsky)
Comment on attachment 244349 [details] [diff] [review]
v1.1

Looks good.
Attachment #244349 - Flags: superreview?(bzbarsky)
Attachment #244349 - Flags: superreview+
Attachment #244349 - Flags: review?(bzbarsky)
Attachment #244349 - Flags: review+
(Assignee)

Comment 11

11 years ago
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.
Attachment #244349 - Flags: approval1.8.1.1?
Attachment #244349 - Flags: approval1.8.0.9?
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
Attachment #244349 - Flags: approval1.8.1.1?
Attachment #244349 - Flags: approval1.8.1.1+
Attachment #244349 - Flags: approval1.8.0.9?
Attachment #244349 - Flags: approval1.8.0.9+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.9, fixed1.8.1.1
Resolution: --- → FIXED

Comment 13

11 years ago
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

Updated

11 years ago
Flags: blocking1.9?
Flags: blocking1.9+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+

Comment 14

11 years ago
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.
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Whiteboard: [sg:critical] → [sg:critical][need testcase]
Depends on: 360936
(Assignee)

Updated

11 years ago
Depends on: 361180
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.
Whiteboard: [sg:critical][need testcase] → [sg:critical?][need testcase]
(Assignee)

Comment 16

10 years ago
Yeah, I think sg:low makes more sense.
Whiteboard: [sg:critical?][need testcase] → [sg:low][need testcase]
Group: security
Crash Signature: [@ nsExpatDriver::ParseBuffer]
You need to log in before you can comment on or make changes to this bug.