1855640 - AddressSanitizer: heap-use-after-free [@ mozilla::WebrtcGmpVideoEncoder::Terminated]

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

Attachment: gmp.shutdown.txt

I got a shutdown use-after-free while playing around with a GMP video encoder demo. The shortened version is this:

==1372907==ERROR: AddressSanitizer: heap-use-after-free on address 0x51e0000c00a8 at pc 0x7f2421de3ff4 bp 0x7f2404d30b00 sp 0x7f2404d30af8
WRITE of size 8 at 0x51e0000c00a8 thread T28 (GMPThread)
console.error: ({})
    #0 0x7f2421de3ff3 in mozilla::WebrtcGmpVideoEncoder::Terminated() dom/media/webrtc/libwebrtcglue/WebrtcGmpVideoCodec.cpp:511:8
    #1 0x7f2420fd2bfb in mozilla::gmp::GMPVideoEncoderParent::Shutdown() dom/media/gmp/GMPVideoEncoderParent.cpp:203:16
    #2 0x7f2420fd48fc in RecvShutdown dom/media/gmp/GMPVideoEncoderParent.cpp:253:3
    #3 0x7f2420fd48fc in non-virtual thunk to mozilla::gmp::GMPVideoEncoderParent::RecvShutdown() dom/media/gmp/GMPVideoEncoderParent.cpp
    [...]

0x51e0000c00a8 is located 40 bytes inside of 2736-byte region [0x51e0000c0080,0x51e0000c0b30)
freed by thread T28 (GMPThread) here:
    #0 0x55702ae69f46 in free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3
    #1 0x7f2421dfaf93 in mozilla::WebrtcGmpVideoEncoder::Release() dom/media/webrtc/libwebrtcglue/WebrtcGmpVideoCodec.h:161:3
    #2 0x7f2420fd288f in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:54:40
    #3 0x7f2420fd288f in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:420:36
    #4 0x7f2420fd288f in assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:73:7
    #5 0x7f2420fd288f in operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:188:5
    #6 0x7f2420fd288f in mozilla::gmp::GMPVideoEncoderParent::Close() dom/media/gmp/GMPVideoEncoderParent.cpp:58:13
    #7 0x7f2421de3f38 in mozilla::WebrtcGmpVideoEncoder::Terminated() dom/media/webrtc/libwebrtcglue/WebrtcGmpVideoCodec.cpp:510:9
    #8 0x7f2420fd2bfb in mozilla::gmp::GMPVideoEncoderParent::Shutdown() dom/media/gmp/GMPVideoEncoderParent.cpp:203:16
    [...]

The issue seems to be in dom/media/webrtc/libwebrtcglue/WebrtcGmpVideoCodec.cpp, specifically in WebrtcGmpVideoEncoder::Terminated:

// GMPVideoEncoderCallback virtual functions.
void WebrtcGmpVideoEncoder::Terminated() {
  GMP_LOG_DEBUG("GMP Encoder Terminated: %p", (void*)this);

  mGMP->Close();                       <---- Free 
  mGMP = nullptr;                      <---- Use
  mHost = nullptr;
  mInitting = false;
  // Could now notify that it's dead
}

Given that this is literally the next line of code, I would assume it's very difficult to exploit.

Comment 1

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

Probably needs a self refptr?

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

This seems to be the same issue we try to avoid in another method:
https://searchfox.org/mozilla-central/rev/37d9d6f0a77147292a87ab2d7f5906a62644f455/dom/media/webrtc/libwebrtcglue/WebrtcGmpVideoCodec.cpp#266

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1855640.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e952ea93800
r=media-playback-reviewers,alwu

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e952ea93800

Comment 6

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:aosmond, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9356420 [details]
Bug 1855640.

Beta/Release Uplift Approval Request

• User impact if declined: Use-after-free sec issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are duplicating logic to handle a similar case in another code path.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Use-after-free sec issue
• User impact if declined:
• Fix Landed on Version: 120
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are duplicating logic to handle a similar case in another code path.

Comment 8

[Donal Meehan [:dmeehan]]

Comment on attachment 9356420 [details]
Bug 1855640.

Approved for 119.0b6

Comment 9

[Pulsebot]

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

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9356420 [details]
Bug 1855640.

Approved for 115.4esr.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/c1f564537915

Comment 12

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter