1338767 - Crash in mozilla::DisplayItemClip::GetCommonRoundedRectCount or mozilla::PaintedLayerData::UpdateCommonClipCount

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-b02c5782-c3d9-4cee-b78a-8c0f42170211.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::DisplayItemClip::GetCommonRoundedRectCount(mozilla::DisplayItemClip const&, unsigned int) 	layout/base/DisplayItemClip.cpp:391
1 	xul.dll 	mozilla::PaintedLayerData::UpdateCommonClipCount(mozilla::DisplayItemClip const&) 	layout/base/FrameLayerBuilder.cpp:2747
2 	xul.dll 	mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) 	layout/base/FrameLayerBuilder.cpp:4469
3 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) 	layout/base/FrameLayerBuilder.cpp:5538
4 	xul.dll 	nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) 	layout/base/nsDisplayList.cpp:4939
5 	xul.dll 	nsDisplaySubDocument::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) 	layout/base/nsDisplayList.cpp:4989
6 	xul.dll 	mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) 	layout/base/FrameLayerBuilder.cpp:4310
7 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) 	layout/base/FrameLayerBuilder.cpp:5538
8 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/base/nsDisplayList.cpp:1861
9 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) 	layout/base/nsLayoutUtils.cpp:3651
10 	xul.dll 	PresShell::Paint(nsView*, nsRegion const&, unsigned int) 	layout/base/nsPresShell.cpp:6391
11 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) 	view/nsViewManager.cpp:484
12 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) 	view/nsViewManager.cpp:415
13 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/nsViewManager.cpp:1118
14 	xul.dll 	nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:2012
15 	xul.dll 	nsRefreshDriver::DoTick() 	layout/base/nsRefreshDriver.cpp:1483
16 	xul.dll 	nsRefreshDriver::FinishedWaitingForTransaction() 	layout/base/nsRefreshDriver.cpp:2108
17 	xul.dll 	nsRefreshDriver::NotifyTransactionCompleted(unsigned __int64) 	layout/base/nsRefreshDriver.cpp:2162
18 	xul.dll 	mozilla::layers::ClientLayerManager::DidComposite(unsigned __int64, mozilla::TimeStamp const&, mozilla::TimeStamp const&) 	gfx/layers/client/ClientLayerManager.cpp:463
19 	xul.dll 	mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned __int64 const&, unsigned __int64 const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) 	gfx/layers/ipc/CompositorBridgeChild.cpp:550
20 	xul.dll 	mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1390
21 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp:1743
22 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message&&) 	ipc/glue/MessageChannel.cpp:1681
23 	xul.dll 	mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) 	ipc/glue/MessageChannel.cpp:1572
24 	xul.dll 	mozilla::ipc::MessageChannel::MessageTask::Run() 	ipc/glue/MessageChannel.cpp:1597
25 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1216
26 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:361
27 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
28 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:225
29 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:205
30 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
31 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
32 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
33 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4472
34 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp:4605
35 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4696
36 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:282
37 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
38 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
39 	kernel32.dll 	BaseThreadInitThunk 	
40 	ntdll.dll 	__RtlUserThreadStart 	
41 	ntdll.dll 	_RtlUserThreadStart

this is a cross-platform crash signature that regressed in volume starting with firefox 49 builds - it's still rather low-volume though.

Comment 1

[[:philipp]]

after looking closer, a portion of them may be UAF though.

Comment 2

[Daniel Veditz [:dveditz]]

The stacks for the UAFs match those of the more benign near-null derefs. It's an OBR in a routine that returns a number so by itself not that exploitable or revealing. Could be more interesting if there are other uses of the same deleted object. Not sure how to rate this.

Comment 3

[Andrew McCreight [:mccr8]]

