1858509 - Use-after-free crash in [@ mozilla::MediaSourceTrackDemuxer::HasManager]

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt back Feb 10)]

+++ This bug was initially created as a clone of Bug #1853201 +++

Crash report: https://crash-stats.mozilla.org/report/index/10831076-cca8-441e-a4db-b59e10230912

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  RefPtr<mozilla::TrackBuffersManager>::get const  mfbt/RefPtr.h:325
0  libxul.so  operator==<mozilla::TrackBuffersManager, mozilla::TrackBuffersManager>  mfbt/RefPtr.h:544
0  libxul.so  mozilla::MediaSourceTrackDemuxer::HasManager const  dom/media/mediasource/MediaSourceDemuxer.cpp:513
0  libxul.so  mozilla::MediaSourceDemuxer::DoDetachSourceBuffer  dom/media/mediasource/MediaSourceDemuxer.cpp:200
1  libxul.so  mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::TrackBuffersManager>&&>::apply<mozilla::MediaSourceDemuxer, void  const  xpcom/threads/nsThreadUtils.h:1164
1  libxul.so  std::__ndk1::__invoke_constexpr<mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::TrackBuffersManager>&&>::apply<mozilla::MediaSourceDemuxer, void   /builds/worker/fetches/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/type_traits:3880
1  libxul.so  std::__ndk1::__apply_tuple_impl<mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::TrackBuffersManager>&&>::apply<mozilla::MediaSourceDemuxer, void   /builds/worker/fetches/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/tuple:1415
1  libxul.so  std::__ndk1::apply<mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::TrackBuffersManager>&&>::apply<mozilla::MediaSourceDemuxer, void   /builds/worker/fetches/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/tuple:1424
1  libxul.so  mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::TrackBuffersManager>&&>::apply<mozilla::MediaSourceDemuxer, void   xpcom/threads/nsThreadUtils.h:1162
1  libxul.so  mozilla::detail::RunnableMethodImpl<mozilla::MediaSourceDemuxer*, void   xpcom/threads/nsThreadUtils.h:1213

Mostly but not entirely on Android. Window crash: bp-796852dc-f800-444c-9d6e-ef29c0230908

We're in this loop:

  for (auto& demuxer : mDemuxers) {
    if (demuxer->HasManager(aSourceBuffer)) {
      demuxer->DetachManager();
    }
  }

Could one of the DetachManager calls cause mDemuxers to have something added or removed? I tried looking over the code but I couldn't figure out how that might happen. mDemuxers has strong references so it isn't clear why a demuxer in the list would have gone away, even though we're running this code off a runnable.

19 crashes with this signature in the last 6 months, and all but 1 are on poison values.

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

From bug 1853201 comment 7:

GetTrackDemuxer modifies mDemuxers without asserting OnTaskQueue().

MediaFormatReader::DemuxerProxy uses its own TaskQueue.
Can that call MediaSourceDemuxer::GetTrackDemuxer()?

Comment 2

[Jim Mathies [:jimm]]

We will follow back up on this after the parent has had some time to bake.

Comment 3

[Jim Mathies [:jimm]]

Parent fix was uplifted to 119.

Comment 4

[Jim Mathies [:jimm]]

Hmm, seem to have picked up a similar crash report in 119 -
https://crash-stats.mozilla.org/report/index/5f31400d-dd13-488a-b68f-9f3750231031

Comment 5

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

Could this be because aSourceBuffer is captured by reference? It seems it is SourceBuffer::mTrackBuffersManager which will go out of scope after the call to MediaSourceDemuxer::DetachSourceBuffer:
https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/dom/media/mediasource/MediaSourceDemuxer.cpp#182
https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/dom/media/mediasource/SourceBuffer.cpp#453

If so, the fix is just something like sourceBuffer = RefPtr{aSourceBuffer}.

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: reference-capture.cpp

