1313527 - red parser goes into endless loop on broken SDP

Status: RESOLVED FIXED

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

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1313527: exit red parser loop if stroul failed.

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

Comment 2

[Michael Froman [:mjf]]

Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

https://reviewboard.mozilla.org/r/89058/#review88262

::: media/webrtc/signaling/test/sdp_unittests.cpp:444
(Diff revision 1)
>  
>  TEST_F(SdpTest, parseRtcpFbFooBarBaz) {
>    ParseSdp(kVideoSdp + "a=rtcp-fb:120 foo bar baz\r\n");
>  }
>  
> +static const std::string kVideoSdpWithUnknonwBrokenFtmp =

typo Unknonw should be Unknown

Comment 3

[Michael Froman [:mjf]]

Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

https://reviewboard.mozilla.org/r/89058/#review88270

One nit, otherwise good.

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aeb0df69182a
exit red parser loop if stroul failed. r=mjf

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

Approval Request Comment
[Feature/regressing bug #]: Bug 1275360 added support for parsing red, which has a problem.

[User impact if declined]: Firefox users who use WebRTC services or connect to WebRTC devices which send broken SDP to Firefox can cause Firefox's main thread to enter an endless loop from which the user can only terminate Firefox via the OS.

[Describe test coverage new/current, TreeHerder]: The patch contains a new unit test which shows that their is no longer an endless loop.

[Risks and why]: The risk is pretty low as it only set an additional exit criteria from a loop which should only be entered in rare corner cases.

[String/UUID change made/needed]: N/A

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Note: this could be used to trivially DOS the browser (though I'm sure there are other ways to do so easily).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aeb0df69182a

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

Fixes a hang (and see comment 7) with invalid SDP, Aurora51+, Beta50+

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4feda7704c1f85b8ac4a91bd2d9c430351ad28a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/207127298f7a26e48f553be88499ae043b3c1c35

https://hg.mozilla.org/releases/mozilla-release/rev/207127298f7a26e48f553be88499ae043b3c1c35