335047 - Firefox 1.5.0.2 topcrash [@ nsExpatDriver::ParseBuffer]

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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

Comment 2

[Axel Hecht]

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]

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

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

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

Attachment: v1

ML_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 [:bzbarsky]]

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

(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]]

Attachment: 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 [:bzbarsky]]

Comment on attachment 244349 [details] [diff] [review]
v1.1

Looks good.

Comment 11

[Peter Van der Beken [:peterv]]

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

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]

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

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

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

Yeah, I think sg:low makes more sense.