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

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

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

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: SetRemoteDescription should fail if peer gives no ICE info

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: SetRemoteDescription should fail if peer gives no ICE info

Comment 3

[Ethan Hugg [:ehugg]]

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.

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: SetRemoteDescription should fail if peer gives no ICE info

Comment 5

[Ethan Hugg [:ehugg]]

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.

Comment 6

[Adam Roach [:abr]]

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

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: SetRemoteDescription should fail if peer gives no ICE info

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: SetRemoteDescription should fail if peer gives no ICE info

Comment 9

[Ethan Hugg [:ehugg]]

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

Comment 10

[Adam Roach [:abr]]

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.

Comment 11

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90a56a45da6d

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90a56a45da6d