Bug 1432936: Added sanity check for sendonly attribute. r=bwc draft
authorJohannes Willbold <j.willbold@mozilla.com>
Mon, 04 Jun 2018 10:05:27 -0700
changeset 805041 c5732414f6e199c1e6a85d7d47921ca6ee601550
parent 804982 1ab062fd31db7d4367a479fedb350dc6fcee7a3f
push id112530
push userbmo:johannes.willbold@rub.de
push dateThu, 07 Jun 2018 00:04:03 +0000
reviewersbwc
bugs1432936
milestone62.0a1
Bug 1432936: Added sanity check for sendonly attribute. r=bwc Moved RustSdpAttributeType and renamaed it to SdpAttributeType Replaced the has_extmap_attribute functions by a generic has_attribute function Added a sanity check, that checks for sendonly and if simulcast defines receive options MozReview-Commit-ID: DXAEVu0SRap
media/webrtc/signaling/gtest/sdp_unittests.cpp
media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs
media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
--- a/media/webrtc/signaling/gtest/sdp_unittests.cpp
+++ b/media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ -3815,17 +3815,16 @@ TEST_P(NewSdpTest, ParseInvalidSimulcast
            "a=rtpmap:120 VP8/90000" CRLF
            "a=recvonly" CRLF
            "a=simulcast: send pt=120" CRLF,
            false);
   ASSERT_NE("", GetParseErrors());
 }
 
 TEST_P(NewSdpTest, ParseInvalidSimulcastNotReceiving) {
-  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432936
   ParseSdp("v=0" CRLF
            "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
            "s=SIP Call" CRLF
            "c=IN IP4 198.51.100.7" CRLF
            "b=CT:5000" CRLF
            "t=0 0" CRLF
            "m=video 56436 RTP/SAVPF 120" CRLF
            "a=rtpmap:120 VP8/90000" CRLF
--- a/media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
@@ -575,16 +575,104 @@ impl FromStr for SdpAttribute {
             "ssrc" => parse_ssrc(val),
             _ => {
                 Err(SdpParserInternalError::Unsupported(format!("Unknown attribute type {}", name)))
             }
         }
     }
 }
 
+#[derive(Clone, PartialEq)]
+pub enum SdpAttributeType {
+    BundleOnly,
+    Candidate,
+    EndOfCandidates,
+    Extmap,
+    Fingerprint,
+    Fmtp,
+    Group,
+    IceLite,
+    IceMismatch,
+    IceOptions,
+    IcePwd,
+    IceUfrag,
+    Identity,
+    ImageAttr,
+    Inactive,
+    Label,
+    MaxMessageSize,
+    MaxPtime,
+    Mid,
+    Msid,
+    MsidSemantic,
+    Ptime,
+    Rid,
+    Recvonly,
+    RemoteCandidate,
+    Rtpmap,
+    Rtcp,
+    Rtcpfb,
+    RtcpMux,
+    RtcpRsize,
+    Sctpmap,
+    SctpPort,
+    Sendonly,
+    Sendrecv,
+    Setup,
+    Simulcast,
+    Ssrc,
+    SsrcGroup,
+}
+
+impl<'a> From<&'a SdpAttribute> for SdpAttributeType {
+    fn from(other: &SdpAttribute) -> Self {
+        match *other {
+            SdpAttribute::BundleOnly{..} => SdpAttributeType::BundleOnly,
+            SdpAttribute::Candidate{..} => SdpAttributeType::Candidate,
+            SdpAttribute::EndOfCandidates{..} => SdpAttributeType::EndOfCandidates,
+            SdpAttribute::Extmap{..} => SdpAttributeType::Extmap,
+            SdpAttribute::Fingerprint{..} => SdpAttributeType::Fingerprint,
+            SdpAttribute::Fmtp{..} => SdpAttributeType::Fmtp,
+            SdpAttribute::Group{..} => SdpAttributeType::Group,
+            SdpAttribute::IceLite{..} => SdpAttributeType::IceLite,
+            SdpAttribute::IceMismatch{..} => SdpAttributeType::IceMismatch,
+            SdpAttribute::IceOptions{..} => SdpAttributeType::IceOptions,
+            SdpAttribute::IcePwd{..} => SdpAttributeType::IcePwd,
+            SdpAttribute::IceUfrag{..} => SdpAttributeType::IceUfrag,
+            SdpAttribute::Identity{..} => SdpAttributeType::Identity,
+            SdpAttribute::ImageAttr{..} => SdpAttributeType::ImageAttr,
+            SdpAttribute::Inactive{..} => SdpAttributeType::Inactive,
+            SdpAttribute::Label{..} => SdpAttributeType::Label,
+            SdpAttribute::MaxMessageSize{..} => SdpAttributeType::MaxMessageSize,
+            SdpAttribute::MaxPtime{..} => SdpAttributeType::MaxPtime,
+            SdpAttribute::Mid{..} => SdpAttributeType::Mid,
+            SdpAttribute::Msid{..} => SdpAttributeType::Msid,
+            SdpAttribute::MsidSemantic{..} => SdpAttributeType::MsidSemantic,
+            SdpAttribute::Ptime{..} => SdpAttributeType::Ptime,
+            SdpAttribute::Rid{..} => SdpAttributeType::Rid,
+            SdpAttribute::Recvonly{..} => SdpAttributeType::Recvonly,
+            SdpAttribute::RemoteCandidate{..} => SdpAttributeType::RemoteCandidate,
+            SdpAttribute::Rtcp{..} => SdpAttributeType::Rtcp,
+            SdpAttribute::Rtcpfb{..} => SdpAttributeType::Rtcpfb,
+            SdpAttribute::RtcpMux{..} => SdpAttributeType::RtcpMux,
+            SdpAttribute::RtcpRsize{..} => SdpAttributeType::RtcpRsize,
+            SdpAttribute::Rtpmap{..} => SdpAttributeType::Rtpmap,
+            SdpAttribute::Sctpmap{..} => SdpAttributeType::Sctpmap,
+            SdpAttribute::SctpPort{..} => SdpAttributeType::SctpPort,
+            SdpAttribute::Sendonly{..} => SdpAttributeType::Sendonly,
+            SdpAttribute::Sendrecv{..} => SdpAttributeType::Sendrecv,
+            SdpAttribute::Setup{..} => SdpAttributeType::Setup,
+            SdpAttribute::Simulcast{..} => SdpAttributeType::Simulcast,
+            SdpAttribute::Ssrc{..} => SdpAttributeType::Ssrc,
+            SdpAttribute::SsrcGroup{..} => SdpAttributeType::SsrcGroup
+        }
+    }
+}
+
+
 fn string_or_empty(to_parse: &str) -> Result<String, SdpParserInternalError> {
     if to_parse.is_empty() {
         Err(SdpParserInternalError::Generic("This attribute is required to have a value"
                                                 .to_string()))
     } else {
         Ok(to_parse.to_string())
     }
 }
