commit 9df10fea93b483af6646ef2f7aab35598fbaab2f Author: Nils Ohlmeier [:drno] Date: Thu Nov 6 12:21:57 2014 -0500 Bug 1089207: fix parsing of invalid fmtp att r=drno,jesup a=lmandel Modified media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c diff --git a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c index fa5ca2e..33d26c0 100644 --- a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c +++ b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c @@ -458,7 +458,6 @@ sdp_result_e sdp_parse_attr_fmtp (sdp_t *sdp_p, sdp_attr_t *attr_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; @@ -495,29 +494,11 @@ sdp_result_e sdp_parse_attr_fmtp (sdp_t *sdp_p, sdp_attr_t *attr_p, fmtp_p->packetization_mode = 0; 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) { Modified media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c diff --git a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c index 0be02aa..9760d4e 100644 --- a/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c +++ b/media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c @@ -1002,7 +1002,12 @@ sdp_result_e sdp_parse (sdp_t *sdp_p, char **bufp, u16 len) */ 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); Modified media/webrtc/signaling/test/sdp_unittests.cpp diff --git a/media/webrtc/signaling/test/sdp_unittests.cpp b/media/webrtc/signaling/test/sdp_unittests.cpp index 51df09b..9f98eed 100644 --- a/media/webrtc/signaling/test/sdp_unittests.cpp +++ b/media/webrtc/signaling/test/sdp_unittests.cpp @@ -755,13 +755,13 @@ 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) { @@ -789,6 +789,29 @@ TEST_F(SdpTest, addFmtpMaxFsFr) { 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(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); +} + } // End namespace test. int main(int argc, char **argv) {