Covers parse, serialization, API, and tests for imageattr.

Byron - I believe you want to own this bug for Q3 (part of delivering simulcast for this year). So I'm assigning it to you.

Bug 1173599: (WIP) a=imageattr support

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: (WIP) a=imageattr support

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support r?mt

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt https://reviewboard.mozilla.org/r/14463/#review13477 I'm not scared of bespoke parsing code like others, but this is impressively large. I couldn't find any serious issues, but there are a few things that I'd like to see fixed up before you get an r+. The testing looks pretty comprehensive, which is good. Do you have any idea what the coverage actually looks like? ::: media/webrtc/signaling/src/sdp/SdpAttribute.h:687 (Diff revision 4) > + bool PushEntry(const std::string& raw, std::string* error, size_t* errorPos); I'm not sure that I'm happy about this construction. Perhaps only from a consistency perspective though. Other PushEntry methods take an instance of the class that you want to push, which in this case would be Imageattr. ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:309 (Diff revision 4) > +GetSPValue(std::istream& is, float* value, std::string* error) Did you consider using GetUInt with the extra range changes here? You would have to change the name to GetUnsigned though. Does the grammar permit values in scientific notation like 0.09e2 ? Because this is going to allow that. ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:339 (Diff revision 4) > +GetQValue(std::istream& is, float* value, std::string* error) Maybe GetUnsigned can take a value range. That would avoid the problem with the leading sign character by setting the minimum to 1, 0.09f, or 0.0f. ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:150 (Diff revision 4) > +SdpImageattrAttributeList::XYRange::Serialize(std::ostream& os) const There is a *ton* of imageattr code here. Maybe split it into a new file. ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:503 (Diff revision 4) > + key.push_back(is.get()); tolower might be needed here, see below ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:574 (Diff revision 4) > + if (ParseKey(is, error) != "x") { RFC 6236 defines these using ABNF strings, which allows for X= as well as x=. Same goes for Q=, PAR=, SAR=, etc... (I have no problem with losing case in the process, but just saying.) ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:795 (Diff revision 4) > + std::istringstream is; > + is.str(raw); + is >> std::noskipws; ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:298 (Diff revision 4) > + // Single discrete value > + uint32_t value; > + if (!GetUInt(is, &value, error)) { The spec places a limit on the values of x and y that you aren't enforcing. That is 999999, which is a long way from the overflow point. ::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp:503 (Diff revision 4) > + for (uint16_t i = 1; i < UINT16_MAX; ++i) { This looks like it is open to exploitation. Do we really have to accept that many a=imageattr lines? ::: media/webrtc/signaling/test/sdp_unittests.cpp:2618 (Diff revision 4) > +TEST(NewSdpTestNoFixture, CheckImageattrXYRangeParseValid) None of these test serialization. That seems like a relatively easy addition to the test. I'd also suggest moving the end delimiter test to a common function. e.g., ``` SdpImageattrAttributeList::XYRange ParseXYRange(const std::string& input) { std::istringstream is; is.str(input + ",y="); SdpImageattrAttributeList::XYRange range; std::string error; ASSERT_TRUE(range.parse(is, &error)); ASSERT_EQ(input, range.Serialize()); ASSERT_EQ(',', is.get()); return range; } ``` ::: media/webrtc/signaling/test/sdp_unittests.cpp:2733 (Diff revision 4) > +TEST(NewSdpTestNoFixture, CheckImageattrSRangeParseValid) As above. ::: media/webrtc/signaling/test/sdp_unittests.cpp:2783 (Diff revision 4) > +#define ParseInvalidSRange(input, last) \ As above. ::: media/webrtc/signaling/test/sdp_unittests.cpp:2681 (Diff revision 4) > +#define ParseInvalidXYRange(input, last) \ static function please.

https://reviewboard.mozilla.org/r/14463/#review13477 > Did you consider using GetUInt with the extra range changes here? You would have to change the name to GetUnsigned though. > > Does the grammar permit values in scientific notation like 0.09e2 ? Because this is going to allow that. Also, only two decimal places are permitted by the grammar.

https://reviewboard.mozilla.org/r/14463/#review13477 > RFC 6236 defines these using ABNF strings, which allows for X= as well as x=. > > Same goes for Q=, PAR=, SAR=, etc... > > (I have no problem with losing case in the process, but just saying.) SEND and RECV are valid as well.