--- a/media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs
@@ -4,17 +4,17 @@ use std::net::IpAddr;
 use std::fmt;
 
 pub mod attribute_type;
 pub mod error;
 pub mod media_type;
 pub mod network;
 pub mod unsupported_types;
 
-use attribute_type::{SdpAttribute, parse_attribute};
+use attribute_type::{SdpAttribute, SdpAttributeType, parse_attribute};
 use error::{SdpParserInternalError, SdpParserError};
 use media_type::{SdpMedia, SdpMediaLine, parse_media, parse_media_vector};
 use network::{parse_addrtype, parse_nettype, parse_unicast_addr};
 use unsupported_types::{parse_email, parse_information, parse_key, parse_phone, parse_repeat,
                         parse_uri, parse_zone};
 
 #[derive(Clone)]
 pub enum SdpBandwidth {
@@ -155,25 +155,18 @@ impl SdpSession {
     pub fn has_timing(&self) -> bool {
         self.timing.is_some()
     }
 
     pub fn has_attributes(&self) -> bool {
         !self.attribute.is_empty()
     }
 
-    // FIXME this is a temporary hack until we re-oranize the SdpAttribute enum
-    // so that we can build a generic has_attribute(X) function
-    fn has_extmap_attribute(&self) -> bool {
-        for attribute in &self.attribute {
-            if let &SdpAttribute::Extmap(_) = attribute {
-                return true;
-            }
-        }
-        false
+    pub fn get_attribute(&self, t: SdpAttributeType) -> Option<&SdpAttribute> {
+       self.attribute.iter().filter(|a| SdpAttributeType::from(*a) == t).next()
     }
 
     pub fn has_media(&self) -> bool {
         !self.media.is_empty()
     }
 }
 
 fn parse_session(value: &str) -> Result<SdpType, SdpParserInternalError> {
@@ -595,28 +588,41 @@ fn sanity_check_sdp_session(session: &Sd
     if !session.has_media() {
         return Err(SdpParserError::Sequence {
                        message: "Missing media setion".to_string(),
                        line_number: 0,
                    });
     }
 
     // Check that extmaps are not defined on session and media level
-    if session.has_extmap_attribute() {
+    if session.get_attribute(SdpAttributeType::Extmap).is_some() {
         for msection in &session.media {
-            if msection.has_extmap_attribute() {
+            if msection.get_attribute(SdpAttributeType::Extmap).is_some() {
                 return Err(SdpParserError::Sequence {
                                message: "Extmap can't be define at session and media level"
                                    .to_string(),
                                line_number: 0,
                            });
             }
         }
     }
 
+    for msection in &session.media {
+        if msection.get_attribute(SdpAttributeType::Sendonly).is_some() {
+            if let Some(&SdpAttribute::Simulcast(ref x)) = msection.get_attribute(SdpAttributeType::Simulcast) {
+                if x.receive.len() > 0 {
+                    return Err(SdpParserError::Sequence {
+                        message: "Simulcast can't define receive parameters for sendonly".to_string(),
+                        line_number: 0,
+                    });
+                }
+            }
+        }
+    }
+
     Ok(())
 }
 
 #[cfg(test)]
 fn create_dummy_sdp_session() -> SdpSession {
     let origin = parse_origin("mozilla 506705521068071134 0 IN IP4 0.0.0.0");
     assert!(origin.is_ok());
     let sdp_session;
@@ -669,31 +675,31 @@ fn test_sanity_check_sdp_session_extmap(
     let extmap;
     if let SdpType::Attribute(a) = attribute.unwrap() {
         extmap = a;
     } else {
         panic!("SdpType is not Attribute");
     }
     let ret = sdp_session.add_attribute(&extmap);
     assert!(ret.is_ok());
-    assert!(sdp_session.has_extmap_attribute());
+    assert!(sdp_session.get_attribute(SdpAttributeType::Extmap).is_some());
 
     assert!(sanity_check_sdp_session(&sdp_session).is_ok());
 
     let mattribute = parse_attribute("extmap:1/sendonly urn:ietf:params:rtp-hdrext:ssrc-audio-level",);
     assert!(mattribute.is_ok());
     let mextmap;
     if let SdpType::Attribute(ma) = mattribute.unwrap() {
         mextmap = ma;
     } else {
         panic!("SdpType is not Attribute");
     }
     let mut second_media = create_dummy_media_section();
     assert!(second_media.add_attribute(&mextmap).is_ok());
-    assert!(second_media.has_extmap_attribute());
+    assert!(second_media.get_attribute(SdpAttributeType::Extmap).is_some());
 
     sdp_session.extend_media(vec![second_media]);
     assert!(sdp_session.media.len() == 2);
 
     assert!(sanity_check_sdp_session(&sdp_session).is_err());
 
     sdp_session.attribute = Vec::new();
 
--- a/media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
@@ -1,11 +1,11 @@
 use std::fmt;
 use {SdpType, SdpLine, SdpBandwidth, SdpConnection};
-use attribute_type::SdpAttribute;
+use attribute_type::{SdpAttribute, SdpAttributeType};
 use error::{SdpParserError, SdpParserInternalError};
 
 #[derive(Clone)]
 pub struct SdpMediaLine {
     pub media: SdpMediaValue,
     pub port: u32,
     pub port_count: u32,
     pub proto: SdpProtocolValue,
@@ -132,25 +132,18 @@ impl SdpMedia {
     pub fn add_attribute(&mut self, attr: &SdpAttribute) -> Result<(), SdpParserInternalError> {
         if !attr.allowed_at_media_level() {
             return Err(SdpParserInternalError::Generic(format!("{} not allowed at media level",
                                                                attr)));
         }
         Ok(self.attribute.push(attr.clone()))
     }
 
-    // FIXME this is a temporary hack until we re-oranize the SdpAttribute enum
-    // so that we can build a generic has_attribute(X) function
-    pub fn has_extmap_attribute(&self) -> bool {
-        for attribute in &self.attribute {
-            if let &SdpAttribute::Extmap(_) = attribute {
-                return true;
-            }
-        }
-        false
+    pub fn get_attribute(&self, t: SdpAttributeType) -> Option<&SdpAttribute> {
+        self.attribute.iter().filter(|a| SdpAttributeType::from(*a) == t).next()
     }
 
     pub fn has_connection(&self) -> bool {
         self.connection.is_some()
     }
 
     pub fn get_connection(&self) -> &Option<SdpConnection> {
         &self.connection
--- a/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
@@ -1,13 +1,13 @@
 use std::slice;
 use libc::{size_t, uint8_t, uint16_t, uint32_t, int64_t};
 
 use rsdparsa::SdpSession;
-use rsdparsa::attribute_type::{SdpAttribute, SdpAttributePayloadType, SdpAttributeFingerprint, SdpAttributeSetup, SdpAttributeSsrc, SdpAttributeRtpmap, SdpAttributeMsid, SdpAttributeMsidSemantic, SdpAttributeGroupSemantic, SdpAttributeGroup, SdpAttributeRtcp, SdpAttributeRtcpFb, SdpAttributeSctpmap, SdpAttributeRemoteCandidate, SdpAttributeExtmap, SdpAttributeDirection};
+use rsdparsa::attribute_type::{SdpAttribute, SdpAttributeType, SdpAttributePayloadType, SdpAttributeFingerprint, SdpAttributeSetup, SdpAttributeSsrc, SdpAttributeRtpmap, SdpAttributeMsid, SdpAttributeMsidSemantic, SdpAttributeGroupSemantic, SdpAttributeGroup, SdpAttributeRtcp, SdpAttributeRtcpFb, SdpAttributeSctpmap, SdpAttributeRemoteCandidate, SdpAttributeExtmap, SdpAttributeDirection};
 use nserror::{nsresult, NS_OK, NS_ERROR_INVALID_ARG};
 
 use types::StringView;
 use network::RustIpAddr;
 
 
 #[repr(C)]
 #[derive(Clone, Copy, PartialEq, Eq)]