receiver: fix idle-reset use-after-free and re-latch feedback on mid-session source change#74
Open
o2d2-2d2o wants to merge 2 commits into
Open
Conversation
When no RTP arrives for >2 s, the main loop deletes and recreates the global ScreamRx instance to reset receiver state. The pointer swap ran with no lock held, while rtcpPeriodicThread concurrently dereferences the same pointer (getRtcpFbInterval/isFeedback/checkIfFlushAck/ getLastFeedbackT in its feedback-decision condition, evaluated before it acquired lock_scream). If the periodic thread touched screamRx in the window between delete and the new assignment, it read freed memory and the receiver crashed with SIGSEGV. This is easy to hit when the RTP source flaps: a sender behind a flaky cellular/NAT link that repeatedly stalls >2 s then resumes drives the idle reset over and over, eventually losing the race. Fix, all under the existing lock_scream: - hold the lock across the delete/new pointer swap in the idle-reset path; - evaluate the periodic thread's feedback condition and build the feedback packet under the lock (keeping the sendto network I/O outside it, matching the main loop), instead of reading screamRx unlocked in the if-condition; - move pthread_mutex_init(&lock_scream) ahead of pthread_create so the periodic thread, which now locks every iteration, never operates on an uninitialized mutex. No behavioral change beyond synchronization.
In learn-from-source mode the receiver latched the sender's address from the first RTP packet and never revised it. When the sender's public NAT mapping moves mid-session (common on cellular/CGNAT links after tens of minutes), feedback kept going to the stale address, so the sender stopped receiving congestion feedback and its rate controller ran blind. Re-latch outgoing_rtcp_addr(6) whenever the observed source ip:port changes, gated strictly to learn-from-source mode so a seeded receiver (explicit sender_ip) never changes its target. The periodic feedback thread now snapshots the target under lock_scream before sendto, since the receive thread may rewrite it concurrently. Signed-off-by: Otto Nilsson <otto.nilsson@ericsson.com>
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
2 participants
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.
Two receiver robustness fixes, both surfaced running
scream_bw_test_rxinlearn-from-source (reply-to-source) mode against senders on cellular/CGNAT
links. Both are independent commits on top of current
master.1. Use-after-free on the idle reset (
ac45e78)When no RTP arrives for >2 s the receive loop does
delete screamRx; screamRx = new ScreamRx(...)to reset state. The pointerswap and the periodic RTCP thread's reads of
screamRx(its feedback-decisioncondition) were both unlocked. A sender on a flapping link (repeated >2 s
stalls then resume, e.g. a probe losing cellular coverage) can drive the reset
repeatedly until the periodic thread dereferences freed memory and crashes the
receiver with SIGSEGV.
Fix, all under the existing
lock_scream:delete/newswap;packet under the lock, keeping
sendtooutside the critical section;pthread_mutex_initahead ofpthread_create(the periodic thread nowlocks on every iteration, so the mutex must be valid before it starts).
2. Re-latch feedback target on a mid-session source change (
72e2cdc)In learn-from-source mode the receiver latched the sender's address from the
first RTP packet and never revised it. When the sender's public NAT mapping
moves mid-session (routine on CGNAT after tens of minutes), feedback kept going
to the stale address, so the sender received no congestion feedback and its
rate controller ran blind — throughput collapses toward
minrate.Fix: re-latch
outgoing_rtcp_addr(6)whenever the observed RTP source ip:portchanges, gated strictly to learn-from-source mode so a seeded receiver
(explicit
sender_ip) never changes its target. A newsame_peer()helpercompares family + address + port. The periodic feedback thread now snapshots
the target under
lock_screambeforesendto, since the receive thread mayrewrite it concurrently.
Testing
Builds clean (no warnings) with
cmake+makeongcc:13. Verified inproduction: a receiver whose feedback was stranded at a stale CGNAT mapping
(~2.5 Mbit/s, pinned near
minrate) recovered to ~23 Mbit/s once the sourcechange was re-latched.