Closed Bug 1219403 Opened 5 years ago Closed 5 years ago
TSan: data race dom/media/Media
Stream Graph .cpp:376 Update Stream Order (race on Media Stream Graph Impl::m Stream Order Dirty)
The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer). * Specific information about this bug MediaStreamGraphImpl::SetStreamOrderDirty updates mStreamOrderDirty without any locks held on the main thread. MediaStreamGraphimpl::UpdateStreamOrder then reads the value held therein on a helper thread (started by ALSA?), also without any locks. * General information about TSan, data races, etc. Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) . If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.  http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong  _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
MediaInputPort::Disconnect() is a graph thread method, and I expect there are memory safety risks from RemoveInput/RemoveConsumer on the wrong thread. I expect these are hard to exploit, but not sure. Destroy() should be sufficient. https://hg.mozilla.org/mozilla-central/rev/946e320a1776#l3.62 Introduced in 42. Assuming these version also affected. (I don't know whether this might have been disabled somehow in some versions.)
Paul -- Do you have the bandwidth to take this bug? I'd like to get a fix for this soon.
Priority: -- → P1
This is related to audio capture, and audio capture is behind a disabled pref for now. This report comes probably from a mochitest run, and the mochitest flips the pref so that the feature can be tested. The faulty code is therefore unreachable in the wild. That said, Karl is right in comment 1, ::Destroy is sufficient here, patch coming up.
I'll be doing some work in bug 1221021 to prevent this in the future.
Assignee: nobody → padenot
Attachment #8682388 - Flags: review?(karlt)
Hrm, forgot I was not in a MediaStreamGraphImpl method.
Attachment #8682402 - Flags: review?(karlt)
Daniel, considering comment 3, can we just land this without sec-approval?
Turning on audio capture is bug 1187333. Is there a target release version yet? Until we have one I guess we can keep flipping the status to "disabled". (In reply to Paul Adenot (:padenot) from comment #6) > Daniel, considering comment 3, can we just land this without sec-approval? Yup, please do. No worries about who we're putting at risk should the patch be reverse engineered. (For a racy bug that's difficult/unlikely anyway even if it weren't disabled.)
Paul -- Will the fix for this bug also fix Bug 1221884?
As Karl said, it's not clear, but it could be. We'll see, this just got landed.
Whiteboard: [tsan][b2g-adv-main2.5-] → [tsan][b2g-adv-main2.5-][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.