crash [@ fsmdef_ev_setremotedesc] Fix null deref in SetRemoteDescription

VERIFIED FIXED in Firefox 22

Status

()

--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: abr, Assigned: abr)

Tracking

({crash, reproducible})

22 Branch
mozilla22
crash, reproducible
Points:
---

Firefox Tracking Flags

(firefox21 disabled, firefox22+ verified, firefox23+ verified)

Details

(Whiteboard: [WebRTC][blocking-webrtc-], crash signature, URL)

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 758818 [details] [diff] [review]
Move dcb null check above first use (Beta)

I'll take a review from whomever can get to it first.
Attachment #758818 - Flags: review?(rjesup)
Attachment #758818 - Flags: review?(ethanhugg)
Attachment #758818 - Flags: review?(rjesup) → review+
(Assignee)

Updated

6 years ago
Attachment #758818 - Flags: review?(ethanhugg)
(Assignee)

Comment 2

6 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

6 years ago
status-firefox21: --- → unaffected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → fixed
This needs to land before we set the flag.
status-firefox24: fixed → affected
>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

6 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

6 years ago
Attachment #758818 - Attachment description: Move dcb null check above first use → Move dcb null check above first use (Beta)
(Assignee)

Comment 7

6 years ago
Created attachment 758853 [details] [diff] [review]
Move dcb null check above first use (Aurora)
In that case we should set unaffected to 24 then.
status-firefox24: affected → unaffected
(Assignee)

Comment 9

6 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)
Attachment #758853 - Flags: review?(rjesup) → review+
(Assignee)

Comment 10

6 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?
Whiteboard: [WebRTC][blocking-webrtc-]
Duplicate of this bug: 880139
Bug 880139 shows this is getting hit in the field.
status-firefox21: unaffected → disabled
status-firefox24: unaffected → ---
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
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
Flags: needinfo?(jsmith)
Keywords: crash
Summary: Fix potential null deref in SetRemoteDescription → Fix null deref in SetRemoteDescription
(Assignee)

Updated

6 years ago
Crash Signature: [@ fsmdef_ev_setremotedesc ]
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
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?
Flags: needinfo?(twalker)
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
Bingo. http://www.sharefest.me/stan17 crashes for me immediately upon accessing it.
Keywords: needURLs → reproducible
(Assignee)

Comment 20

6 years ago
Created attachment 759137 [details]
Unparsable SDP

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.
tracking-firefox22: ? → +
tracking-firefox23: ? → +
(Assignee)

Comment 21

6 years ago
Created attachment 759779 [details]
Simplified test case

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+
https://hg.mozilla.org/releases/mozilla-beta/rev/50aa2dd3b0bd
status-firefox22: affected → fixed
Target Milestone: --- → mozilla22
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
Last Resolved: 6 years ago
status-firefox22: fixed → verified
Resolution: --- → 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
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
(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.