843595 - WebRTC crash [@nr_ice_candidate_pair_destroy]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

This crash happened while fuzzing the SDP and seems to be a null ptr crash. ekr and I have decided to hide that bug first because of the free() function involved. The callstack contains also the output of NSPR_LOG.


media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c:715

int nr_ice_candidate_pair_destroy(nr_ice_cand_pair **pairp)
  {
    nr_ice_cand_pair *pair;

    if(!pairp || !*pairp)
      return(0);

    pair=*pairp;
    *pairp=0;

    RFREE(pair->as_string);
    RFREE(pair->foundation);
    nr_ice_socket_deregister(pair->local->isock,pair->stun_client_handle);
    RFREE(pair->stun_client->params.ice_binding_request.username); <--

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

The problem seems to be a missing a=ice-ufrag attribute.

Tested with m-i changeset: 122510:4e6834c859a8

Comment 2

[Adam Roach [:abr]]

Attachment: Null checks on pair dtor

Comment 3

[Adam Roach [:abr]]

I did a quick audit of all the object dtors to look for any similar issues. I found nothing glaring, but I added some paranoia checks in a couple of other places.

Comment 4

[Adam Roach [:abr]]

Attachment: Null checks on pair dtor

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 717916 [details] [diff] [review]
Null checks on pair dtor

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

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1990,5 @@
> +  a2_.CreateAnswer(constraints, offer, OFFER_AV | ANSWER_AV);
> +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);
> +  a1_.SetRemote(TestObserver::ANSWER, a2_.answer(), true);
> +  // We don't check anything in particular for success here -- simply not
> +  // crashing by now is enough to declare success.

Don't you need a wait here to give the thread time to run?

Comment 6

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> Comment on attachment 717916 [details] [diff] [review]
> Null checks on pair dtor
> 
> Review of attachment 717916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +1990,5 @@
> > +  a2_.CreateAnswer(constraints, offer, OFFER_AV | ANSWER_AV);
> > +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);
> > +  a1_.SetRemote(TestObserver::ANSWER, a2_.answer(), true);
> > +  // We don't check anything in particular for success here -- simply not
> > +  // crashing by now is enough to declare success.
> 
> Don't you need a wait here to give the thread time to run?

SetRemote() waits for a response (success or error) before it returns -- all the new "true" parameter suppresses is checking that the response was an error; it still waits for the response to happen.

Comment 7

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77269eb211df

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77269eb211df