CVE-2024-11708

Low CVSS 6.5 firefox Race 🔧 Commit mapped

Overview

Low

Severity

6.5

CVSS

Exploited ITW

Fixed

Fix Status

Impactlow

DescriptionMissing thread synchronization primitives could have led to a data race on members of the PlaybackParams structure.

ComponentDOM

Bug ClassRace

Tracker1922912

Fix commit4752a783de5f (firefox) +48/-48

CWECWE-362 (Race condition)

CVSS vectorCVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:N/I:H/A:N

CISA KEVNot listed

CreditedSerban Stanca

Disclosed2024-11-26

Background

`PlaybackParams`: A small struct holding audio playback state (mVolume, mPlaybackRate, mPreservesPitch) that is owned by AudioSinkWrapper on its owner (MDSM) thread.
`AudioSink`: The object that allocates and drives an AudioStream; its public methods InitializeAudioStream and Start are documented as not thread-safe.
`AudioSinkWrapper::MaybeAsyncCreateAudioSink`: A helper that offloads the expensive AudioSink initialization onto a background mAsyncInitTaskQueue and then completes startup back on the owner thread.
Data race: Two threads accessing the same memory without synchronization where at least one access is a write, yielding undefined behavior.

Root Cause Analysis

Before the fix, AudioSink::InitializeAudioStream took a const PlaybackParams& aParams and applied aParams.mVolume, aParams.mPlaybackRate, and aParams.mPreservesPitch to the freshly created mAudioStream. In the async startup path, AudioSinkWrapper::MaybeAsyncCreateAudioSink runs InitializeAudioStream on the background mAsyncInitTaskQueue, so those reads of the wrapper’s mParams happened off the owner thread while the owner thread could concurrently mutate mParams (e.g. via volume or rate changes), violating the invariant that PlaybackParams is only touched from the owner thread.

The fix moves the reads of aParams out of InitializeAudioStream and into AudioSink::Start, which asserts mOwnerThread->IsCurrentThreadIn() and always executes on the owner thread. Because Start is the only place that now reads the params, and it runs on the owner thread that also owns mParams, the concurrent read/write against mParams is eliminated. The initialization step that stays on the background thread no longer references PlaybackParams at all.

Key insight

The core mistake was reading the owner-thread-owned PlaybackParams from within the background-thread initialization step; the fix relocates those reads into AudioSink::Start, which is guaranteed to run on the owner thread, so no cross-thread access to mParams remains.

Attack Path

Begin async audio startup Media playback triggers AudioSinkWrapper::MaybeAsyncCreateAudioSink, dispatching AudioSink::InitializeAudioStream onto the background mAsyncInitTaskQueue.
Concurrent parameter mutation While the background task reads mParams inside InitializeAudioStream, script on the main/MDSM thread changes volume or playback rate, writing the same PlaybackParams fields.
Unsynchronized overlap With no lock or thread confinement, the background read and owner-thread write of mVolume/mPlaybackRate/mPreservesPitch overlap, producing a data race.
Undefined behavior The race yields a torn or stale value being pushed into mAudioStream, and, per C++ memory-model rules, undefined behavior that could manifest as incorrect playback state.

Impact Assessment

An attacker gains only a non-deterministic data race on the PlaybackParams fields within the content/media process, with no demonstrated memory-corruption primitive, consistent with the low severity rating. Triggering it requires racing a background AudioSink initialization against an owner-thread parameter change during media playback, so the preconditions are timing-dependent and not attacker-controlled with precision.

Changed Functions

Function	Change	Notes
`if` dom/media/mediasink/AudioSink.cpp	modified

Files Changed

dom/media/mediasink/AudioSink.cpp
dom/media/mediasink/AudioSink.h
dom/media/mediasink/AudioSinkWrapper.cpp
dom/media/mediasink/AudioSinkWrapper.h

Audit Directions

Cross-thread struct reads
Look for owner-thread-owned structs like PlaybackParams whose fields are read inside lambdas dispatched to background task queues such as mAsyncInitTaskQueue.
Task-queue offload paths
Audit InvokeAsync/Then chains in dom/media where “not thread-safe” methods run off the documented owner thread and touch shared members.
Event-target mismatches
Check for Then continuations dispatched to a raw thread rather than GetEventTarget(), which can let owner-thread-only state be accessed from the wrong context.

Patch

view commit

