Bug 1089207: fix parsing of invalid fmtp att r=drno,jesup a=lmandel
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Thu, 06 Nov 2014 12:21:57 -0500
changeset 225980 f30e1c0c0694
parent 225979 557655b23004
child 225981 8e812440658b
push id4092
push userrjesup@wgate.com
push date2014-11-06 17:22 +0000
treeherdermozilla-beta@f30e1c0c0694 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdrno, jesup, lmandel
bugs1089207
milestone34.0
Bug 1089207: fix parsing of invalid fmtp att r=drno,jesup a=lmandel on a CLOSED TREE
media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c
media/webrtc/signaling/test/sdp_unittests.cpp
--- a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
+++ b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ -453,17 +453,16 @@ sdp_result_e sdp_parse_attr_fmtp (sdp_t 
     sdp_result_e  result1 = SDP_SUCCESS;
     sdp_result_e  result2 = SDP_SUCCESS;
     tinybool      done = FALSE;
     tinybool      codec_info_found = FALSE;
     sdp_fmtp_t   *fmtp_p;
     char          tmp[SDP_MAX_STRING_LEN];
     char          *src_ptr;
     char          *temp_ptr = NULL;
-    tinybool flag=FALSE;
     char         *tok=NULL;
     char         *temp=NULL;
     u16          custom_x=0;
     u16          custom_y=0;
     u16          custom_mpi=0;
     u16          par_height=0;
     u16          par_width=0;
     u16          cpcf=0;
@@ -490,39 +489,21 @@ sdp_result_e sdp_parse_attr_fmtp (sdp_t 
      * set default value of packetization mode and level-asymmetry-allowed. If
      * remote sdp does not specify any value for these two parameters, then the
      * default value will be assumed for remote sdp. If remote sdp does specify
      * any value for these parameters, then default value will be overridden.
     */
     fmtp_p->packetization_mode = SDP_DEFAULT_PACKETIZATION_MODE_VALUE;
     fmtp_p->level_asymmetry_allowed = SDP_DEFAULT_LEVEL_ASYMMETRY_ALLOWED_VALUE;
 
-    /* BEGIN - a typical macro fn to replace '/' with ';' from fmtp line*/
-    /* This ugly replacement of '/' with ';' is only done because
-    *  econf/MS client sends in this wierd /illegal format.
-    * fmtp parameters MUST be  separated by ';'
-    */
     temp_ptr = cpr_strdup(ptr);
     if (temp_ptr == NULL) {
         return (SDP_FAILURE);
     }
     fmtp_ptr = src_ptr = temp_ptr;
-    while (flag == FALSE) {
-        if (*src_ptr == '\n') {
-            flag = TRUE;
-            break;
-        }
-        if (*src_ptr == '/') {
-            *src_ptr =';' ;
-        }
-        src_ptr++;
-    }
-    /* END */
-    /* Once we move to RFC compliant video codec implementations, the above
-    *  patch should be removed */
 
     src_ptr = temp_ptr;
     while (!done) {
       fmtp_ptr = sdp_getnextstrtok(fmtp_ptr, tmp, sizeof(tmp), "= \t", &result1);
       if (result1 == SDP_SUCCESS) {
         if (cpr_strncasecmp(tmp, sdp_fmtp_codec_param[1].name,
 	                sdp_fmtp_codec_param[1].strlen) == 0) {
 	    fmtp_ptr = sdp_getnextstrtok(fmtp_ptr, tmp, sizeof(tmp), "; \t", &result1);
--- a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c
+++ b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c
@@ -999,17 +999,22 @@ sdp_result_e sdp_parse (sdp_t *sdp_p, ch
      * we find a parsing error.
      */
     while (!end_found) {
         /* If the last char of this line goes beyond the end of the buffer,
          * we don't parse it.
          */
         ptr = next_ptr;
         line_end = sdp_findchar(ptr, "\n");
-        if (line_end >= (*bufp + len)) {
+        if ((line_end >= (*bufp + len)) ||
+           (*line_end == '\0')) {
+            /* As this does not update the result value the SDP up to this point
+             * is still accept as valid. So encountering this is not treated as
+             * an error.
+             */
             sdp_parse_error(sdp_p->peerconnection,
                 "%s End of line beyond end of buffer.",
                 sdp_p->debug_str);
             CSFLogError(logTag, "SDP: Invalid SDP, no \\n (len %u): %*s", len, len, *bufp);
             end_found = TRUE;
             break;
         }
 
--- a/media/webrtc/signaling/test/sdp_unittests.cpp
+++ b/media/webrtc/signaling/test/sdp_unittests.cpp
@@ -750,23 +750,23 @@ TEST_F(SdpTest, parseExtMap) {
             1);
 
 }
 
 TEST_F(SdpTest, parseFmtpMaxFs) {
   u32 val = 0;
   ParseSdp(kVideoSdp + "a=fmtp:120 max-fs=300;max-fr=30\r\n");
   ASSERT_EQ(sdp_attr_get_fmtp_max_fs(sdp_ptr_, 1, 0, 1, &val), SDP_SUCCESS);
-  ASSERT_EQ(val, 300);
+  ASSERT_EQ(val, 300U);
 }
 TEST_F(SdpTest, parseFmtpMaxFr) {
   u32 val = 0;
   ParseSdp(kVideoSdp + "a=fmtp:120 max-fs=300;max-fr=30\r\n");
   ASSERT_EQ(sdp_attr_get_fmtp_max_fr(sdp_ptr_, 1, 0, 1, &val), SDP_SUCCESS);
-  ASSERT_EQ(val, 30);
+  ASSERT_EQ(val, 30U);
 }
 
 TEST_F(SdpTest, addFmtpMaxFs) {
   InitLocalSdp();
   int level = AddNewMedia(SDP_MEDIA_VIDEO);
   AddNewFmtpMaxFs(level, 300);
   std::string body = SerializeSdp();
   ASSERT_NE(body.find("a=fmtp:120 max-fs=300\r\n"), std::string::npos);
@@ -784,16 +784,39 @@ TEST_F(SdpTest, addFmtpMaxFsFr) {
   InitLocalSdp();
   int level = AddNewMedia(SDP_MEDIA_VIDEO);
   AddNewFmtpMaxFsFr(level, 300, 30);
   std::string body = SerializeSdp();
   ASSERT_NE(body.find("a=fmtp:120 max-fs=300;max-fr=30\r\n"),
             std::string::npos);
 }
 
+static const std::string kBrokenFmtp =
+  "v=0\r\n"
+  "o=- 137331303 2 IN IP4 127.0.0.1\r\n"
+  "s=SIP Call\r\n"
+  "t=0 0\r\n"
+  "m=video 56436 RTP/SAVPF 120\r\n"
+  "c=IN IP4 198.51.100.7\r\n"
+  "a=rtpmap:120 VP8/90000\r\n"
+  /* Note: the \0 in this string triggered bz://1089207
+   */
+  "a=fmtp:120 max-fs=300;max\0fr=30";
+
+TEST_F(SdpTest, parseBrokenFmtp) {
+  u32 val = 0;
+  char *buf = const_cast<char *>(kBrokenFmtp.data());
+  ResetSdp();
+  /* We need to manually invoke the parser here to be able to specify the length
+   * of the string beyond the \0 in last line of the string.
+   */
+  ASSERT_EQ(sdp_parse(sdp_ptr_, &buf, 165), SDP_SUCCESS);
+  ASSERT_EQ(sdp_attr_get_fmtp_max_fs(sdp_ptr_, 1, 0, 1, &val), SDP_INVALID_PARAMETER);
+}
+
 TEST_F(SdpTest, addIceLite) {
     InitLocalSdp();
     u16 inst_num = 0;
     EXPECT_EQ(sdp_add_new_attr(sdp_ptr_, SDP_SESSION_LEVEL, 0,
                                SDP_ATTR_ICE_LITE, &inst_num), SDP_SUCCESS);
     std::string body = SerializeSdp();
     ASSERT_NE(body.find("a=ice-lite\r\n"), std::string::npos);
 }