[4] WebRTC incoming sources removed their sink after derived members died
The unregister call ran, correctly, after everything it protected was gone
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. WhileRTCPeerConnection::doClose()normally stops sources viarequestToEnd(), there are code paths where the source can reach destruction while still producing data — for example,requestToEnd()is blocked if anyRealtimeMediaSourceObserverreturns true frompreventSourceFromEnding(). If the source is still producing data when destruction begins, the base class destructor'sstop()does callRemoveSink()(which properly synchronizes with in-progressOnDatacallbacks viasink_lock_inRemoteAudioSource), but by that point derived members likem_audioBufferListare already destroyed. The fix ensures derived destructors callstop()to remove sinks before any member destruction occurs. Originally landed as305413.429@rapid/safari-7624.2.5.110-branch.
Source/WebCore/platform/mediastream/RealtimeIncomingAudioSource.cpp
Source/WebCore/platform/mediastream/cocoa/RealtimeIncomingAudioSourceCocoa.cpp
Patch Details
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.
Background
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.
Analysis
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.
Audit directions
- Cross-thread callback deregistration placed in a base destructor. This is dangerous because C++ guarantees derived members die before the base destructor body runs, so the deregistration is always too late for exactly the state the callback touches. Narrow: grep
Source/WebCore/platform/mediastreamandSource/WebCore/Modules/mediastreamfor base-class destructors callingstop(),RemoveSink,removeObserver, orUnregisterObserver, and check whether any concrete subclass declares members read by the corresponding callback — the tell is a base~Foo()performing deregistration whileFooCocoa.h/FooGStreamer.hdeclaresstd::unique_ptr/RetainPtr/Lockmembers named in the callback body. Wider: the same class covers any deferred-unregistration mechanism —NotificationCenter/KVO observer removal,CFRunLoopSourceinvalidation,dispatch_source_cancelwithout waiting for the cancel handler,Timer::stop()on a timer whose fire handler reads derived state — so look for classes where the registration is created in a derived constructor but torn down in a base destructor. Widest: unregister from any concurrent producer at the top of the most-derived destructor, and consider the object destructible only once the producer has confirmed no callback is in flight; applies to Chromium'sbase::ObserverList/SequenceBoundteardown, Rust'sDroporder withArc<dyn Trait>callbacks, and any Java/ObjC listener whoseremoveListenersits in a superclass finalizer. Match tell at that rung: if the unregister call and the state the callback reads live in different classes of the same hierarchy, that is a hit regardless of language. - Release-build behaviour of debug-only lifecycle contracts.
ASSERT(!isProducingData())compiles out in release, so a future subclass omitting its ownstop()-calling destructor would leave the sink registered forever rather than merely racing with it. Narrow: enumerate all subclasses ofRealtimeIncomingAudioSourceandRealtimeIncomingVideoSourceacross every port (cocoa/,libwebrtc/gstreamer/) and confirm each declares a destructor whose first statement isstop(); the tell is afinalsubclass header with no~Foo();declaration. Wider: grepSource/WebCoreforASSERT(inside destructors that encodes a cross-class contract rather than a local sanity check — comments of the form "Subclasses must …" next to a plainASSERTare the shape, and each is a candidate forRELEASE_ASSERTor for a non-virtual-interface helper that cannot be forgotten. Widest: a safety contract enforced only by a build-configuration-dependent check is not enforced in the configuration that ships; carry it into any codebase usingassert/debug_assert!/DCHECKto police invariants a subclass author must uphold, and treat "the assert would have caught it" as a non-answer for release builds. - Investigate whether the veto path that made this reachable creates other "destroyed while still active" states. Cooperative-shutdown APIs that any observer can refuse mean the graceful path is not guaranteed, so every resource released on the graceful path needs a destructor-time fallback. Narrow: trace all callers of
requestToEnd()and all overrides ofpreventSourceFromEnding()inSource/WebCore/platform/mediastream, and for eachRealtimeMediaSourcesubclass check whatstop()/stopProducingData()releases that the destructor does not independently release; the tell is state torn down only instopProducingData()with no corresponding destructor handling. Wider: the same shape appears in any vetoable-teardown protocol —beforeunload-style cancellable shutdown,MediaStreamTrackend negotiation, page-lifecycle freeze/suspend handlers a client can refuse — audit each for what happens when the veto wins and the object is destroyed anyway. Widest: a shutdown step that a third party can veto must not be the only place a resource is released; applies from POSIX signal handlers that decline to exit to Kubernetes preStop hooks. Match tell: any cleanup living exclusively behind a call site guarded by a predicate someone else controls.