1432930 - Rust SDP Parser fails to produce an error on NewSdpTest.CheckMalformedImageattr

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3)

Description

[Dan Minor [:dminor]]

version: 0
origin: Mozilla-SIPUA-35.0a1, 5184, 0, 0.0.0.0
session: SIP Call
connection: 224.0.0.1
timing: 0, 0
media: Video, 9, Rtp/Savpf, [120]
connection: 0.0.0.0
/home/dminor/src/firefox/media/webrtc/signaling/gtest/sdp_unittests.cpp:3723: Failure
Expected: ("") != (GetParseErrors()), actual: "" vs ""
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckMalformedImageattr/0, where GetParam() = (false, false) (0 ms)

Comment 1

[Johannes Willbold]

Attachment: Bug 1432930: Part 1: Added imageattr parsing in rust,

Added imageattr parsing in rust.
Expanded the imageattr test cases in rust.

Review commit: https://reviewboard.mozilla.org/r/254346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254346/

Comment 2

[Johannes Willbold]

Attachment: Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

Added C++/Rust glue code for imageattr.
Added U32vec interface
Added F32vec interface
Enabled C++ unit test CheckMalformedImageattr
Removed redundant parsing code with "Bug 1469702"

Review commit: https://reviewboard.mozilla.org/r/254348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254348/

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8989285 [details]
Bug 1432930: Part 1: Added imageattr parsing in rust,

https://reviewboard.mozilla.org/r/254346/#review263434

Man, I forgot how sloppy and horrible the imageattr spec is.

I think that you're going to want to have helper functions that can scan/split strings based on bracket pairs, because the other delimiter tokens (',' and ' ') are not really safe to use.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:289
(Diff revision 1)
> +    DiscrateValues(Vec<u32>),
> +    Value(u32)

s/Discrate/Discrete/

Also, not sure why we need a separate entry for the case where there is only one discrete value, even if they are represented a little differently syntactically.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:297
(Diff revision 1)
> +    DiscrateValues(Vec<f32>),
> +    Value(f32)

Same things here.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1201
(Diff revision 1)
> +            ))
> +        }
> +
> +        if to_parse.contains(":") {
> +            // Range values
> +            let range_tokens:Vec<&str> = to_parse[1..to_parse.len()-1].split(":").collect();

This stuff would be more readable if you created a slice that did not have the '[' and ']'. You could make a helper function that did this slicing, and also the check for the trailing ']'.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1221
(Diff revision 1)
> +            // Discrete values
> +            let values = to_parse[1..to_parse.len()-1]
> +                            .split(",")
> +                            .map(|x| x.parse::<u32>())
> +                            .collect::<Result<Vec<u32>, _>>()?;
> +
> +            if values.len() == 0 {
> +                return Err(SdpParserInternalError::Generic(
> +                    "imageattr's discrete value list is empty".to_string()
> +                ))
> +            }

I wonder if we should check that these values are ascending? The spec does not say this, but I would be surprised if the authors and implementation community did not think it...

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1222
(Diff revision 1)
> +            let values = to_parse[1..to_parse.len()-1]
> +                            .split(",")
> +                            .map(|x| x.parse::<u32>())
> +                            .collect::<Result<Vec<u32>, _>>()?;
> +
> +            if values.len() == 0 {
> +                return Err(SdpParserInternalError::Generic(
> +                    "imageattr's discrete value list is empty".to_string()
> +                ))
> +            }
> +
> +            Ok(SdpAttributeImageAttrXYRange::DiscrateValues(values))

Seems like this will accept "[640]".

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1272
(Diff revision 1)
> +    }
> +
> +
> +    let mut tokens = tokens.into_iter();
> +
> +    let x_token = result_from_option(tokens.next(),"imageattr set is missing the 'x=' token")?;

Let's use ok_or for this checking.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1296
(Diff revision 1)
> +        let parse_prange_value = |maybe_next: Option<&str>| -> Result<f32, SdpParserInternalError> {
> +            match maybe_next {
> +                Some(x) => Ok(x.parse::<f32>()?),
> +                None => Err(SdpParserInternalError::Generic(
> +                                "imageattr's par and sar ranges must have two components"
> +                                .to_string()))
> +            }
> +        };
> +
> +        let mut minmax_pair = range_tokens.split("-");
> +
> +        let min = parse_prange_value(minmax_pair.next())?;
> +        let max = parse_prange_value(minmax_pair.next())?;

