Closed Bug 892161 Opened 6 years ago Closed 6 years ago

Fail SetRemote() when you don't have ice ufrag/passwd

Categories

(Core :: WebRTC: Signaling, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: ekr, Assigned: ehugg)

Details

Attachments

(1 file, 4 obsolete files)

SetRemote() should fail immediately when you have no ICE ufrag/passwd.

Right now, we fail when we try to enter the connected state, i.e., 
with Set*(ANSWER).
Assignee: nobody → ethanhugg
Attachment #776757 - Attachment is obsolete: true
Comment on attachment 777197 [details] [diff] [review]
SetRemoteDescription should fail if peer gives no ICE info

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

Added a check for ICE ufrag and pwd after the SDP is parsed, but before SetRemoteDescription returns.  It should call the error callback now if either ufrag or pwd are missing from the SDP.
Attachment #777197 - Flags: review?(adam)
Attachment #777197 - Attachment is obsolete: true
Attachment #777197 - Flags: review?(adam)
Comment on attachment 777367 [details] [diff] [review]
SetRemoteDescription should fail if peer gives no ICE info


Fixed a typo in the .h causing a warning.
Attachment #777367 - Flags: review?(adam)
Comment on attachment 777367 [details] [diff] [review]
SetRemoteDescription should fail if peer gives no ICE info

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

r- for removing critical state checks from signaling_unittests.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3719,5 @@
> +            ui_set_remote_description(evSetRemoteDescError, fcb->state, line,
> +            call_id, dcb->caller_id.call_instance_id, strlib_empty(),
> +              PC_INTERNAL_ERROR, "ICE attributes missing; cause = %s",
> +              cc_cause_name(cause));
> +            return (fsmdef_release(fcb, cause, FALSE));

I think releasing the FCB here is a bit of an overreaction -- while the setRemoteDescription call needs to fail (you've taken care of that), I think the overall PeerConnection should remain in a salvageable state. I suggest that the return should be:

  return (SM_RC_END);

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +6533,5 @@
> +/*
> + * gsmsdp_check_peer_ice_attributes_exist
> + *
> + * Read ICE parameters from the SDP and return failure
> + * if they are not complete.

This code doesn't handle the allowable (if odd) circumstances in which one of the attributes is provided at session level and the other is provided at media level. It's probably not terribly *important* -- but since it's easy to fix and it's allowed, I would be inclined to handle it properly.

@@ +6539,5 @@
> + * fcb_p - pointer to the fcb
> + *
> + */
> +cc_causes_t
> +gsmsdp_check_ice_attributes_exist(fsm_fcb_t *fcb_p) {

Although not rigorously applied, the local convention in this file appears to be a four-space indent.

@@ +6550,5 @@
> +  /* Check for valid ICE parameters */
> +  sdp_res = sdp_attr_get_ice_attribute(dcb_p->sdp->dest_sdp,
> +    SDP_SESSION_LEVEL, 0, SDP_ATTR_ICE_UFRAG, 1, &ufrag);
> +  if (sdp_res != SDP_SUCCESS)
> +    ufrag = NULL;

Braces around this statement

@@ +6555,5 @@
> +
> +  sdp_res = sdp_attr_get_ice_attribute(dcb_p->sdp->dest_sdp,
> +    SDP_SESSION_LEVEL, 0, SDP_ATTR_ICE_PWD, 1, &pwd);
> +  if (sdp_res != SDP_SUCCESS)
> +    pwd = NULL;

Braces around this statement

@@ +6565,5 @@
> +
> +  /* No ICE params at session level, check all media levels */
> +  GSMSDP_FOR_ALL_MEDIA(media, dcb_p) {
> +    if (!GSMSDP_MEDIA_ENABLED(media))
> +      continue;

Braces

@@ +6570,5 @@
> +
> +    sdp_res = sdp_attr_get_ice_attribute(dcb_p->sdp->dest_sdp,
> +      media->level, 0, SDP_ATTR_ICE_UFRAG, 1, &ufrag);
> +    if (sdp_res != SDP_SUCCESS)
> +      ufrag = NULL;

Braces

@@ +6575,5 @@
> +
> +    sdp_res = sdp_attr_get_ice_attribute(dcb_p->sdp->dest_sdp,
> +      media->level, 0, SDP_ATTR_ICE_PWD, 1, &pwd);
> +    if (sdp_res != SDP_SUCCESS)
> +      pwd = NULL;

Braces

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +820,2 @@
>      if (!ignoreError) {
> +      ASSERT_EQ(signaling_state(), endState);

No, that's not right. There are several test cases in here that set ignoreError, but still rely on proper checking of the passed-in endState.

See, for example, SetRemoteAnswerInHaveRemoteOffer -- it relies on this check to ensure that the ending state is kSignalingHaveRemoteOffer. In particular, we need to catch regressions that would have resulted in a transition to stable or closed, both of which could result from a logic error in SIPCC.

The contract here is that any test that calls SetRemote in a way that results in a non-sunny-day end state must be able to correctly indicate what the expected ending state is.

@@ +2287,1 @@
>    a2_.SetRemote(TestObserver::OFFER, offer, true);

a2_.SetRemote(TestObserver::OFFER, offer, true, sipcc::PeerConnectionImpl::kSignalingStable);
Attachment #777367 - Flags: review?(adam) → review-
Attachment #777367 - Attachment is obsolete: true
Attachment #778589 - Attachment is obsolete: true
Comment on attachment 778635 [details] [diff] [review]
SetRemoteDescription should fail if peer gives no ICE info


This should resolve the review comments.  Pushed to try to double-check the tests: https://tbpl.mozilla.org/?tree=Try&rev=d292d2b7fdb2
Attachment #778635 - Flags: review?(adam)
Comment on attachment 778635 [details] [diff] [review]
SetRemoteDescription should fail if peer gives no ICE info

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

Looks good. One formatting nit, below.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +2283,5 @@
>    // FSM. This may change in the future.
>    a1_.CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);
>    a1_.SetLocal(TestObserver::OFFER, offer, true);
> +  // We now detect the missing ICE parameters at SetRemoteDescription
> +  a2_.SetRemote(TestObserver::OFFER, offer, true, sipcc::PeerConnectionImpl::kSignalingStable);

Wrap to 80 characters, please.
Attachment #778635 - Flags: review?(adam) → review+
https://hg.mozilla.org/mozilla-central/rev/90a56a45da6d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.