The default bug view has changed. See this FAQ.
Bug 1293985 (CVE-2016-9905)

Crash in FlushLayoutRecursive

RESOLVED FIXED in Firefox 49

Status

()

Core
Layout
--
critical
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: philipp, Assigned: mats)

Tracking

(4 keywords)

49 Branch
mozilla51
x86
All
crash, csectype-uaf, regression, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 fixed, firefox50 fixed, firefox51 fixed, firefox-esr4550+ fixed)

Details

(Whiteboard: [adv-esr45.6+], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
This bug was filed from the Socorro interface and is 
report bp-4329e93a-6811-4fc5-ab7b-1ca962160810.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 		@0x5 	
1 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
2 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
3 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
4 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
5 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
6 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
7 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
8 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
9 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
10 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
11 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
12 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
13 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
14 	xul.dll 	PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*, bool) 	layout/base/nsPresShell.cpp:3685
15 	xul.dll 	PresShell::ProcessSynthMouseMoveEvent(bool) 	layout/base/nsPresShell.cpp:5627
16 	xul.dll 	nsCOMPtr_base::~nsCOMPtr_base() 	obj-firefox/dist/include/nsCOMPtr.h:295
17 	xul.dll 	PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) 	layout/base/nsPresShell.h:667
18 	xul.dll 	mozilla::IMEStateManager::SetInputContext(nsIWidget*, mozilla::widget::InputContext const&, mozilla::widget::InputContextAction const&) 	dom/events/IMEStateManager.cpp:1085
19 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:508

this crash signature is regressing since firefox 49 and seems to occur in the code of the changes from bug 881832.
it's currently the #35 crasher on 449.0b1 making up 0.27% of all crashes.
(Reporter)

Updated

8 months ago
status-firefox48: --- → unaffected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
Matt or Tim, can you take a look at this new crash in beta? Thanks!
Flags: needinfo?(tnikkel)
Flags: needinfo?(matt.woodrow)
I took a look at bp-9fc65d48-5d23-4c6b-a890-a24b12160812, and it looks like FlushLayoutRecursive is being called with aDocument == 0xE5E5E5E5E5E5E5E5.  That one was amd64, where the "Crash Address" field tends to be less reliable.  On x86 crashes, most crashes appear to have a crash address of 0xe5e5e5e5.

That's apparently how jemalloc marks freed memory.
(Assignee)

Comment 3

7 months ago
> aDocument == 0xE5E5E5E5E5E5E5E5

That implies a deallocated shell, which is a bit strange since the only caller
of DispatchSynthMouseMove holds a strong ref on that shell on the stack:
https://hg.mozilla.org/mozilla-unified/annotate/6cf0089510fa/layout/base/nsPresShell.cpp#l5858
and DispatchSynthMouseMove calls FlushLayoutRecursive with mDocument
(line 3718) which is a nsCOMPtr member on that shell.
https://hg.mozilla.org/mozilla-unified/annotate/6cf0089510fa/layout/base/nsPresShell.cpp#l3687

It seems possible that aDocument might not be alive after line 3691 though,
in case mDocument changed as a result of line 3691, but then it seems like
it should indicate line 3692 as the crash site.
(Assignee)

Comment 4

7 months ago
Created attachment 8781751 [details] [diff] [review]
fix?

It seems like we should fix this anyway.
Let's land it and see if the crashes go away.
Attachment #8781751 - Flags: review?(tnikkel)
Comment on attachment 8781751 [details] [diff] [review]
fix?

If the list of subdocuments that EnumerateSubDocuments is iterating over changes while iterating over it (because we flush in a callback that causes it to change) then we'd also have a problem, no?
Attachment #8781751 - Flags: review?(tnikkel) → review+
I had a look at the minidump, it's aDocument itself that is 0xe5e5e5e5, not the contents of the document.

That would suggest that the SubDocMapEntry (referenced in EnumerateSubDocuments) that has been deleted and poisoned.
(Assignee)

Comment 7

7 months ago
Yeah, that seems more likely.  I assumed we use something there that's resilient to
entries being added/removed but we're not - we're using PLDHashTable::Iterator.
(Assignee)

Comment 8

7 months ago
Created attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.
Assignee: nobody → mats
Flags: needinfo?(tnikkel)
Flags: needinfo?(matt.woodrow)
Attachment #8782077 - Flags: review?(tnikkel)
(Assignee)

Comment 9

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73160480c6d1&selectedJob=25880129
Attachment #8782077 - Flags: review?(tnikkel) → review+

Comment 10

7 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6f2742f7b
part 1 - Hold a strong ref on aDocument.  r=tn
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e93a5912b99
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.  r=tn

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0bd6f2742f7b
https://hg.mozilla.org/mozilla-central/rev/0e93a5912b99
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 12

7 months ago
Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Approval Request Comment
[Feature/regressing bug #]: bug 881832
[User impact if declined]: crash, see comment 2
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk
[String/UUID change made/needed]: none
Attachment #8782077 - Flags: approval-mozilla-beta?
Attachment #8782077 - Flags: approval-mozilla-aurora?
Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Fix for a new crash in beta, please uplift. This should make it into beta 6 or 7.
Attachment #8782077 - Flags: approval-mozilla-beta?
Attachment #8782077 - Flags: approval-mozilla-beta+
Attachment #8782077 - Flags: approval-mozilla-aurora?
Attachment #8782077 - Flags: approval-mozilla-aurora+

Comment 14

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba8075cd3fb9
https://hg.mozilla.org/releases/mozilla-aurora/rev/44330fe94f7b
status-firefox50: affected → fixed

Comment 15

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7f2fa6e2e0bb
https://hg.mozilla.org/releases/mozilla-beta/rev/e454d5a9aa40
status-firefox49: affected → fixed
Blocks: 928187
Keywords: csectype-uaf, sec-high
tracking-firefox-esr45: --- → 50+
Mats, turns out we need this for 45.5.0esr. Can you request esr45 uplift?  Does this need any sort of rebasing for esr? Thanks.
Flags: needinfo?(mats)
Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Actually, let's just go ahead and try landing this on esr45. Dan and I discussed it and we have some time to respond if there are issues.
Attachment #8782077 - Flags: approval-mozilla-esr45+
(Assignee)

Comment 18

5 months ago
I think both patches should apply as is, let me know there are problems.
Flags: needinfo?(mats)
status-firefox-esr45: --- → affected
has problem to apply to esr45

grafting 363087:7f2fa6e2e0bb "Bug 1293985 part 1 - Hold a strong ref on aDocument.  r=tn a=lizzard"
merging layout/base/nsPresShell.cpp
warning: conflicts while merging layout/base/nsPresShell.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue'
Flags: needinfo?(mats)
(Assignee)

Comment 20

4 months ago
I can't produce patches for esr45 because bug 881832 etc needs to land on esr45 first.
I suspect that this will apply without conflicts once they have.
Flags: needinfo?(mats)

Comment 21

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr45/rev/aebd3687e05e
https://hg.mozilla.org/releases/mozilla-esr45/rev/63d8e5cd27cb
status-firefox-esr45: affected → fixed
Whiteboard: [adv-esr45.6+]
Alias: CVE-2016-9905
You need to log in before you can comment on or make changes to this bug.