It looks like the test case for bug 1341149 is on this line:
  if (aClipState.mHasRoundedCorners) {

So that sounds related possibly?

Comment 4

[Markus Stange [:mstange]]

It does sound related! Nice find.

Comment 5

[Julien Cristau [:jcristau]]

tracking for 53/54 as sec-high.

Comment 6

[Markus Stange [:mstange]]

I don't see any reports on the 53/54/55 channels after the February 21 build:
https://crash-stats.mozilla.com/signature/?product=Firefox&version=54.0a1&version=55.0a1&version=53.0b&version=53.0a2&version=54.0a2&version=53.0b1&version=53.0a1&signature=mozilla%3A%3ADisplayItemClip%3A%3AGetCommonRoundedRectCount&date=%3E%3D2016-12-08T19%3A14%3A18.000Z&date=%3C2017-03-08T19%3A14%3A18.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1

The newest buildid in those reports is 20170221030205.

Bug 1341149 was fixed on mozilla-central (Firefox 54) on Feb 24 and uplifted to 53 on March 5.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

OK, bug 1341149 may have helped with some of these issues, but there's clearly still another issue going on here that affects more than 53+. Fx53b1 is already showing crashes with this signature:
https://crash-stats.mozilla.com/report/index/03a798f3-e927-4e87-8933-bac8d2170309

And 52 has scary-looking ones like:
https://crash-stats.mozilla.com/report/index/4caa7607-7647-42a2-89bb-8f4792170309

On the other hand, no hits on 54 since 15-Feb and none on 55 either.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

We have the scary looking ones in 54 as well, and I'll assume 55 (volume is low enough that it's unlikely to hit in nightly much if at all).

We need someone from the layout team to look at this.

Comment 9

[Jet Villegas (inactive)]

Matt: symptoms indicate a freed DisplayItemClip remaining in the DisplayList used for painting. Are you able to extract a test URL from the memory dump?

Comment 10

[Matt Woodrow (:mattwoodrow)]

The URLs are all over the place, facebook, youtube, a few porn sites, some russian new sites. I wasn't able to reproduce a crash by loading any of them.

I'd have expected this to have changed as a result of bug 1298218, which reworked how we allocated clipping a fair bit, but it doesn't appear to have.

It definitely looks like 'itemClip' in ProcessDisplayItems() has been destroyed.

There's quite a few other places in that function that optionally dereference itemClip earlier on, do we have other similar bugs for similar crashes with DisplayItemClip/FrameLayerBuilder?

The other option is that something within ProcessDisplayItems itself is causing the memory to be freed/overwritten.

I found a few dumps where the 'itemType' variable was available and it was TYPE_TEXT in both of them. Unsure if that's a useful lead, or just a coincidence.

Comment 11

[Frederik Braun [:freddy]]

Jet, what's the next step here?

Comment 12

[Matt Woodrow (:mattwoodrow)]

Markus, do you have any ideas here?

The DisplayItemClipChain changes changed the way we allocate/manage clips, so I'd have expected that to at least have had an effect on the crashes here, but they don't seem to have.

Comment 13

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> The DisplayItemClipChain changes changed the way we allocate/manage clips,

Not all that much, though.

I don't have any ideas for what to do about this bug.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://crash-stats.mozilla.com/report/index/e0ef0dfe-ed14-47b2-b0fc-7eaf02170406 may be interesting; the poison value is 'deadbeef', which I find used a bunch in layout.  (Also a little in the JIT, ProxyAutoConfig, etc)

Comment 15

[Astley Chen (inactive)]

There is no crash reported from FF55 so far.

It's also unable to reproduce locally with links attached in reports.
As the crash rate is still in low volume on 53 and 54 in last 3 months, keep monitoring.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

55->? since the general crash volume is too low to take lack-of-crash in Nightly as being the same as "unaffected"

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 53, but we could still take a patch in 54.

Comment 18

[Gerry Chang [:gchang]]

Hi Astley,
Can you help take a look at this?

Comment 19

[Astley Chen (inactive)]

matt, mstange, similar issues had been found recent days as bug 1359233, bug 1359569 that are about UAF.
Do we have a general methodology to move this kind of issues forward?

Comment 20

[Frederik Braun [:freddy]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #19)
> matt, mstange, similar issues had been found recent days as bug 1359233, bug
> 1359569 that are about UAF.
> Do we have a general methodology to move this kind of issues forward?

Besides not using raw pointers? :-)

Comment 21

[Gerry Chang [:gchang]]

Track 55+/56+ as sec-high.

Comment 22

[Mike Taylor [:miketaylr]]

In the past 2 months, just 2 crashes in 56, and a few more on 57. Setting 56 to wontfix, because nobody is actively working on it.

Comment 23

[Wennie]

Hi Matt:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 24

[Matt Woodrow (:mattwoodrow)]

Low crash rate, no need to track for esr.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Looks like there haven't been any crashes in 61+, so marking this as fixed.