imageattr support in sdparta

RESOLVED FIXED in Firefox 42

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
10
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Covers parse, serialization, API, and tests for imageattr.
(Assignee)

Updated

3 years ago
Blocks: 1173602
(Assignee)

Updated

3 years ago
Rank: 10
Priority: -- → P1
(Assignee)

Updated

3 years ago
backlog: --- → webRTC+
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.
Assignee: nobody → docfaraday
(Assignee)

Comment 2

2 years ago
Created attachment 8641329 [details]
MozReview Request: Bug 1173599: a=imageattr support r?mt

Bug 1173599: (WIP) a=imageattr support
(Assignee)

Comment 3

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

Bug 1173599: (WIP) a=imageattr support
(Assignee)

Comment 4

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

Bug 1173599: a=imageattr support
Attachment #8641329 - Attachment description: MozReview Request: Bug 1173599: (WIP) a=imageattr support → MozReview Request: Bug 1173599: a=imageattr support
(Assignee)

Comment 5

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

Bug 1173599: a=imageattr support r?mt
Attachment #8641329 - Attachment description: MozReview Request: Bug 1173599: a=imageattr support → MozReview Request: Bug 1173599: a=imageattr support r?mt
Attachment #8641329 - Flags: review?(martin.thomson)

Updated

2 years ago
Attachment #8641329 - Flags: review?(martin.thomson)
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.
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

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

Bug 1173599: a=imageattr support r?mt
(Assignee)

Comment 11

2 years ago
https://reviewboard.mozilla.org/r/14463/#review13477

Running this through try before re-asking review.
(Assignee)

Comment 12

2 years ago
Check back on try.
Flags: needinfo?(docfaraday)
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());
(Assignee)

Comment 14

2 years ago
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?
(Assignee)

Comment 16

2 years ago
https://reviewboard.mozilla.org/r/14463/#review13495

> Isn't that what .eof would guarantee?

Yeah, I guess that'll work.
(Assignee)

Comment 17

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

Bug 1173599: a=imageattr support r?mt
(Assignee)

Comment 18

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

Bug 1173599: a=imageattr support r?mt
(Assignee)

Comment 19

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

Bug 1173599: a=imageattr support r?mt
Attachment #8641329 - Flags: review?(martin.thomson)
Comment on attachment 8641329 [details]
MozReview Request: Bug 1173599: a=imageattr support r?mt

https://reviewboard.mozilla.org/r/14463/#review13619

Ship It!
Attachment #8641329 - Flags: review?(martin.thomson) → review+
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)?
(Assignee)

Comment 22

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(docfaraday)
Keywords: checkin-needed

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b953b237ecad
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b953b237ecad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

2 years ago
Blocks: 1192390
You need to log in before you can comment on or make changes to this bug.