fix: evict the oldest pending ICE candidate, not the newest

Candidates arrive in gathering order — host, then srflx, then relay — so a
full buffer was discarding exactly the ones that traverse NAT while keeping
host ones that only work on a shared LAN. Evict from the front instead.

Also document why the controller's ICE bridge must not reconnect on error, in
contrast to the controlled side's per-candidate retry: its socket address is
the return route itself (mangled into PunchHole.socket_addr, echoed back in
IceCandidate.socket_addr, resolved through tcp_punch), so a reconnect would
arrive from an address no route points at, and the server drops the old entry
when the connection closes. Once it dies both directions are dead, and
abandoning WebRTC is the correct response rather than retrying.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
This commit is contained in:
rustdesk
2026-08-06 08:29:06 +08:00
parent c631cdcf22
commit c40cb2a93e

View File

@@ -613,7 +613,9 @@ impl Client {
}
/// Max ICE candidates buffered during the punch window before the answer is applied.
/// Bounds memory if a misbehaving rendezvous floods candidates.
/// Bounds memory if a misbehaving rendezvous floods candidates. On overflow the oldest is
/// evicted: gathering order is host, then srflx, then relay, so the newest arrivals are the
/// ones that traverse NAT.
const MAX_PENDING_WEBRTC_ICE: usize = 64;
/// Prefer-P2P window: how long a WebRTC attempt outranks an already-established relay
@@ -627,6 +629,16 @@ impl Client {
/// flight; the remote ICE agent dedups repeats, so the second copy is free.
const WEBRTC_ICE_RESEND_DELAY: Duration = Duration::from_millis(400);
/// Bridge local ICE candidates to the peer over the punch socket, and feed the peer's back
/// into the pc.
///
/// This socket must not be reconnected on error, unlike the controlled side's sender which
/// dials a fresh connection per candidate. Its address *is* the return route: the server
/// mangles it into `PunchHole.socket_addr`, the peer echoes it back as
/// `IceCandidate.socket_addr`, and the server resolves it through `tcp_punch`. A reconnect
/// would arrive from a new address that no route points at, and the server drops the old
/// entry when this connection closes — so once it dies, both directions are dead and
/// abandoning WebRTC for another transport is the correct response, not retrying.
fn spawn_webrtc_ice_bridge(
mut socket: Stream,
mut local_ice_rx: Option<UnboundedReceiver<String>>,
@@ -1140,14 +1152,18 @@ impl Client {
}
Some(rendezvous_message::Union::IceCandidate(ice)) => {
if Self::is_expected_webrtc_ice_candidate(&ice, &webrtc_session_key) {
if pending_webrtc_ice.len() < Self::MAX_PENDING_WEBRTC_ICE {
pending_webrtc_ice.push(ice.candidate);
} else {
// Evict the oldest, not the newest. Candidates arrive in gathering
// order — host first, then srflx, then relay — so dropping arrivals
// would discard exactly the ones that traverse NAT and keep the
// host ones that only work on a shared LAN.
if pending_webrtc_ice.len() >= Self::MAX_PENDING_WEBRTC_ICE {
log::warn!(
"dropping WebRTC ICE candidate: pending buffer full ({})",
"WebRTC ICE pending buffer full ({}), dropping oldest candidate",
Self::MAX_PENDING_WEBRTC_ICE
);
pending_webrtc_ice.remove(0);
}
pending_webrtc_ice.push(ice.candidate);
} else {
log::debug!(
"dropping ICE candidate for unexpected WebRTC session key {}",