1387659 - heap-use-after-free in [@ GetLayerManager]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

This popped up while fuzzing video. I am working on collecting a test case.

==11736==ERROR: AddressSanitizer: heap-use-after-free on address 0x619003741098 at pc 0x7f61420db42e bp 0x7f6123cf2ad0 sp 0x7f6123cf2ac8
READ of size 8 at 0x619003741098 thread T34 (Compositor)
    #0 0x7f61420db42d in Manager /home/worker/workspace/build/src/obj-firefox/dist/include/Layers.h:852:36
    #1 0x7f61420db42d in GetLayerManager /home/worker/workspace/build/src/gfx/layers/composite/CompositableHost.cpp:171
    #2 0x7f61420db42d in mozilla::layers::ImageHost::UseTextureHost(nsTArray<mozilla::layers::CompositableHost::TimedTexture> const&) /home/worker/workspace/build/src/gfx/layers/composite/ImageHost.cpp:77
    #3 0x7f614213cbce in mozilla::layers::CompositableParentManager::ReceiveCompositableUpdate(mozilla::layers::CompositableOperation const&) /home/worker/workspace/build/src/gfx/layers/ipc/CompositableTransactionParent.cpp:168:23
    #4 0x7f614218ba58 in mozilla::layers::ImageBridgeParent::RecvUpdate(nsTArray<mozilla::layers::CompositableOperation>&&, nsTArray<mozilla::layers::OpDestroy>&&, unsigned long const&) /home/worker/workspace/build/src/gfx/layers/ipc/ImageBridgeParent.cpp:197:10
    #5 0x7f6140de861d in mozilla::layers::PImageBridgeParent::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PImageBridgeParent.cpp:296:20
    #6 0x7f6140bfe7ee in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2092:25
    #7 0x7f6140bfb934 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2018:17
    #8 0x7f6140bfd214 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1887:5
    #9 0x7f6140bfd7f8 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1920:15
    #10 0x7f6140b6aeb3 in RunTask /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:452:9
    #11 0x7f6140b6aeb3 in DeferOrRunPendingTask /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:460
    #12 0x7f6140b6aeb3 in MessageLoop::DoWork() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:535
    #13 0x7f6140b6cb09 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/chromium/src/base/message_pump_default.cc:36:31
    #14 0x7f6140b68a8b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #15 0x7f6140b68a8b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #16 0x7f6140b68a8b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #17 0x7f6140b86c39 in base::Thread::ThreadMain() /home/worker/workspace/build/src/ipc/chromium/src/base/thread.cc:181:16
    #18 0x7f6140b76f4c in ThreadFunc(void*) /home/worker/workspace/build/src/ipc/chromium/src/base/platform_thread_posix.cc:38:13
    #19 0x7f615dcd26b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #20 0x7f615cd5b3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment 1

[Daniel Veditz [:dveditz]]

Milan: Tyson's not going to be able to capture a testcase here. Is the log at all useful in finding a bug? If not we can close this as incomplete

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Sotaro, it looks lije the LayerManager has been deleted (not just in the destroyed state), can you think of a way the above stack helps us?

Comment 3

[Sotaro Ikeda [:sotaro]]

It seems that there is a case that is not detached correctly.

Comment 4

[Sotaro Ikeda [:sotaro]]

:tsmith, is there a specific STR to reproduce the problem?

Comment 5

[Sotaro Ikeda [:sotaro]]

It seems better to address the problem similar way to Bug 1307458.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ClearCachedResources() calling

Comment 7

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4b6f5e31e76ae24bf37a7c233bf3a205c775a1

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8897742 [details] [diff] [review]
patch - Fix ClearCachedResources() calling

