Disconnect pipeline on STS Thread

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

({sec-moderate})

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(firefox17 disabled, firefox18- disabled, firefox19- disabled, firefox20+ fixed, firefox21 fixed, firefox-esr10 unaffected, firefox-esr17- unaffected, b2g18 disabled)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+] [qa-][adv-main20-])

Attachments

(1 attachment)

No description provided.
Now that we have decided not to use thread-locked sigslot, we need to make sure all sigslot manipulation happens on one thread. This patch moves signal connection/disconnection for mediapipeline onto the STS thread.
Comment on attachment 696519 [details] [diff] [review]
Disconnect pipeline on STS Thread

Review of attachment 696519 [details] [diff] [review]:
-----------------------------------------------------------------

Jesup,

Please review.


Adam,

Please verify that this fixes the issue you were seeing.
Attachment #696519 - Flags: review?(rjesup)
Attachment #696519 - Flags: feedback?(adam)
Group: core-security
Whiteboard: [WebRTC], [blocking-webrtc+]
Comment on attachment 696519 [details] [diff] [review]
Disconnect pipeline on STS Thread

Review of attachment 696519 [details] [diff] [review]:
-----------------------------------------------------------------

r+ modulo explaining or removing the disconnect_all()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +99,5 @@
>  // the pipeline on the main thread.
>  void MediaPipeline::DetachTransport_s() {
>    ASSERT_ON_THREAD(sts_thread_);
>  
> +  disconnect_all();

Is this supposed to be part of this patch, or part of jib's?  Or another one?
Attachment #696519 - Flags: review?(rjesup) → review+
It's part of this patch. I believe JIB is adding disconnect_all() to PeerConnectionMedia.
Comment on attachment 696519 [details] [diff] [review]
Disconnect pipeline on STS Thread

With this patch applied as well as the patches for Bug 824851 and Bug 824359, I have been able to run mochi tests 8 consecutive runs without seeing any crashes. My 9th run did turn up a crash which ekr indicated is likely related to Bug 825477 (which I believe is already landed on m-i).

Given the high frequency with which this bug was turning up before, I think it's safe to say this patch fixes it.
Attachment #696519 - Flags: feedback?(adam) → feedback+
* How easily can the security issue be deduced from the patch?
Not obviously. This is basically a free memory read, so
while it may be exploitable, there is not an immediately
obvious exploit.

* 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 versions of WebRTC (from FF 18 on) but they are preffed
off.


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

This has likely been present since the landing of WebRTC but was masked
by other crash bugs and then made serious by a bunch of changes
that allowed the relevant objects to be cleaned up at all.


* Do you have backports for the affected branches? If not, how different,
  hard to create, and risky will they be?

No. However, this patch should be easily portable to those
branches. Probably low risk but would need testing.


*  How likely is this patch to cause regressions; how much testing does
it need?

Not likely to cause regressions. It's an obvious fix and we
have tested it locally.
Comment on attachment 696519 [details] [diff] [review]
Disconnect pipeline on STS Thread

Review of attachment 696519 [details] [diff] [review]:
-----------------------------------------------------------------

* How easily can the security issue be deduced from the patch?
Not obviously. This is basically a free memory read, so
while it may be exploitable, there is not an immediately
obvious exploit.

* 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 versions of WebRTC (from FF 18 on) but they are preffed
off.


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

This has likely been present since the landing of WebRTC but was masked
by other crash bugs and then made serious by a bunch of changes
that allowed the relevant objects to be cleaned up at all.


* Do you have backports for the affected branches? If not, how different,
  hard to create, and risky will they be?

No. However, this patch should be easily portable to those
branches. Probably low risk but would need testing.


*  How likely is this patch to cause regressions; how much testing does
it need?

Not likely to cause regressions. It's an obvious fix and we
have tested it locally.
Attachment #696519 - Flags: sec-approval?
Attachment #696519 - Flags: sec-approval? → sec-approval+
Not tracking for FF18/19 as webRTC is disabled by default.Please feel free to renominate for FF19 if this is critical & this patch needs to be uplifted.
https://hg.mozilla.org/mozilla-central/rev/ad35aa6a56ca
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+] [qa-]
Whiteboard: [WebRTC], [blocking-webrtc+] [qa-] → [WebRTC], [blocking-webrtc+] [qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.