Closed Bug 1645219 Opened 2 years ago Closed 2 years ago

Clean up DataChannelShutdown

Categories

(Core :: WebRTC: Networking, task)

task

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 --- fixed
firefox-esr78 --- fixed
firefox79 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(2 files)

DataChannelShutdown appears to be a workaround to prevent UAF of DataChannelConnection objects, by delaying destruction by 30 seconds. This is kinda hacky, and we really should implement some other way of doing this. It turns out to be easy to re-purpose DataChannelShutdown into a DataChannelRegistry object that allows an opaque handle to be used instead of a void* in usr_sctp (basically, using a weak pointer instead of a strong one). Other simplifications/cleanup can be done in this code also.

See Also: → 1628536
Duplicate of this bug: 1628536
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc9bea1151d3
Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r=ng

Looks like there's a race down in libusrsctp's shutdown code. We were avoiding this before because we were deferring shutdown of usrsctp until xpcom shutdown. Let me see what I can do about this.

Flags: needinfo?(docfaraday)

Have filed bug 1646716 for the race in usrsctp_finish. Trying a workaround now.

See Also: → 1646716

Ok, it seems that the media tests are never run on TSan; we were tripping over a TSan test outside of the media test suites.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9882ef4994c7524ca5008c4b49f73335bbb97262

I wonder just how terrible we do when run under TSan...

Try looks ok.

Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5048d0cf94c7
Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r=ng
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
See Also: → 1646996

Comment on attachment 9156096 [details]
Bug 1645219: Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r?ng

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined:
  • Fix Landed on Version: 79
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This code is a bit crufty/fragile already, so any clean-up has some inherent risk.
  • String or UUID changes made by this patch: None
Attachment #9156096 - Flags: approval-mozilla-esr68?

Comment on attachment 9156096 [details]
Bug 1645219: Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r?ng

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined:
  • Fix Landed on Version: 79
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
Attachment #9156096 - Flags: approval-mozilla-esr78?

Comment on attachment 9156096 [details]
Bug 1645219: Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r?ng

Hi, this needs a rebased patch for ESR68.

Flags: needinfo?(docfaraday)
Attachment #9156096 - Flags: approval-mozilla-esr68?

Comment on attachment 9156096 [details]
Bug 1645219: Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r?ng

Approved for 78.1esr.

Attachment #9156096 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Comment on attachment 9161664 [details]
Bug 1645219: (ESR68 backport) Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics)

Approved for ESR68, thanks for rebasing.

Flags: needinfo?(docfaraday)
Attachment #9161664 - Flags: approval-mozilla-esr68+
Duplicate of this bug: CVE-2020-6514
You need to log in before you can comment on or make changes to this bug.