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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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

[Alastor Wu [:alwu]]

I will check this tomorrow, keep NI.

Comment 2

[Alastor Wu [:alwu]]

I don't see any possibility of the track demuxer being released while running that for loop. The track demuxer is hold by a strong reference in the source demuxer until the source demuxer dies. So the one possibility I see if the entire source demuxer got destroyed.

That seems possible to be triggered by following steps

1. When shutting down the source decoder, call media source's detach (note, releasing demuxer in line#192)

2. Clear the source buffer list

3. Trigger source buffer's detach

4. Call source demuxer's DetachSourceBuffer

5. Dispatch a runnable to the task queue.

6. Back to 1, if the demuxer gets destroyed (on main thread) while running MediaSourceDemuxer::DoDetachSourceBuffer (on task queue), then it's possible that the whole source demuxer becomes poison value.

---

Consider the very low crash volume, lower the bug's severity.

Comment 3

[Andrew McCreight [:mccr8]]

The point of this bug is that crashes in the wild might indicate an exploitable security problem, not that the crash volume is bad. Maybe the security rating could be lowered, as from comment 2 it sounds like a race so maybe it is difficult to cause it deliberately. Hard to say, though.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Comment 5

[Karl Tomlinson (:karlt)]

NewRunnableMethod() takes a strong reference to the MediaSourceDemuxer. Does that address the concerns in comment 2?

Comment 6

[Karl Tomlinson (:karlt)]

I had some difficulty understanding what DetachSourceBuffer() was trying to do. Its parameter is mutable RefPtr<TrackBuffersManager>& aSourceBuffer, but its caller does not expect its RefPtr<TrackBuffersManager> mTrackBuffersManager argument to be stolen.
I think const RefPtr<TrackBuffersManager>& aSourceBuffer was intended for the parameter. The parameter was simply TrackBuffersManager* prior to https://hg.mozilla.org/mozilla-central/rev/05b222ac8a540186f26caf7cfe514956399ecdfd#l2.12

The Storages template parameter for NewRunnableMethod<typename... Storages, > is the rvalue reference RefPtr<TrackBuffersManager>&& which means store as RefPtr<TrackBuffersManager> and is intended to be used with a RefPtr<TrackBuffersManager> argument passed with std::move().
Here instead the argument is the lvalue reference RefPtr<TrackBuffersManager>& aSourceBuffer.

So what happens with the mutable reference argument?
The corresponding NewRunnableMethod() parameter is Args&&... aArgs, which is forwarded to construct a RunnableMethodArguments<Storages...> mArgs member on the runnable, which resolves to StoreCopyPassByRRef, which forwards the lvalue reference, and so IIUC should be using the RefPtr(const RefPtr<I>& aSmartPtr) copy constructor as intended.

Comment 7

[Karl Tomlinson (:karlt)]

GetTrackDemuxer modifies mDemuxers without asserting OnTaskQueue().

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

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:alwu, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 9

[Alastor Wu [:alwu]]

Bump severity to S2 per comment 3, also I will check what Karl said again later.

Comment 10

[Alastor Wu [:alwu]]

I agree with the comment 5, the source demuxer has been hold by the runnable already, so my patch won't fix the crash. (Unless there is something we miss) But per Karl's comment in D190241, it would be still beneficial to land the patch. So I am still going to land my patch.

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 9356952 [details]
Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: We haven't found the real root cause yet, but we suspect it's caused by a race condition which is hard to reproduce. It'd be very hard to construct an exploit based on current patch or the crash information we have.
• 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?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: We are not sure when the issue was introduced, and also not sure if the problem can be fixed by current patch. So we don't have a plan to uplift this patch to previous version.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely, it would function exactly the same as before. We just make some improvement to make the ref-count relationship clearer.
• Is Android affected?: Yes

Comment 12

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

Comment on attachment 9356952 [details]
Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Approved to request uplift and land

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 9356952 [details]
Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Beta/Release Uplift Approval Request

• User impact if declined: Crash
• Is this code covered by automated tests?: No
• 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): This is a small improvement to make the ref-count relationship clearer, which doesn't change any functionality or add new feature.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21dd9eaaec1a
replace NewRunnableMethod with NS_NewRunnableFunction. r=media-playback-reviewers,karlt

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/21dd9eaaec1a

Comment 16

[Alastor Wu [:alwu]]

Add NI to check whether the crash still exists in months.

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9356952 [details]
Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Approved for 119.0b9

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/947c35ca4d4c

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5e0ca64ad8f0

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9356952 [details]
Bug 1853201 - replace NewRunnableMethod with NS_NewRunnableFunction.

Approved for 115.4esr.

Comment 21

[Daniel Veditz [:dveditz]]

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