1656063 - SUMMARY: ThreadSanitizer: data race /builds/worker/checkouts/gecko/dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp:128:21 in mozilla::FFVPXRuntimeLinker::GetRDFTFuncs(FFmpegRDFTFuncs*)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: bcampen [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=311427409&repo=try
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/AezTkbN5S8mu2EMeZZ6WUg/runs/0/artifacts/public/logs/live_backing.log

---

Comment 1

[Byron Campen [:bwc]]

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp#128

vs

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/FFTBlock.h#86

at

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/FFTBlock.cpp#40

Comment 2

[Byron Campen [:bwc]]

Are we running init code multiple times here?

Comment 3

[Byron Campen [:bwc]]

Starting here:

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/FFTBlock.h#43-48

Init has a check to see whether it has already run:

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp#60-62

Whereas GetRDFTFuncs does not:

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp#123-134

It seems like the intent here is to run this just once, though.

Comment 4

[Byron Campen [:bwc]]

This is a racy access to a function pointer.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1656063 - Only load function pointer in FFTBlock once. r?karlt

Comment 6

[Karl Tomlinson (:karlt)]

Undefined behavior due to reading from memory that may be concurrently re-written with its existing value, but we'd be very unlucky if the compiler was doing something that would make this exploitable.

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 9167008 [details]
Bug 1656063 - Only load function pointer in FFTBlock once. r?karlt

Security Approval Request

• How easily could an exploit be constructed based on the patch?: https://bugzilla.mozilla.org/show_bug.cgi?id=1656063#c6 explains what is happening. It's reasonably easy to understand what's going on from the patch only, but it's a concurrent write of a pointer by the same value, from different threads, it's not horrible.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This backports cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: I don't expect any regression.

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9167008 [details]
Bug 1656063 - Only load function pointer in FFTBlock once. r?karlt

Approved to land. If uplift is warranted (it seems from c6 this isn't likely exploitable?) feel free to request.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/af3620fc2747c1b4dbb72504fd731e9e4e3779d3
https://hg.mozilla.org/mozilla-central/rev/af3620fc2747