bugfix: PeerConnectionInterface::AddIceCandidate - #1388
Conversation
Changeset incompleteThis PR's changeset is missing version bumps for packages that are affected by the change. The following packages still require a bump:
Already covered:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| resolved with `add_ice_candidate cancelled`, and the later callback read freed memory, | ||
| crashing with `SIGSEGV`/`SIGBUS`. | ||
|
|
||
| The completion state now lives in a shared, single-use object that outlives the call, is |
There was a problem hiding this comment.
this is sort of the main item to highlight in the whole PR
…usly and reacxhes an invalidated reference in edge cases
232f391 to
b925200
Compare
…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.
|
Read through this one properly since it overlaps with #1392; the completion ownership and the single-use claim look right to me, and the three
Happy to close #1392 once this lands. |
|
consumed by #1392 |
…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.
2 participants
Overview
PeerConnection::add_ice_candidatehanded libwebrtc a[&]lambda that capturedctx(therust::Box<PeerContext>) andon_completeby reference — both stack parameters of the shim.PeerConnectionInterface::AddIceCandidatecompletes asynchronously, so whenever libwebrtc'soperations chain deferred the completion, the shim returned and destroyed the captures first.
Dropping the context dropped the pending
oneshot::Sender, so the Rust future resolved withadd_ice_candidate cancelled; the later callback then read freed memory (SIGSEGV/SIGBUS).The callback state now lives in
AddIceCandidateCompletion, held behind ashared_ptrbymake_add_ice_candidate_callback. It outlives the call, is safe for the copies libwebrtc makes(
AddIceCandidatetakes a copyablestd::function, andOperationsChaincopies it), and anatomic guard moves the context back to Rust exactly once.
Reproducing failure (before merge)
The bug only shows when libwebrtc's operations chain is non-empty:
ChainOperationruns anoperation immediately when the chain is idle, so the callback normally fires synchronously inside
the blocking proxy marshal and the dangling captures are still alive. Stacking a
CreateOfferon afreshly created peer connection (its DTLS certificate is still generating) keeps the chain busy and
defers the completion:
Tests
webrtc-sys— four unit tests over a test-only seam that drives the production callbackfactory the way libwebrtc does (build it in a frame that returns, copy it, drop the original, then
invoke): survives-the-frame, error forwarding, exactly-once across repeated invocations,
release-on-drop.
libwebrtc— three integration tests on the existing peer-connection fixture: deferredcompletion resolves, close during a deferred completion, and 25 repeated deferred completions. Each
asserts the chain is genuinely occupied first, so they cannot pass vacuously.
Against the old
[&]implementation: 3/4webrtc-systests SIGBUS, all 3libwebrtctests SIGSEGV.cargo fmt -- --checkclang-formaton new C++ regionscargo test -p libwebrtc -p webrtc-sys -- --test-threads=1cargo clippy -p webrtc-sys -p libwebrtc --all-targetscargo check --workspace --all-targetsMallocScribble/MallocPreScribble/MallocGuardEdgesASan was not run:
-Zsanitizer=addressneeds nightly and the repo pins stable1.97.1; theprebuilt libwebrtc is not instrumented either. macOS malloc guards were used instead.
API breaking changes
None.
PeerConnection::add_ice_candidatekeeps its signature and its success/error results; thechange is confined to how the native shim owns the completion state.