1164740 - GeckoMediaPlugin use-after-free in child on shutdown in GMPVideoHostImpl::CreateFrame

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This is with a Linux ASan debug build, using https://mozilla.github.io/webrtc-landing/pc_test.html with the H.264 checkbox checked, in e10s mode.  When quitting the browser while the video is happening, there's a use-after-free error reported in the GMP child process:

==125765==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000039d48 at pc 0x7f18407720b1 bp 0x7fffebcc75c0 sp 0x7fffebcc75b8
READ of size 8 at 0x60d000039d48 thread T0
0x60d000039d48 is located 104 bytes inside of 136-byte region [0x60d000039ce0,0x60d000039d68) freed by thread T0 previously allocated by thread T0

The read is here:

libxul.so+0x000000000680a0b0
mozilla::gmp::GMPVideoHostImpl::CreateFrame(GMPVideoFrameFormat, GMPVideoFrame**)
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoHost.cpp:26
  (Followed by a frame from the OpenH264 plugin that I don't have symbols for, so I'm not sure if this is right:)
libxul.so+0x00000000067f4949
mozilla::gmp::SyncRunnable::Run()
/home/jld/src/gecko-dev/dom/media/gmp/GMPPlatform.cpp:85
libxul.so+0x000000000278c9a2
MessageLoop::RunTask(Task*)
/home/jld/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:361

The free is here:

libxul.so+0x00000000067db482
mozilla::gmp::GMPVideoDecoderChild::Release()
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoDecoderChild.h:26
libxul.so+0x00000000067af915
mozilla::gmp::GMPContentChild::DeallocPGMPVideoDecoderChild(mozilla::gmp::PGMPVideoDecoderChild*)
/home/jld/src/gecko-dev/dom/media/gmp/GMPContentChild.cpp:89
libxul.so+0x00000000032a9789
mozilla::gmp::PGMPContentChild::RemoveManagee(int, mozilla::ipc::IProtocol*)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPContentChild.cpp:203
libxul.so+0x0000000002997e13
mozilla::gmp::PGMPVideoDecoderChild::Send__delete__(mozilla::gmp::PGMPVideoDecoderChild*)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPVideoDecoderChild.cpp:68
libxul.so+0x00000000067ffb32
mozilla::gmp::GMPVideoDecoderChild::RecvDecodingComplete()
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoDecoderChild.cpp:211
libxul.so+0x000000000299c481
mozilla::gmp::PGMPVideoDecoderChild::OnMessageReceived(IPC::Message const&)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPVideoDecoderChild.cpp:509
libxul.so+0x00000000032ac4c1
mozilla::gmp::PGMPContentChild::OnMessageReceived(IPC::Message const&)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPContentChild.cpp:427
libxul.so+0x000000000282fba1
mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)
/home/jld/src/gecko-dev/ipc/glue/MessageChannel.cpp:1279
libxul.so+0x000000000282db8a
mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&)
/home/jld/src/gecko-dev/ipc/glue/MessageChannel.cpp:1198
libxul.so+0x000000000282bc87
mozilla::ipc::MessageChannel::Call(IPC::Message*, IPC::Message*)
/home/jld/src/gecko-dev/ipc/glue/MessageChannel.cpp:981
libxul.so+0x000000000299a962
mozilla::gmp::PGMPVideoDecoderChild::CallNeedShmem(unsigned int const&, mozilla::ipc::Shmem*)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPVideoDecoderChild.cpp:278
libxul.so+0x0000000006800198
mozilla::gmp::GMPVideoDecoderChild::Alloc(unsigned long, mozilla::ipc::SharedMemory::SharedMemoryType, mozilla::ipc::Shmem*)
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoDecoderChild.cpp:226
libxul.so+0x00000000067f65b0
mozilla::gmp::GMPSharedMemManager::MgrAllocShmem(mozilla::gmp::GMPSharedMem::GMPMemoryClasses, unsigned long, mozilla::ipc::SharedMemory::SharedMemoryType, mozilla::ipc::Shmem*)
/home/jld/src/gecko-dev/dom/media/gmp/GMPSharedMemManager.cpp:42
libxul.so+0x000000000680b538
mozilla::gmp::GMPPlaneImpl::MaybeResize(int)
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoPlaneImpl.cpp:87
libxul.so+0x000000000680be77
mozilla::gmp::GMPPlaneImpl::Copy(int, int, unsigned char const*)
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoPlaneImpl.cpp:154
libxul.so+0x000000000680d012
mozilla::gmp::GMPVideoi420FrameImpl::CreateFrame(int, unsigned char const*, int, unsigned char const*, int, unsigned char const*, int, int, int, int, int)
/home/jld/src/gecko-dev/dom/media/gmp/GMPVideoi420FrameImpl.cpp:187
  (And then it goes into the OpenH264 plugin, and ASan doesn't unwind past that.)

The allocation is here:

libxul.so+0x00000000067af8eb
operator new(unsigned long)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/dom/media/gmp/../../../dist/include/mozilla/mozalloc.h:186
mozilla::gmp::GMPContentChild::AllocPGMPVideoDecoderChild()
/home/jld/src/gecko-dev/dom/media/gmp/GMPContentChild.cpp:81
libxul.so+0x00000000032ad00f
mozilla::gmp::PGMPContentChild::OnMessageReceived(IPC::Message const&)
/home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/./PGMPContentChild.cpp:527
libxul.so+0x000000000282fba1
mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)
/home/jld/src/gecko-dev/ipc/glue/MessageChannel.cpp:1279

