879999 - crash [@ fsmdef_ev_setremotedesc] Fix null deref in SetRemoteDescription

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

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.

Comment 1

[Adam Roach [:abr]]

Attachment: Move dcb null check above first use (Beta)

I'll take a review from whomever can get to it first.

Comment 2

[Adam Roach [:abr]]

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

Comment 3

[Jason Smith [:jsmith]]

This needs to land before we set the flag.

Comment 4

[Ethan Hugg [:ehugg]]

>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.

Comment 5

[Adam Roach [:abr]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=bd6fd8973dff

Comment 6

[Adam Roach [:abr]]

(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).

Comment 7

[Adam Roach [:abr]]

Attachment: Move dcb null check above first use (Aurora)

Comment 8

[Jason Smith [:jsmith]]

In that case we should set unaffected to 24 then.

Comment 9

[Adam Roach [:abr]]

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).

Comment 10

[Adam Roach [:abr]]

Comment on attachment 758853 [details] [diff] [review]
Move dcb null check above first use (Aurora)

See comment 2.

Comment 12

[Maire Reavy [:mreavy]]

Bug 880139 shows this is getting hit in the field.

Comment 13

[Jason Smith [:jsmith]]

Actually should be blocking after now seeing crash stats. This is pretty evident all over crash stats on beta.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 15

[Jason Smith [:jsmith]]

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.

Comment 16

[Jason Smith [:jsmith]]

fair enough, I'll move it back to non-blocking then

Comment 17

[Jason Smith [:jsmith]]

Tracy - Can you add the URLs for the crashes to this bug?

Comment 18

[Tracy Walker [:tracy]]

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

Comment 19

[Jason Smith [:jsmith]]

Bingo. http://www.sharefest.me/stan17 crashes for me immediately upon accessing it.

Comment 20

[Adam Roach [:abr]]

Attachment: 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.

Comment 21

[Adam Roach [:abr]]

Attachment: Simplified test case

I've attached a simplified test case that elicits the failure, to prevent the need to rely on external web sites.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/50aa2dd3b0bd

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/443b1a5821a2

Comment 24

[Bogdan Maris, Desktop Test Engineering]

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.

Comment 25

[Bogdan Maris, Desktop Test Engineering]

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

[Tracy Walker [:tracy]]

It looks to me likes this is fixed everywhere it it supposed to be.  so resolution is correct. changing to verified per comment 24

Comment 27

[Jason Smith [:jsmith]]

(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.