[9] RealtimeIncoming{Audio,Video}Source removes sink after derived members destroyed

Severity: Medium | Component: WebCore WebRTC media pipeline | 02e76c6

Rated Medium because the diff fixes a race-window UAF where libwebrtc media-thread callbacks can dereference destroyed derived state; reaching the window from web content requires driving RTCPeerConnection teardown while a preventSourceFromEnding observer blocks the normal requestToEnd path.

Crashes occurred when WebRTC audio/video callbacks accessed destroyed member variables during object destruction. The compiler-generated destructors in derived classes (e.g. RealtimeIncomingAudioSourceCocoa) destroy derived members before calling the base class destructor, but the base class destructor's stop() call is what removes the audio/video track sink. The fix ensures derived destructors call stop() to remove sinks before any member destruction occurs.

Source/WebCore/platform/mediastream/RealtimeIncomingAudioSource.cpp

 RealtimeIncomingAudioSource::~RealtimeIncomingAudioSource()
 {
-    stop();
+    // Subclasses must call stop() in their destructors to ensure the audio
+    // track sink is removed BEFORE derived members are destroyed. Otherwise,
+    // the OnData callback may access destroyed members on the audio thread.
+    ASSERT(!isProducingData());
     m_audioTrack->UnregisterObserver(this);
 }

Source/WebCore/platform/mediastream/cocoa/RealtimeIncomingAudioSourceCocoa.cpp

+RealtimeIncomingAudioSourceCocoa::~RealtimeIncomingAudioSourceCocoa()
+{
+    stop();
+}

Source/WebCore/platform/mediastream/cocoa/RealtimeIncomingVideoSourceCocoa.mm

+RealtimeIncomingVideoSourceCocoa::~RealtimeIncomingVideoSourceCocoa()
+{
+    stop();
+}

Patch Details

Explicit destructors are added to all four concrete subclasses (RealtimeIncomingAudioSourceCocoa, RealtimeIncomingVideoSourceCocoa, RealtimeIncomingAudioSourceLibWebRTC, RealtimeIncomingVideoSourceLibWebRTC). Each calls stop() to remove the WebRTC audio/video track sink BEFORE C++ proceeds to destroy any derived members. The base destructors no longer call stop(); they retain only m_audioTrack->UnregisterObserver(this) / m_videoTrack->UnregisterObserver(this) and now ASSERT(!isProducingData()) to document and verify the new contract.

Use-after-free across C++ destruction order: a base-class destructor unregistered a still-active multi-threaded callback after the derived members it accesses had already been destroyed.

Background

When a remote peer sends audio or video via RTCPeerConnection, libwebrtc surfaces each remote track as a webrtc::AudioTrackInterface/webrtc::VideoTrackInterface. WebCore wraps each track in a RealtimeIncoming*Source that implements libwebrtc's sink interfaces (AudioTrackSinkInterface::OnData, VideoSinkInterface::OnFrame). The WebKit object registers itself as a sink via AddSink/AddOrUpdateSink from startProducingData() and must remove itself via RemoveSink from stopProducingData() before it dies. libwebrtc invokes OnData/OnFrame on its own media threads, not on the WebCore main thread. RemoveSink inside libwebrtc takes a sink_lock_ that serialises against the callback dispatch.

For a derived class D : B, ~D runs first, then D's member destructors, then ~B — so any operation that must complete while derived state is alive has to happen inside ~D. RTCPeerConnection::doClose() is the normal teardown path and calls requestToEnd() on its sources, but a RealtimeMediaSourceObserver whose preventSourceFromEnding() returns true can block requestToEnd(), so the source can reach destruction with isProducingData() still true.

Analysis

When a RealtimeIncomingAudioSourceCocoa is destroyed, the compiler first runs the derived destructor, then destroys derived data members (m_audioBufferList, the pixel buffer pool), and only then invokes the base destructor. The base destructor was the one calling stop(), which removes the WebRTC sink. That means a media-thread OnData or OnFrame callback could already be running, or could be entered, while the WebRTC track still held a pointer to the partially-destroyed subclass; the callback then dereferences members like m_audioBufferList that the C++ runtime has already torn down.

Although RemoveSink synchronises with in-progress callbacks via sink_lock_ inside libwebrtc/RemoteAudioSource, that synchronisation happens too late — only after the derived state is gone. The normal RTCPeerConnection::doClose() path calls requestToEnd() to stop the source first, but that path can be blocked when a RealtimeMediaSourceObserver returns true from preventSourceFromEnding(), so destruction can be reached while isProducingData() is still true. The fix moves stop() into each derived destructor so the sink is unregistered (and any pending callback joined) while all derived members are still alive; the base destructor now only enforces the invariant via ASSERT(!isProducingData()).

This vulnerability weakened memory safety along the WebRTC remote-media ingestion path. The invariant being violated is that an object implementing a libwebrtc sink interface must outlive any pending sink callback — RemoveSink must complete before any subclass state the callback reads is destroyed. A remote peer (or attacker-controlled JS driving the RTCPeerConnection lifecycle) could race the destruction of a RealtimeIncoming*Source against a still-arriving audio/video callback, producing reads and writes against a partially-destroyed C++ object on a WebRTC media thread. If those memory regions are reclaimed by attacker-influenced allocations, this exposes a UAF primitive inside the WebContent sandbox. Sink-lock synchronisation inside libwebrtc is necessary but not sufficient — it only protects the callback from racing with RemoveSink, not from racing with the derived destructor that runs before RemoveSink.

Audit directions

Derived classes that implement a third-party callback interface (libwebrtc Sink, AVFoundation delegate, GStreamer pad probe, CoreAudio render callback, IOSurface listener) and register this with the foreign system but unregister in a base-class destructor. Audit all classes that inherit from webrtc::AudioTrackSinkInterface, webrtc::VideoSinkInterface, webrtc::ObserverInterface, rtc::VideoSinkInterface and verify that RemoveSink/UnregisterObserver runs from the most-derived destructor. Grep Source/WebCore/platform/mediastream and Source/ThirdParty/libwebrtc/*Sink for AddSink/AddOrUpdateSink and trace the matching RemoveSink.
RealtimeMediaSource subclasses that hold thread-touched state (audio buffers, pixel buffer pools, locks) and override startProducingData/stopProducingData. Verify each concrete subclass has its own destructor calling stop(). Check outgoing-source analogues RealtimeOutgoing{Audio,Video}Source* for symmetric issues.
Teardown invariants protected only by RTCPeerConnection::doClose()/requestToEnd() that can be subverted by a RealtimeMediaSourceObserver returning true from preventSourceFromEnding(). Audit every preventSourceFromEnding() implementation to determine which observers can block source teardown, and for each blocker, verify the source's destruction path remains safe when isProducingData() is still true.
ASSERT(!isProducingData())-style structural contracts on base destructors. Grep ASSERT(!isProducing and ASSERT(!is.*Started) across the WebCore platform layer and check that every subclass — especially those without an explicit destructor — actually satisfies the contract.