Closed Bug 843595 Opened 8 years ago Closed 8 years ago

WebRTC crash [@nr_ice_candidate_pair_destroy]

Categories

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

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- disabled
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: posidron, Assigned: abr)

References

Details

(Keywords: crash, testcase, Whiteboard: [webrtc][blocking-webrtc+][qa-][adv-main22-])

Attachments

(3 files, 1 obsolete file)

Attached file 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); <--
Attached file testcase
The problem seems to be a missing a=ice-ufrag attribute.

Tested with m-i changeset: 122510:4e6834c859a8
Keywords: testcase
Flags: in-testsuite?
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift]
Assignee: nobody → adam
Priority: -- → P1
Attached patch Null checks on pair dtor (obsolete) — Splinter Review
Attachment #717906 - Flags: review?(ekr)
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.
Status: NEW → ASSIGNED
Attachment #717906 - Attachment is obsolete: true
Attachment #717906 - Flags: review?(ekr)
Attachment #717906 - Flags: review?(ekr)
Attachment #717916 - Flags: review?(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?
Attachment #717916 - Flags: review?(ekr) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/77269eb211df
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+][webrtc-uplift][qa-]
Attachment #717906 - Flags: review?(ekr)
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift][qa-] → [webrtc][blocking-webrtc+][qa-]
Whiteboard: [webrtc][blocking-webrtc+][qa-] → [webrtc][blocking-webrtc+][qa-][adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.