fix(webrtc-sys): capture add_ice_candidate completion state by value - #1392
Conversation
|
|
|
The The same test fails identically on |
stephen-derosa
left a comment
stephen-derosa
left a comment
There was a problem hiding this comment.
@cliqer thanks for opening this fix! We have a PR for i believe the same bug here:
#1388. #1388 does guard against a second invocation of completion, but to be frank I dont think this is a reachable code path is reachable. It also adds tests for the explicit dereferencing of freed stack memory.
Would you be open to pulling in the relevant tests from #1388 into this PR?
Im glad you have been proving out this fix for the last few months!
|
@stephen-derosa happy to. Pulled the three
I left out the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Done: reverted |
|
@cliqer after you rebase, tests should pass and we can merge in |
…alue AddIceCandidate's completion callback fires asynchronously on the signaling thread and is deferred behind the operations chain when it is busy (for example while a SetRemoteDescription is in flight). The lambda captured `ctx` and `on_complete` by reference, so a deferred completion dereferenced a stack frame that had already returned and crashed the signaling thread with a null function pointer call. Move both into a shared_ptr owned by the lambda so the callback holds its own state regardless of when libwebrtc runs it.
…operations chain Ports the three regression tests from livekit#1388: a completion deferred behind a busy operations chain resolves, closing the peer connection during a deferred completion does not read released state, and the deferred path repeated 25 times surfaces no use-after-free. Without the capture-by-value fix the first test fails with "add_ice_candidate cancelled" and the test process dies on the freed callback state.
libwebrtc keeps the completion in a copyable std::function. Hold the context in a shared optional and exchange it out before calling into Rust, so a second invocation of any copy is ignored instead of moving an already-moved Box.
This reverts commit 01466f3.
… by the operations chain" This reverts commit 7a2a8d6.
1714a4d to
5a84f00
Compare
|
Rebased onto main ( Thanks @stephen-derosa and @lukasIO for the quick reviews and for sorting the test-server drift. Happy to see this land; it lets us drop the vendored |
|
@cliqer this will be in the next release! |
4 participants
Problem
PeerConnection::add_ice_candidate(webrtc-sys/src/peer_connection.cpp) passes libwebrtc a completion lambda that capturesctxandon_completeby reference:PeerConnectionInterface::AddIceCandidatewith a callback completes asynchronously on the signaling thread, and libwebrtc defers it behind the operations chain whenever the chain is busy (typically while aSetRemoteDescriptionis still in flight). In that case the lambda runs afteradd_ice_candidatehas returned, soctxandon_completeare dangling references into a dead stack frame. The deferred call then invokes freed memory.Observed in production as a SIGSEGV on the
signaling_threadwith PC=0 / LR=0 (a null function pointer call) on the first remote ICE candidate that arrived while the answer was being applied. Reproduces reliably when candidates are forwarded immediately afterset_remote_descriptionwithout waiting for it to settle.Fix
Move
ctxinto ashared_ptrowned by the lambda and captureon_completeby value, so the callback holds its own state for as long as libwebrtc keeps it. No API change.We have shipped this as a vendored patch for several months on macOS desktop builds and the crash has not recurred.