Closed Bug 667512 Opened 10 years ago Closed 10 years ago

Crash [@ nsIFrame::PeekOffset] with removing br and table cell and pressing right arrow key

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla8
Tracking Status
firefox5 - affected
firefox6 + fixed
firefox7 + fixed
firefox8 + fixed
status2.0 --- wontfix
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: martijn.martijn, Assigned: ehsan)

References

Details

(5 keywords, Whiteboard: [sg:critical?][qa!])

Crash Data

Attachments

(2 files)

Attached file testcase
See testcase, which crashes current trunk build after pressing the right arrow key.

https://crash-stats.mozilla.com/report/index/9bc6ca38-beec-412b-bd06-161462110627
0 		@0x6c0070 	
1 	xul.dll 	nsIFrame::PeekOffset 	layout/generic/nsFrame.cpp:5598
2 	xul.dll 	nsFrameSelection::MoveCaret 	layout/generic/nsSelection.cpp:1240
3 	xul.dll 	nsFrameSelection::MoveCaret 	layout/generic/nsSelection.cpp:1105
4 	xul.dll 	nsFrameSelection::CharacterMove 	layout/generic/nsSelection.cpp:2191
5 	xul.dll 	PresShell::CharacterMove 	layout/base/nsPresShell.cpp:3150
6 	xul.dll 	nsSelectionMoveCommands::DoCommand 	editor/libeditor/base/nsEditorCommands.cpp:718
7 	xul.dll 	nsControllerCommandTable::DoCommand 	embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191
8 	xul.dll 	nsBaseCommandController::DoCommand 	embedding/components/commandhandler/src/nsBaseCommandController.cpp:169
9 	xul.dll 	nsXBLPrototypeHandler::DispatchXBLCommand 	content/xbl/src/nsXBLPrototypeHandler.cpp:499
10 	xul.dll 	nsXBLPrototypeHandler::ExecuteHandler 	
11 	xul.dll 	nsXBLPrototypeHandler::ModifiersMatchMask 	content/xbl/src/nsXBLPrototypeHandler.cpp:1027
12 	xul.dll 	nsCOMPtr_base::assign_from_helper 	obj-firefox/xpcom/build/nsCOMPtr.cpp:152
13 	xul.dll 	nsCOMPtr<nsIDOMElement>::nsCOMPtr<nsIDOMElement> 	obj-firefox/dist/include/nsCOMPtr.h:644
14 	xul.dll 	nsXBLWindowKeyHandler::GetElement 	content/xbl/src/nsXBLWindowKeyHandler.cpp:589
15 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	obj-firefox/xpcom/build/nsCOMPtr.cpp:88
16 	xul.dll 	nsRefPtr<nsIDOMEventListener>::~nsRefPtr<nsIDOMEventListener> 	obj-firefox/dist/include/nsAutoPtr.h:969
17 	xul.dll 	nsHashtable::Get 	xpcom/ds/nsHashtable.cpp:246
18 	xul.dll 	nsXBLDocumentInfo::GetPrototypeBinding 	content/xbl/src/nsXBLDocumentInfo.cpp:545
19 	xul.dll 	nsXBLDocumentInfo::GetPrototypeBinding 	content/xbl/src/nsXBLDocumentInfo.cpp:545
20 	xul.dll 	nsXBLSpecialDocInfo::GetHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:153
21 	xul.dll 	nsXBLSpecialDocInfo::GetAllHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:173
Hg blame isn't useful at nsFrame.cpp:5598 (as too often the case with hg blame, unfortunately.

I get a regression range between 20101104 and 20110105. I don't have any builds inbetween, atm.
I moved the crash stack to the crash signature field - [@ nsIFrame::PeekOffset(nsPeekOffsetStruct*) ] is the only thing we want in that field.
Crash Signature: 0 @0x6c0070 1 xul.dll nsIFrame::PeekOffset layout/generic/nsFrame.cpp:5598 2 xul.dll nsFrameSelection::MoveCaret layout/generic/nsSelection.cpp:1240 3 xul.dll nsFrameSelection::MoveCaret layout/generic/nsSelection.cpp:1105 4 xul.dll cont… → [@ nsIFrame::PeekOffset(nsPeekOffsetStruct*) ]
WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.18) Gecko/20110614 Firefox/3.6.18

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:5.0.1) Gecko/20100101 Firefox/5.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110723 Firefox/7.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110723 Firefox/8.0a1

Last good nightly: 2010-11-14
First bad nightly: 2010-11-15

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=674f2ed15cea&tochange=edf41ff32f08

Linux crashes:
bp-90b7715d-9f9b-4710-9953-6634f2110723
bp-3242ba3f-b275-4e8e-b853-267d92110723
bp-76ae7ef4-65cd-49be-965f-262132110723
bp-4f371a10-ff90-4fe3-b45e-272562110723
bp-2a1a42d2-3f75-4eee-a51a-c00e02110723
bp-b517a0c1-6d3b-4230-bca5-e63792110723
bp-321bee19-d34b-40fc-b91d-a7fd02110723
bp-b46e14a6-a14b-498e-811e-2199e2110723
bp-b61ad8f2-14c2-4c00-ae66-1f7902110723
bp-22806048-4d0e-4994-9938-2566e2110723
bp-431e64d3-f6e8-4cba-a7f7-723c02110723
Crash Signature: [@ nsIFrame::PeekOffset(nsPeekOffsetStruct*) ] → [@ nsIFrame::PeekOffset(nsPeekOffsetStruct*) ] [@ nsFrameIterator::GetFirstChild ] [@ nsPlaceholderFrame::GetRealFrameFor ] [@ libxul.so@0x746c7e ] [@ libxul.so@0x749682 ] [@ libxul.so@0x738cae ] [@ libxul.so@0x73853e ]
OS: Windows 7 → All
Hardware: x86 → All
Please ignore bp-90b7715d-9f9b-4710-9953-6634f2110723 (the first Report ID) in comment 3. It's probably unrelated to this bug.
Local track down:

