← All reports

[4] WebRTC incoming sources removed their sink after derived members died

MediumWebCore MediaStream (WebRTC)UAF

The unregister call ran, correctly, after everything it protected was gone

02e76c6

Medium. The unregister call that synchronizes against in-flight media callbacks sat in the base destructor, which C++ guarantees runs only after the derived members those callbacks touch are already gone. Escalation past a crash requires winning a narrow timing race and reclaiming the freed buffer with groomed data.

Object lifetime across a callback registration is normally kept safe by one rule: unregister before you tear down, and make sure the unregister call has confirmed no callback is still in flight. WebKit's WebRTC receive path builds a media source object that registers itself as a sink on a remote audio or video track, and the libwebrtc library then delivers decoded media to it on its own threads rather than the main thread. RemoveSink() is the call that takes libwebrtc's internal sink lock and therefore serializes against a callback already running, so an object is only safe to dismantle once that call has returned.

The angle: a page that keeps an observer vetoing graceful shutdown while dropping the last reference to an incoming track could land a decoded-audio callback inside the destruction window, writing a pointer and an attacker-influenced size field into a freed heap allocation.

Crashes occurred when WebRTC audio/video callbacks accessed destroyed member variables during object destruction. The root cause is C++ destruction-order behavior: the compiler-generated destructors in derived classes 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. While RTCPeerConnection::doClose() normally stops sources via requestToEnd(), there are code paths where the source can reach destruction while still producing data — for example, requestToEnd() is blocked if any RealtimeMediaSourceObserver returns true from preventSourceFromEnding(). If the source is still producing data when destruction begins, the base class destructor's stop() does call RemoveSink() (which properly synchronizes with in-progress OnData callbacks via sink_lock_ in RemoteAudioSource), but by that point derived members like m_audioBufferList are already destroyed. The fix ensures derived destructors call stop() to remove sinks before any member destruction occurs. Originally landed as 305413.429@rapid/safari-7624.2.5.110-branch.

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();
+}
+
void RealtimeIncomingAudioSourceCocoa::startProducingData()
...
void RealtimeIncomingAudioSourceCocoa::OnData(const void* audioData, int bitsPerSample, int sampleRate, size_t numberOfChannels, size_t numberOfFrames)
{
...
auto& bufferList = *m_audioBufferList->buffer(0);
bufferList.mDataByteSize = numberOfChannels * numberOfFrames * bitsPerSample / 8;
bufferList.mNumberChannels = numberOfChannels;
bufferList.mData = const_cast<void*>(audioData);
audioSamplesAvailable(mediaTime, *m_audioBufferList, m_streamDescription, numberOfFrames);
}

The change moves sink removal out of the two base-class destructors into every concrete subclass, adds four out-of-line subclass destructors spanning the Cocoa and GStreamer ports, and leaves a debug-build contract check behind in the bases.

RealtimeIncomingAudioSource::~RealtimeIncomingAudioSource() and RealtimeIncomingVideoSource::~RealtimeIncomingVideoSource() no longer call stop(); they now carry ASSERT(!isProducingData()) plus a comment stating the new contract, followed by the existing m_audioTrack->UnregisterObserver(this) / m_videoTrack->UnregisterObserver(this).

Four subclass destructors are added or given a body whose first and only statement is stop(): RealtimeIncomingAudioSourceCocoa and RealtimeIncomingVideoSourceCocoa (both new, declared in their headers), RealtimeIncomingAudioSourceLibWebRTC (new), and RealtimeIncomingVideoSourceLibWebRTC (previously an empty inline { } body in the header, now out-of-line with the stop() call). startProducingData() / stopProducingData() are untouched and still perform AddSink/AddOrUpdateSink and RemoveSink on the libwebrtc track interfaces.

Teardown of a cross-thread callback registration performed in the base destructor, after derived members the callback reads have already been destroyed.

C++ destruction order. When an object is destroyed, the most-derived destructor body runs first, then that class's non-static data members are destroyed in reverse declaration order, and only then does the base class destructor body run. A base destructor therefore cannot observe or protect derived-class members.

libwebrtc sinks. webrtc::AudioTrackInterface::AddSink()/RemoveSink() and webrtc::VideoTrackInterface::AddOrUpdateSink()/RemoveSink() register an object to receive decoded remote media. The callbacks — AudioTrackSinkInterface::OnData, VideoSinkInterface::OnFrame — are delivered on libwebrtc's own audio and video threads, not the main thread. RemoveSink() is the operation that takes libwebrtc's internal sink lock and therefore serializes against callbacks already in progress.

RealtimeMediaSource lifecycle. start()/stop() toggle the producing state; stop() calls the virtual stopProducingData(), which for these classes performs RemoveSink(this). isProducingData() reports whether the source is currently active. requestToEnd() is the cooperative shutdown path used by RTCPeerConnection::doClose(), and an observer can veto it by returning true from RealtimeMediaSourceObserver::preventSourceFromEnding().