(In reply to Andrew Osmond [:aosmond] (he/him) from comment #5)

Could this be because aSourceBuffer is captured by reference?
I also wasn't certain about by-copy capture of a reference so I wrote this to check. Output is

construct A
Direct
const copy A
From reference
const copy A
From const reference
const copy A
From rvalue reference
const copy A
From inline rvalue reference
move A
From move
move A
destruct A
destruct A
destruct A
destruct A
destruct A
destruct A
destruct A

so aSourceBuffer is a non-reference RefPtr in the lambda expression.

I suspect mDemuxers is being modified from the MediaFormatReader TaskQueue.

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Adding an OnTaskQueue() assertion to MediaSourceDemuxer::GetTrackDemuxer() confirms that it is modifying mDemuxers off the task queue, when running test_BufferedSeek.html, for example.

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

This issue has existed since https://hg.mozilla.org/mozilla-central/rev/7fd2d15b8fae36cb51122e44db546fdcbb1c1845#l1.13
Prior to that, there was no obvious reason for the existence of mDemuxers because it was otherwise unused.

When mDemuxers was added, there was a racy read OnTaskQueue(). That was removed when CI identified the race.

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

The mDemuxers array element entry is also removed when no longer required.

Comment 10

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1858509 clarify that some TrackBuffersManager reference parameters are not mutable through side-effects r?alwu

Depends on D193616

Comment 11

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1858509 replace Monitor with Mutex where a condition variable is not used r?alwu

Depends on D193617

Comment 12

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 9363617 [details]
Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This would be difficult to exploit due to the need to have one thread iterate the array while another is adding to it, neither of which threads run content JS.

D193616 identifies the two sections of code that would need to race. The other patches do not highlight the security flaw.

• 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?: All.
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: D193616 applies to ESR115.

D193617 and D193618 merely clarify and simplify, and so do not need to be uplifted.

• How likely is this patch to cause regressions; how much testing does it need?: Unlikely because the changes in behavior are small and there are many automated tests exercising this code.
• Is Android affected?: Yes

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9363617 [details]
Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

Approved to land and uplift

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f09bc1d010b
add thread-safety annotations around MediaSourceDemuxer::mMonitor r=alwu

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ec171cb7fb1
clarify that some TrackBuffersManager reference parameters are not mutable through side-effects r=alwu

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8f6bb99beb4
Backed out changeset 4f09bc1d010b for causing process-crashes on MediaSourceDemuxer.cpp

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

4f09bc1d010b got backed out for causing process-crashes in MediaSourceDemuxer.cpp:

https://hg.mozilla.org/integration/autoland/rev/b8f6bb99beb4206d1b0fb7e2f5a053f02dddf1a3

Push with failures
Failure log

PROCESS-CRASH | MOZ_ASSERT(!mDemuxers.Contains(aSourceBuffer.get(), MatchTrackDemuxerManager())) [@ mozilla::MediaSourceDemuxer::DoDetachSourceBuffer] | /media-source/mediasource-detach.html

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7e938ac6494
add thread-safety annotations around MediaSourceDemuxer::mMonitor r=alwu

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9259a4ff84a4
replace Monitor with Mutex where a condition variable is not used r=alwu

Comment 21

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

mDemuxers array elements are also removed when no longer required.

Original Revision: https://phabricator.services.mozilla.com/D193616

Comment 22

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: yes
• String changes made/needed: None
• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Low, but some risk of deadlock if there's something i've missed.
• Needs manual QE test: no
• User impact if declined: sec-high
• Explanation of risk level: Low because there are many automated tests, which caught an incorrect assumption in a previous version of this patch.
• Code covered by automated testing: yes

Comment 23

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

mDemuxers array elements are also removed when no longer required.

Original Revision: https://phabricator.services.mozilla.com/D193616

Comment 24

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: yes
• String changes made/needed: none
• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Low, but some risk of deadlock if there's something i've missed.
• Needs manual QE test: no
• User impact if declined: sec-high
• Explanation of risk level: Low because there are many automated tests, which caught an incorrect assumption in a previous version of this patch.
• Code covered by automated testing: yes

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7e938ac6494
https://hg.mozilla.org/mozilla-central/rev/9259a4ff84a4

Comment 26

[Donal Meehan [:dmeehan]]

:karlt this bug has a leave-open keyword, should that be removed and the bug resolved?

Comment 27

[Karl Tomlinson (:karlt back Feb 10)]

Yes, thanks.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9367406 [details]
Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

Approved for 121.0b9.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9367408 [details]
Bug 1858509 add thread-safety annotations around MediaSourceDemuxer::mMonitor r?alwu

Approved for 115.6esr.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/889c2b945041

Comment 31

[Pulsebot]

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

Comment 32

[Daniel Veditz [:dveditz]]

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