Crash in [@ mozilla::SdpFmtpAttributeList::Fmtp::operator== ]
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
People
(Reporter: jib, Assigned: bwc)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
Found this nullptr offset crash researching if we're impacted by bug 1778130. Not a sec-bug perhaps, but the repro steps are identical so I decided to mark it that way for now to reduce visibility.
In short, I shaped the unit-tests in Chrome's CVE fix referenced in bug 1778130 comment 7 into a fiddle, as I wanted to double-check that Chrome's fixes in high-level code we don't share, wasn't protecting vulnerable lower-level libwebrtc code we do share, and found this crash in our high-level equivalent. Once we fix this crash, we should check again.
STRs:
Expected results: No crash
Actual results: bp-c9b9df46-06f9-4db6-a8a3-c1d5e0220908
Regression range:
6:01.47 INFO: Got as far as we can go bisecting nightlies...
6:01.47 INFO: Last good revision: c9c63f8447027d14ec2c541e82aae1014f08f812 (2019-10-02)
6:01.47 INFO: First bad revision: 2e1bfb7458de41a36432671c405eee62d897a761 (2019-10-03)
6:01.47 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9c63f8447027d14ec2c541e82aae1014f08f812&tochange=2e1bfb7458de41a36432671c405eee62d897a761
6:01.47 INFO: Switching bisection method to taskcluster
6:01.47 INFO: Getting mozilla-central builds between c9c63f8447027d14ec2c541e82aae1014f08f812 and 2e1bfb7458de41a36432671c405eee62d897a761
6:02.84 WARNING: Skipping build 2e1bfb7458de: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.2e1bfb7458de41a36432671c405eee62d897a761.firefox.macosx64-opt'
...
6:08.91 INFO: There are no build artifacts for these changesets (they are probably too old).
Assignee | ||
Comment 1•2 years ago
|
||
This is a pretty easy fix; that really should never be null, but it looks like this code can cause that to happen:
We should probably not be calling PushEntry if we didn't recognize the type.
Comment 2•2 years ago
|
||
Set release status flags based on info from the regressing bug 1470568
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
This is interesting. This crash has nothing to do with rids, it has to do with an extra \r\n in the middle of the SDP.
Comment 5•2 years ago
|
||
Yeah, this version, which doesn't insert spurious new lines, doesn't crash: https://jsfiddle.net/9x86guma/ .
Assignee | ||
Comment 6•2 years ago
•
|
||
I think this just boils down to having an fmtp line, but no rtpmap for that payload type. The extra \r\n was basically truncating the SDP, wiping out all the rtpmap lines. Boiling down a crashtest now.
Assignee | ||
Comment 7•2 years ago
|
||
Interesting. It does not happen if the rtpmap is simply absent. It has to exist, but after a stray \r\n.
Assignee | ||
Comment 8•2 years ago
|
||
Assignee | ||
Comment 9•2 years ago
|
||
Depends on D156899
Assignee | ||
Comment 10•2 years ago
|
||
Test case without fix:
https://treeherder.mozilla.org/jobs?repo=try&revision=8578cb736443f5031a04b6c0bf146ccaca13f09f
With test case and fix:
https://treeherder.mozilla.org/jobs?repo=try&revision=b6d89cc05824bceb20d8ad3e72cadd43795a3e26
https://treeherder.mozilla.org/jobs?repo=try&revision=59c980365ed6206802ca7b40768e3d24e5456160
Assignee | ||
Comment 11•2 years ago
|
||
Can we remove sec on this? This is just a content process nullptr crash.
Updated•2 years ago
|
Comment 12•2 years ago
|
||
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7803147ce38 Test case for bug. r=ng https://hg.mozilla.org/integration/autoland/rev/c6c731fb582d If we cannot determine the type for an fmtp, do not add it to the list of successfully parsed fmtp attributes. r=ng
Comment 13•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7803147ce38
https://hg.mozilla.org/mozilla-central/rev/c6c731fb582d
Updated•2 years ago
|
Description
•