WebAudioBufferList. A WebCore wrapper owning a CoreAudio AudioBufferList allocation; buffer(0) returns a reference to the first AudioBuffer struct inside that allocation, whose mData/mDataByteSize/mNumberChannels fields the audio callback fills in before handing the list to audioSamplesAvailable().

RetainPtr and Lock. RetainPtr is a CF/Objective-C smart pointer that releases its object on destruction; WTF::Lock is a plain mutex object whose lifetime is tied to the enclosing object.

This is a use-after-free produced by destruction order colliding with a cross-thread callback.

  ~RealtimeIncomingAudioSourceCocoa (pre-fix)   WebRTC audio thread
  ──────────────────────────────────────────    ───────────────────
  derived dtor body: (empty)
  destroy m_logTimer
  destroy m_audioBufferList -> free()
  destroy m_streamDescription
                                    ────────►   OnData(): m_audioBufferList->buffer(0)
                                                writes mDataByteSize / mNumberChannels
                                                / mData into the freed allocation
  ~RealtimeIncomingAudioSource:
    stop() -> RemoveSink()  <-- synchronization arrives too late

Until RemoveSink() returns, the WebRTC delivery thread can be executing OnData()/OnFrame() on this object. Because that call happened only in the base destructor, there was a window running from the start of derived member destruction to the base destructor body, in which a concurrent OnData() dereferences m_audioBufferList after its unique_ptr has already run ~WebAudioBufferList() and freed the backing allocation, then writes three fields into that freed allocation and passes *m_audioBufferList to audioSamplesAvailable(). On the video side, a concurrent OnFrame() would take m_pixelBufferPoolLock after that Lock object itself had been destroyed and read m_pixelBufferPool / m_blackFrame RetainPtrs whose CFRelease had already run.

Why destruction can begin while the source is still producing data comes from the commit message: RTCPeerConnection::doClose() normally ends sources through requestToEnd(), but any observer returning true from preventSourceFromEnding() blocks that path, so the final reference can drop on a still-active source. None of doClose(), requestToEnd(), or preventSourceFromEnding() appears in the supplied files, so that reachability precondition is relayed from the commit message.

Reachability is from web content: a page creates an RTCPeerConnection, negotiates an inbound audio and/or video track, and thereby causes a RealtimeIncomingAudioSourceCocoa/RealtimeIncomingVideoSourceCocoa to be created and started — RealtimeIncomingAudioSource::create() calls source->start() immediately. Both sides of the race are script-influenced: the destruction moment follows when the last Ref to the source drops (closing the peer connection, dropping MediaStreamTrack references, removing transceivers), and the callback cadence follows the remote peer, whose packet rate and audio format the attacker also controls if they own the far end of the connection.

The immediate observable effect is memory corruption on the WebRTC audio or video thread and, most commonly, a crash — which is what the commit message reports. If the freed WebAudioBufferList allocation were reclaimed by an attacker-groomed object before OnData() executes its field stores, the write of mData and mDataByteSize into that reclaimed slot could give a constrained heap-write primitive: one pointer-sized value the attacker does not directly choose (a libwebrtc buffer address, useful as a pointer plant) and one 32-bit size field they do influence through channel count, frame count, and sample width. If instead the freed allocation were reclaimed by attacker-controlled data before buffer(0) is dereferenced, the subsequent audioSamplesAvailable(..., *m_audioBufferList, m_streamDescription, numberOfFrames) consumption could turn a forged AudioBuffer into an out-of-bounds read or write in the audio pipeline — conditional, since audioSamplesAvailable()'s implementation is outside the supplied context. On the video side, operating on a destroyed WTF::Lock and on already-released RetainPtrs could yield an over-release or reclaimed-CF-object condition rather than a linear heap write.

This vulnerability weakens memory safety in the process hosting the WebRTC receive path. The assumption at stake is that an object registered as a callback sink on another thread is fully unregistered — with the registrar's synchronization completing — before any of its state is torn down; before the fix that held only for base-class members, not for derived ones. The GStreamer variants carry the same shape on non-Apple ports.

The fix deliberately trades a structurally tidy pattern (deregister once, in the base destructor) for a duplicated but correct one (deregister in every leaf destructor), because the base destructor is architecturally too late. That is the general rule for RAII teardown of a cross-thread registration: unregister at the top of the most-derived destructor, the earliest point at which all of the object's state is still intact. Worth carrying forward, though: the safety net for the new contract is ASSERT(!isProducingData()), compiled out in release builds. A future subclass added without its own stop()-calling destructor would, in release builds, never remove the sink at all — leaving a fully dangling sink pointer registered on the libwebrtc track, a strictly worse failure mode than the one being fixed. A RELEASE_ASSERT, or retaining the base-class stop() as a belt-and-braces second call, would close that gap.