diff --git a/dom/media/mediasink/AudioSink.cpp b/dom/media/mediasink/AudioSink.cpp
index f879adfe9ae..6a824bb7dd7 100644
--- a/dom/media/mediasink/AudioSink.cpp
+++ b/dom/media/mediasink/AudioSink.cpp
@@ -98,7 +98,7 @@ AudioSink::~AudioSink() {
 }
 
 nsresult AudioSink::InitializeAudioStream(
-    const PlaybackParams& aParams, const RefPtr<AudioDeviceInfo>& aAudioDevice,
+    const RefPtr<AudioDeviceInfo>& aAudioDevice,
     AudioSink::InitializationType aInitializationType) {
   if (aInitializationType == AudioSink::InitializationType::UNMUTING) {
     // Consider the stream to be audible immediately, before initialization
@@ -133,19 +133,19 @@ nsresult AudioSink::InitializeAudioStream(
     return rv;
   }
 
-  // Set playback params before calling Start() so they can take effect
-  // as soon as the 1st DataCallback of the AudioStream fires.
-  mAudioStream->SetVolume(aParams.mVolume);
-  mAudioStream->SetPlaybackRate(aParams.mPlaybackRate);
-  mAudioStream->SetPreservesPitch(aParams.mPreservesPitch);
-
   return NS_OK;
 }
 
 RefPtr<MediaSink::EndedPromise> AudioSink::Start(
-    const media::TimeUnit& aStartTime) {
+    const PlaybackParams& aParams, const media::TimeUnit& aStartTime) {
   MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
 
+  // Set playback params before calling Start() so they can take effect
+  // as soon as the 1st DataCallback of the AudioStream fires.
+  mAudioStream->SetVolume(aParams.mVolume);
+  mAudioStream->SetPlaybackRate(aParams.mPlaybackRate);
+  mAudioStream->SetPreservesPitch(aParams.mPreservesPitch);
+
   mAudioQueueListener = mAudioQueue.PushEvent().Connect(
       mOwnerThread, this, &AudioSink::OnAudioPushed);
   mAudioQueueFinishListener = mAudioQueue.FinishEvent().Connect(
diff --git a/dom/media/mediasink/AudioSink.h b/dom/media/mediasink/AudioSink.h
index 154e1bdf619..bda060bc656 100644
--- a/dom/media/mediasink/AudioSink.h
+++ b/dom/media/mediasink/AudioSink.h
@@ -47,13 +47,13 @@ class AudioSink : private AudioStream::DataSource {
   ~AudioSink();
 
   // Allocate and initialize mAudioStream. Returns NS_OK on success.
-  nsresult InitializeAudioStream(const PlaybackParams& aParams,
-                                 const RefPtr<AudioDeviceInfo>& aAudioDevice,
+  nsresult InitializeAudioStream(const RefPtr<AudioDeviceInfo>& aAudioDevice,
                                  InitializationType aInitializationType);
 
   // Start audio playback.  aStartTime is compared with MediaData::mTime to
   // identify the first audio frame to be played.
-  RefPtr<MediaSink::EndedPromise> Start(const media::TimeUnit& aStartTime);
+  RefPtr<MediaSink::EndedPromise> Start(const PlaybackParams& aParams,
+                                        const media::TimeUnit& aStartTime);
 
   /*
    * All public functions are not thread-safe.
diff --git a/dom/media/mediasink/AudioSinkWrapper.cpp b/dom/media/mediasink/AudioSinkWrapper.cpp
index 909de78fd9d..45688614938 100644
--- a/dom/media/mediasink/AudioSinkWrapper.cpp
+++ b/dom/media/mediasink/AudioSinkWrapper.cpp
@@ -332,8 +332,8 @@ void AudioSinkWrapper::StartAudioSink(UniquePtr<AudioSink> aAudioSink,
   AssertOwnerThread();
   MOZ_ASSERT(!mAudioSink);
   mAudioSink = std::move(aAudioSink);
-  mAudioSink->Start(aStartTime)
-      ->Then(mOwnerThread.get(), __func__, this,
+  mAudioSink->Start(mParams, aStartTime)
+      ->Then(mOwnerThread.GetEventTarget(), __func__, this,
              &AudioSinkWrapper::OnAudioEnded)
       ->Track(mAudioSinkEndedRequest);
 }
@@ -370,37 +370,36 @@ RefPtr<GenericPromise> AudioSinkWrapper::MaybeAsyncCreateAudioSink(
   ++mAsyncCreateCount;
   using Promise =
       MozPromise<UniquePtr<AudioSink>, nsresult, /* IsExclusive = */ true>;
-  return InvokeAsync(mAsyncInitTaskQueue,
-                     "MaybeAsyncCreateAudioSink (Async part: initialization)",
-                     [self = RefPtr<AudioSinkWrapper>(this),
-                      audioSink{std::move(audioSink)},
-                      audioDevice = mAudioDevice, this]() mutable {
-                       if (!audioSink || !mAsyncInitTaskQueue->IsEmpty()) {
-                         // Either an AudioSink is not required or there's a
-                         // pending task to init an AudioSink with a possibly
-                         // different device.
-                         return Promise::CreateAndResolve(nullptr, __func__);
-                       }
-
-                       LOG("AudioSink initialization on background thread");
-                       // This can take about 200ms, e.g. on Windows, we don't
-                       // want to do it on the MDSM thread, because it would
-                       // make the clock not update for that amount of time, and
-                       // the video would therefore not update. The Start() call
-                       // is very cheap on the other hand, we can do it from the
-                       // MDSM thread.
-                       nsresult rv = audioSink->InitializeAudioStream(
-                           mParams, audioDevice,
-                           AudioSink::InitializationType::UNMUTING);
-                       if (NS_FAILED(rv)) {
-                         LOG("Async AudioSink initialization failed");
-                         return Promise::CreateAndReject(rv, __func__);
-                       }
-                       return Promise::CreateAndResolve(std::move(audioSink),
-                                                        __func__);
-                     })
+  return InvokeAsync(
+             mAsyncInitTaskQueue,
+             "MaybeAsyncCreateAudioSink (Async part: initialization)",
+             [self = RefPtr<AudioSinkWrapper>(this),
+              audioSink{std::move(audioSink)}, audioDevice = mAudioDevice,
+              this]() mutable {
+               if (!audioSink || !mAsyncInitTaskQueue->IsEmpty()) {
+                 // Either an AudioSink is not required or there's a
+                 // pending task to init an AudioSink with a possibly
+                 // different device.
+                 return Promise::CreateAndResolve(nullptr, __func__);
+               }
+
+               LOG("AudioSink initialization on background thread");
+               // This can take about 200ms, e.g. on Windows, we don't
+               // want to do it on the MDSM thread, because it would
+               // make the clock not update for that amount of time, and
+               // the video would therefore not update. The Start() call
+               // is very cheap on the other hand, we can do it from the
+               // MDSM thread.
+               nsresult rv = audioSink->InitializeAudioStream(
+                   audioDevice, AudioSink::InitializationType::UNMUTING);
+               if (NS_FAILED(rv)) {
+                 LOG("Async AudioSink initialization failed");
+                 return Promise::CreateAndReject(rv, __func__);
+               }
+               return Promise::CreateAndResolve(std::move(audioSink), __func__);
+             })
       ->Then(
-          mOwnerThread,
+          mOwnerThread.GetEventTarget(),
           "MaybeAsyncCreateAudioSink (Async part: start from MDSM thread)",
           [self = RefPtr<AudioSinkWrapper>(this), audioDevice = mAudioDevice,
            this](Promise::ResolveOrRejectValue&& aValue) mutable {
@@ -478,7 +477,7 @@ nsresult AudioSinkWrapper::SyncCreateAudioSink(const TimeUnit& aStartTime) {
 
   UniquePtr<AudioSink> audioSink = mSinkCreator();
   nsresult rv = audioSink->InitializeAudioStream(
-      mParams, mAudioDevice, AudioSink::InitializationType::INITIAL);
+      mAudioDevice, AudioSink::InitializationType::INITIAL);
   if (NS_FAILED(rv)) {
     LOG("Sync AudioSinkWrapper initialization failed");
     // If a specific device has been specified through setSinkId()
diff --git a/dom/media/mediasink/AudioSinkWrapper.h b/dom/media/mediasink/AudioSinkWrapper.h
index 3f0343d5f63..1545ea5def7 100644
--- a/dom/media/mediasink/AudioSinkWrapper.h
+++ b/dom/media/mediasink/AudioSinkWrapper.h
@@ -8,6 +8,7 @@
 #define AudioSinkWrapper_h_
 
 #include "mozilla/AbstractThread.h"
+#include "mozilla/EventTargetCapability.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/UniquePtr.h"
@@ -85,8 +86,8 @@ class AudioSinkWrapper : public MediaSink {
   void OnMuted(bool aMuted);
   virtual ~AudioSinkWrapper();
 
-  void AssertOwnerThread() const {
-    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
+  void AssertOwnerThread() const MOZ_ASSERT_CAPABILITY(mOwnerThread) {
+    mOwnerThread.AssertOnCurrentThread();
   }
 
   bool NeedAudioSink();
@@ -121,7 +122,7 @@ class AudioSinkWrapper : public MediaSink {
 
   bool IsAudioSourceEnded(const MediaInfo& aInfo) const;
 
-  const RefPtr<AbstractThread> mOwnerThread;
+  const EventTargetCapability<AbstractThread> mOwnerThread;
   const RefPtr<TaskQueue> mAsyncInitTaskQueue;
   SinkCreator mSinkCreator;
   UniquePtr<AudioSink> mAudioSink;
@@ -133,7 +134,7 @@ class AudioSinkWrapper : public MediaSink {
   MozPromiseHolder<EndedPromise> mEndedPromiseHolder;
   // true between Start() and Stop()
   bool mIsStarted = false;
-  PlaybackParams mParams;
+  PlaybackParams MOZ_GUARDED_BY(mOwnerThread) mParams;
   // mClockStartTime is null before Start(), after Stop(), and between
   // SetPlaying(false) and SetPlaying(true).  When the system time is used for
   // the clock, this is the time corresponding to mPositionAtClockStart.  When

Loading diff…

References

Vendor advisory Fix commit Bug tracker NVD

On This Page