What does '?' do when it fails inside an "Ok" (or other argument list)? Does the "Ok" become an error?

Also, we will happily parse "1-2-3" here.

I think this would be more readable (and correct) if we collected after the '-' split, checked that there are exactly 2 tokens, and then tried to parse each of the tokens.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1335
(Diff revision 1)
> +                    let values = value_token[1..value_token.len()-1]
> +                                    .split(",")
> +                                    .map(|x| x.parse::<f32>())
> +                                    .collect::<Result<Vec<f32>, _>>()?;
> +
> +                    if values.len() == 0 {
> +                        return Err(SdpParserInternalError::Generic(
> +                            "imageattr's sar discrete value list is empty".to_string()
> +                        ))
> +                    }

Seems like this will accept "[1]".

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1377
(Diff revision 1)
> +        } else {
> +            return Err(SdpParserInternalError::Generic(
> +                format!("imageattr set contains unknown value token '{:?}'", current_token)
> +                .to_string()
> +            ))
> +        }

We should be ignoring unknown key/value pairs, which also means we need to parse known key/value pairs preceded by unknown ones (ie; we need to be able to skip unknown key/value pairs and resume parsing after them). This is tricky, especially considering that the RFC does not specify any syntax rules about what can appear in the keys and values. The current c++ code assumes that ',' will always be enclosed in brackets, and that brackets are balanced.

Another reasonable assumption is that keys/values cannot contain '=', and that keys cannot contain ',' either. (This would allow you to split based on '=', and the last ',' before each '=') Given that you're already going to need to pay close attention to bracket balancing elsewhere, you might not want to do it this way though.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1395
(Diff revision 1)
> +    let result_from_option = |opt, error_msg: &str| {
> +        match opt {
> +            Some(x) => Ok(x),
> +            None => Err(SdpParserInternalError::Generic(error_msg.to_string()))
> +        }

Let's use ok_or instead of this.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1401
(Diff revision 1)
> +        match opt {
> +            Some(x) => Ok(x),
> +            None => Err(SdpParserInternalError::Generic(error_msg.to_string()))
> +        }
> +    };
> +    let mut tokens = to_parse.split(' ').peekable();

Hmm. If someone defines a key/value pair that can contain spaces, we're in trouble here...

This is yet another place where you're going to want to base your parse code on brackets.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1403
(Diff revision 1)
> +    let pt = parse_payload_type(result_from_option(tokens.next(),
> +                                                   "imageattr's requires a payload token")?)?;

Again, I'm not totally clear what '?' does when it is in an argument list...

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