Review of attachment 8897742 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have access to Bug 1307458, so I am missing context about the issue and the approach you chose to fix it. Could you provide some more details here?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1439,5 @@
>  LayerComposite::Destroy()
>  {
>    if (!mDestroyed) {
>      mDestroyed = true;
> +    //CleanupResources();

Is this intentional?

Comment 9

[Tyson Smith [:tsmith]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> :tsmith, is there a specific STR to reproduce the problem?

No sorry nothing that I found will reproduce the issue. I was opening a new tab playing a video and closing the tab repeatedly.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #8)
> Comment on attachment 8897742 [details] [diff] [review]
> patch - Fix ClearCachedResources() calling
> 
> Review of attachment 8897742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have access to Bug 1307458, so I am missing context about the issue
> and the approach you chose to fix it. Could you provide some more details
> here?
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1439,5 @@
> >  LayerComposite::Destroy()
> >  {
> >    if (!mDestroyed) {
> >      mDestroyed = true;
> > +    //CleanupResources();
> 
> Is this intentional?

When I created the patch, I was going to remove the call, but I did not conclude it. I need to re-think about it.

Comment 11

[Sotaro Ikeda [:sotaro]]

From attachment 8894034 [details], it seems that layer manager was already destructed. LayerManagerComposite was destructed only by  CompositorBridgeParent::StopAndClearResources(). Therefore, from it, there seemed to be existed a layer that hold the destructed pointer of LayerManagerComposite even after CompositorBridgeParent::StopAndClearResources() call.

   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#433

From it, from it I thought there is a layer that did not clean up its resources that belonged to CompositorBridgeParent. Current implementation tried to clean up layer's tree resources by layer tree traversal. To make clear that all belonged layers clean its resources, it seems better to clean up the resources by mLayerMap.Iter() instead of layer tree traversal. Similar thing was done by Bug 1307458.

Comment 12

[Sotaro Ikeda [:sotaro]]

But during looking into Bug 1390731, I found another route to cause holding LayerManagerComposite pointer after the CompositorBridgeParent::StopAndClearResources(). LayerState destruction does not force to release LayerManagerComposite pointer, and LayerTransactionParent and its layers could exist until its destruction.

LayerState destruction sequence was like the following.

RenderFrameParent::ActorDestroy(ActorDestroyReason why)
->GPUProcessManager::UnmapLayerTreeId()
->mGPUChild->SendRemoveLayerTreeIdMapping();
->GPUParent::RecvRemoveLayerTreeIdMapping()
->CompositorBridgeParent::DeallocateLayerTreeId();
->CompositorLoop()->PostTask(NewRunnableFunction(&EraseLayerState, aId));
->EraseLayerState(uint64_t aId)

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch -Add clearing pointer of LayerManager at CompositorBridgeParent

Comment 14

[Sotaro Ikeda [:sotaro]]

attachment 8898654 [details] [diff] [review] changed a way of fixing the problem. attachment 8897742 [details] [diff] [review] tried to clear all layers resources. Instread attachment 8898654 [details] [diff] [review] tried to address the problem by clearing LayerManager pointer of all layers when the pointer becomes unnecessary to avoid stale pointer.

Comment 15

[Sotaro Ikeda [:sotaro]]

attachment 8898654 [details] [diff] [review] tried to address the root cause.

Comment 16

[Sotaro Ikeda [:sotaro]]

attachment 8898654 [details] [diff] [review] caused a lot of reftest failure :( It seems better to minimize a code change for this bug.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd1ff8b11159a403b4f6d949e19821463f3752d&selectedJob=124088213

Comment 17

[Sotaro Ikeda [:sotaro]]

From attachment 8894034 [details], pointer of Layer that was held by ImageHost was invalid. The pointer should be cleaned befpre deleting ImageLayerComposite. LayerTransactionParent::RecvReleaseLayer() actually do it by calling ImageLayerComposite::Disconnect().
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp#1035

By attachment 8894034 [details] the ImageLayerComposite was deleted in ContainerLayerComposite::~ContainerLayerComposite(). It means that LayerTransactionParent already released a ref of ImageLayerComposite. From it, the ImageLayerComposite seemed not get ImageLayerComposite::Disconnect() called.

It means there is a route that LayerTransactionParent released the ImageLayerComposite without calling ImageLayerComposite::Disconnect(). LayerTransactionParent::RecvReleaseLayer() seemed not called for the ImageLayerComposite.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> 
> It means there is a route that LayerTransactionParent released the
> ImageLayerComposite without calling ImageLayerComposite::Disconnect().
> LayerTransactionParent::RecvReleaseLayer() seemed not called for the
> ImageLayerComposite.

When LayerTransactionParent is destructed, it does not call Layer::Disconnect() for layers in mLayerMap.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Layer::Disconnect() calling in LayerTransactionParent::Destroy()

Comment 20

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54751cd99f5d541eceb47770be708be7ad2b4e34

Comment 21

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fdab6aa8efae0b220dbe7f6027277f50981294

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1fdab6aa8ef

Comment 23

[Ryan VanderMeulen [:RyanVM]]

What other branches does this affect? Any idea on a severity rating? We probably should have had those answers before landing this :\
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8899053 [details] [diff] [review]
patch - Add Layer::Disconnect() calling in LayerTransactionParent::Destroy()

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
 > It is not easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
 > No

Which older supported branches are affected by this flaw?
 > It exist since firefox 53

If not all supported branches, which bug introduced the flaw?
 > Bug 1323539

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  >  It is easy to back port.


How likely is this patch to cause regressions; how much testing does it need?
 > It is simple fix. And it just add function calls as before. It seems not likely to cause the regression.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

I think you can just go ahead and nominate for Beta approval too while you're at it :)

Comment 26

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8899053 [details] [diff] [review]
patch - Add Layer::Disconnect() calling in LayerTransactionParent::Destroy()

Approval Request Comment
[Feature/Bug causing the regression]: bug 1323539
[User impact if declined]: It might rarely cause a crash during closing a Tab.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: It is simple fix. And it just add function calls like before.
[String changes made/needed]: None.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ to land on trunk. I'll approve Beta as well.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5944141d777

Comment 29

[Andrei Vaida [:avaida]]

(In reply to Sotaro Ikeda [:sotaro  PTO 1/Sep - 13/Sep] from comment #26)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Sotaro Ikeda's assessment on manual testing needs and the fact that this fix has automated coverage.