Bug 892161 - SetRemoteDescription should fail if peer gives no ICE info r=abr
authorEthan Hugg <ethanhugg@gmail.com>
Fri, 19 Jul 2013 12:46:09 -0700
changeset 151631 90a56a45da6d0eec8546498fdd3f2142b64c30d6
parent 151630 244ff7612e87b252b4e3594afa34a94910382a5a
child 151632 05fd9daf14929a777c2a234091c54d43b525b720
push id2859
push userakeybl@mozilla.com
push dateMon, 16 Sep 2013 19:14:59 +0000
treeherdermozilla-beta@87d3c51cd2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersabr
bugs892161
milestone25.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 892161 - SetRemoteDescription should fail if peer gives no ICE info r=abr
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h
media/webrtc/signaling/test/signaling_unittests.cpp
--- a/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
+++ b/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ -3707,16 +3707,27 @@ fsmdef_ev_setremotedesc(sm_event_t *even
         if (cause != CC_CAUSE_OK) {
             ui_set_remote_description(evSetRemoteDescError, fcb->state, line,
             call_id, dcb->caller_id.call_instance_id, strlib_empty(),
               PC_INTERNAL_ERROR, "Could not negotiate media lines; cause = %s",
               cc_cause_name(cause));
             return (fsmdef_release(fcb, cause, FALSE));
         }
 
+        /* Now that the SDP is digested we need to sanity check
+           for ICE parameters */
+        cause = gsmsdp_check_ice_attributes_exist(fcb);
+        if (cause != CC_CAUSE_OK) {
+            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 (SM_RC_END);
+        }
+
         gsmsdp_clean_media_list(dcb);
 
         fsm_change_state(fcb, __LINE__, FSMDEF_S_HAVE_REMOTE_OFFER);
         break;
 
     case JSEP_ANSWER:
         if (fcb->state != FSMDEF_S_HAVE_LOCAL_OFFER &&
             fcb->state != FSMDEF_S_HAVE_REMOTE_PRANSWER) {
--- a/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
+++ b/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ -6524,16 +6524,86 @@ gsmsdp_process_offer_sdp (fsm_fcb_t *fcb
         /* Note that there should not a previous version here as well */
     }
 
     gsmsdp_set_remote_sdp(dcb_p, dcb_p->sdp);
 
     return (status);
 }
 
+
+/*
+ * gsmsdp_check_peer_ice_attributes_exist
+ *
+ * Read ICE parameters from the SDP and return failure
+ * if they are not complete.
+ *
+ * fcb_p - pointer to the fcb
+ *
+ */
+cc_causes_t
+gsmsdp_check_ice_attributes_exist(fsm_fcb_t *fcb_p) {
+    fsmdef_dcb_t    *dcb_p = fcb_p->dcb;
+    sdp_result_e     sdp_res;
+    char            *ufrag;
+    char            *pwd;
+    fsmdef_media_t  *media;
+    boolean          has_session_ufrag = FALSE;
+    boolean          has_session_pwd = FALSE;
+
+    /* 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) {
+        has_session_ufrag = TRUE;
+    }
+
+    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) {
+        has_session_pwd = TRUE;
+    }
+
+    if (has_session_ufrag && has_session_pwd) {
+        /* Both exist at session level, success */
+        return CC_CAUSE_OK;
+    }
+
+    /* Incomplete ICE params at session level, check all media levels */
+    GSMSDP_FOR_ALL_MEDIA(media, dcb_p) {
+        if (!GSMSDP_MEDIA_ENABLED(media)) {
+            continue;
+        }
+
+        if (!has_session_ufrag) {
+            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) {
+                GSM_ERR_MSG(GSM_L_C_F_PREFIX"missing ICE ufrag parameter.",
+                            dcb_p->line, dcb_p->call_id, __FUNCTION__);
+                return CC_CAUSE_ERROR;
+            }
+        }
+
+        if (!has_session_pwd) {
+            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) {
+                GSM_ERR_MSG(GSM_L_C_F_PREFIX"missing ICE pwd parameter.",
+                            dcb_p->line, dcb_p->call_id, __FUNCTION__);
+                return CC_CAUSE_ERROR;
+            }
+        }
+    }
+
+    return CC_CAUSE_OK;
+}
+
 /*
  * gsmsdp_install_peer_ice_attributes
  *
  * Read ICE parameters from the SDP and set them into
  * the ice engine. Check SESSION_LEVEL first then each media line.
  *
  * fcb_p - pointer to the fcb
  *
--- a/media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h
+++ b/media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h
@@ -124,16 +124,17 @@ extern void gsmsdp_clean_media_list(fsmd
 extern fsmdef_media_t *gsmsdp_find_media_by_refid(fsmdef_dcb_t *dcb_p,
                                                   media_refid_t refid);
 extern boolean gsmsdp_handle_media_cap_change(fsmdef_dcb_t *dcb_p,
                                               boolean refresh, boolean hold);
 extern boolean gsmsdp_update_local_sdp_media_capability(fsmdef_dcb_t *dcb_p,
                                               boolean refresh, boolean hold);
 boolean is_gsmsdp_media_ip_updated_to_latest( fsmdef_dcb_t * dcb );
 
+cc_causes_t gsmsdp_check_ice_attributes_exist(fsm_fcb_t *fcb_p);
 cc_causes_t gsmsdp_install_peer_ice_attributes(fsm_fcb_t *fcb_p);
 cc_causes_t gsmsdp_configure_dtls_data_attributes(fsm_fcb_t *fcb_p);
 cc_causes_t gsmsdp_find_level_from_mid(fsmdef_dcb_t * dcb, const char * mid, uint16_t *level);
 void gsmsdp_process_cap_constraints(fsmdef_dcb_t *dcb,
                                     cc_media_constraints_t* constraints);
 cc_causes_t
 gsmsdp_get_offered_media_types (fsm_fcb_t *fcb_p, cc_sdp_t *sdp_p, boolean *has_audio, boolean *has_video, boolean *has_data);
 fsmdef_media_t* gsmsdp_find_media_by_media_type(fsmdef_dcb_t *dcb, sdp_media_e 	media_type);
--- a/media/webrtc/signaling/test/signaling_unittests.cpp
+++ b/media/webrtc/signaling/test/signaling_unittests.cpp
@@ -2278,25 +2278,20 @@ TEST_F(SignalingTest, missingUfrag)
     "a=fmtp:HuRUu]Dtcl\\zM,7(OmEU%O$gU]x/z\tD protocol=webrtc-datachannel;"
       "streams=16\r\n"
     "a=sendrecv\r\n";
 
   // Need to create an offer, since that's currently required by our
   // FSM. This may change in the future.
   a1_.CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);
   a1_.SetLocal(TestObserver::OFFER, offer, true);
-  // Really we should detect failure at the SetRemote point,
-  // since without a ufrag, we aren't going to be successful.
-  // But for now, this isn't detected till SIPCC tries to impose
-  // the parameters on the ICE stack in SetLocal. Bug 892161.
-  a2_.SetRemote(TestObserver::OFFER, offer, true);
-  a2_.CreateAnswer(constraints, offer, OFFER_AV | ANSWER_AV);
-  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(),
-               true, sipcc::PeerConnectionImpl::kSignalingHaveRemoteOffer);
-  // Since things have now failed, just stop.
+  // We now detect the missing ICE parameters at SetRemoteDescription
+  a2_.SetRemote(TestObserver::OFFER, offer, true, 
+    sipcc::PeerConnectionImpl::kSignalingStable);
+  ASSERT_TRUE(a2_.pObserver->state == TestObserver::stateError);
 }
 
 } // End namespace test.
 
 bool is_color_terminal(const char *terminal) {
   if (!terminal) {
     return false;
   }