Closed
Bug 879999
Opened 11 years ago
Closed 11 years ago
crash [@ fsmdef_ev_setremotedesc] Fix null deref in SetRemoteDescription
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
VERIFIED
FIXED
mozilla22
People
(Reporter: abr, Assigned: abr)
References
()
Details
(Keywords: crash, reproducible, Whiteboard: [WebRTC][blocking-webrtc-])
Crash Data
Attachments
(4 files)
2.33 KB,
patch
|
jesup
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.37 KB,
patch
|
jesup
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.42 KB,
text/plain
|
Details | |
2.58 KB,
text/html
|
Details |
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.
Assignee | ||
Comment 1•11 years ago
|
||
I'll take a review from whomever can get to it first.
Attachment #758818 -
Flags: review?(rjesup)
Attachment #758818 -
Flags: review?(ethanhugg)
Updated•11 years ago
|
Attachment #758818 -
Flags: review?(rjesup) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #758818 -
Flags: review?(ethanhugg)
Assignee | ||
Comment 2•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
status-firefox21:
--- → unaffected
status-firefox22:
--- → affected
status-firefox23:
--- → affected
status-firefox24:
--- → fixed
Comment 4•11 years ago
|
||
>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.
Assignee | ||
Comment 6•11 years ago
|
||
(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).
Assignee | ||
Updated•11 years ago
|
Attachment #758818 -
Attachment description: Move dcb null check above first use → Move dcb null check above first use (Beta)
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
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 -
Attachment description: Move dcb null check above first use (aurora) → Move dcb null check above first use (Aurora)
Attachment #758853 -
Flags: review?(rjesup)
Updated•11 years ago
|
Attachment #758853 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 758853 [details] [diff] [review] Move dcb null check above first use (Aurora) See comment 2.
Attachment #758853 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-]
Comment 12•11 years ago
|
||
Bug 880139 shows this is getting hit in the field.
Comment 13•11 years ago
|
||
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+]
Comment 14•11 years ago
|
||
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
Flags: needinfo?(jsmith)
Keywords: crash
Summary: Fix potential null deref in SetRemoteDescription → Fix null deref in SetRemoteDescription
Assignee | ||
Updated•11 years ago
|
Crash Signature: [@ fsmdef_ev_setremotedesc ]
Comment 15•11 years ago
|
||
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.
Flags: needinfo?(jsmith)
Keywords: needURLs
Comment 16•11 years ago
|
||
fair enough, I'll move it back to non-blocking then
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc-]
Comment 17•11 years ago
|
||
Tracy - Can you add the URLs for the crashes to this bug?
Flags: needinfo?(twalker)
Comment 18•11 years ago
|
||
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
Flags: needinfo?(twalker)
Summary: Fix null deref in SetRemoteDescription → crash [@ fsmdef_ev_setremotedesc] Fix null deref in SetRemoteDescription
Comment 19•11 years ago
|
||
Bingo. http://www.sharefest.me/stan17 crashes for me immediately upon accessing it.
Keywords: needURLs → reproducible
Updated•11 years ago
|
Assignee | ||
Comment 20•11 years ago
|
||
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.
Updated•11 years ago
|
Assignee | ||
Comment 21•11 years ago
|
||
I've attached a simplified test case that elicits the failure, to prevent the need to rely on external web sites.
Updated•11 years ago
|
Attachment #758853 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #758818 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/50aa2dd3b0bd
Target Milestone: --- → mozilla22
Comment 24•11 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 25•11 years ago
|
||
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.
Comment 26•11 years ago
|
||
It looks to me likes this is fixed everywhere it it supposed to be. so resolution is correct. changing to verified per comment 24
Status: RESOLVED → VERIFIED
Comment 27•11 years ago
|
||
(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.
Description
•