1683417 - ThreadSanitizer: data race [@DataChannelConnection::SetSignals] vs. [@ DataChannelConnection::SetReadyState]

Status: RESOLVED FIXED

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

Description

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace-dec-2020.txt

Found while bringing up wpt under tsan / turning on the full firefox shutdown code. Marking security because this is racing on the value of a std::string.

mTransportId needs to be guarded by a Mutex.

Permalinks to problematic lines:

DataChannelConnection::SetSignals

vs

DataChannelConnection::SetReadyState

General information about TSan reports

Why fix races?

Data races are undefined behavior and can cause crashes as well as correctness issues. Compiler optimizations can cause racy code to have unpredictable and hard-to-reproduce behavior.

Rating

If you think this race can cause crashes or correctness issues, it would be great to rate the bug appropriately as P1/P2 and/or indicating this in the bug. This makes it a lot easier for us to assess the actual impact that these reports make and if they are helpful to you.

False Positives / Benign Races

Typically, races reported by TSan are not false positives [1], but it is possible that the race is benign. Even in this case it would be nice to come up with a fix if it is easily doable and does not regress performance. Every race that we cannot fix will have to remain on the suppression list and slows down the overall TSan performance. Also note that seemingly benign races can possibly be harmful (also depending on the compiler, optimizations and the architecture) [2][3].

[1] One major exception is the involvement of uninstrumented code from third-party libraries.
[2] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[3] How to miscompile programs with "benign" data races: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
Suppressing unfixable races

If the bug cannot be fixed, then a runtime suppression needs to be added in mozglue/build/TsanOptions.cpp. The suppressions match on the full stack, so it should be picked such that it is unique to this particular race. The bug number of this bug should also be included so we have some documentation on why this suppression was added.

Comment 1

[Andrew McCreight [:mccr8]]

This SCTP stuff is actually WebRTC: Networking, not Networking.

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1683417 - supress issue found while enabling full tsan shutdown. r?decoder

Depends on D100266

Comment 3

[Andrew McCreight [:mccr8]]

The DataChannelConnection::SetReadyState read looks like it is logging, so I think this is only an issue in debug builds.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc9f511b243a
supress issue found while enabling full tsan shutdown. r=decoder

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 6

[Christian Holler (:decoder)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

The DataChannelConnection::SetReadyState read looks like it is logging, so I think this is only an issue in debug builds.

TSan is built with --disable-debug. The logging statement in question is also in opt builds. The question is how much we care about logging being accurate here. This could lead to wrong/misleading logging output.

Byron, do you have an opinion about this kind of bug?

Comment 7

[Byron Campen [:bwc]]

Er, this is a race on a string, so this seems a little worse than bad logging, right? The pointer could be pointing at bad memory.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?

Comment 9

[Byron Campen [:bwc]]

No, this still needs fixing.

Comment 10

[Byron Campen [:bwc]]

So, I'm thinking that putting the transport id in that log-line is not super important, and probably not important enough to merit adding locking. I'm just going to pull it out.

Comment 11

[Byron Campen [:bwc]]

Hmm, there are other places we use mTransportId off STS, so we probably do need the locking after all.

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=d9e87033c034f90510c92727a7e9262fa35e0c1a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7f49d57a958df4c8611d1277e52105162386b4

Comment 13

[Byron Campen [:bwc]]

Attachment: Bug 1683417: Remove tsan suppression and fix race. r?ng,decoder

Also add a thread assertion.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a61fd1e3298
Remove tsan suppression and fix race. r=ng,decoder

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7a61fd1e3298