This crash is followed by the parent process crash I reported as 1164739.

Comment 1

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

Soooo...

Basically it appears we destroyed the actor (with "delete aActor", not just dropping a ref).  But while that was happening, there was a frame creation in-flight in the child in a runnable. 

  void Decode_m (GMPVideoEncodedFrame* inputFrame, ...) {
    ...
    // Translate the image.
    GMPErr err = host_->CreateFrame (kGMPI420VideoFrame, &ftmp);

Host is set in this manner:
  OpenH264VideoDecoder (GMPVideoHost* hostAPI) :
    host_ (hostAPI),

But RecvDecodingComplete() calls DecodingComplete(), which in OpenH264 is:
  virtual void DecodingComplete() {
    delete this;
  }

Which is probably not good (nor letting the delete of the actor/host happen) while there's a runnable in-flight.

Comment 2

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

Ethan, Chris - what do you want to do here?

Comment 3

[Ethan Hugg [:ehugg]]

Well my first thought would be to change gmp-openh264.cpp to do a syncrunonmainthread call for EncodingComplete and DecodingComplete like we do for Encode_m and Decode_m.

Comment 4

[Ethan Hugg [:ehugg]]

This is the patch I'm testing to see if it does the job:
https://rbcommons.com/s/OpenH264/r/1227/

Comment 5

[Ethan Hugg [:ehugg]]

Tested on Linux with latest M-C ASAN build with e10s enabled and was unable to get it to crash with or without my patch.  If anyone can get a reliable crash I can send a build of OpenH264 with my proposed fix.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Doesn't somebody just need to port the fix from Bug 1162358 over from the GMPVideoDecoderParent to the GMPVideoEncoderParent?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Ethan Hugg [:ehugg] from comment #5)
> Tested on Linux with latest M-C ASAN build with e10s enabled and was unable
> to get it to crash with or without my patch.  If anyone can get a reliable
> crash I can send a build of OpenH264 with my proposed fix.

When I reported this bug I could reproduce it reliably, but I tried now and couldn't.  (I did, however, reproduce bug 1164739 without also seeing this one, which is interesting.)

Comment 8

[Ethan Hugg [:ehugg]]

If bug 1164739 is related to GMP, could you add me to the CC list?

Comment 9

[Maire Reavy [:mreavy]]

Hey Ethan -- I think you make the most sense as an assignee.  I know you're already working on a patch.  If you need me to find another owner, let me know.  Thanks.

Comment 10

[Ethan Hugg [:ehugg]]

I suspect we cannot reproduce this because it was also fixed by bug 1162358 which landed on the 5/18 (this was reported on 5/13).   However I am inclined to push this dispatch fix to OpenH264 anyway which would make it more difficult to regress and hit this race condition in the future.  Let me know if anyone objects to this plan.  If not, I'll make the OpenH264 pull req and mark this bug 'worksforme'.

Regarding whether bug 1162358 needs to be re-done for GMPVideoEncoderParent from comment #6 - I'm guessing that it does and I can open a separate bug for that.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Ethan Hugg [:ehugg] from comment #10)
> I suspect we cannot reproduce this because it was also fixed by bug 1162358
> which landed on the 5/18 (this was reported on 5/13).   However I am
> inclined to push this dispatch fix to OpenH264 anyway which would make it
> more difficult to regress and hit this race condition in the future.  Let me
> know if anyone objects to this plan.  If not, I'll make the OpenH264 pull
> req and mark this bug 'worksforme'.

I didn't read the call stack in comment 0 properly, the problem reported was indeed in GMPVideoDecodoerChild.cpp, not in the EncoderChild. So yes, I expect that bug 1162358 would fix this.

The OpenH264 patch in your review board adds sync dispatch from the main thread to the main thread (if I'm reading it correctly), which will fail. So the DecodingComplete() callback will never be called with your patch.

So I don't think you should commit that patch. In the non-WebRTC it case would result in a shutdown hang (at least until we timed out and killed the plugin). I don't know what affect it would have on WebRTC. Using OpenH264 for normal <video> playback isn't preffed on by default yet, but we'd like to use it in future on WinXP and Linux.

 
> Regarding whether bug 1162358 needs to be re-done for GMPVideoEncoderParent
> from comment #6 - I'm guessing that it does and I can open a separate bug
> for that.

I think bug 1162358 should be re-done for the GMPVideoEncoder, or someone needs to look at it and decide whether it's needed at least.

Comment 12

[Ethan Hugg [:ehugg]]

You're right.  I was mis-reading my logging.  I'm going to close this as a duplicate of bug 1162358 and make no changes to OpenH264.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I just got a GMPVideoDecoderChild use-after-free in a tree that has the fix for bug 1162358.  The free is DeallocPGMPVideoDecoderChild dropping the last ref, and the use is a SyncRunnable calling plugin code that calls GMPVideoHostImpl::CreateFrame on a freed object — it looks like the same stacks as in comment #0.  

It's not 100% reproducible, however; this was one run out of 4 or 5, while trying to answer the needinfo in bug 1164739.  (For future reference, `git rev-parse origin/master` currently says 4a50d5c15eb10e47ac0a31a1688df2670873d640.)

(Also, bug 1162358 landed on m-c on 05-08, it looks like; 05-18 was when its uplift landed.)

Comment 14

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

Chris - FYI this is not a dup of bug 1162358 (per comment 13 above), which you fixed and landed, but seems similar.  Can you take this one?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jed Davis [:jld] {UTC-7} from comment #13)
> I just got a GMPVideoDecoderChild use-after-free in a tree that has the fix
> for bug 1162358.  The free is DeallocPGMPVideoDecoderChild dropping the last
> ref, and the use is a SyncRunnable calling plugin code that calls
> GMPVideoHostImpl::CreateFrame on a freed object — it looks like the same
> stacks as in comment #0.  

This sounds like the crash in gmp-clearkey that I fixed in bug 1160914.

It's a bug in OpenH264; OpenH264 is calling the GMPVideoDecoderHost after its associated GMPVideoDecoder::DecodingComplete() implementation has run.

We guard against this in the gmp-clearkey and Adobe's GMP by having the class implementing GMPVideoDecoder refcounted, and when we dispatch a GMPTask (for example to call CreateFrame()) we keep a refcount on the GMPVideoDecoder implementation to keep its memory allocated. Our DecodingComplete() implementation sets a mHasShutdown flag. Before our refcounted tasks run, they check the flag and don't run if the mHasShutdown flag is set. This prevents us calling back after the DecodingComplete() has run.

So someone needs to do something like what I did in bug 1160914 for OpenH264.

Our GMPVideoDecoder::DecodingComplete() implementation:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp#396

Code to wrap a GMPTask in a task that addref's our GMPVideoDecoder's concrete class:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp#410

I also added a WrapTaskRefCounted to gmp-task-utils-generated.h, to make creating refcounted tasks easier:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/gmp-task-utils-generated.h#1934

Comment 16

[Maire Reavy [:mreavy]]

Ethan - Sounds like this is a bug in the plugin. See comment 15 for cpearce's analysis and recommendations.  Can you take this?

Comment 17

[Ethan Hugg [:ehugg]]

 
I put a patch that should do this for OpenH264 here:
https://rbcommons.com/s/OpenH264/r/1253/

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Ethan Hugg [:ehugg] from comment #17)
>  
> I put a patch that should do this for OpenH264 here:
> https://rbcommons.com/s/OpenH264/r/1253/

Looking over that patch, I think it will fix the bug as I understand it.

If you ever change from doing sync dispatches from the worker to the main thread to return output (i.e. the task to run the OpenH264VideoEncoder::Encode_m function etc) you'll need to ensure those tasks are refcounted too. As is, I believe the above change should be fix this crash.

Comment 19

[Andrew McCreight [:mccr8]]

Has the patch landed yet? Can this be closed?

Comment 20

[Ethan Hugg [:ehugg]]

It has not landed yet in OpenH264 master.  I expect it to this week.

I have been closing these bugs when a version of OpenH264 that fixes them starts shipping with Firefox and not when the patches land in the master branch of the OpenH264 repo.  I noticed on bug 1156342 you closed it already based on it being in the OpenH264 repo.    We can do this either way, but right now we are inconsistent.

Comment 21

[Andrew McCreight [:mccr8]]

Well, I think you are right that it is generally good to leave it open until it is fixed in a version that is being shipped, but in the particular case of bug 1156342 it looked like the bug never affected a released version, so that's why I closed it.

Comment 22

[Ethan Hugg [:ehugg]]

Good point. That sounds fine then.  Thanks.

Comment 25

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

Has this been fixed and shipped?

Comment 26

[Ethan Hugg [:ehugg]]

It's been fixed in the OpenH264 Repo, but we need a new build of the OpenH264 plugin released for it to be fixed for Firefox.  The fix is not in the v1.4 release we currently ship.

Comment 27

[sijchen]

we are planning the 1.5 release after some more tests for fixes related to the recent bugs reported on bugzilla

Comment 28

[Rui Zhang]

Assign to Hank for verification of 1.5 release on this.

Comment 29

[Rui Zhang]

During the 1.5 release integration test, we did not encounter this issue. Mark results to be resolved now, and if it happens again will re-open

Comment 30

[BMO Automation]

Version and milestone values are being reset to defaults as part of product refactoring.