1645219 - Clean up DataChannelShutdown

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, task)

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

Attachment: Bug 1645219: Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r?ng

Comment 2

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=458685b2040db61c33fd003b0d5e7739f4fea880 (compare to base revision https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f601ebf6035480e6fa8fd97fbd5ddd928f909d5)

Gtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5dc69656a80f89126ef2afc1808644682a8162

Comment 4

[Intermittent Failures Robot]

8 failures in 5211 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 6
• mozilla-central: 2

Platform breakdown:

• windows7-32: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1645219&startday=2020-06-08&endday=2020-06-14&tree=all

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8390810fd9d76c4924ace11956ef5d631218636b

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc9bea1151d3
Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r=ng

Comment 7

[Alexandru Michis [:malexandru]]

Backed out changeset cc9bea1151d3 for causing failures in netinet/sctp_usrreq.c

Backout link: https://hg.mozilla.org/integration/autoland/rev/7f0b0cbecd946aee526a869853a46a14ee44b1f9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&fromchange=789bdea432a2c503cdde3ce04fa4d3a163f6dd7b&searchStr=linux%2C18.04%2Cx64%2Ctsan%2Copt%2Cmochitests%2Ctest-linux1804-64-tsan%2Fopt-mochitest-plain-e10s&tochange=7f0b0cbecd946aee526a869853a46a14ee44b1f9&selectedTaskRun=QwqFQ_oeSr-tQkqLsd_Msg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306713866&repo=autoland&lineNumber=1895

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

[Byron Campen [:bwc]]

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

Comment 10

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2200ee18d40e82bdf3004c8069fb3765accfe446

fuzzy --full: https://treeherder.mozilla.org/#/jobs?repo=try&revision=067d7b2e3bcb7d67e8137d38f207f44aaf211e98

Fuzzy --full doesn't allow me to run tsan tests. Trying to figure out how to run them...

Comment 11

[Byron Campen [:bwc]]

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...

Comment 12

[Byron Campen [:bwc]]

Try looks ok.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5048d0cf94c7
Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics) r=ng

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5048d0cf94c7

Comment 15

[Intermittent Failures Robot]

51 failures in 872 pushes (0.058 failures/push) were associated with this bug yesterday.

Repository breakdown:

• autoland: 51

Platform breakdown:

• windows7-32: 51

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1645219&startday=2020-06-18&endday=2020-06-18&tree=all

Comment 16

[Intermittent Failures Robot]

64 failures in 4035 pushes (0.016 failures/push) were associated with this bug in the last 7 days.

This is the #20 most frequent failure this week.

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:

• autoland: 60
• mozilla-central: 4

Platform breakdown:

• windows7-32: 64

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1645219&startday=2020-06-15&endday=2020-06-21&tree=all

Comment 17

[Byron Campen [:bwc]]

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

Comment 18

[Byron Campen [:bwc]]

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:

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/e12fdc01fa6e

Comment 22

[Byron Campen [:bwc]]

Attachment: Bug 1645219: (ESR68 backport) Repurpose DataChannelShutdown into DataChannelRegistry (which gives us some weak-pointer-like semantics)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/848f2565543b