Last Comment Bug 1293985 - (CVE-2016-9905) Crash in FlushLayoutRecursive
(CVE-2016-9905)
: Crash in FlushLayoutRecursive
Status: RESOLVED FIXED
[adv-esr45.6+]
: crash, csectype-uaf, regression, sec-high
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 49 Branch
: x86 All
-- critical (vote)
: mozilla51
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 881832 CVE-2016-5283
  Show dependency treegraph
 
Reported: 2016-08-10 04:18 PDT by [:philipp]
Modified: 2016-12-09 14:52 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed
fixed
50+
fixed


Attachments
fix? (825 bytes, patch)
2016-08-16 16:17 PDT, Mats Palmgren (:mats)
tnikkel: review+
Details | Diff | Splinter Review
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents. (1.17 KB, patch)
2016-08-17 11:00 PDT, Mats Palmgren (:mats)
tnikkel: review+
lhenry: approval‑mozilla‑aurora+
lhenry: approval‑mozilla‑beta+
lhenry: approval‑mozilla‑esr45+
Details | Diff | Splinter Review

Description User image [:philipp] 2016-08-10 04:18:23 PDT
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.
Comment 1 User image Liz Henry (:lizzard) (needinfo? me) 2016-08-12 10:17:44 PDT
Matt or Tim, can you take a look at this new crash in beta? Thanks!
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2016-08-12 15:22:59 PDT
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.
Comment 3 User image Mats Palmgren (:mats) 2016-08-16 15:55:30 PDT
> 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.
Comment 4 User image Mats Palmgren (:mats) 2016-08-16 16:17:08 PDT
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.
Comment 5 User image Timothy Nikkel (:tnikkel) 2016-08-16 22:00:37 PDT
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?
Comment 6 User image Matt Woodrow (:mattwoodrow) 2016-08-16 22:04:07 PDT
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.
Comment 7 User image Mats Palmgren (:mats) 2016-08-17 10:59:03 PDT
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.
Comment 8 User image Mats Palmgren (:mats) 2016-08-17 11:00:24 PDT
Created attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.
Comment 10 User image Pulsebot 2016-08-19 10:56:47 PDT
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 12 User image Mats Palmgren (:mats) 2016-08-20 09:50:51 PDT
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
Comment 13 User image Liz Henry (:lizzard) (needinfo? me) 2016-08-22 10:41:46 PDT
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.
Comment 16 User image Liz Henry (:lizzard) (needinfo? me) 2016-10-28 12:35:32 PDT
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.
Comment 17 User image Liz Henry (:lizzard) (needinfo? me) 2016-10-28 12:38:46 PDT
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.
Comment 18 User image Mats Palmgren (:mats) 2016-10-28 12:52:19 PDT
I think both patches should apply as is, let me know there are problems.
Comment 19 User image Carsten Book [:Tomcat] 2016-11-29 07:39:02 PST
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'
Comment 20 User image Mats Palmgren (:mats) 2016-11-29 09:45:06 PST
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.

Note You need to log in before you can comment on or make changes to this bug.