Closed Bug 1219403 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Web Audio, defect, P1)

42 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- unaffected
firefox42 --- disabled
firefox43 --- disabled
firefox44 --- disabled
firefox45 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: froydnj, Assigned: padenot)

References

(Blocks 3 open bugs)

Details

(Keywords: regression, sec-moderate, Whiteboard: [tsan][b2g-adv-main2.5-][post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

Attached file 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
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.)
Blocks: 1156472
Group: core-security
Has Regression Range: --- → yes
Flags: needinfo?(padenot)
Version: unspecified → 42 Branch
Component: Audio/Video: MSG/cubeb/GMP → Web Audio
Blocks: 1219981
Paul -- Do you have the bandwidth to take this bug?  I'd like to get a fix for this soon.
Rank: 15
Priority: -- → P1
Group: core-security → media-core-security
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.
Flags: needinfo?(padenot)
Attached patch patch (obsolete) — Splinter Review
I'll be doing some work in bug 1221021 to prevent this in the future.
Assignee: nobody → padenot
Attachment #8682388 - Flags: review?(karlt)
Attached patch r=Splinter Review
Hrm, forgot I was not in a MediaStreamGraphImpl method.
Attachment #8682402 - Flags: review?(karlt)
Attachment #8682388 - Attachment is obsolete: true
Attachment #8682388 - Flags: review?(karlt)
Attachment #8682402 - Flags: review?(karlt) → review+
Daniel, considering comment 3, can we just land this without sec-approval?
Flags: needinfo?(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.)
Blocks: 1221884
Paul -- Will the fix for this bug also fix Bug 1221884?
Flags: needinfo?(padenot)
As Karl said, it's not clear, but it could be. We'll see, this just got landed.
Flags: needinfo?(padenot)
Group: media-core-security → core-security-release
Whiteboard: [tsan] → [tsan][b2g-adv-main2.5-]
Whiteboard: [tsan][b2g-adv-main2.5-] → [tsan][b2g-adv-main2.5-][post-critsmash-triage]
Group: core-security-release
No longer blocks: 1219981
You need to log in before you can comment on or make changes to this bug.