https://reviewboard.mozilla.org/r/254348/#review263492

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1025
(Diff revision 1)
> +SdpImageattrAttributeList::XYRange
> +LoadImageattrXYRange(const RustSdpAttributeImageAttrXYRange& rustXYRange)
> +{
> +  SdpImageattrAttributeList::XYRange xyRange;
> +
> +  if (rustXYRange.type == 0) {

Let's define these constants somewhere.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1029
(Diff revision 1)
> +
> +  if (rustXYRange.type == 0) {
> +    xyRange.min = rustXYRange.min;
> +    xyRange.max = rustXYRange.max;
> +
> +    if (rustXYRange.step != std::numeric_limits<uint32_t>::max()) {

The RFC says that this has a default value of 1, so let's just set that default down in the parser instead of doing checking here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1064
(Diff revision 1)
> +    set.xRange = LoadImageattrXYRange(rustSet.x);
> +    set.yRange = LoadImageattrXYRange(rustSet.y);
> +
> +    if (rustSet.has_sar) {
> +      const RustSdpAttributeImageAttrSRange& rustSRange = rustSet.sar;
> +      SdpImageattrAttributeList::SRange sRange;

Not sure why we need this temporary variable.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1066
(Diff revision 1)
> +
> +    if (rustSet.has_sar) {
> +      const RustSdpAttributeImageAttrSRange& rustSRange = rustSet.sar;
> +      SdpImageattrAttributeList::SRange sRange;
> +
> +      if (rustSRange.type == 0) {

Let's define these constants too.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1083
(Diff revision 1)
> +    if (rustSet.has_par) {
> +      SdpImageattrAttributeList::PRange pRange;
> +      pRange.min = rustSet.par.min;
> +      pRange.max = rustSet.par.max;
> +
> +      set.pRange = pRange;

Again, not sure why we need this temp variable.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1086
(Diff revision 1)
> +    if (rustSet.q >= 0) {
> +      set.qValue = rustSet.q;
> +    }

RFC says the q-value defaults to 0.5, so let's set that in the rust parser and not worry about checks here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.cpp:82
(Diff revision 1)
> +{
> +  std::vector<std::uint32_t> ret;
> +
> +  size_t len = u32_vec_len(vec);
> +  for (size_t i = 0; i < len; i++) {
> +    uint32_t dword;

This windows-specific terminology is wrong; this is at most one word, and likely half of a word, on modern architectures. Just call it "number" or something.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:130
(Diff revision 1)
> +  U32Vec* discrete_values;
> +  uint32_t value;

Let's not represent these differently. We might be able to remove the |type| member if we do this also, which would save us the trouble of defining constants for it.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:138
(Diff revision 1)
> +  F32Vec* discrete_values;
> +  float value;

Same here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:159
(Diff revision 1)
> +  bool is_wildcard;
> +  RustSdpAttributeImageAttrSetVec* sets;

Can we leave |sets| null (or empty) to indicate the wildcard case?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:164
(Diff revision 1)
> +  bool is_wildcard;
> +  RustSdpAttributeImageAttrSetVec* sets;
> +};
> +
> +struct RustSdpAttributeImageAttr {
> +  uint32_t pt;

We call this payloadType elsewhere in this file.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:730
(Diff revision 1)
> +                xyrange.step = match step {
> +                    Some(x) => x,
> +                    None => u32::max_value(),
> +                };

Let's not make |step| an Optional, let's just set it to 1 if it wasn't parsed.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:853
(Diff revision 1)
> +            q: match other.q {
> +                Some(x) => x,
> +                None => -1.0
> +            }

Let's not make |q| an Optional, just make it 0.5 by default.

Comment 5

[Johannes Willbold]

Comment on attachment 8989285 [details]
Bug 1432930: Part 1: Added imageattr parsing in rust,

https://reviewboard.mozilla.org/r/254346/#review263434

I am doing the parsing of the tokens a little bit different now than you suggested but I still think it is a good solution. The token parsing is done in "parse_imageattr_tokens".

Comment 6

[Johannes Willbold]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

https://reviewboard.mozilla.org/r/254348/#review263492

> Let's define these constants somewhere.

I am using the discrete_values pointer for this purpose now.

Comment 7

[Johannes Willbold]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254348/diff/1-2/

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

https://reviewboard.mozilla.org/r/254348/#review263798

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1205
(Diff revision 2)
> +        }
> +
> +        current_tokens.push(token.to_string());
> +
> +        if open_braces_counter == 0 {
> +            tokens.push(current_tokens.join(","));

We should join with the passed separator, I think.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1298
(Diff revision 2)
>  
>      let mut sar = None;
>      let mut par = None;
>      let mut q = None;
>  
>      let parse_ps_range = |x: &str| -> Result<(f32, f32), SdpParserInternalError> {

Let's use some name other than "x" here, since imageattrs deal with resolution.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1323
(Diff revision 2)
>  
>      while let Some(current_token) = tokens.next() {
>          if current_token.starts_with("sar=") {
>              let value_token = &current_token[4..];
>              if value_token.starts_with("[") {
> -                if !value_token.ends_with("]") {
> +                let value_tokens = parse_imagettr_braced_token(value_token).ok_or(

Let's call "value_tokens" something else like "sar_values".

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1372
(Diff revision 2)
>                  return Err(SdpParserInternalError::Generic(
> -                    "imageattr's par value is missing braces".to_string()
> +                    "imageattr's par value must start with '['".to_string()
>                  ))
>              }
>  
> -            let range = parse_ps_range(&braced_value_token)?;
> +            let value_tokens = parse_imagettr_braced_token(braced_value_token).ok_or(

Let's call this "par_values", similar to previous comment.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1400
(Diff revision 2)
> -                                                   "imageattr's requires a payload token")?)?;
> -    let first_direction = parse_single_direction(result_from_option(tokens.next(), "")?)?;
> +        "imageattr's requires a payload token".to_string()
> +    ))?.as_str())?;
> +    let first_direction = parse_single_direction(tokens.next().ok_or(
> +        SdpParserInternalError::Generic(
> +            "imageattr's second token must be a direction token".to_string()

Nit: Remove the "'s" (two places).

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1407
(Diff revision 2)
>      let parse_set = |set_token:&str| -> Result<SdpAttributeImageAttrSet, SdpParserInternalError> {
>          Ok(parse_image_attr_set(&set_token[1..set_token.len()-1])?)
>      };

We could probably use parse_imagettr_braced_token in here; I think this will truncate the last character whether it is a ']' or not.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1431
(Diff revision 2)
>      };
>  
>      let mut second_set_list = SdpAttributeImageAttrSetList::Sets(Vec::new());
>  
>      // Check if there is a second direction defined
>      if let Some(x) = tokens.next() {

Let's not use "x" for this, call it "direction_token" or something.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1439
(Diff revision 2)
> -        second_set_list = match result_from_option(tokens.next(),
> -                                 "imageattr must have a parameter set after a direction token")? {
> +        second_set_list = match tokens.next().ok_or(SdpParserInternalError::Generic(
> +                "imageattr must have a parameter set after a direction token".to_string())
> +            )?.as_str() {
>              "*" => SdpAttributeImageAttrSetList::Wildcard,
> -            x @ _ => {
> +            x => {
>                  let mut sets = vec![parse_set(x)?];
>                  while let Some(set_str) = tokens.clone().peek() {
>                      if set_str.starts_with("[") && set_str.ends_with("]") {
> -                        sets.push(parse_set(tokens.next().unwrap())?);
> +                        sets.push(parse_set(tokens.next().unwrap().as_str())?);
>                      } else {
>                          break;
>                      }
>                  }
>  
>                  SdpAttributeImageAttrSetList::Sets(sets)
>              }
>          };

There's some opportunity for code reuse here.

I think what you want is a function that takes a ref to a peekable iterator, and tries to extract a direction and list of sets. You call it twice; if it returns None on the first call, that's an error, but not on the second call. If the second call returns something, then you check whether there are tokens remaining in the iterator; if there are, there's trailing garbage. Then you verify that the directions are different.

Comment 9

[Johannes Willbold]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

https://reviewboard.mozilla.org/r/254348/#review263798

> There's some opportunity for code reuse here.
> 
> I think what you want is a function that takes a ref to a peekable iterator, and tries to extract a direction and list of sets. You call it twice; if it returns None on the first call, that's an error, but not on the second call. If the second call returns something, then you check whether there are tokens remaining in the iterator; if there are, there's trailing garbage. Then you verify that the directions are different.

I really tried that in the first place, never made it work, until now. Thanks for reminding me of this!

Comment 10

[Johannes Willbold]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254348/diff/2-3/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

https://reviewboard.mozilla.org/r/254348/#review264194

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1206
(Diff revisions 2 - 3)
>          }
>  
>          current_tokens.push(token.to_string());
>  
>          if open_braces_counter == 0 {
> -            tokens.push(current_tokens.join(","));
> +            tokens.push(current_tokens.join(&seperator.to_string()));

s/seperator/separator/

Comment 12

[Johannes Willbold]

Comment on attachment 8989285 [details]
Bug 1432930: Part 1: Added imageattr parsing in rust,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254346/diff/1-2/

Comment 13

[Johannes Willbold]

Comment on attachment 8989286 [details]
Bug 1432930: Part 2: Added C++/Rust glue code for imageattr,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254348/diff/3-4/

Comment 14

[Byron Campen [:bwc]]

It looks like the edits for my review on part 1 ended up in part 2. Looking...

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8989285 [details]
Bug 1432930: Part 1: Added imageattr parsing in rust,

https://reviewboard.mozilla.org/r/254346/#review265156

r+ with fixes in part 2.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e22dde265f6a
Part 1: Added imageattr parsing in rust, r=bwc
https://hg.mozilla.org/integration/autoland/rev/85f1c6e5ddac
Part 2: Added C++/Rust glue code for imageattr, r=bwc

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e22dde265f6a
https://hg.mozilla.org/mozilla-central/rev/85f1c6e5ddac