Closed Bug 335047 Opened 16 years ago Closed 15 years ago
.5 .0 .2 topcrash [@ ns Expat Driver::Parse Buffer]
Crashes at nsExpatDriver::ParseBuffer are one of the close-to-the-top crashes in Firefox 220.127.116.11. 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.
This may be related to bug 314660, although I don't see how.
Peter should have fixed this in bug 320375 for 18.104.22.168, I would think. At least that's the obvious candidate in the logic involved.
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
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
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).
Status: NEW → ASSIGNED
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?
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?
(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.
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 on attachment 244349 [details] [diff] [review] v1.1 Looks good.
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 on attachment 244349 [details] [diff] [review] v1.1 a=mconnor on behalf of drivers for 22.214.171.124 and 126.96.36.199 checkin risk seems iffy, but an exploitable topcrash makes it worth getting in ASAP IMO
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
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 188.8.131.52 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.
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]
Yeah, I think sg:low makes more sense.
Whiteboard: [sg:critical?][need testcase] → [sg:low][need testcase]
Crash Signature: [@ nsExpatDriver::ParseBuffer]
You need to log in before you can comment on or make changes to this bug.