The first bad revision is:
changeset:   57481:54d00e3411ab
user:        Ehsan Akhgari <ehsan@mozilla.com>
date:        Thu Nov 11 16:40:52 2010 -0500
summary:     Bug 611182 - Part 3: Handle dynamic changes to the editable documents and create/remove the bogus node if needed; r=bzbarsky a=blocking-beta8+
Blocks: 611182
Hmm, this patch has been backed out, so I don't know how it could cause a crash in current nightlies.  I'll take a look though.
Attached patch Patch (v1)Splinter Review
The patch is pretty simple, we should just check to see if we have any visual frames.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #548317 - Flags: review?(smontagu)
Oh, and this is most certainly not a regression from bug 611182!
No longer blocks: 611182
Component: Selection → Layout
QA Contact: selection → layout
Group: core-security
Whiteboard: [sg:critical?]
This affects Firefox 4, 5, 6, 7 and 8 (trunk).

We're returning the contents of stack memory as an nsIFrame*.  Which means that a carefully engineered web page can probably trigger this in a way that would be exploitable.
generally assume EXCEPTION_ACCESS_VIOLATION_EXEC is exploitable unless you're 100% sure both 1) an attacker can't influence the address AND 2) there's no way for an attacker to put data at the address.

The fix is deceptively simple. Why did this suddenly appear to break on Nov 14 2010? The only change that seems at all relevant to the testcase is bug 595758. That one is guarding against a null-deref and if that path were coming up you'd be crashing due to that bug, not this.
Now I've redone the track down, and once again I hit the same changeset as in comment 5, so it must have been some kind of local regression. However, I also noticed that I had to press right arrow twice to trigger the crash near the regression.

Anyway, how do I find out if and when a patch has been backed out? (using the hg command or http://hg.mozilla.org/ ?) It could be interesting to see if we can find a good Nightly after that.
(In reply to comment #10)
> generally assume EXCEPTION_ACCESS_VIOLATION_EXEC is exploitable unless
> you're 100% sure both 1) an attacker can't influence the address AND 2)
> there's no way for an attacker to put data at the address.
> 
> The fix is deceptively simple. Why did this suddenly appear to break on Nov
> 14 2010? The only change that seems at all relevant to the testcase is bug
> 595758. That one is guarding against a null-deref and if that path were
> coming up you'd be crashing due to that bug, not this.

Smontagu could say for sure, I don't know this code very well.  But it's quite possible that the bug has been there for some time, we just didn't have a testcase for it.

(In reply to comment #11)
> Anyway, how do I find out if and when a patch has been backed out? (using
> the hg command or http://hg.mozilla.org/ ?) It could be interesting to see
> if we can find a good Nightly after that.

Your best bet is to look at the bug to see if it has been reopened.  hg doesn't give you that information easily...
Hmm, this might actually be a regression from bug 610638.  Its patch falls into the regression range, and it touches the guilty functions here: <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.
Blocks: 610638
(In reply to comment #13)
> Hmm, this might actually be a regression from bug 610638.  Its patch falls
> into the regression range, and it touches the guilty functions here:
> <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.

Do you mean that? That patch just makes some prefs localizable.
Comment on attachment 548317 [details] [diff] [review]
Patch (v1)

Review of attachment 548317 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #548317 - Flags: review?(smontagu) → review+
(In reply to comment #14)
> (In reply to comment #13)
> > Hmm, this might actually be a regression from bug 610638.  Its patch falls
> > into the regression range, and it touches the guilty functions here:
> > <http://hg.mozilla.org/mozilla-central/rev/7034c271ebd2>.
> 
> Do you mean that? That patch just makes some prefs localizable.

No, you're right, I think I confused that with another changeset.  Honestly, I'm not sure what has regressed this any more...
No longer blocks: 610638
Landed without the testcase on trunk:

http://hg.mozilla.org/mozilla-central/rev/029dd90bd20e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attachment #548317 - Flags: approval-mozilla-beta?
Attachment #548317 - Flags: approval-mozilla-aurora?
Comment on attachment 548317 [details] [diff] [review]
Patch (v1)

Approved for mozilla-beta and mozilla-aurora. Please land asap.
Attachment #548317 - Flags: approval-mozilla-beta?
Attachment #548317 - Flags: approval-mozilla-beta+
Attachment #548317 - Flags: approval-mozilla-aurora?
Attachment #548317 - Flags: approval-mozilla-aurora+
qa+ for verification on Firefox 7
Whiteboard: [sg:critical?] → [sg:critical?][qa+]
Crash does not reproduce in Firefox 6.0.2 or 7 on OS X or Windows 7.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011

Verified fixed using the attached testcase.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical?][qa+] → [sg:critical?][qa!]
Group: core-security
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f8058b2b21
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.