1219403 - TSan: data race dom/media/MediaStreamGraph.cpp:376 UpdateStreamOrder (race on MediaStreamGraphImpl::mStreamOrderDirty)

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Nathan Froyd [:froydnj]]

Attachment: TSan stack trace

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) [1][2].

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.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Comment 1

[Karl Tomlinson (:karlt)]

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

Comment 2

[Maire Reavy [:mreavy]]

Paul -- Do you have the bandwidth to take this bug?  I'd like to get a fix for this soon.

Comment 3

[Paul Adenot (:padenot)]

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.

Comment 4

[Paul Adenot (:padenot)]

Attachment: patch

I'll be doing some work in bug 1221021 to prevent this in the future.

Comment 5

[Paul Adenot (:padenot)]

Attachment: r=

Hrm, forgot I was not in a MediaStreamGraphImpl method.

Comment 6

[Paul Adenot (:padenot)]

Daniel, considering comment 3, can we just land this without sec-approval?

Comment 7

[Daniel Veditz [:dveditz]]

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

Comment 8

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdf0710beb8 and a followup https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d8668ab1e1

Comment 9

[Maire Reavy [:mreavy]]

Paul -- Will the fix for this bug also fix Bug 1221884?

Comment 10

[Paul Adenot (:padenot)]

As Karl said, it's not clear, but it could be. We'll see, this just got landed.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0cdf0710beb8
https://hg.mozilla.org/mozilla-central/rev/b7d8668ab1e1
https://hg.mozilla.org/mozilla-central/rev/a082d90d2732