3e92f02010c65e93fc10e9a21062a3eb31b6a28b  audio: Guard OutputController::snoopers_ with snooper_lock_

The bug at a glance

This is a data-race hardening fix in the audio service with a plausible but unproven memory-safety impact. Pre-patch, snoopers_ was searched (std::ranges::contains / std::ranges::find) on the control thread outside snooper_lock_, while the real-time audio thread reads the same vector under the lock in OnMoreData. A concurrent push_back / swap-pop that reallocates or mutates the vector during an unlocked read on the audio thread is undefined behavior and could in principle produce a torn read or transient dangling iterator. However, the unlocked operations here were the search steps done by the mutating thread itself, and the original code comment asserted the list only updates on one thread — so the exposure is a narrow window and no crash or attacker path is demonstrated. It is also gated largely behind DCHECKs. The audio service is sandboxed and the trigger requires precise start/stop-snooping timing, so impact is low (CVSS ~4.0), watch-tier.

OutputController fans out rendered audio to ‘snoopers’ (e.g. loopback / WebRTC audio capture consumers). The security question is whether the snoopers_ vector can be structurally mutated on the control thread at the same instant the real-time audio thread iterates it inside OnMoreData. The original design put the container search operations (contains before push_back, find before swap-pop) OUTSIDE the lock and only took snooper_lock_ around the actual mutation, on the theory that ’the list only updates on this thread.’ That theory ignores that a vector search racing a concurrent locked read is still a data race on the underlying storage during reallocation, and the mutation window is not fully covered. The fix simply widens the lock to cover search + mutate atomically.

Root cause

The patch adds GUARDED_BY(snooper_lock_) to snoopers_ so the compiler’s Clang thread-safety analysis enforces that every access holds the lock. It then moves ‘base::AutoLock lock(snooper_lock_);’ ABOVE the std::ranges::contains check in StartSnooping and above the std::ranges::find in StopSnooping, so the search and the subsequent push_back / swap-pop happen as one critical section that cannot interleave with the audio thread’s locked reads in OnMoreData. In the destructor, the DCHECK(snoopers_.empty()) is wrapped in a DCHECK_IS_ON() block that acquires snooper_lock_ before touching snoopers_, satisfying the new annotation and avoiding an unsynchronized read even in teardown. base/dcheck_is_on.h is included for that guard. No functional behavior changes for well-ordered callers; the change is purely about making all container access lock-covered and compiler-verifiable.

Key code

Lock is taken before the container search, and snoopers_ is GUARDED_BY the lock so misuse is a compile error.

// StartSnooping
-  DCHECK(!std::ranges::contains(snoopers_, snooper));
   base::AutoLock lock(snooper_lock_);
+  DCHECK(!std::ranges::contains(snoopers_, snooper));
   snoopers_.push_back(snooper);

// StopSnooping
-  const auto it = std::ranges::find(snoopers_, snooper);
-  CHECK(it != snoopers_.end());
   base::AutoLock lock(snooper_lock_);
+  const auto it = std::ranges::find(snoopers_, snooper);
+  CHECK(it != snoopers_.end());

// output_controller.h
-  std::vector<raw_ptr<Snooper>> snoopers_;
+  std::vector<raw_ptr<Snooper>> snoopers_ GUARDED_BY(snooper_lock_);

Patch walkthrough

• services/audio/output_controller.cc — Three changes. (1) Destructor: the DCHECK(snoopers_.empty()) is now inside ‘#if DCHECK_IS_ON() { base::AutoLock lock(…); DCHECK(…); }’, so even the teardown read is lock-covered and satisfies the annotation. (2) StartSnooping: the AutoLock now precedes the std::ranges::contains DCHECK, making search+push_back atomic. (3) StopSnooping: the AutoLock now precedes std::ranges::find and its CHECK, making search+swap-pop atomic. Adds #include base/dcheck_is_on.h.
• services/audio/output_controller.h — Adds #include base/thread_annotations.h and annotates snoopers_ with GUARDED_BY(snooper_lock_). This is the enforcement lever: Clang’s -Wthread-safety now flags any future access to snoopers_ that does not hold snooper_lock_, preventing regression of the exact class of bug being fixed.

Background

OutputController snoopers — Snooper objects grab copies of rendered audio from OnMoreData for loopback/capture (e.g. tab audio capture, WebRTC). snoopers_ is a vector of raw_ptr read on the real-time audio thread and mutated on the owning task-runner thread.

OnMoreData realtime thread — The audio device callback runs on a dedicated real-time thread and iterates snoopers_ under snooper_lock_ to deliver audio. This is the concurrent reader that unlocked control-thread searches were racing.

snooper_lock_ — base::Lock intended to serialize access to snoopers_ between the control thread and the real-time audio thread. Pre-patch it covered only the mutation, not the preceding search.

GUARDED_BY / Clang thread-safety — A compile-time annotation checked by -Wthread-safety; requires the named lock be held for any access to the annotated member, converting a latent runtime race into a build-time error.

raw_ptr — BackupRefPtr-wrapped pointer; a torn/reallocated vector read under a race could surface a dangling raw_ptr, which is the memory-safety concern behind the race.

Vulnerability window

1. root cause — Searches on snoopers_ were performed outside snooper_lock_ on the assumption that only one thread mutates the list.
2. 2026-09-15 fix — Michael Wilson widens the lock scope and adds GUARDED_BY; reviewed by Thomas Guilbert, landed main@{#1698345}.
3. post-landing — No known exploit; treated as thread-safety hardening / watch-tier.

Proof of concept

No test or exploit ships with this CL; the change is a lock-scope widening plus a compile-time annotation. A demonstration would be a TSan (ThreadSanitizer) reproduction rather than an exploit: run the audio service under TSan and drive rapid StartSnooping/StopSnooping on the control thread (repeatedly attach/detach a tab-audio or loopback capturer) while an output stream is active so OnMoreData iterates snoopers_ on the real-time thread. On the pre-patch code TSan should report a data race on the snoopers_ vector between the unlocked contains/find and the locked realtime read; a reallocation during that window is the theoretical UAF/torn-read path. No memory-corruption exploit is demonstrated and reliable exploitation is not established.

Exploitation

1. precondition — Active audio output with snooping consumers, plus the ability to rapidly toggle snooper registration to align with OnMoreData iterations.
2. primitive — A racing vector reallocation could yield a torn read / transient dangling raw_ptr on the real-time thread — potential UAF read in the sandboxed audio service.
3. reality — Timing-sensitive, sandboxed, and unproven. No control-flow primitive demonstrated; realistic outcome is a crash, not code execution.

Detection & hunting

For defenders and detection engineers:

• TSan data-race reports on snoopers_ — Build the audio service with ThreadSanitizer and exercise snoop start/stop under load; pre-patch races are reported at the contains/find sites.
• Crashes in OnMoreData iterating snoopers_ — Real-time-thread crashes with corrupted vector state or dangling raw_ptr in output_controller are the runtime symptom.

Audit directions

• Other audio-service containers — Look for vectors/maps in services/audio touched by both the control thread and the real-time audio thread that lack GUARDED_BY; apply the same annotate-and-widen pattern.
• Lock-scope-vs-search patterns — Grep for std::ranges::find/contains immediately followed by base::AutoLock (search-before-lock ordering); each is a candidate race where the search should be inside the critical section.