Closed Bug 1297099 Opened 3 years ago Closed 3 years ago

LoadManagerSingleton weak reference used on multiple threads

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P1, critical)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 --- unaffected
firefox50 + fixed
firefox51 + fixed

People

(Reporter: mayhemer, Assigned: pehrsons)

References

Details

(Keywords: sec-critical, Whiteboard: [post-critsmash-triage][adv-main49+])

Attachments

(1 file)

Introduced in https://hg.mozilla.org/mozilla-central/rev/08e7e6ddff87, bug 1208371.

Bug 956338 adds assertions to check whether weak referencing is correctly used only on a single thread.  For LoadManagerSingleton this fails.

https://treeherder.mozilla.org/logviewer.html#?job_id=26046916&repo=try#L13081
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cbb9192861e6cbc46a615940c28b0b24c5a3d8

tests: dom/media/tests/mochitest/test_peerConnection_*

 11:06:52     INFO -  Assertion failure: _empty || _owningThread == std::this_thread::get_id() (WeakPtr used on multiple threads), at /builds/slave/try-lx-d-000000000000000000000/build/src/obj-firefox/dist/include/mozilla/WeakPtr.h:146
 11:06:52     INFO -  #01: mozilla::detail::WeakReference<mozilla::LoadManagerSingleton>::get [mfbt/WeakPtr.h:146]
 11:06:52     INFO -  #02: mozilla::LoadManager::NormalUsage [dom/media/systemservices/LoadManager.h:111]
 11:06:52     INFO -  #03: webrtc::OveruseFrameDetector::Process [media/webrtc/trunk/webrtc/video_engine/overuse_frame_detector.cc:538]
 11:06:52     INFO -  #04: webrtc::ProcessThreadImpl::Process [media/webrtc/trunk/webrtc/modules/utility/source/process_thread_impl.cc:213]
 11:06:52     INFO -  #05: webrtc::ThreadPosix::Run [media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc:174]


This is likely a highly critical bug.
Flags: needinfo?(pkerr)
Flags: needinfo?(pehrson)
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
Rank: 15
Flags: needinfo?(pehrson)
Priority: -- → P1
Per mayhemer, marking as sec-critical (might be high, not sure of the failure modes for this threading issue without looking at the code for the WeakRef).
Keywords: sec-critical
We check that the singleton is created and destroyed on main thread, and it's only destroyed by an xpcom-shutdown observer.

Question is how many LoadManager consumers are alive after xpcom-shutdown.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> We check that the singleton is created and destroyed on main thread, and
> it's only destroyed by an xpcom-shutdown observer.
> 
> Question is how many LoadManager consumers are alive after xpcom-shutdown.

None I hope, but this isn't at xpcom-shutdown; this is flagging that the WeakRef was used from two threads, which is invalid.
If you can strongly ensure that the weak referenced object over-lives all of its referrers, then you don't need support for weak referencing at all.  That would be the best solution here.

The problem the added assertions in bug 956338 try to catch is attempt to access weak ptr on one thread while the real object is just being destroyed on another thread.  Hence the simple restriction to query weak ptrs only on a single thread.

Dereference of a weak ptr to the same object on more then one thread (say A and B) is _LIKELY_ bad, at least it's a very bad programing architecture.  If you expect the real object go away on either thread A or B or even some thread X, then you cannot avoid very dangerous race conditions when referrers, still alive, keep the weak ref and try to query it.
Well it was added for a reason. IIRC there were intermittents at shutdown.

Isn't it less critical in this mode though? I.e., when we only reset the ptr at shutdown rather than doing it whenever?
Flags: needinfo?(pkerr)
Attachment #8784250 - Flags: review?(rjesup)
Attachment #8784250 - Flags: review?(rjesup) → review+
Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily

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? 48 onwards

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should go clean on all the branches affected.

How likely is this patch to cause regressions; how much testing does it need? The only risk is a leak but initial try runs look good.
Attachment #8784250 - Flags: sec-approval?
Thanks for fixing this so quickly!
sec-approval+ for trunk. Please nominate patches for Aurora and Beta as well. We need to get all of these in this week.
Attachment #8784250 - Flags: sec-approval? → sec-approval+
Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

Approval Request Comment
[Feature/regressing bug #]: Track cloning

[User impact if declined]: Security issue would be fixed on nightly, not beta/aurora.  May be very hard to exploit, even if you know where it is, since it's being released during shutdown.

[Describe test coverage new/current, TreeHerder]: Was found by Honza on a Try push I believe due to a new assertion.

[Risks and why]: Low risk - basically converting a weak ref to a strong one.  Biggest risk would be a leak.

[String/UUID change made/needed]: none
Attachment #8784250 - Flags: approval-mozilla-beta?
Attachment #8784250 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/521e476f0c71
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

Fix for sec-critical issue, let's uplift this for beta 8.
Attachment #8784250 - Flags: approval-mozilla-beta?
Attachment #8784250 - Flags: approval-mozilla-beta+
Attachment #8784250 - Flags: approval-mozilla-aurora?
Attachment #8784250 - Flags: approval-mozilla-aurora+
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main49+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.