fix(decklink): wait for playback to stop before destroying consumer (teardown-race use-after-free) - #1763
Open
C-alcaide wants to merge 1 commit into
Open
fix(decklink): wait for playback to stop before destroying consumer (teardown-race use-after-free)#1763C-alcaide wants to merge 1 commit into
C-alcaide wants to merge 1 commit into
Conversation
…to fix teardown-race use-after-free The DeckLink consumer (and each secondary port) registers itself as the IDeckLinkVideoOutputCallback via SetScheduledFrameCompletionCallback(this). ScheduledFrameCompleted() runs on the driver's thread and dereferences many members (graph_, mode_, buffer_, secondary_port_contexts_, ...). On destruction the code called StopScheduledPlayback() and then immediately DisableVideoOutput() and destroyed its members, without waiting for the driver to confirm that playback had stopped. StopScheduledPlayback is asynchronous: the driver may still deliver ScheduledFrameCompleted callbacks until it fires ScheduledPlaybackHasStopped. If such a callback is in flight while the consumer is being torn down, it touches members that are being destroyed - a use-after-free that can corrupt the heap (STATUS_HEAP_CORRUPTION / 0xC0000374) or access-violate, typically at end-of-broadcast teardown or channel reconfiguration. Fix follows the Blackmagic DeckLink SDK SynchronizedPlayback example: after StopScheduledPlayback, block until ScheduledPlaybackHasStopped signals a condition variable, then unregister the completion callback (SetScheduledFrameCompletionCallback(nullptr)) before DisableVideoOutput and member destruction. A 2s timeout guards against a missing callback. Applied to both decklink_consumer and decklink_secondary_port.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The DeckLink consumer can crash the server during teardown with heap corruption (
STATUS_HEAP_CORRUPTION/0xC0000374) or an access violation, because it destroys itself while a frame-completion callback may still be running on the DeckLink driver's thread. This PR makes teardown wait for the driver to confirm playback has stopped, and unregisters the completion callback before destruction — following the sequence in Blackmagic's ownSynchronizedPlaybackSDK example.The bug
decklink_consumer(and eachdecklink_secondary_port) registers itself as theIDeckLinkVideoOutputCallbackviaSetScheduledFrameCompletionCallback(this). The primary consumer'sScheduledFrameCompleted()runs on the driver's thread and dereferences many members (graph_,mode_,buffer_,secondary_port_contexts_, …).On destruction the code did:
StopScheduledPlaybackis asynchronous — the driver can keep deliveringScheduledFrameCompletedcallbacks until it firesScheduledPlaybackHasStopped. The destructor never waited for that, and never unregistered the callback. So if a completion callback is in flight while the consumer is being torn down, it touches members that are being destroyed → use-after-free.This is timing-dependent (the callback must be in flight at the exact moment of teardown), so it manifests as a rare, log-silent crash — most often at end-of-broadcast teardown (
REMOVE/ consumer uninitialize) or on channel reconfiguration/restart.Why the async behaviour is expected
The Blackmagic DeckLink SDK
SynchronizedPlaybackexample maintains a dedicatedstd::mutex+std::condition_variable+bool m_stoppedfor exactly this: it callsStopScheduledPlayback, thenwaitForPlaybackStop()(m_stopCondition.wait), and only proceeds toDisableVideoOutput()+SetScheduledFrameCompletionCallback(nullptr)+ release onceScheduledPlaybackHasStopped()has fired. The existence of that wait in the reference code is the confirmation thatStopScheduledPlaybackdoes not synchronously fence callbacks.The fix
Mirror the SDK teardown sequence in both
decklink_consumeranddecklink_secondary_port:StopScheduledPlayback(...)ScheduledPlaybackHasStopped()signals a condition variable (with a 2s timeout as a safety net against a missing callback).SetScheduledFrameCompletionCallback(nullptr)— unregister so the driver cannot call back into the object.DisableVideoOutput()and destroy members.ScheduledPlaybackHasStopped()now sets the flag and notifies;start_playback()resets it beforeStartScheduledPlayback. For sync-group secondary ports (which are stopped via the primary), the primary already waits before it destroys them, so they only unregister their callback and disable output.Scope / risk
+58/-1; no behavioural change to the running (non-teardown) path.Validation
Verified by code/SDK analysis and a clean build. Because the underlying crash is rare and timing-dependent, this has not been reproduced on a bench; it is offered as a correctness fix that closes a real use-after-free window and matches the vendor's documented teardown sequence. Production soak (absence of
0xC0000374at teardown) is the intended real-world validation.