1280215 - null pointer crashes in PeerConnectionMedia::EnsureIceGathering_s

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Byron Campen [:bwc]]

From crash-stats:

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3APeerConnectionMedia%3A%3AEnsureIceGathering_s&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

This suggests to me that the code that cancels the proxy lookup when the PeerConnectionMedia begins tearing down isn't working reliably. We don't seem to have any asserts that would catch this.

Marking as security for now, since a flaw in the proxy lookup cancellation code could imply security bugs elsewhere, and could maybe cause a dangling pointer to a PCM to be used (which is not what is happening here).

Comment 1

[Byron Campen [:bwc]]

Attachment: Assert that PCM is not being torn down in SetProxyOnPcm

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8762841 [details] [diff] [review]
Assert that PCM is not being torn down in SetProxyOnPcm

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

This is a diagnostic patch. Since this is a fairly rare crash, it may be some time before we have confirmation that this is indeed the problem. If this is the problem, we'll have a fun bug to fix...

Comment 3

[Byron Campen [:bwc]]

Attachment: Assert that PCM is not being torn down in OnProxyAvailable

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8762844 [details] [diff] [review]
Assert that PCM is not being torn down in OnProxyAvailable

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

LGTM

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f0bab9fbcaf

Comment 6

[Byron Campen [:bwc]]

Welp, upon reading the code in nsAsyncResolveRequest, calling Cancel(NS_ERROR_ABORT) is no guarantee that the callback will come back with NS_ERROR_ABORT. So, we need to check something on the PCM to see whether it wants a callback anymore.

Comment 7

[Byron Campen [:bwc]]

Fortunately, we won't UAF since the callback handler holds onto a RefPtr<PeerConnectionMedia>.

Comment 8

[Byron Campen [:bwc]]

Attachment: Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6f10ee4b0e

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8762877 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

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

Might be worth addressing my one suggestion below. But otherwise LGTM.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +210,2 @@
>      return NS_OK;
>    }

Looking at nsHttpChannel and nsFtpConnectionThread we should probably add here a reset like this
  pcm_->mProxyRequest = nullptr;
here to be on the safe side.

Comment 11

[Byron Campen [:bwc]]

Attachment: Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

MozReview-Commit-ID: AIZm4VNZJtV

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f4bc17a195

Comment 13

[Byron Campen [:bwc]]

Can someone mark this as non-security sensitive?

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

Approval Request Comment
[Feature/regressing bug #]:

   Bug 949703

[User impact if declined]:

   Occasional null pointer crashes when a peerconnection is closed soon after creation.

[Describe test coverage new/current, TreeHerder]:

   We have some tests that destroy peerconnections soon after they're created, but the timing is sensitive enough that this would be really hard to reproduce.

[Risks and why]: 

   This is a very safe patch, I don't see any risk.

[String/UUID change made/needed]:

   None.

Comment 15

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/50f2e8c8491e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/50f2e8c8491e

Comment 17

[Sylvestre Ledru [:Sylvestre]]

We shipped several versions with this issue. Why this didn't get a sec approval?

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

As it landed already in m-c, taking it in the other branches.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6b52e21758a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc0749421398

Comment 21

[Byron Campen [:bwc]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> We shipped several versions with this issue. Why this didn't get a sec
> approval?

Because it is just a null ptr crash. (see comment 13)