https://reviewboard.mozilla.org/r/14463/#review13477 I have not run any coverage analysis on it, but when writing the tests I was explicitly trying to hit all the branches. > Also, only two decimal places are permitted by the grammar. I don't think it is worth spending lots of time enforcing things like decimal points on the parse (making sure we follow the rules on serialization is important though). I'm also not too sad that we'll accept scientific notation, despite how silly that is. I think I will merge GetUInt and GetSPValue though, since it looks like I need to do bounds checking on the xy values anyhow. > SEND and RECV are valid as well. Right. I'll do that. > This looks like it is open to exploitation. Do we really have to accept that many a=imageattr lines? We do that in bunches of other places, and there are less complicated memory exhaustion attacks. > None of these test serialization. That seems like a relatively easy addition to the test. I'd also suggest moving the end delimiter test to a common function. > > e.g., > ``` > SdpImageattrAttributeList::XYRange > ParseXYRange(const std::string& input) { > std::istringstream is; > is.str(input + ",y="); > > SdpImageattrAttributeList::XYRange range; > std::string error; > ASSERT_TRUE(range.parse(is, &error)); > ASSERT_EQ(input, range.Serialize()); > ASSERT_EQ(',', is.get()); > return range; > } > ``` Lots of these won't re-serialize the same way (for example, the way we emit floating point might have extra trailing 0s, or unknown key/value pairs on Set won't be serialized, or q=0.5 might be serialized when it wasn't present since that's a default, etc). Breaking some of the logic out into a separate function could be good though. > static function please. That makes it hard to trace errors to the line where they occurred, but maybe I can output sufficient context to trace things. > + is >> std::noskipws; I prefer to put this close to the stuff that might skip whitespace. > There is a *ton* of imageattr code here. Maybe split it into a new file. Yeah, I'm not super happy that all of the SDP attribute stuff lives in one giant file either. > I'm not sure that I'm happy about this construction. Perhaps only from a consistency perspective though. Other PushEntry methods take an instance of the class that you want to push, which in this case would be Imageattr. PushEntry typically takes the stuff you need to construct whatever the internal representation is.

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support r?mt

https://reviewboard.mozilla.org/r/14463/#review13477 Running this through try before re-asking review.

Check back on try.

https://reviewboard.mozilla.org/r/14463/#review13495 ::: media/webrtc/signaling/test/sdp_unittests.cpp:2625 (Diff revisions 4 - 5) > + EXPECT_TRUE(range.Parse(is, &error)) << error; > + EXPECT_EQ(',', is.get()); > + EXPECT_EQ('y', is.get()); > + EXPECT_EQ('=', is.get()); > + return range; You might simplify this by removing the 'y' and '-' and then adding EXPECT_TRUE(is.eof());

https://reviewboard.mozilla.org/r/14463/#review13495 > You might simplify this by removing the 'y' and '-' and then adding > EXPECT_TRUE(is.eof()); I wanted to avoid any possibility that |is| would somehow be pointing at another ',' earlier in the string.

https://reviewboard.mozilla.org/r/14463/#review13495 > I wanted to avoid any possibility that |is| would somehow be pointing at another ',' earlier in the string. Isn't that what .eof would guarantee?

https://reviewboard.mozilla.org/r/14463/#review13495 > Isn't that what .eof would guarantee? Yeah, I guess that'll work.

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support r?mt

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support r?mt

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt Bug 1173599: a=imageattr support r?mt

Comment on attachment 8641329 [details] MozReview Request: Bug 1173599: a=imageattr support r?mt https://reviewboard.mozilla.org/r/14463/#review13619 Ship It!

https://reviewboard.mozilla.org/r/14461/#review13615 ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:206 (Diff revisions 6 - 7) > + if (PeekChar(is, error) == '-') { > + *error = "Value is less than 0"; > + return false; > + } Hmm, yes, good catch. ::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:687 (Diff revisions 6 - 7) > - if (!GetNumeric<uint16_t>(is, 0, UINT16_MAX, &value, error)) { > + if (!GetUnsigned<uint16_t>(is, 0, UINT16_MAX, &value, error)) { Do we have to worry about a warning as a result of comparing a uint16_t value with UINT16_MAX in (v > max)?

https://reviewboard.mozilla.org/r/14461/#review13615 > Do we have to worry about a warning as a result of comparing a uint16_t value with UINT16_MAX in (v > max)? I did not see any warnings either locally or on try. I'm guessing the compiler doesn't warn you about that when passing it into an inlined function?