Closed Bug 879999 Opened 6 years ago Closed 6 years ago
crash [@ fsmdef
_ev _setremotedesc] Fix null deref in Set Remote Description
2.33 KB, patch
|Details | Diff | Splinter Review|
2.37 KB, patch
|Details | Diff | Splinter Review|
1.42 KB, text/plain
2.58 KB, text/html
There are code paths that allow us to try to use the dcb before it is set up (in fsmdef_ev_setremotedesc). This was introduced in Bug 837523 (landed in FFx22), and is fixed by the "Part 1" patch in Bug 784591, which landed here: https://hg.mozilla.org/integration/mozilla-inbound/rev/c115f48ef9e4 https://hg.mozilla.org/mozilla-central/rev/c115f48ef9e4 Presently, the URL http://www.sharefest.me/stan17 predicably elicits the crash. This is an ephemeral URL; however, most attempts to receive files from sharefest.me with FFx22 are likely to have the same issue.
I'll take a review from whomever can get to it first.
Attachment #758818 - Flags: review?(rjesup) → review+
Comment on attachment 758818 [details] [diff] [review] Move dcb null check above first use (Beta) [Approval Request Comment] >Bug caused by (feature/regressing bug #): Bug 837523 >User impact if declined: Browser crashes on certain kinds of WebRTC failures >Testing completed (on m-c, etc.): Run through mochi, crash, unit tests locally. I will push to try when Bug 880025 resolves. >Risk to taking this patch (and alternatives if risky): Extremely low; there is no new code, simply a reordering of a null pointer check versus the first use of said pointer. >String or IDL/UUID changes made by this patch: None
Attachment #758818 - Flags: approval-mozilla-beta?
>This needs to land before we set the flag. Take a look at the top comment, this is fixed in Nightly right now by bug 784591.
(In reply to Jason Smith [:jsmith] from comment #3) > This needs to land before we set the flag. A fix did land. If you hit the URL in Comment 0, it will crash Beta (22) and Aurora (23), but not Nightly (24).
Attachment #758818 - Attachment description: Move dcb null check above first use → Move dcb null check above first use (Beta)
Comment on attachment 758853 [details] [diff] [review] Move dcb null check above first use (Aurora) Jesup: Same change as before, but this patch applies cleanly to Aurora (there was some minor code reformatting between the releases).
Attachment #758853 - Flags: review?(rjesup) → review+
Comment on attachment 758853 [details] [diff] [review] Move dcb null check above first use (Aurora) See comment 2.
Attachment #758853 - Flags: approval-mozilla-aurora?
Bug 880139 shows this is getting hit in the field.
Actually should be blocking after now seeing crash stats. This is pretty evident all over crash stats on beta.
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc+]
Crashes probably mostly come from one site that got linked to on HackerNews that crashes beta every time (and was used to transfer 30GB+ yesterday without any going through the servers). Some might be from other tested-with-Chrome-but-not-FF sites, of course. This is an important fix, though most sites will never cause it to happen, so the impact is more limited than the crash stats would imply. So while a critical fix, I'm not sure I'd block the feature on it, especially if it's mostly caused by one site (which we could get them to change). Jason - what will be needed to be done to get this verified for Beta, as we're now past where they'll take speculative fixes?
Severity: normal → critical
Summary: Fix potential null deref in SetRemoteDescription → Fix null deref in SetRemoteDescription
I'm probably going to need URLs here. Then, we can figure out which URL can reproduce the crash on a build that still generates the crash. Next, I can test the fix on the URLs confirmed to have the crash.
fair enough, I'll move it back to non-blocking then
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc-]
Tracy - Can you add the URLs for the crashes to this bug?
There are only 10 crashes with this signature in crash-stats (6 Mac, 4 linux). Here are the few URL's associated with this crash: 5 http://www.sharefest.me/stan17 1 https://en.wikipedia.org/wiki/DVD-Video 1 https://mail.google.com/mail/u/0/?ui=2&shva=1#inbox
Summary: Fix null deref in SetRemoteDescription → crash [@ fsmdef_ev_setremotedesc] Fix null deref in SetRemoteDescription
Bingo. http://www.sharefest.me/stan17 crashes for me immediately upon accessing it.
As I noted in Comment 0, the http://www.sharefest.me/stan17 is potentially quite ephemeral, so I'm putting notes in here in case we need an independent reproduction. The overall flow to cause this problem is: 1) Creating a PC 2) Call setRemoteDescription with problematic SDP 3) Call setRemoteDescription *again* (contents of SDP don't matter the second time). I've attached an example of the SDP that can be used to trigger this behavior.
I've attached a simplified test case that elicits the failure, to prevent the need to rely on external web sites.
Attachment #758853 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #758818 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 Unable to crash Firefox 22 beta 5 (buildID: 20130612084701) and latest Aurora (buildID: 20130612004016) with the test case provided at comment 21 and used the links from comment 18 as well so it`s verified fixed.
Ups! Sorry for changing the status of the bug to Resolved Fixed it was by mistake. Can anyone change the status back to New if that is the case? Thanks and sorry again.
It looks to me likes this is fixed everywhere it it supposed to be. so resolution is correct. changing to verified per comment 24
(In reply to Tracy Walker [:tracy] from comment #26) > It looks to me likes this is fixed everywhere it it supposed to be. so > resolution is correct. changing to verified per comment 24 That's right. We don't need a patch here on central. This is only needed for Aurora & Beta.
You need to log in before you can comment on or make changes to this bug.