From d4e727306f28aded5f2e461b1aaaad741e9e35fa Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sun, 17 May 2026 15:13:56 +0800 Subject: [PATCH 01/35] feat: add rendezvous WebRTC signaling fields --- protos/rendezvous.proto | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index b2e5c01572..8b7d694ce5 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -29,6 +29,7 @@ message PunchHoleRequest { int32 upnp_port = 9; bytes socket_addr_v6 = 10; string switch_code = 11; + string webrtc_sdp_offer = 12; } message ControlPermissions { @@ -64,6 +65,8 @@ message PunchHole { bytes socket_addr_v6 = 7; ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; + string webrtc_sdp_offer = 10; + string requester_id = 11; } message TestNatRequest { @@ -90,6 +93,7 @@ message PunchHoleSent { string version = 5; int32 upnp_port = 6; bytes socket_addr_v6 = 7; + string webrtc_sdp_answer = 8; } message RegisterPk { @@ -135,6 +139,7 @@ message PunchHoleResponse { bool is_udp = 9; int32 upnp_port = 10; bytes socket_addr_v6 = 11; + string webrtc_sdp_answer = 12; } message ConfigUpdate { @@ -169,6 +174,7 @@ message RelayResponse { int32 feedback = 9; bytes socket_addr_v6 = 10; int32 upnp_port = 11; + string webrtc_sdp_answer = 12; } message SoftwareUpdate { string url = 1; } @@ -240,6 +246,13 @@ message HttpProxyResponse { string error = 4; } +message IceCandidate { + string from_id = 1; + string to_id = 2; + string session_key = 3; + string candidate = 4; +} + message RendezvousMessage { oneof union { RegisterPeer register_peer = 6; @@ -265,5 +278,6 @@ message RendezvousMessage { HealthCheck hc = 26; HttpProxyRequest http_proxy_request = 27; HttpProxyResponse http_proxy_response = 28; + IceCandidate ice_candidate = 29; } } From 9277af24523f209152a2d3938064e80bc99d5ac1 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sun, 17 May 2026 15:18:25 +0800 Subject: [PATCH 02/35] feat: support trickle ICE in WebRTCStream --- src/webrtc.rs | 144 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 109 insertions(+), 35 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 8f3c410cc7..79af7bcceb 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1,13 +1,14 @@ use std::collections::HashMap; use std::io::{Error, ErrorKind}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; -use std::sync::Arc; +use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; use webrtc::api::setting_engine::SettingEngine; use webrtc::api::APIBuilder; use webrtc::data_channel::RTCDataChannel; use webrtc::ice::mdns::MulticastDnsMode; +use webrtc::ice_transport::ice_candidate::RTCIceCandidateInit; use webrtc::ice_transport::ice_server::RTCIceServer; use webrtc::peer_connection::configuration::RTCConfiguration; use webrtc::peer_connection::peer_connection_state::RTCPeerConnectionState; @@ -18,8 +19,7 @@ use webrtc::peer_connection::RTCPeerConnection; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use base64::Engine; use bytes::{Bytes, BytesMut}; -use tokio::sync::watch; -use tokio::sync::Mutex; +use tokio::sync::{mpsc, watch, Mutex}; use tokio::time::timeout; use url::Url; @@ -28,10 +28,19 @@ use crate::protobuf::Message; use crate::sodiumoxide::crypto::secretbox::Key; use crate::ResultType; +#[derive(Clone, Debug, PartialEq, Eq)] +enum WebRTCConnectionState { + Pending, + Open, + Closed(String), +} + pub struct WebRTCStream { pc: Arc, stream: Arc>>, - state_notify: watch::Receiver, + state_notify: watch::Receiver, + local_ice_rx: Arc>>>, + session_key: String, send_timeout: u64, } @@ -59,6 +68,8 @@ impl Clone for WebRTCStream { pc: self.pc.clone(), stream: self.stream.clone(), state_notify: self.state_notify.clone(), + local_ice_rx: self.local_ice_rx.clone(), + session_key: self.session_key.clone(), send_timeout: self.send_timeout, } } @@ -243,16 +254,40 @@ impl WebRTCStream { ..Default::default() }; - let (notify_tx, notify_rx) = watch::channel(false); + let (notify_tx, notify_rx) = watch::channel(WebRTCConnectionState::Pending); + let (ice_tx, ice_rx) = mpsc::unbounded_channel::(); // Create a new RTCPeerConnection let pc = Arc::new(api.new_peer_connection(config).await?); + let local_ice_tx = ice_tx.clone(); + pc.on_ice_candidate(Box::new(move |candidate| { + let local_ice_tx = local_ice_tx.clone(); + Box::pin(async move { + let Some(candidate) = candidate else { + return; + }; + match candidate.to_json() { + Ok(candidate) => match serde_json::to_string(&candidate) { + Ok(candidate_json) => { + let _ = local_ice_tx.send(candidate_json); + } + Err(err) => { + log::warn!("failed to serialize local ICE candidate: {}", err); + } + }, + Err(err) => { + log::warn!("failed to convert local ICE candidate to JSON: {}", err); + } + } + }) + })); + let bootstrap_dc = if start_local_offer { let dc_open_notify = notify_tx.clone(); // Create a data channel with label "bootstrap" let dc = pc.create_data_channel("bootstrap", None).await?; dc.on_open(Box::new(move || { log::debug!("Local data channel bootstrap open."); - let _ = dc_open_notify.send(true); + let _ = dc_open_notify.send(WebRTCConnectionState::Open); Box::pin(async {}) })); dc @@ -277,7 +312,7 @@ impl WebRTCStream { *stream_lock = dc.clone(); drop(stream_lock); dc.on_open(Box::new(move || { - let _ = dc_open_notify2.send(true); + let _ = dc_open_notify2.send(WebRTCConnectionState::Open); Box::pin(async {}) })); }) @@ -297,7 +332,9 @@ impl WebRTCStream { RTCPeerConnectionState::Disconnected | RTCPeerConnectionState::Failed | RTCPeerConnectionState::Closed => { - let _ = on_connection_notify.send(true); + let _ = on_connection_notify.send(WebRTCConnectionState::Closed( + s.to_string(), + )); log::debug!("WebRTC session closing due to disconnected"); let _ = stream_for_close2.lock().await.close().await; log::debug!("WebRTC session stream closed"); @@ -339,9 +376,7 @@ impl WebRTCStream { // process offer/answer if start_local_offer { let sdp = pc.create_offer(None).await?; - let mut gather_complete = pc.gathering_complete_promise().await; pc.set_local_description(sdp.clone()).await?; - let _ = gather_complete.recv().await; log::debug!("local offer:\n{}", sdp.sdp); // get local sdp key @@ -351,9 +386,7 @@ impl WebRTCStream { let sdp = serde_json::from_str::(&remote_offer)?; pc.set_remote_description(sdp.clone()).await?; let answer = pc.create_answer(None).await?; - let mut gather_complete = pc.gathering_complete_promise().await; pc.set_local_description(answer).await?; - let _ = gather_complete.recv().await; log::debug!("remote offer:\n{}", sdp.sdp); // get remote sdp key @@ -371,6 +404,8 @@ impl WebRTCStream { pc, stream, state_notify: notify_rx, + local_ice_rx: Arc::new(StdMutex::new(Some(ice_rx))), + session_key: key.clone(), send_timeout: ms_timeout, }; final_lock.insert(key, webrtc_stream.clone()); @@ -397,6 +432,38 @@ impl WebRTCStream { Ok(()) } + #[inline] + pub fn take_local_ice_rx(&self) -> Option> { + self.local_ice_rx.lock().ok().and_then(|mut rx| rx.take()) + } + + #[inline] + pub async fn add_remote_ice_candidate(&self, candidate_json: &str) -> ResultType<()> { + if candidate_json.is_empty() { + return Ok(()); + } + let candidate = serde_json::from_str::(candidate_json)?; + self.pc.add_ice_candidate(candidate).await?; + Ok(()) + } + + #[inline] + pub fn session_key(&self) -> &str { + &self.session_key + } + + pub async fn wait_connected(&mut self, ms: u64) -> ResultType<()> { + if ms > 0 { + match timeout(Duration::from_millis(ms), self.wait_for_connect_result()).await { + Ok(result) => result?, + Err(_) => return Err(anyhow::anyhow!("WebRTC wait_connected timeout")), + } + } else { + self.wait_for_connect_result().await?; + } + Ok(()) + } + #[inline] pub fn set_raw(&mut self) { // not-supported @@ -435,33 +502,30 @@ impl WebRTCStream { } #[inline] - async fn wait_for_connect_result(&mut self) { - if *self.state_notify.borrow() { - return; + async fn wait_for_connect_result(&mut self) -> ResultType<()> { + loop { + match self.state_notify.borrow().clone() { + WebRTCConnectionState::Open => return Ok(()), + WebRTCConnectionState::Closed(reason) => { + return Err(anyhow::anyhow!("WebRTC connection closed: {}", reason)); + } + WebRTCConnectionState::Pending => {} + } + self.state_notify.changed().await?; } - let _ = self.state_notify.changed().await; } pub async fn send_bytes(&mut self, bytes: Bytes) -> ResultType<()> { - if self.send_timeout > 0 { - match timeout( - Duration::from_millis(self.send_timeout), - self.wait_for_connect_result(), - ) - .await + if let Err(err) = self.wait_connected(self.send_timeout).await { + self.pc.close().await.ok(); + let kind = if err.to_string().contains("deadline") + || err.to_string().contains("timeout") { - Ok(_) => {} - Err(_) => { - self.pc.close().await.ok(); - return Err(Error::new( - ErrorKind::TimedOut, - "WebRTC send wait for connect timeout", - ) - .into()); - } - } - } else { - self.wait_for_connect_result().await; + ErrorKind::TimedOut + } else { + ErrorKind::Other + }; + return Err(Error::new(kind, err.to_string()).into()); } let stream = self.stream.lock().await.clone(); stream.send(&bytes).await?; @@ -470,7 +534,10 @@ impl WebRTCStream { #[inline] pub async fn next(&mut self) -> Option> { - self.wait_for_connect_result().await; + if let Err(err) = self.wait_for_connect_result().await { + self.pc.close().await.ok(); + return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); + } let stream = self.stream.lock().await.clone(); // TODO reuse buffer? @@ -767,4 +834,11 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc "connect to an 'answer' webrtc endpoint should error" ); } + + #[tokio::test] + async fn test_webrtc_wait_connected_timeout() { + let mut stream = WebRTCStream::new("", false, 100).await.unwrap(); + let err = stream.wait_connected(10).await.unwrap_err(); + assert!(err.to_string().contains("timeout")); + } } From 1998a198ec083725497bd37fb2ca3df784fed035 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Mon, 18 May 2026 18:46:58 +0800 Subject: [PATCH 03/35] fix: route WebRTC ICE without requester id --- protos/rendezvous.proto | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index 8b7d694ce5..cdc57ebee4 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -66,7 +66,7 @@ message PunchHole { ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; string webrtc_sdp_offer = 10; - string requester_id = 11; + reserved 11; } message TestNatRequest { @@ -247,8 +247,8 @@ message HttpProxyResponse { } message IceCandidate { - string from_id = 1; - string to_id = 2; + string id = 1; + bytes socket_addr = 2; string session_key = 3; string candidate = 4; } From f98f3e8732198aacbc3be04662a8f5d89600d5cf Mon Sep 17 00:00:00 2001 From: rustdesk Date: Wed, 22 Jul 2026 11:17:30 +0800 Subject: [PATCH 04/35] feat: WebRTC data-plane framing, DTLS binding, and pc-leak fixes - 1-byte-header fragmentation past the 64KB SCTP cap; empty-message and clean-EOF handling - is_relayed() via selected candidate-pair stats for the direct/relayed flag - IdPk.dtls_fingerprint + rendezvous webrtc SDP/IceCandidate proto fields - fix pc leaks: Weak capture breaks the state-handler Arc self-cycle; close pc on new() error paths Co-Authored-By: Claude Opus 4.8 --- Cargo.toml | 4 +- protos/message.proto | 4 + src/bytes_codec.rs | 6 +- src/stream.rs | 58 +++++ src/webrtc.rs | 503 ++++++++++++++++++++++++++++++++++++------- 5 files changed, 492 insertions(+), 83 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 11a49653a2..f22f69e0bb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,7 +68,7 @@ rustls-pki-types = "1.11" rustls-native-certs = "0.8" webpki-roots = "1.0.4" async-recursion = "1.1" -webrtc = { version = "0.14.0", optional = true } +webrtc = { version = "0.13.0", optional = true } libloading = "0.8" [target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies] @@ -81,7 +81,7 @@ protobuf-codegen = { version = "3.7" } [dev-dependencies] clap = "4.5.51" -webrtc = "0.14.0" +webrtc = "0.13.0" [target.'cfg(target_os = "windows")'.dependencies] winapi = { version = "0.3", features = [ diff --git a/protos/message.proto b/protos/message.proto index 8b21368114..2024634cf5 100644 --- a/protos/message.proto +++ b/protos/message.proto @@ -38,6 +38,10 @@ message VideoFrame { message IdPk { string id = 1; bytes pk = 2; + // DTLS certificate fingerprint of the signer's WebRTC endpoint, signed together with id/pk so + // a WebRTC peer's DTLS channel can be bound to its verified identity (defeats a rendezvous/relay + // that swaps SDP fingerprints). Empty for non-WebRTC handshakes. + string dtls_fingerprint = 3; } message DisplayInfo { diff --git a/src/bytes_codec.rs b/src/bytes_codec.rs index cbd53d918f..5bd5590c14 100644 --- a/src/bytes_codec.rs +++ b/src/bytes_codec.rs @@ -4,6 +4,8 @@ use tokio_util::codec::{Decoder, Encoder}; // Bound speculative allocation from untrusted frame headers. const MAX_PREALLOCATED_PAYLOAD_LEN: usize = 256 * 1024; +/// Largest payload representable by the four-byte RustDesk frame header. +pub const MAX_FRAME_LENGTH: usize = 0x3FFF_FFFF; #[derive(Debug, Clone, Copy)] pub struct BytesCodec { @@ -132,7 +134,7 @@ impl Encoder for BytesCodec { let h = (data.len() << 2) as u32 | 0x2; buf.put_u16_le((h & 0xFFFF) as u16); buf.put_u8((h >> 16) as u8); - } else if data.len() <= 0x3FFFFFFF { + } else if data.len() <= MAX_FRAME_LENGTH { buf.put_u32_le((data.len() << 2) as u32 | 0x3); } else { return Err(io::Error::new(io::ErrorKind::InvalidInput, "Overflow")); @@ -290,7 +292,7 @@ mod tests { fn decode_large_frame_header_caps_preallocation() { let mut codec = BytesCodec::new(); let mut buf = BytesMut::new(); - let n = 0x3FFFFFFFusize; + let n = MAX_FRAME_LENGTH; const MAX_REASONABLE_CAPACITY: usize = MAX_PREALLOCATED_PAYLOAD_LEN * 4; buf.put_u32_le((n << 2) as u32 | 0x3); diff --git a/src/stream.rs b/src/stream.rs index a8e6b6c2d1..1c9f381fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -74,6 +74,64 @@ impl Stream { } } + /// Whether this is a WebRTC transport. Used to enforce the WebRTC-only DTLS fingerprint + /// binding (fail closed) in the secure handshake. + #[inline] + pub fn is_webrtc(&self) -> bool { + match self { + #[cfg(feature = "webrtc")] + Stream::WebRTC(_) => true, + #[allow(unreachable_patterns)] + _ => false, + } + } + + /// Close the underlying WebRTC peer connection if this is a WebRTC stream; no-op otherwise. + /// A WebRTC pc is kept alive by the global session cache and its cleanup handler only fires on + /// a terminal ICE state, so it must be closed explicitly at session end. TCP/WebSocket streams + /// release their resources on drop and need nothing here. + #[inline] + pub async fn close_webrtc(&self) { + match self { + #[cfg(feature = "webrtc")] + Stream::WebRTC(s) => s.close().await, + #[allow(unreachable_patterns)] + _ => {} + } + } + + /// Whether an established WebRTC transport runs through a TURN relay (used for the UI's + /// direct/relayed flag). None for non-WebRTC transports or before ICE selects a pair. + #[inline] + pub async fn webrtc_relayed(&self) -> Option { + match self { + #[cfg(feature = "webrtc")] + Stream::WebRTC(s) => s.is_relayed().await, + #[allow(unreachable_patterns)] + _ => None, + } + } + + /// DTLS certificate fingerprint for a WebRTC stream (`local`=true for this endpoint's own + /// cert, false for the peer's), used to bind the channel to the signed peer identity. + /// Returns None for non-WebRTC transports, which authenticate via the secretbox key exchange. + #[inline] + #[cfg_attr(not(feature = "webrtc"), allow(unused_variables))] + pub async fn dtls_fingerprint(&self, local: bool) -> Option { + match self { + #[cfg(feature = "webrtc")] + Stream::WebRTC(s) => { + if local { + s.local_dtls_fingerprint().await.ok() + } else { + s.remote_dtls_fingerprint().await.ok() + } + } + #[allow(unreachable_patterns)] + _ => None, + } + } + #[inline] pub async fn next_timeout( &mut self, diff --git a/src/webrtc.rs b/src/webrtc.rs index 79af7bcceb..e05ae04f7c 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1,28 +1,33 @@ use std::collections::HashMap; use std::io::{Error, ErrorKind}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; use webrtc::api::setting_engine::SettingEngine; use webrtc::api::APIBuilder; +use webrtc::data::data_channel::DataChannel as DetachedDataChannel; use webrtc::data_channel::RTCDataChannel; use webrtc::ice::mdns::MulticastDnsMode; use webrtc::ice_transport::ice_candidate::RTCIceCandidateInit; +use webrtc::ice_transport::ice_candidate_type::RTCIceCandidateType; use webrtc::ice_transport::ice_server::RTCIceServer; use webrtc::peer_connection::configuration::RTCConfiguration; use webrtc::peer_connection::peer_connection_state::RTCPeerConnectionState; use webrtc::peer_connection::policy::ice_transport_policy::RTCIceTransportPolicy; use webrtc::peer_connection::sdp::session_description::RTCSessionDescription; use webrtc::peer_connection::RTCPeerConnection; +use webrtc::stats::StatsReportType; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use base64::Engine; -use bytes::{Bytes, BytesMut}; +use bytes::{BufMut, Bytes, BytesMut}; use tokio::sync::{mpsc, watch, Mutex}; use tokio::time::timeout; use url::Url; +use crate::bytes_codec::MAX_FRAME_LENGTH; use crate::config; use crate::protobuf::Message; use crate::sodiumoxide::crypto::secretbox::Key; @@ -42,12 +47,41 @@ pub struct WebRTCStream { local_ice_rx: Arc>>>, session_key: String, send_timeout: u64, + // Built with Relay-only ICE policy (force_relay): every selected pair goes through TURN. + relay_only: bool, + // Detached data channel, cached after the first `detach()` so send/recv do not re-lock and + // re-fetch it per message. Shared across clones; `detach()` is idempotent. + detached: Arc>>>, + // Receive-side reassembly state, guarded by a single mutex so the fragment accumulator + // survives `next()` cancellation (e.g. `next_timeout`) instead of losing already-read + // fragments mid-message. Assumes a single reader, consistent with the rest of the stream API. + recv_state: Arc>, + // True once the controller has completed the RustDesk identity binding (DTLS fingerprint + // matched to the signed peer id, via `set_key`). DTLS always encrypts; this flag mirrors TCP's + // "secured after key exchange" so key-less / unbound WebRTC is not shown as peer-authenticated. + peer_verified: Arc, } -/// Standard maximum message size for WebRTC data channels (RFC 8831, 65535 bytes). -/// Most browsers, including Chromium, enforce this protocol limit. -const DATA_CHANNEL_BUFFER_SIZE: u16 = u16::MAX; +#[derive(Default)] +struct RecvState { + // Accumulated payload of the logical message currently being reassembled. + acc: BytesMut, + // Reused read scratch buffer, avoiding a per-message allocation. + scratch: Vec, +} +// The SCTP data channel's 65536-byte max message size is handled by +// splitting a logical message into fragments carrying a 1-byte header. Fragment payload is kept +// well under the limit so header+payload never reaches the exact-65536 boundary that the +// receiver's reassembly would truncate with data loss. +const MAX_FRAGMENT_PAYLOAD: usize = 60000; +/// Receive scratch size: must be >= 1 (fragment header) + `MAX_FRAGMENT_PAYLOAD` and fit the +/// negotiated SCTP max message size. +const RECV_BUF_SIZE: usize = 64 * 1024; +/// Fragment header byte: more fragments follow for this logical message. +const FRAG_MORE: u8 = 1; +/// Fragment header byte: final (or only) fragment of a logical message. +const FRAG_END: u8 = 0; // use 3 public STUN servers to find out the NAT type, 2 must be the same address but different ports // https://stackoverflow.com/questions/72805316/determine-nat-mapping-behaviour-using-two-stun-servers // luckily nextcloud supports two ports for STUN @@ -71,6 +105,10 @@ impl Clone for WebRTCStream { local_ice_rx: self.local_ice_rx.clone(), session_key: self.session_key.clone(), send_timeout: self.send_timeout, + relay_only: self.relay_only, + detached: self.detached.clone(), + recv_state: self.recv_state.clone(), + peer_verified: self.peer_verified.clone(), } } } @@ -128,12 +166,25 @@ impl WebRTCStream { Ok(fingerprint.to_string()) } + /// Process-local SESSIONS-map key: the DTLS fingerprint prefixed by role. An offerer and an + /// answerer that share a fingerprint (a single process connecting to its own id) would + /// otherwise collide, handing the offerer back as the answerer. The wire-level `session_key` + /// used for ICE-candidate routing stays the bare fingerprint so both peers still match. + #[inline] + fn cache_key(fingerprint: &str, is_offerer: bool) -> String { + format!( + "{}:{}", + if is_offerer { "offer" } else { "answer" }, + fingerprint + ) + } + #[inline] fn get_key_for_sdp_json(sdp_json: &str) -> ResultType { if sdp_json.is_empty() { return Ok("".to_string()); } - let sdp = serde_json::from_str::(&sdp_json)?; + let sdp = serde_json::from_str::(sdp_json)?; Self::get_key_for_sdp(&sdp) } @@ -177,6 +228,17 @@ impl WebRTCStream { } } + /// Whether the ICE configuration contains a usable TURN server. A Relay-policy peer + /// connection (force_relay) can only gather relay candidates, so without a TURN server it can + /// never connect — callers use this to skip building a guaranteed-dead pc. + pub fn has_turn_server() -> bool { + Self::get_ice_servers().iter().any(|s| { + s.urls + .iter() + .any(|u| u.starts_with("turn:") || u.starts_with("turns:")) + }) + } + #[inline] fn get_ice_servers() -> Vec { let mut ice_servers = Vec::new(); @@ -217,7 +279,12 @@ impl WebRTCStream { force_relay: bool, ms_timeout: u64, ) -> ResultType { - log::debug!("New webrtc stream to endpoint: {}", remote_endpoint); + // The endpoint contains a Base64-encoded SDP with host addresses and live ICE + // credentials. Log only its size so debug logs cannot disclose that information. + log::debug!( + "New webrtc stream (remote endpoint: {} bytes)", + remote_endpoint.len() + ); let remote_offer = if remote_endpoint.is_empty() { "".into() } else { @@ -225,16 +292,16 @@ impl WebRTCStream { }; let mut key = Self::get_key_for_sdp_json(&remote_offer)?; - let sessions_lock = SESSIONS.lock().await; - if let Some(cached_stream) = sessions_lock.get(&key) { - if !key.is_empty() { + let start_local_offer = remote_offer.is_empty(); + if !key.is_empty() { + let sessions_lock = SESSIONS.lock().await; + if let Some(cached_stream) = + sessions_lock.get(&Self::cache_key(&key, start_local_offer)) + { log::debug!("Start webrtc with cached peer"); return Ok(cached_stream.clone()); } } - drop(sessions_lock); - - let start_local_offer = remote_offer.is_empty(); // Create a SettingEngine and enable Detach let mut s = SettingEngine::default(); s.detach_data_channels(); @@ -284,7 +351,14 @@ impl WebRTCStream { let bootstrap_dc = if start_local_offer { let dc_open_notify = notify_tx.clone(); // Create a data channel with label "bootstrap" - let dc = pc.create_data_channel("bootstrap", None).await?; + let dc = match pc.create_data_channel("bootstrap", None).await { + Ok(dc) => dc, + Err(e) => { + // Close before propagating: the pc is live and would otherwise leak. + pc.close().await.ok(); + return Err(e.into()); + } + }; dc.on_open(Box::new(move || { log::debug!("Local data channel bootstrap open."); let _ = dc_open_notify.send(WebRTCConnectionState::Open); @@ -321,7 +395,12 @@ impl WebRTCStream { // This will notify you when the peer has connected/disconnected let stream_for_close = stream.clone(); - let pc_for_close = pc.clone(); + // Weak, not strong: a handler stored inside the pc that captured a strong + // `Arc` forms a pc -> internal -> handler -> pc cycle that `close()` + // never breaks (it only fires the handler) and no `Drop` clears, permanently leaking every + // pc and the ICE-candidate sender's forwarding task. Upgrade inside the handler; if the pc + // is already gone there is nothing left in SESSIONS to evict. + let pc_for_close = Arc::downgrade(&pc); pc.on_peer_connection_state_change(Box::new(move |s: RTCPeerConnectionState| { let stream_for_close2 = stream_for_close.clone(); let on_connection_notify = notify_tx.clone(); @@ -329,21 +408,35 @@ impl WebRTCStream { Box::pin(async move { log::debug!("WebRTC session peer connection state: {}", s); match s { - RTCPeerConnectionState::Disconnected - | RTCPeerConnectionState::Failed - | RTCPeerConnectionState::Closed => { + // `Disconnected` is a transient, recoverable ICE state (webrtc-ice fires it + // after ~5s without consent and returns to `Connected` when traffic resumes). + // Only tear down on the terminal states so a short network blip (Wi-Fi roam, + // sleep/wake, cell handover) does not permanently kill an established session. + RTCPeerConnectionState::Failed | RTCPeerConnectionState::Closed => { let _ = on_connection_notify.send(WebRTCConnectionState::Closed( s.to_string(), )); - log::debug!("WebRTC session closing due to disconnected"); + log::debug!("WebRTC session closing due to {}", s); let _ = stream_for_close2.lock().await.close().await; log::debug!("WebRTC session stream closed"); + let Some(pc_for_close2) = pc_for_close2.upgrade() else { + return; + }; let mut sessions_lock = SESSIONS.lock().await; match Self::get_key_for_peer(&pc_for_close2, start_local_offer).await { - Ok(k) => { - sessions_lock.remove(&k); - log::debug!("WebRTC session removed key: {}", k); + Ok(fingerprint) => { + let k = Self::cache_key(&fingerprint, start_local_offer); + // Only evict if the cached entry IS this pc: a duplicate offer + // resolves to the same key, and closing the discarded duplicate pc + // must not remove the live winner sharing that key. + if sessions_lock + .get(&k) + .is_some_and(|s| Arc::ptr_eq(&s.pc, &pc_for_close2)) + { + sessions_lock.remove(&k); + log::debug!("WebRTC session removed key: {}", k); + } } Err(e) => { log::error!( @@ -374,31 +467,45 @@ impl WebRTCStream { })); // process offer/answer - if start_local_offer { - let sdp = pc.create_offer(None).await?; - pc.set_local_description(sdp.clone()).await?; - - log::debug!("local offer:\n{}", sdp.sdp); - // get local sdp key - key = Self::get_key_for_sdp(&sdp)?; - log::debug!("Start webrtc with local key: {}", key); - } else { - let sdp = serde_json::from_str::(&remote_offer)?; - pc.set_remote_description(sdp.clone()).await?; - let answer = pc.create_answer(None).await?; - pc.set_local_description(answer).await?; - - log::debug!("remote offer:\n{}", sdp.sdp); - // get remote sdp key - key = Self::get_key_for_sdp(&sdp)?; - log::debug!("Start webrtc with remote key: {}", key); - } - - let mut final_lock = SESSIONS.lock().await; - if let Some(session) = final_lock.get(&key) { - pc.close().await.ok(); - return Ok(session.clone()); + // + // Trickle ICE: the local description is returned WITHOUT waiting for candidate gathering + // (candidates stream out via `take_local_ice_rx` afterwards), so this block is local-only + // work — pc construction, DTLS cert keygen, SDP marshal — at sub-millisecond cost. The + // controlled side awaits answer creation inline on its punch-reply critical path and + // relies on that: adding any gathering/network wait here would delay the TCP/UDP + // hole-punch reply for every connection. + // Any failure below leaves a live pc with handlers already registered; its state handler + // only fires on a terminal ICE state, so a bare `?`-drop would leak it (remotely + // triggerable: a crafted `type:"answer"` offer passes the JSON+fingerprint pre-check but + // fails `set_remote_description`). Close the pc before propagating any such error. + let offer_answer: ResultType = async { + if start_local_offer { + let sdp = pc.create_offer(None).await?; + pc.set_local_description(sdp.clone()).await?; + // SDP carries host/srflx IPs and ICE ufrag/pwd; log only its size, not the body. + log::debug!("local offer SDP built ({} bytes)", sdp.sdp.len()); + let k = Self::get_key_for_sdp(&sdp)?; + log::debug!("Start webrtc with local key: {}", k); + Ok(k) + } else { + let sdp = serde_json::from_str::(&remote_offer)?; + pc.set_remote_description(sdp.clone()).await?; + let answer = pc.create_answer(None).await?; + pc.set_local_description(answer).await?; + log::debug!("remote offer SDP received ({} bytes)", sdp.sdp.len()); + let k = Self::get_key_for_sdp(&sdp)?; + log::debug!("Start webrtc with remote key: {}", k); + Ok(k) + } } + .await; + key = match offer_answer { + Ok(k) => k, + Err(e) => { + pc.close().await.ok(); + return Err(e); + } + }; let webrtc_stream = Self { pc, @@ -407,8 +514,30 @@ impl WebRTCStream { local_ice_rx: Arc::new(StdMutex::new(Some(ice_rx))), session_key: key.clone(), send_timeout: ms_timeout, + relay_only: force_relay, + detached: Arc::new(Mutex::new(None)), + recv_state: Arc::new(Mutex::new(RecvState::default())), + peer_verified: Arc::new(AtomicBool::new(false)), + }; + // Insert into the session cache, but never `await pc.close()` while holding this lock: + // `close()` fires the peer-connection-state handler inline, which itself locks SESSIONS, + // self-deadlocking the whole process. Resolve any duplicate off-lock. + let cache_key = Self::cache_key(&key, start_local_offer); + let duplicate = { + let mut final_lock = SESSIONS.lock().await; + if let Some(session) = final_lock.get(&cache_key) { + Some(session.clone()) + } else { + final_lock.insert(cache_key, webrtc_stream.clone()); + None + } }; - final_lock.insert(key, webrtc_stream.clone()); + if let Some(session) = duplicate { + // A concurrent `new()` already cached an equivalent stream; discard this pc's + // resources (off-lock) and return the cached one. + webrtc_stream.close().await; + return Ok(session); + } Ok(webrtc_stream) } @@ -426,12 +555,58 @@ impl WebRTCStream { #[inline] pub async fn set_remote_endpoint(&self, endpoint: &str) -> ResultType<()> { let offer = Self::get_remote_offer(endpoint)?; - log::debug!("WebRTC set remote sdp: {}", offer); + log::debug!("WebRTC set remote sdp ({} bytes)", offer.len()); let sdp = serde_json::from_str::(&offer)?; self.pc.set_remote_description(sdp).await?; Ok(()) } + /// DTLS certificate fingerprint of the local description (this endpoint's own cert). + #[inline] + pub async fn local_dtls_fingerprint(&self) -> ResultType { + Self::get_key_for_peer(&self.pc, true).await + } + + /// DTLS certificate fingerprint of the remote description (the peer's cert). webrtc-rs + /// verifies the negotiated peer certificate against this fingerprint during the DTLS + /// handshake, so once the channel is open a matching fingerprint identifies the peer's cert. + #[inline] + pub async fn remote_dtls_fingerprint(&self) -> ResultType { + Self::get_key_for_peer(&self.pc, false).await + } + + /// Whether the established connection runs through a TURN relay: `Some(true)` when the pc is + /// Relay-policy (TURN is the only possibility) or the selected ICE candidate pair uses a + /// relay candidate; `None` before a pair is selected. Feeds the UI's direct/relayed flag. + pub async fn is_relayed(&self) -> Option { + if self.relay_only { + return Some(true); + } + let dtls = self.pc.sctp().transport(); + dtls.ice_transport().get_selected_candidate_pair().await?; + + // webrtc 0.13 keeps RTCIceCandidatePair's candidates private. Its stats report exposes + // the selected (nominated) pair and the corresponding candidate types instead. + let stats = self.pc.get_stats().await; + let pair = stats.reports.values().find_map(|report| match report { + StatsReportType::CandidatePair(pair) if pair.nominated => Some(pair), + _ => None, + })?; + let is_relay = |candidate_id: &str| { + matches!( + stats.reports.get(candidate_id), + Some( + StatsReportType::LocalCandidate(candidate) + | StatsReportType::RemoteCandidate(candidate) + ) if RTCIceCandidateType::from(candidate.candidate_type) + == RTCIceCandidateType::Relay + ) + }; + Some( + is_relay(&pair.local_candidate_id) || is_relay(&pair.remote_candidate_id), + ) + } + #[inline] pub fn take_local_ice_rx(&self) -> Option> { self.local_ice_rx.lock().ok().and_then(|mut rx| rx.take()) @@ -464,6 +639,19 @@ impl WebRTCStream { Ok(()) } + /// Explicitly tear down the peer connection. + /// + /// Dropping a `WebRTCStream` handle is not enough to release the underlying + /// `RTCPeerConnection`: the global `SESSIONS` map holds a clone, so the pc (and its + /// ICE/DTLS/STUN resources) would stay alive until it happens to reach a terminal ICE + /// state. Closing here fires `on_peer_connection_state_change`, which removes the + /// `SESSIONS` entry, so callers that abandon a stream (e.g. a raced offerer that lost to + /// another transport) should call this to avoid leaking it. + #[inline] + pub async fn close(&self) { + self.pc.close().await.ok(); + } + #[inline] pub fn set_raw(&mut self) { // not-supported @@ -481,14 +669,16 @@ impl WebRTCStream { #[inline] pub fn set_key(&mut self, _key: Key) { - // not-supported - // WebRTC uses built-in DTLS encryption for secure communication. - // DTLS handles key exchange and encryption automatically, so explicit key management is not required. + // WebRTC traffic is DTLS-encrypted regardless; the secretbox key is unused. + // Callers invoke set_key only after the controller has bound the DTLS fingerprint to the + // verified peer identity (or the controlled side has completed the matching handshake). + // Mark peer-verified so is_secured() matches TCP's post-key-exchange meaning. + self.peer_verified.store(true, Ordering::Release); } #[inline] pub fn is_secured(&self) -> bool { - true + self.peer_verified.load(Ordering::Acquire) } #[inline] @@ -515,20 +705,73 @@ impl WebRTCStream { } } + /// Fetch (and cache) the detached data channel. `detach()` is idempotent and returns a + /// clone of the same underlying channel, so caching it just avoids re-locking per message. + async fn detached_dc(&self) -> ResultType> { + { + let cache = self.detached.lock().await; + if let Some(dc) = cache.as_ref() { + return Ok(dc.clone()); + } + } + let raw = self.stream.lock().await.clone(); + let dc = raw.detach().await?; + let mut cache = self.detached.lock().await; + // Another task may have cached it while we were detaching. + if let Some(existing) = cache.as_ref() { + return Ok(existing.clone()); + } + *cache = Some(dc.clone()); + Ok(dc) + } + pub async fn send_bytes(&mut self, bytes: Bytes) -> ResultType<()> { - if let Err(err) = self.wait_connected(self.send_timeout).await { - self.pc.close().await.ok(); - let kind = if err.to_string().contains("deadline") - || err.to_string().contains("timeout") + let send_timeout = self.send_timeout; + // Bound the WHOLE data-channel send (wait-for-open + every write) by send_timeout, + // mirroring FramedStream. Without this a write can park indefinitely on SCTP + // pending-queue backpressure and connection.rs's timeout timer never runs. + if send_timeout > 0 { + match timeout( + Duration::from_millis(send_timeout), + self.send_bytes_inner(bytes), + ) + .await { - ErrorKind::TimedOut - } else { - ErrorKind::Other - }; - return Err(Error::new(kind, err.to_string()).into()); + Ok(res) => res, + Err(_) => { + self.pc.close().await.ok(); + Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()) + } + } + } else { + self.send_bytes_inner(bytes).await + } + } + + async fn send_bytes_inner(&mut self, bytes: Bytes) -> ResultType<()> { + if bytes.len() > MAX_FRAME_LENGTH { + return Err(Error::new(ErrorKind::InvalidInput, "Overflow").into()); + } + self.wait_for_connect_result().await?; + let dc = self.detached_dc().await?; + let data = bytes.as_ref(); + let mut offset = 0; + // Always emit at least one fragment (a lone FRAG_END header for an empty message), so a + // zero-length data-channel message — which the receiver cannot distinguish from EOF — is + // never sent. + loop { + let end = (offset + MAX_FRAGMENT_PAYLOAD).min(data.len()); + let is_last = end >= data.len(); + let chunk = &data[offset..end]; + let mut framed = BytesMut::with_capacity(1 + chunk.len()); + framed.put_u8(if is_last { FRAG_END } else { FRAG_MORE }); + framed.put_slice(chunk); + dc.write(&framed.freeze()).await?; + offset = end; + if is_last { + break; + } } - let stream = self.stream.lock().await.clone(); - stream.send(&bytes).await?; Ok(()) } @@ -538,30 +781,53 @@ impl WebRTCStream { self.pc.close().await.ok(); return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); } - let stream = self.stream.lock().await.clone(); - - // TODO reuse buffer? - let mut buffer = BytesMut::zeroed(DATA_CHANNEL_BUFFER_SIZE as usize); - let dc = stream.detach().await.ok()?; - let n = match dc.read(&mut buffer).await { - Ok(n) => n, + let dc = match self.detached_dc().await { + Ok(dc) => dc, Err(err) => { + self.pc.close().await.ok(); + return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); + } + }; + // Hold recv_state across the reassembly loop: the accumulator must survive `next()` + // cancellation (e.g. next_timeout) so already-read fragments are not lost mid-message. + let mut st = self.recv_state.lock().await; + if st.scratch.len() < RECV_BUF_SIZE { + st.scratch.resize(RECV_BUF_SIZE, 0); + } + loop { + let RecvState { acc, scratch } = &mut *st; + let n = match dc.read(scratch.as_mut_slice()).await { + Ok(n) => n, + Err(err) => { + self.pc.close().await.ok(); + return Some(Err(Error::new( + ErrorKind::Other, + format!("data channel read error: {}", err), + ))); + } + }; + if n == 0 { + // Clean EOF: the remote reset the stream or shut its write half. An empty logical + // message is represented by a 1-byte header, so it is never confused with EOF. + self.pc.close().await.ok(); + return None; + } + acc.extend_from_slice(&scratch[1..n]); + // Match TCP's maximum frame size while preventing an unbounded FRAG_MORE stream from + // exhausting memory. + if acc.len() > MAX_FRAME_LENGTH { + acc.clear(); self.pc.close().await.ok(); return Some(Err(Error::new( ErrorKind::Other, - format!("data channel read error: {}", err), + "WebRTC reassembled message exceeded maximum frame size", ))); } - }; - if n == 0 { - self.pc.close().await.ok(); - return Some(Err(Error::new( - ErrorKind::Other, - "data channel read exited with 0 bytes", - ))); + if scratch[0] == FRAG_END { + let msg = std::mem::take(acc); + return Some(Ok(msg)); + } } - buffer.truncate(n); - Some(Ok(buffer)) } #[inline] @@ -583,6 +849,8 @@ mod tests { use crate::config; use crate::webrtc::WebRTCStream; use crate::webrtc::DEFAULT_ICE_SERVERS; + use std::time::Duration; + use tokio::time::timeout; use webrtc::peer_connection::sdp::session_description::RTCSessionDescription; #[test] @@ -841,4 +1109,81 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc let err = stream.wait_connected(10).await.unwrap_err(); assert!(err.to_string().contains("timeout")); } + + async fn connect_loopback() -> (WebRTCStream, WebRTCStream) { + let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let offer = offerer.get_local_endpoint().await.unwrap(); + let answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); + let answer = answerer.get_local_endpoint().await.unwrap(); + offerer.set_remote_endpoint(&answer).await.unwrap(); + + // Bridge trickle candidates directly between the two peers, both directions. + let mut off_ice = offerer.take_local_ice_rx().unwrap(); + let mut ans_ice = answerer.take_local_ice_rx().unwrap(); + let answerer_for_ice = answerer.clone(); + let offerer_for_ice = offerer.clone(); + tokio::spawn(async move { + while let Some(c) = off_ice.recv().await { + let _ = answerer_for_ice.add_remote_ice_candidate(&c).await; + } + }); + tokio::spawn(async move { + while let Some(c) = ans_ice.recv().await { + let _ = offerer_for_ice.add_remote_ice_candidate(&c).await; + } + }); + + offerer.wait_connected(20000).await.unwrap(); + let mut answerer = answerer; + answerer.wait_connected(20000).await.unwrap(); + (offerer, answerer) + } + + // In-process offerer<->answerer loopback exercising the send/next data plane that the framing, + // empty-message, and EOF fixes live in. Connects over host candidates (works offline; any + // configured/default STUN just fails in the background without blocking the host pair). + #[tokio::test] + async fn test_webrtc_loopback_roundtrip() { + let connect = async { + let (mut offerer, mut answerer) = connect_loopback().await; + + // Host-candidate loopback is direct, never TURN-relayed. + assert_eq!(offerer.is_relayed().await, Some(false)); + + // Small message. + offerer.send_raw(b"hello".to_vec()).await.unwrap(); + let got = answerer.next().await.unwrap().unwrap(); + assert_eq!(&got[..], b"hello"); + + // Empty message: must round-trip as an empty frame, not be seen as EOF. + offerer.send_raw(Vec::new()).await.unwrap(); + let got = answerer.next().await.unwrap().unwrap(); + assert_eq!(got.len(), 0, "empty message must not be treated as EOF"); + + // Payload far above the 64KB single-message cap: must be fragmented and reassembled. + let big = vec![0xABu8; 200_000]; + offerer.send_raw(big.clone()).await.unwrap(); + let got = answerer.next().await.unwrap().unwrap(); + assert_eq!(got.len(), big.len(), "large message must survive fragmentation"); + assert_eq!(&got[..], &big[..]); + + // Reverse direction. + answerer.send_raw(b"world".to_vec()).await.unwrap(); + let got = offerer.next().await.unwrap().unwrap(); + assert_eq!(&got[..], b"world"); + + // Peer close: the other side observes a clean EOF (None) or a close error, never a hang. + offerer.close().await; + match timeout(Duration::from_secs(10), answerer.next()).await { + Ok(None) | Ok(Some(Err(_))) => {} + Ok(Some(Ok(b))) => panic!("expected EOF after peer close, got {} bytes", b.len()), + Err(_) => panic!("answerer.next() hung after peer close"), + } + answerer.close().await; + }; + timeout(Duration::from_secs(40), connect) + .await + .expect("webrtc loopback did not complete in time"); + } + } From 0952f18b8e734d4851a76c0342c841da7e5b62f1 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Wed, 22 Jul 2026 17:40:51 +0800 Subject: [PATCH 05/35] fix: preserve WebRTC endpoint and send semantics --- src/webrtc.rs | 136 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 122 insertions(+), 14 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index e05ae04f7c..edcfea33de 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -23,8 +23,8 @@ use webrtc::stats::StatsReportType; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use base64::Engine; use bytes::{BufMut, Bytes, BytesMut}; -use tokio::sync::{mpsc, watch, Mutex}; -use tokio::time::timeout; +use tokio::sync::{mpsc, watch, Mutex, Semaphore}; +use tokio::time::{timeout, timeout_at, Instant}; use url::Url; use crate::bytes_codec::MAX_FRAME_LENGTH; @@ -52,6 +52,9 @@ pub struct WebRTCStream { // Detached data channel, cached after the first `detach()` so send/recv do not re-lock and // re-fetch it per message. Shared across clones; `detach()` is idempotent. detached: Arc>>>, + // Serialize a complete logical message across clones. Each fragment is a separate SCTP + // message, so serializing only individual writes would allow two large messages to interleave. + send_gate: Arc, // Receive-side reassembly state, guarded by a single mutex so the fragment accumulator // survives `next()` cancellation (e.g. `next_timeout`) instead of losing already-read // fragments mid-message. Assumes a single reader, consistent with the rest of the stream API. @@ -107,6 +110,7 @@ impl Clone for WebRTCStream { send_timeout: self.send_timeout, relay_only: self.relay_only, detached: self.detached.clone(), + send_gate: self.send_gate.clone(), recv_state: self.recv_state.clone(), peer_verified: self.peer_verified.clone(), } @@ -516,6 +520,7 @@ impl WebRTCStream { send_timeout: ms_timeout, relay_only: force_relay, detached: Arc::new(Mutex::new(None)), + send_gate: Arc::new(Semaphore::new(1)), recv_state: Arc::new(Mutex::new(RecvState::default())), peer_verified: Arc::new(AtomicBool::new(false)), }; @@ -543,6 +548,18 @@ impl WebRTCStream { #[inline] pub async fn get_local_endpoint(&self) -> ResultType { + // Preserve the original one-shot endpoint contract: callers that only exchange this SDP + // do not have a separate path for `take_local_ice_rx`, so their endpoint must contain the + // gathered host/srflx/relay candidates. + let mut gather_complete = self.pc.gathering_complete_promise().await; + let _gathering_channel_closed = gather_complete.recv().await; + self.get_local_endpoint_trickle().await + } + + /// Return the current local description immediately for callers that signal candidates via + /// `take_local_ice_rx`. Unlike `get_local_endpoint`, this does not wait for ICE gathering. + #[inline] + pub async fn get_local_endpoint_trickle(&self) -> ResultType { if let Some(local_desc) = self.pc.local_description().await { let sdp = serde_json::to_string(&local_desc)?; let endpoint = Self::sdp_to_endpoint(&sdp); @@ -727,23 +744,46 @@ impl WebRTCStream { pub async fn send_bytes(&mut self, bytes: Bytes) -> ResultType<()> { let send_timeout = self.send_timeout; + let send_gate = self.send_gate.clone(); // Bound the WHOLE data-channel send (wait-for-open + every write) by send_timeout, - // mirroring FramedStream. Without this a write can park indefinitely on SCTP - // pending-queue backpressure and connection.rs's timeout timer never runs. + // including time queued behind another clone. Without this a write can park indefinitely + // on SCTP pending-queue backpressure and connection.rs's timeout timer never runs. if send_timeout > 0 { - match timeout( - Duration::from_millis(send_timeout), - self.send_bytes_inner(bytes), - ) - .await - { + let deadline = Instant::now() + Duration::from_millis(send_timeout); + let _send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { + Ok(Ok(permit)) => permit, + Ok(Err(err)) => { + return Err(Error::new( + ErrorKind::BrokenPipe, + format!("WebRTC send gate closed: {}", err), + ) + .into()); + } + Err(_) => { + if let Err(err) = self.pc.close().await { + log::warn!("failed to close WebRTC after send timeout: {}", err); + } + return Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()); + } + }; + match timeout_at(deadline, self.send_bytes_inner(bytes)).await { Ok(res) => res, Err(_) => { - self.pc.close().await.ok(); + // Keep the logical-message permit while closing so no waiting clone can append + // a new message after a partially-written fragment sequence. + if let Err(err) = self.pc.close().await { + log::warn!("failed to close WebRTC after send timeout: {}", err); + } Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()) } } } else { + let _send_permit = send_gate.acquire_owned().await.map_err(|err| { + Error::new( + ErrorKind::BrokenPipe, + format!("WebRTC send gate closed: {}", err), + ) + })?; self.send_bytes_inner(bytes).await } } @@ -849,7 +889,8 @@ mod tests { use crate::config; use crate::webrtc::WebRTCStream; use crate::webrtc::DEFAULT_ICE_SERVERS; - use std::time::Duration; + use std::{sync::Arc, time::Duration}; + use tokio::sync::Barrier; use tokio::time::timeout; use webrtc::peer_connection::sdp::session_description::RTCSessionDescription; @@ -1112,9 +1153,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc async fn connect_loopback() -> (WebRTCStream, WebRTCStream) { let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); - let offer = offerer.get_local_endpoint().await.unwrap(); + let offer = offerer.get_local_endpoint_trickle().await.unwrap(); let answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); - let answer = answerer.get_local_endpoint().await.unwrap(); + let answer = answerer.get_local_endpoint_trickle().await.unwrap(); offerer.set_remote_endpoint(&answer).await.unwrap(); // Bridge trickle candidates directly between the two peers, both directions. @@ -1139,6 +1180,26 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc (offerer, answerer) } + // One-shot callers exchange only the endpoints and never consume `take_local_ice_rx`. + #[tokio::test] + async fn test_webrtc_loopback_gathered_endpoints() { + let connect = async { + let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let offer = offerer.get_local_endpoint().await.unwrap(); + let mut answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); + let answer = answerer.get_local_endpoint().await.unwrap(); + offerer.set_remote_endpoint(&answer).await.unwrap(); + + offerer.wait_connected(20000).await.unwrap(); + answerer.wait_connected(20000).await.unwrap(); + offerer.close().await; + answerer.close().await; + }; + timeout(Duration::from_secs(40), connect) + .await + .expect("gathered-endpoint WebRTC loopback did not complete in time"); + } + // In-process offerer<->answerer loopback exercising the send/next data plane that the framing, // empty-message, and EOF fixes live in. Connects over host candidates (works offline; any // configured/default STUN just fails in the background without blocking the host pair). @@ -1186,4 +1247,51 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .expect("webrtc loopback did not complete in time"); } + #[tokio::test] + async fn test_webrtc_concurrent_large_sends_preserve_boundaries() { + let connect = async { + let (offerer, mut answerer) = connect_loopback().await; + let mut sender_a = offerer.clone(); + let mut sender_b = offerer.clone(); + let expected_a = vec![0xAA; 200_000]; + let expected_b = vec![0xBB; 200_000]; + let payload_a = expected_a.clone(); + let payload_b = expected_b.clone(); + let barrier = Arc::new(Barrier::new(3)); + + let barrier_a = barrier.clone(); + let send_a = tokio::spawn(async move { + barrier_a.wait().await; + sender_a.send_raw(payload_a).await + }); + let barrier_b = barrier.clone(); + let send_b = tokio::spawn(async move { + barrier_b.wait().await; + sender_b.send_raw(payload_b).await + }); + + barrier.wait().await; + let receive = async { + let first = answerer.next().await.unwrap().unwrap(); + let second = answerer.next().await.unwrap().unwrap(); + (first, second) + }; + let (send_a, send_b, (first, second)) = tokio::join!(send_a, send_b, receive); + send_a.unwrap().unwrap(); + send_b.unwrap().unwrap(); + + let boundaries_preserved = (first.as_ref() == expected_a.as_slice() + && second.as_ref() == expected_b.as_slice()) + || (first.as_ref() == expected_b.as_slice() + && second.as_ref() == expected_a.as_slice()); + assert!(boundaries_preserved, "concurrent messages were interleaved"); + + offerer.close().await; + answerer.close().await; + }; + timeout(Duration::from_secs(40), connect) + .await + .expect("concurrent WebRTC sends did not complete in time"); + } + } From 6aa8fbe46b3773470ec7f15ead2702b445786814 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sun, 26 Jul 2026 23:46:01 +0800 Subject: [PATCH 06/35] docs: webrtc 0.13 MSRV pin rationale and upgrade checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cargo.toml: record why webrtc is pinned to 0.13 — >=0.14 pulls sdp 0.10 / webrtc-util 0.12 using usize::is_multiple_of (needs rustc >=1.87), while rustdesk CI builds with Rust 1.75 (sciter i128 ABI pin) - module-level upgrade checklist in src/webrtc.rs listing the version-coupled webrtc-rs internals this transport relies on (SCTP write backpressure, 64KB message cap, detach() semantics, handler-capture leak cycle, Disconnected transience, stats-based is_relayed), all verified against webrtc 0.13 / webrtc-data 0.11 / webrtc-sctp 0.12 - send_bytes: document the bounded-backpressure mechanism (128 KiB PendingQueue semaphore + cwnd/rwnd cap) and that it is NOT cancel-safe Co-Authored-By: Claude Fable 5 --- Cargo.toml | 6 +++++- src/webrtc.rs | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index f22f69e0bb..09d2850952 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,6 +68,10 @@ rustls-pki-types = "1.11" rustls-native-certs = "0.8" webpki-roots = "1.0.4" async-recursion = "1.1" +# Pinned to 0.13: webrtc >=0.14 pulls sdp 0.10 / webrtc-util 0.12, which use +# usize::is_multiple_of (needs rustc >=1.87), while rustdesk CI builds with Rust 1.75 +# (sciter i128 ABI pin, flutter-build.yml). Bump only after CI's Rust moves past 1.87, +# and work through the upgrade checklist at the top of src/webrtc.rs first. webrtc = { version = "0.13.0", optional = true } libloading = "0.8" @@ -81,7 +85,7 @@ protobuf-codegen = { version = "3.7" } [dev-dependencies] clap = "4.5.51" -webrtc = "0.13.0" +webrtc = "0.13.0" # keep in lockstep with [dependencies] webrtc (rustc 1.75 pin, see above) [target.'cfg(target_os = "windows")'.dependencies] winapi = { version = "0.3", features = [ diff --git a/src/webrtc.rs b/src/webrtc.rs index edcfea33de..7e0be30870 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1,3 +1,32 @@ +//! WebRTC transport for RustDesk streams. +//! +//! # webrtc crate upgrade checklist +//! +//! The webrtc crate version is MSRV-pinned in Cargo.toml (see the comment there). Beyond plain +//! API compatibility, this module relies on webrtc-rs *internals* that its public API does not +//! guarantee. All of them were verified against webrtc 0.13 (webrtc-data 0.11, webrtc-sctp 0.12); +//! re-verify each against the new crate sources when bumping: +//! +//! - **Send backpressure is bounded**: `data::DataChannel::write` PARKS when webrtc-sctp's +//! PendingQueue is full (byte-counting semaphore, `QUEUE_BYTES_LIMIT` = 128 KiB; permits return +//! as chunks drain) and inflight data is cwnd/rwnd-capped (peer default rwnd 1 MiB). +//! `send_bytes` depends on this both for bounded memory on slow links and for its +//! send_timeout-then-close semantics. If a new version buffers unboundedly instead, video can +//! OOM a slow session and the send timeout never fires. +//! - **Max SCTP message size 65536**: `MAX_FRAGMENT_PAYLOAD` + 1 header byte must stay below it. +//! - **`detach()` is an idempotent Arc clone with no close-on-drop** (`detached_dc` caches it and +//! clones are shared across `WebRTCStream` clones). +//! - **`on_*` handlers are stored inside the pc**: a handler capturing a strong +//! `Arc` forms an uncollectable cycle and leaks the pc permanently — see the +//! `Arc::downgrade` in `new()`; any newly added handler must follow it. +//! - **`Disconnected` peer-connection state is transient/recoverable** (ICE consent lapse); +//! only `Failed`/`Closed` are treated as terminal by the state handler. +//! - **Stats-based `is_relayed()`**: `RTCIceCandidatePair`'s candidates are private in 0.13; +//! 0.17+ makes them `pub`, allowing direct field access instead of the stats scan. +//! +//! Then re-run the loopback tests at the bottom of this file (`cargo test --features webrtc +//! webrtc::tests`). + use std::collections::HashMap; use std::io::{Error, ErrorKind}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; @@ -742,12 +771,20 @@ impl WebRTCStream { Ok(dc) } + /// NOT cancel-safe: dropping this future mid-message (e.g. wrapping it in `select!`/`timeout`) + /// can leave a partial fragment sequence on the wire, corrupting reassembly of every later + /// message on this stream. A caller that abandons a send must treat the stream as dead and + /// close it; the built-in `send_timeout` path below already does (it closes the pc). pub async fn send_bytes(&mut self, bytes: Bytes) -> ResultType<()> { let send_timeout = self.send_timeout; let send_gate = self.send_gate.clone(); // Bound the WHOLE data-channel send (wait-for-open + every write) by send_timeout, // including time queued behind another clone. Without this a write can park indefinitely // on SCTP pending-queue backpressure and connection.rs's timeout timer never runs. + // That parking is also what bounds sender memory: webrtc-sctp's PendingQueue admits at + // most 128 KiB (byte-counting semaphore) and inflight data is cwnd/rwnd-capped, so a slow + // link parks the write here until this timeout closes the pc — TCP-send-timeout + // equivalent. Verified against webrtc-sctp 0.12; see the module-level upgrade checklist. if send_timeout > 0 { let deadline = Instant::now() + Duration::from_millis(send_timeout); let _send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { From d18dcee6a1f6ee85f897747bb65967bec718a44c Mon Sep 17 00:00:00 2001 From: rustdesk Date: Thu, 6 Aug 2026 13:41:15 +0800 Subject: [PATCH 07/35] feat: add LogThrottle for sites whose rate a peer controls Debug output is written to the log file, so a log site that fires per received message lets whoever is sending decide how much a machine writes to disk. Dropping the line instead would hide real faults, so collapse it: one line per interval carrying the count of everything suppressed since the last one, with the first occurrence after a quiet period always reported so an isolated fault is not delayed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/lib.rs | 1 + src/log_throttle.rs | 99 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 src/log_throttle.rs diff --git a/src/lib.rs b/src/lib.rs index 2b35642190..c33244dd8a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,6 +57,7 @@ pub use toml; pub use uuid; pub mod fingerprint; pub use flexi_logger; +pub mod log_throttle; pub mod stream; pub mod websocket; #[cfg(feature = "webrtc")] diff --git a/src/log_throttle.rs b/src/log_throttle.rs new file mode 100644 index 0000000000..109ba85382 --- /dev/null +++ b/src/log_throttle.rs @@ -0,0 +1,99 @@ +use std::sync::Mutex; +use std::time::{Duration, Instant}; + +/// Collapses a log site whose call rate is set by someone else — a peer's message rate, or a +/// retry loop — into at most one line per interval. +/// +/// Debug output is written to the log file, so a site that fires per received packet lets a +/// peer decide how much a machine writes to disk. Dropping the line entirely instead would +/// hide real faults, so keep one line per interval and carry the count of everything +/// suppressed since the last one. +/// +/// Declare one per site (they do not share counts): +/// +/// ```ignore +/// static DROPPED_ICE: LogThrottle = LogThrottle::new(Duration::from_secs(60)); +/// +/// if let Some(n) = DROPPED_ICE.due() { +/// log::debug!("dropped {n} ICE candidate(s) with no route"); +/// } +/// ``` +pub struct LogThrottle { + interval: Duration, + state: Mutex, +} + +struct ThrottleState { + suppressed: u64, + last: Option, +} + +impl LogThrottle { + pub const fn new(interval: Duration) -> Self { + Self { + interval, + state: Mutex::new(ThrottleState { + suppressed: 0, + last: None, + }), + } + } + + /// Record one occurrence. Returns the number of occurrences to report (including this one) + /// when a line is due, or `None` while still inside the interval. + /// + /// The first occurrence after a quiet period always reports, so an isolated fault is not + /// delayed by the interval. + pub fn due(&self) -> Option { + let Ok(mut state) = self.state.lock() else { + // A poisoned mutex means another thread panicked mid-update; the count is not worth + // propagating that, and staying silent is better than logging per call. + return None; + }; + state.suppressed += 1; + let due = state + .last + .map_or(true, |last| last.elapsed() >= self.interval); + if !due { + return None; + } + state.last = Some(Instant::now()); + Some(std::mem::replace(&mut state.suppressed, 0)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn first_call_reports_immediately() { + let t = LogThrottle::new(Duration::from_secs(60)); + assert_eq!(t.due(), Some(1)); + } + + #[test] + fn calls_inside_the_interval_are_counted_not_reported() { + let t = LogThrottle::new(Duration::from_secs(60)); + assert_eq!(t.due(), Some(1)); + for _ in 0..100 { + assert_eq!(t.due(), None); + } + } + + #[test] + fn the_next_due_line_carries_everything_suppressed() { + let t = LogThrottle::new(Duration::ZERO); + assert_eq!(t.due(), Some(1)); + // A zero interval is always due, so each call reports exactly itself. + assert_eq!(t.due(), Some(1)); + + let t = LogThrottle::new(Duration::from_millis(30)); + assert_eq!(t.due(), Some(1)); + assert_eq!(t.due(), None); + assert_eq!(t.due(), None); + std::thread::sleep(Duration::from_millis(40)); + // The two suppressed calls plus this one. + assert_eq!(t.due(), Some(3)); + } +} From 5a45b6b14916b7e020b8badad7ef0d6ca9dbd550 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Thu, 6 Aug 2026 14:21:24 +0800 Subject: [PATCH 08/35] fix: cap the log file by size, and keep LogThrottle usable after poisoning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rotating on age alone let a single day's file grow without limit, so whoever can drive a hot log site decided how much disk this uses and no amount of per-site throttling could bound it. Add a size criterion, which covers every call site at once — including ones no throttle was added to. LogThrottle: recover the guard on a poisoned lock rather than returning None. Poisoning only means another thread panicked while holding it; the guarded data is two counters that are still usable, and going silent for the rest of the process is worse than a stale count. AGENTS.md permits handling lock poisoning directly, and it forbids swallowing the error. Keep map_or over clippy's is_none_or: that was stabilized in Rust 1.82 and CI pins 1.75. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/lib.rs | 6 +++++- src/log_throttle.rs | 30 +++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index c33244dd8a..15cc322a0d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -458,7 +458,11 @@ pub fn init_log(_is_async: bool, _name: &str) -> Option Option { - let Ok(mut state) = self.state.lock() else { - // A poisoned mutex means another thread panicked mid-update; the count is not worth - // propagating that, and staying silent is better than logging per call. - return None; - }; + // A poisoned lock only means some other thread panicked while holding it; the guarded + // data is two plain counters that are still usable, and going silent for the rest of + // the process would be worse than a stale count. + let mut state = self + .state + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); state.suppressed += 1; + // `map_or(true, ..)` rather than clippy's preferred `is_none_or`: that was stabilized in + // Rust 1.82 and this crate builds on the 1.75 pinned by CI. + #[allow(clippy::unnecessary_map_or)] let due = state .last .map_or(true, |last| last.elapsed() >= self.interval); @@ -66,6 +71,21 @@ impl LogThrottle { mod tests { use super::*; + // Two directions of one socket need two throttles: an ICMP error on a connected socket is + // reported once and cleared, so the steady state alternates (send succeeds, the next recv + // reports it) and anything shared between them is reset by the succeeding side every cycle. + #[test] + fn separate_throttles_do_not_reset_each_other() { + let send = LogThrottle::new(Duration::from_secs(60)); + let recv = LogThrottle::new(Duration::from_secs(60)); + assert_eq!(recv.due(), Some(1)); + for _ in 0..1_000 { + // The send side succeeding must not hand the recv side a fresh emit slot. + assert_eq!(recv.due(), None); + } + assert_eq!(send.due(), Some(1), "the other direction keeps its own slot"); + } + #[test] fn first_call_reports_immediately() { let t = LogThrottle::new(Duration::from_secs(60)); From 0a36139a587bdb7140ed0be81fa02adcf0170702 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Thu, 6 Aug 2026 14:44:46 +0800 Subject: [PATCH 09/35] fix(webrtc): reject malformed fragment framing, correct receive-path docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `next()` read every non-FRAG_END header as "more fragments", so a peer whose framing had diverged was only caught by the MAX_FRAME_LENGTH cap — and a FRAG_MORE carrying no payload was never caught at all: it adds nothing to the accumulator, so the cap never trips and the loop spins for as long as the peer keeps writing, with no error and no teardown. Decide the header's meaning in one match, so a future header kind cannot be handled in one place and missed in the other. Neither case is reachable from send_bytes_inner, which emits FRAG_MORE only for a full MAX_FRAGMENT_PAYLOAD chunk. Release the accumulator on the error paths rather than truncating it: at the cap that is ~1 GiB still referenced through the SESSIONS clone. Doc corrections, all of them overclaims in the previous pass: - the cancel-safety entry held only for the successful read path. read_data_channel does await after dequeuing on its ErrShortBuffer and DCEP branches, and next() awaits pc.close() on its error paths — where RTCPeerConnection::close latches is_closed before its first await, so a cancelled close silently turns every later close into a no-op and leaves the pc in SESSIONS. - recv_state: cancellation drops the guard mid-message, so it is the single-reader assumption, not the mutex, that ultimately keeps two readers from splicing into one accumulator. - is_relayed: stream.rs promised None before pair selection while webrtc.rs documented Some(true) under Relay policy; align both. - get_local_endpoint: examples/webrtc.rs calls it too, not only the tests. - PunchHole.reserved 11: named the wrong writer — PunchHole is written by the rendezvous server, not by peers. Reserve the name as well as the tag. Co-Authored-By: Claude Opus 5 (1M context) --- protos/rendezvous.proto | 4 ++ src/stream.rs | 4 +- src/webrtc.rs | 148 ++++++++++++++++++++++++++++++---------- 3 files changed, 118 insertions(+), 38 deletions(-) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index cdc57ebee4..6775b53740 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -66,7 +66,11 @@ message PunchHole { ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; string webrtc_sdp_offer = 10; + // Was `string requester_id = 11`, dropped once ICE routing stopped needing it. Reserved so the + // tag is never reassigned: PunchHole is written by the rendezvous server, and an hbbs built + // against the earlier field still puts a string here. reserved 11; + reserved "requester_id"; } message TestNatRequest { diff --git a/src/stream.rs b/src/stream.rs index 1c9f381fd7..8d8b3c47fe 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -101,7 +101,9 @@ impl Stream { } /// Whether an established WebRTC transport runs through a TURN relay (used for the UI's - /// direct/relayed flag). None for non-WebRTC transports or before ICE selects a pair. + /// direct/relayed flag). `None` for non-WebRTC transports, and for a non-relay-policy pc + /// before ICE selects a pair; a Relay-policy pc answers `Some(true)` straight away — see + /// `WebRTCStream::is_relayed`. #[inline] pub async fn webrtc_relayed(&self) -> Option { match self { diff --git a/src/webrtc.rs b/src/webrtc.rs index 7e0be30870..aedfdd8198 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -14,6 +14,14 @@ //! send_timeout-then-close semantics. If a new version buffers unboundedly instead, video can //! OOM a slow session and the send timeout never fires. //! - **Max SCTP message size 65536**: `MAX_FRAGMENT_PAYLOAD` + 1 header byte must stay below it. +//! - **The successful read path is cancel-safe**: `read_sctp` dequeues synchronously and returns +//! with no `.await` after it, and `read_data_channel` adds none on the user-data path. So +//! `next_timeout`, which drops that future routinely, cannot lose a fragment — a version that +//! awaited after dequeuing would, undetectably, since the header carries no length or checksum. +//! Scope: the *read*. `read_data_channel` does await after dequeuing on its ErrShortBuffer and +//! DCEP branches, and `next()` itself awaits `pc.close()` on its error paths — and +//! `RTCPeerConnection::close` latches `is_closed` before its first await, so a cancelled close +//! silently makes every later close a no-op and leaves the pc in `SESSIONS`. //! - **`detach()` is an idempotent Arc clone with no close-on-drop** (`detached_dc` caches it and //! clones are shared across `WebRTCStream` clones). //! - **`on_*` handlers are stored inside the pc**: a handler capturing a strong @@ -84,9 +92,11 @@ pub struct WebRTCStream { // Serialize a complete logical message across clones. Each fragment is a separate SCTP // message, so serializing only individual writes would allow two large messages to interleave. send_gate: Arc, - // Receive-side reassembly state, guarded by a single mutex so the fragment accumulator - // survives `next()` cancellation (e.g. `next_timeout`) instead of losing already-read - // fragments mid-message. Assumes a single reader, consistent with the rest of the stream API. + // Receive-side reassembly state. The accumulator survives a cancelled `next()` because it + // lives here behind the Arc, not in the future. The mutex excludes a concurrent reader only + // while `next()` is actually running — a cancellation drops the guard mid-message, so it is + // the single-reader assumption, not the lock, that ultimately prevents two readers splicing + // into one `acc`. Single reader assumed, like the rest of the stream API. recv_state: Arc>, // True once the controller has completed the RustDesk identity binding (DTLS fingerprint // matched to the signed peer id, via `set_key`). DTLS always encrypts; this flag mirrors TCP's @@ -501,16 +511,12 @@ impl WebRTCStream { // process offer/answer // - // Trickle ICE: the local description is returned WITHOUT waiting for candidate gathering - // (candidates stream out via `take_local_ice_rx` afterwards), so this block is local-only - // work — pc construction, DTLS cert keygen, SDP marshal — at sub-millisecond cost. The - // controlled side awaits answer creation inline on its punch-reply critical path and - // relies on that: adding any gathering/network wait here would delay the TCP/UDP - // hole-punch reply for every connection. - // Any failure below leaves a live pc with handlers already registered; its state handler - // only fires on a terminal ICE state, so a bare `?`-drop would leak it (remotely - // triggerable: a crafted `type:"answer"` offer passes the JSON+fingerprint pre-check but - // fails `set_remote_description`). Close the pc before propagating any such error. + // Trickle ICE: this block is local-only work (pc construction, DTLS keygen, SDP marshal), + // no gathering wait. The controlled side awaits answer creation inline on its punch-reply + // critical path, so adding a network wait here would delay every hole punch. + // A failure below leaves a live pc whose state handler only fires on a terminal ICE state, + // so a bare `?`-drop leaks it — remotely triggerable via a crafted `type:"answer"` offer + // that passes the pre-check but fails `set_remote_description`. Close before propagating. let offer_answer: ResultType = async { if start_local_offer { let sdp = pc.create_offer(None).await?; @@ -575,6 +581,11 @@ impl WebRTCStream { Ok(webrtc_stream) } + /// One-shot endpoint: waits for ICE gathering so the SDP already carries the candidates. + /// The wait is deliberately unbounded — a deadline here would return a half-gathered SDP and + /// defeat the contract; callers needing one should wrap the call or use the trickle variant, + /// which both rustdesk signaling paths do. Remaining callers are the loopback tests and + /// `examples/webrtc.rs`, neither of which bounds it. #[inline] pub async fn get_local_endpoint(&self) -> ResultType { // Preserve the original one-shot endpoint contract: callers that only exchange this SDP @@ -621,9 +632,11 @@ impl WebRTCStream { Self::get_key_for_peer(&self.pc, false).await } - /// Whether the established connection runs through a TURN relay: `Some(true)` when the pc is - /// Relay-policy (TURN is the only possibility) or the selected ICE candidate pair uses a - /// relay candidate; `None` before a pair is selected. Feeds the UI's direct/relayed flag. + /// Whether the connection runs through a TURN relay; feeds the UI's direct/relayed flag. + /// Under Relay policy the answer is known by construction, so that arm returns `Some(true)` + /// without consulting ICE — including before a pair is selected, unlike the `None` the other + /// arm returns then. Both callers ask post-connection, where the arms agree; making the relay + /// arm await a pair would only add a stats round trip. pub async fn is_relayed(&self) -> Option { if self.relay_only { return Some(true); @@ -685,14 +698,10 @@ impl WebRTCStream { Ok(()) } - /// Explicitly tear down the peer connection. - /// - /// Dropping a `WebRTCStream` handle is not enough to release the underlying - /// `RTCPeerConnection`: the global `SESSIONS` map holds a clone, so the pc (and its - /// ICE/DTLS/STUN resources) would stay alive until it happens to reach a terminal ICE - /// state. Closing here fires `on_peer_connection_state_change`, which removes the - /// `SESSIONS` entry, so callers that abandon a stream (e.g. a raced offerer that lost to - /// another transport) should call this to avoid leaking it. + /// Explicitly tear down the peer connection. Dropping the handle is not enough: `SESSIONS` + /// holds a clone, so the pc and its ICE/DTLS resources survive until it happens to reach a + /// terminal ICE state. Closing fires the state handler, which evicts the `SESSIONS` entry — + /// callers that abandon a stream (e.g. an offerer that lost the transport race) must call it. #[inline] pub async fn close(&self) { self.pc.close().await.ok(); @@ -778,13 +787,11 @@ impl WebRTCStream { pub async fn send_bytes(&mut self, bytes: Bytes) -> ResultType<()> { let send_timeout = self.send_timeout; let send_gate = self.send_gate.clone(); - // Bound the WHOLE data-channel send (wait-for-open + every write) by send_timeout, - // including time queued behind another clone. Without this a write can park indefinitely - // on SCTP pending-queue backpressure and connection.rs's timeout timer never runs. - // That parking is also what bounds sender memory: webrtc-sctp's PendingQueue admits at - // most 128 KiB (byte-counting semaphore) and inflight data is cwnd/rwnd-capped, so a slow - // link parks the write here until this timeout closes the pc — TCP-send-timeout - // equivalent. Verified against webrtc-sctp 0.12; see the module-level upgrade checklist. + // Bound the WHOLE send (wait-for-open, queueing behind another clone, every write) by + // send_timeout: otherwise a write parks indefinitely on SCTP backpressure and + // connection.rs's timer never runs. That parking is also what bounds sender memory — + // PendingQueue admits 128 KiB and inflight is cwnd/rwnd-capped — so this timeout is the + // TCP-send-timeout equivalent. See the checklist entry on send backpressure. if send_timeout > 0 { let deadline = Instant::now() + Duration::from_millis(send_timeout); let _send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { @@ -865,8 +872,9 @@ impl WebRTCStream { return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); } }; - // Hold recv_state across the reassembly loop: the accumulator must survive `next()` - // cancellation (e.g. next_timeout) so already-read fragments are not lost mid-message. + // Held across the whole loop for exclusion, not for accumulator survival (see the field). + // Cancelling mid-`dc.read()` loses no data; cancelling mid-`pc.close()` on an error path + // below does leak the pc — see the checklist entry on cancel-safety. let mut st = self.recv_state.lock().await; if st.scratch.len() < RECV_BUF_SIZE { st.scratch.resize(RECV_BUF_SIZE, 0); @@ -889,18 +897,41 @@ impl WebRTCStream { self.pc.close().await.ok(); return None; } + // Two framing violations, both of which would otherwise be read as "more fragments": + // an unrecognized header, and a FRAG_MORE carrying no payload. The latter is the + // nastier one — it adds nothing to `acc`, so the MAX_FRAME_LENGTH cap below never + // trips and the loop spins for as long as the peer keeps sending. `send_bytes_inner` + // emits FRAG_MORE only for a full MAX_FRAGMENT_PAYLOAD chunk, so neither is reachable + // from our own sender. + let header = scratch[0]; + let bad = match header { + FRAG_END => None, + FRAG_MORE if n > 1 => None, + FRAG_MORE => Some("FRAG_MORE fragment carries no payload".to_owned()), + other => Some(format!("fragment header {other} is neither FRAG_END nor FRAG_MORE")), + }; + if let Some(why) = bad { + *acc = BytesMut::new(); + self.pc.close().await.ok(); + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!("WebRTC {why}"), + ))); + } acc.extend_from_slice(&scratch[1..n]); // Match TCP's maximum frame size while preventing an unbounded FRAG_MORE stream from // exhausting memory. if acc.len() > MAX_FRAME_LENGTH { - acc.clear(); + // Release the buffer, don't just truncate it: by definition it is at the cap here, + // and `recv_state` outlives this call through the `SESSIONS` clone. + *acc = BytesMut::new(); self.pc.close().await.ok(); return Some(Err(Error::new( - ErrorKind::Other, + ErrorKind::InvalidData, "WebRTC reassembled message exceeded maximum frame size", ))); } - if scratch[0] == FRAG_END { + if header == FRAG_END { let msg = std::mem::take(acc); return Some(Ok(msg)); } @@ -925,7 +956,8 @@ pub fn is_webrtc_endpoint(endpoint: &str) -> bool { mod tests { use crate::config; use crate::webrtc::WebRTCStream; - use crate::webrtc::DEFAULT_ICE_SERVERS; + use crate::webrtc::{DEFAULT_ICE_SERVERS, FRAG_MORE}; + use bytes::{BufMut, BytesMut}; use std::{sync::Arc, time::Duration}; use tokio::sync::Barrier; use tokio::time::timeout; @@ -1284,6 +1316,48 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .expect("webrtc loopback did not complete in time"); } + // Both framing violations must end the stream. The empty-FRAG_MORE case is the one that + // cannot be caught downstream: it adds nothing to the accumulator, so the MAX_FRAME_LENGTH + // cap never trips and `next()` would otherwise spin for as long as the peer keeps writing. + // The bad frame is injected mid-message so the branch's `acc` reset is exercised too. + #[tokio::test] + async fn test_webrtc_rejects_bad_fragment_framing() { + // (raw frame, expected substring) + let cases: [(&[u8], &str); 2] = [ + (&[0x2A, b'x'], "fragment header 42"), + (&[FRAG_MORE], "carries no payload"), + ]; + for (frame, want) in cases { + let connect = async { + let (offerer, mut answerer) = connect_loopback().await; + let dc = offerer.detached_dc().await.unwrap(); + + // Leave a partial message in the accumulator first, so the rejection has + // something to discard. `send_bytes` only ever emits valid headers, so the bad + // frame itself has to be written through the raw channel below it. + let mut lead = BytesMut::with_capacity(1 + 8); + lead.put_u8(FRAG_MORE); + lead.put_slice(b"leading!"); + dc.write(&lead.freeze()).await.unwrap(); + dc.write(&bytes::Bytes::copy_from_slice(frame)).await.unwrap(); + + let err = answerer + .next() + .await + .expect("bad framing must surface as an error, not EOF") + .expect_err("bad framing must be rejected"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert!(err.to_string().contains(want), "unexpected error: {}", err); + + offerer.close().await; + answerer.close().await; + }; + timeout(Duration::from_secs(40), connect) + .await + .expect("webrtc loopback did not complete in time"); + } + } + #[tokio::test] async fn test_webrtc_concurrent_large_sends_preserve_boundaries() { let connect = async { From 7c4456be9bdb5c53df01ebeac5a36d67a6392a38 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Thu, 6 Aug 2026 15:21:27 +0800 Subject: [PATCH 10/35] proto: drop the reserved tag in PunchHole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `requester_id = 11` was added and removed in the same rebase batch, never reached main, and never reached hbbs — whose vendored copy of this file still stops at field 9. So nothing has ever written or read tag 11, and reserving it guards a wire format that does not exist. It was also inconsistent with what this branch already does: `IceCandidate` retyped tag 2 from `string to_id` to `bytes socket_addr` in place, which is only sound because none of this proto has shipped. Same premise, so tag 11 is free. Co-Authored-By: Claude Opus 5 (1M context) --- protos/rendezvous.proto | 5 ----- 1 file changed, 5 deletions(-) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index 6775b53740..a73e017cc4 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -66,11 +66,6 @@ message PunchHole { ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; string webrtc_sdp_offer = 10; - // Was `string requester_id = 11`, dropped once ICE routing stopped needing it. Reserved so the - // tag is never reassigned: PunchHole is written by the rendezvous server, and an hbbs built - // against the earlier field still puts a string here. - reserved 11; - reserved "requester_id"; } message TestNatRequest { From a992c646bf0e34245f80fb230e2b93b1cdd1da02 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 00:05:27 +0800 Subject: [PATCH 11/35] =?UTF-8?q?proto:=20webrtc=5Fall=5Fice=20=E2=80=94?= =?UTF-8?q?=20full-ICE=20offers=20under=20transport-forced=20relay?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit use_ws() folds into force_relay because a ws tunnel kills classic TCP/UDP punching — but ICE opens its own sockets and does not care how signaling reaches the server. Without a signal, the controlled side must treat every force_relay offer as Relay-only ICE (answer gated on TURN), which locks WebSocket deployments out of direct WebRTC entirely. webrtc_all_ice marks an offer that gathered every candidate type: the controller's force_relay covers only classic punching, not ICE policy. Absent/false keeps today's semantics on every skew combination (old controller, old server dropping the field, old controlled side). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- protos/rendezvous.proto | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index a73e017cc4..d5e9e09560 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -30,6 +30,12 @@ message PunchHoleRequest { bytes socket_addr_v6 = 10; string switch_code = 11; string webrtc_sdp_offer = 12; + // The attached offer gathers every ICE candidate type (host/srflx/relay), so a direct + // WebRTC path may form even when force_relay is set. Sent by clients whose force_relay + // stems from the transport (WebSocket tunnels only the signaling/relay legs and kills + // classic punching, not ICE) rather than from relay-by-policy; absent/false keeps the + // old semantics where force_relay implies a Relay-only-ICE offer. + bool webrtc_all_ice = 13; } message ControlPermissions { @@ -66,6 +72,9 @@ message PunchHole { ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; string webrtc_sdp_offer = 10; + // Forwarded from PunchHoleRequest.webrtc_all_ice; see that field. Tag 11 previously + // carried the never-shipped `requester_id` (added and removed in one unshipped batch). + bool webrtc_all_ice = 11; } message TestNatRequest { From eed7052d1d03fcc9b74becce97f753448542127b Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 11:55:27 +0800 Subject: [PATCH 12/35] webrtc: declare the ICE policy inside the offer envelope, not a proto field Reverts the webrtc_all_ice proto field (64b54ab) in favor of an `ice_policy: "all"` key inside the webrtc:// envelope JSON, next to the RTCSessionDescription fields. Same information, better carrier: - it is a property of the offer itself, so it rides with the offer; - the rendezvous server never has to know: the envelope is an opaque, length-bounded string to hbbs, so no forwarding code and no vendored proto copies to keep in sync; - serde ignores unknown JSON keys when parsing RTCSessionDescription, so every skew combination degrades exactly like the proto field did: absence - not an error - is the old Relay-only reading. endpoint_declares_all_ice() is the receiving side: parse failure, foreign scheme or missing key all read as "not declared". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- protos/rendezvous.proto | 12 ++------ src/webrtc.rs | 62 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index d5e9e09560..8f796502fc 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -29,13 +29,10 @@ message PunchHoleRequest { int32 upnp_port = 9; bytes socket_addr_v6 = 10; string switch_code = 11; + // The offer's envelope declares its own ICE transport policy (`ice_policy` key inside + // the webrtc:// payload): under force_relay it tells the peer whether the relay is + // transport-forced (WebSocket — answer may use full ICE) or policy (Relay-only + TURN). string webrtc_sdp_offer = 12; - // The attached offer gathers every ICE candidate type (host/srflx/relay), so a direct - // WebRTC path may form even when force_relay is set. Sent by clients whose force_relay - // stems from the transport (WebSocket tunnels only the signaling/relay legs and kills - // classic punching, not ICE) rather than from relay-by-policy; absent/false keeps the - // old semantics where force_relay implies a Relay-only-ICE offer. - bool webrtc_all_ice = 13; } message ControlPermissions { @@ -72,9 +69,6 @@ message PunchHole { ControlPermissions control_permissions = 8; ControlledContext controlled_context = 9; string webrtc_sdp_offer = 10; - // Forwarded from PunchHoleRequest.webrtc_all_ice; see that field. Tag 11 previously - // carried the never-shipped `requester_id` (added and removed in one unshipped batch). - bool webrtc_all_ice = 11; } message TestNatRequest { diff --git a/src/webrtc.rs b/src/webrtc.rs index aedfdd8198..0077914d8f 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -186,6 +186,11 @@ impl WebRTCStream { format!("webrtc://{}", encoded_sdp) } + // Envelope JSON key carrying the local description's ICE transport policy, alongside the + // RTCSessionDescription fields (see `get_local_endpoint_trickle`). + const ICE_POLICY_KEY: &str = "ice_policy"; + const ICE_POLICY_ALL: &str = "all"; + #[inline] fn get_key_for_sdp(sdp: &RTCSessionDescription) -> ResultType { let binding = sdp.unmarshal()?; @@ -601,7 +606,21 @@ impl WebRTCStream { #[inline] pub async fn get_local_endpoint_trickle(&self) -> ResultType { if let Some(local_desc) = self.pc.local_description().await { - let sdp = serde_json::to_string(&local_desc)?; + let sdp = if self.relay_only { + serde_json::to_string(&local_desc)? + } else { + // Declare the ICE transport policy inside the envelope. The receiver of an + // offer that arrives with force_relay set must know whether it may answer + // with full ICE (relay forced by the transport, e.g. WebSocket signaling) + // or must stay Relay-only + TURN-gated (relay by policy) — and the envelope + // is the offer's own property, so it rides here rather than in a proto + // field the rendezvous server would have to forward. An extra key is + // invisible to older peers: serde ignores unknown fields when parsing + // RTCSessionDescription, so absence — not an error — is the old semantics. + let mut v = serde_json::to_value(&local_desc)?; + v[Self::ICE_POLICY_KEY] = serde_json::Value::from(Self::ICE_POLICY_ALL); + serde_json::to_string(&v)? + }; let endpoint = Self::sdp_to_endpoint(&sdp); Ok(endpoint) } else { @@ -609,6 +628,24 @@ impl WebRTCStream { } } + /// Whether the peer's endpoint declares it was built with ICE transport policy `all` + /// (W3C RTCIceTransportPolicy), i.e. it gathers host/srflx/relay candidates and a direct + /// pair may form even though the request carries force_relay. Absent key, foreign format + /// or parse failure all mean "not declared" — the old Relay-only reading. + pub fn endpoint_declares_all_ice(endpoint: &str) -> bool { + let Ok(sdp_json) = Self::get_remote_offer(endpoint) else { + return false; + }; + serde_json::from_str::(&sdp_json) + .ok() + .and_then(|v| { + v.get(Self::ICE_POLICY_KEY)? + .as_str() + .map(|p| p == Self::ICE_POLICY_ALL) + }) + .unwrap_or(false) + } + #[inline] pub async fn set_remote_endpoint(&self, endpoint: &str) -> ResultType<()> { let offer = Self::get_remote_offer(endpoint)?; @@ -1037,6 +1074,29 @@ mod tests { ); } + #[test] + fn test_endpoint_ice_policy_declaration() { + // An envelope with the marker declares full ICE; everything else — no marker, + // wrong value, foreign scheme, garbage — reads as the old Relay-only semantics. + let marked = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"all"}"#); + assert!(WebRTCStream::endpoint_declares_all_ice(&marked)); + + let unmarked = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0"}"#); + assert!(!WebRTCStream::endpoint_declares_all_ice(&unmarked)); + + let wrong = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"relay"}"#); + assert!(!WebRTCStream::endpoint_declares_all_ice(&wrong)); + + assert!(!WebRTCStream::endpoint_declares_all_ice("")); + assert!(!WebRTCStream::endpoint_declares_all_ice("webrtc://not-base64!")); + assert!(!WebRTCStream::endpoint_declares_all_ice("https://example.com")); + + // The marker must be invisible to the plain RTCSessionDescription parse old peers do. + let sdp_json = WebRTCStream::get_remote_offer(&marked).unwrap(); + serde_json::from_str::(&sdp_json) + .expect("extra envelope key must not break RTCSessionDescription parsing"); + } + #[test] fn test_webrtc_session_key() { let mut sdp_str = "".to_owned(); From 137bb362f21b353ca92cce7528e708b679c5242b Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 11:56:03 +0800 Subject: [PATCH 13/35] fmt the envelope-marker test Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 0077914d8f..9ecd98da09 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1078,23 +1078,31 @@ mod tests { fn test_endpoint_ice_policy_declaration() { // An envelope with the marker declares full ICE; everything else — no marker, // wrong value, foreign scheme, garbage — reads as the old Relay-only semantics. - let marked = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"all"}"#); + let marked = + WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"all"}"#); assert!(WebRTCStream::endpoint_declares_all_ice(&marked)); let unmarked = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0"}"#); assert!(!WebRTCStream::endpoint_declares_all_ice(&unmarked)); - let wrong = WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"relay"}"#); + let wrong = + WebRTCStream::sdp_to_endpoint(r#"{"type":"offer","sdp":"v=0","ice_policy":"relay"}"#); assert!(!WebRTCStream::endpoint_declares_all_ice(&wrong)); assert!(!WebRTCStream::endpoint_declares_all_ice("")); - assert!(!WebRTCStream::endpoint_declares_all_ice("webrtc://not-base64!")); - assert!(!WebRTCStream::endpoint_declares_all_ice("https://example.com")); + assert!(!WebRTCStream::endpoint_declares_all_ice( + "webrtc://not-base64!" + )); + assert!(!WebRTCStream::endpoint_declares_all_ice( + "https://example.com" + )); // The marker must be invisible to the plain RTCSessionDescription parse old peers do. let sdp_json = WebRTCStream::get_remote_offer(&marked).unwrap(); - serde_json::from_str::(&sdp_json) - .expect("extra envelope key must not break RTCSessionDescription parsing"); + serde_json::from_str::< + webrtc::peer_connection::sdp::session_description::RTCSessionDescription, + >(&sdp_json) + .expect("extra envelope key must not break RTCSessionDescription parsing"); } #[test] From 24ae0c426c25ad116c5a7b57b0dd0102f5833cb7 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 13:45:32 +0800 Subject: [PATCH 14/35] config: OPTION_ENABLE_WEBRTC, defaulted like the punch options Same pattern as enable-udp-punch / enable-ipv6-punch: empty value reads as on against the public server and off against a private one (the injection lives in rustdesk's get_local_option), so self-hosted deployments opt in once their server / TURN is ready. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/config.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config.rs b/src/config.rs index e968ec469a..7ac5fb54f6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2995,6 +2995,7 @@ pub mod keys { // Connection punch-through options pub const OPTION_ENABLE_UDP_PUNCH: &str = "enable-udp-punch"; pub const OPTION_ENABLE_IPV6_PUNCH: &str = "enable-ipv6-punch"; + pub const OPTION_ENABLE_WEBRTC: &str = "enable-webrtc"; pub const OPTION_HIDE_USERNAME_ON_CARD: &str = "hide-username-on-card"; pub const OPTION_HIDE_HELP_CARDS: &str = "hide-help-cards"; pub const OPTION_DEFAULT_CONNECT_PASSWORD: &str = "default-connect-password"; @@ -3126,6 +3127,7 @@ pub mod keys { OPTION_VIDEO_SAVE_DIRECTORY, OPTION_ENABLE_UDP_PUNCH, OPTION_ENABLE_IPV6_PUNCH, + OPTION_ENABLE_WEBRTC, OPTION_TOUCH_MODE, OPTION_SHOW_VIRTUAL_MOUSE, OPTION_SHOW_VIRTUAL_JOYSTICK, From 0d2ca8aa44c4ec2a0f3b702e4a09fc1c192dfec9 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 15:17:12 +0800 Subject: [PATCH 15/35] log_throttle: add throttled_log!, the general per-call-site form The type alone still needs a static plus an `if let` at every use, which is why the codebase kept hand-rolling equivalents. The macro declares the static for itself, so adding a bounded site is one line, and it appends the multiplicity only when there is one to report - an isolated event logs exactly as it would unthrottled. Count semantics stay inclusive (the reported number is the total this line stands for, first occurrence = 1), so a reader needs no arithmetic; the type's docs now point at the macro and say when to reach past it. Also rustfmt the module and webrtc.rs, which had drifted (no CI gate enforces it on this branch). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/log_throttle.rs | 37 ++++++++++++++++++++++++++++++++++++- src/webrtc.rs | 29 +++++++++++++++-------------- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/log_throttle.rs b/src/log_throttle.rs index 02bb7f2089..b8043ff93e 100644 --- a/src/log_throttle.rs +++ b/src/log_throttle.rs @@ -9,6 +9,10 @@ use std::time::{Duration, Instant}; /// hide real faults, so keep one line per interval and carry the count of everything /// suppressed since the last one. /// +/// Prefer the [`throttled_log!`](crate::throttled_log) macro, which declares the static for +/// you. Reach for this type directly only when the count belongs somewhere other than the end +/// of the line, or when the decision drives more than a log call. +/// /// Declare one per site (they do not share counts): /// /// ```ignore @@ -67,6 +71,33 @@ impl LogThrottle { } } +/// Log at most one line per interval from this call site, suffixed with the number of +/// occurrences it stands for. +/// +/// Each expansion declares its own hidden static, so two sites never share a count and +/// adding one is a single line: +/// +/// ```ignore +/// throttled_log!(Duration::from_secs(5), warn, "rejected ipc peer {peer_pid:?}"); +/// ``` +/// +/// An isolated event logs unchanged; a burst collapses to `... (x47)`. The count includes +/// the occurrence being reported, so it reads as a total rather than as "and N more". +#[macro_export] +macro_rules! throttled_log { + ($interval:expr, $level:ident, $($arg:tt)+) => {{ + static THROTTLE: $crate::log_throttle::LogThrottle = + $crate::log_throttle::LogThrottle::new($interval); + if let Some(n) = THROTTLE.due() { + if n > 1 { + $crate::log::$level!("{} (x{})", format_args!($($arg)+), n); + } else { + $crate::log::$level!("{}", format_args!($($arg)+)); + } + } + }}; +} + #[cfg(test)] mod tests { use super::*; @@ -83,7 +114,11 @@ mod tests { // The send side succeeding must not hand the recv side a fresh emit slot. assert_eq!(recv.due(), None); } - assert_eq!(send.due(), Some(1), "the other direction keeps its own slot"); + assert_eq!( + send.due(), + Some(1), + "the other direction keeps its own slot" + ); } #[test] diff --git a/src/webrtc.rs b/src/webrtc.rs index 9ecd98da09..f3c8cce76c 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -461,9 +461,8 @@ impl WebRTCStream { // Only tear down on the terminal states so a short network blip (Wi-Fi roam, // sleep/wake, cell handover) does not permanently kill an established session. RTCPeerConnectionState::Failed | RTCPeerConnectionState::Closed => { - let _ = on_connection_notify.send(WebRTCConnectionState::Closed( - s.to_string(), - )); + let _ = + on_connection_notify.send(WebRTCConnectionState::Closed(s.to_string())); log::debug!("WebRTC session closing due to {}", s); let _ = stream_for_close2.lock().await.close().await; log::debug!("WebRTC session stream closed"); @@ -698,9 +697,7 @@ impl WebRTCStream { == RTCIceCandidateType::Relay ) }; - Some( - is_relay(&pair.local_candidate_id) || is_relay(&pair.remote_candidate_id), - ) + Some(is_relay(&pair.local_candidate_id) || is_relay(&pair.remote_candidate_id)) } #[inline] @@ -945,7 +942,9 @@ impl WebRTCStream { FRAG_END => None, FRAG_MORE if n > 1 => None, FRAG_MORE => Some("FRAG_MORE fragment carries no payload".to_owned()), - other => Some(format!("fragment header {other} is neither FRAG_END nor FRAG_MORE")), + other => Some(format!( + "fragment header {other} is neither FRAG_END nor FRAG_MORE" + )), }; if let Some(why) = bad { *acc = BytesMut::new(); @@ -1068,10 +1067,7 @@ mod tests { "turn:example.com:3478" ); assert_eq!(WebRTCStream::get_ice_servers().len(), 2); - config::Config::set_option( - "ice-servers".to_string(), - "".to_string(), - ); + config::Config::set_option("ice-servers".to_string(), "".to_string()); } #[test] @@ -1362,7 +1358,11 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc let big = vec![0xABu8; 200_000]; offerer.send_raw(big.clone()).await.unwrap(); let got = answerer.next().await.unwrap().unwrap(); - assert_eq!(got.len(), big.len(), "large message must survive fragmentation"); + assert_eq!( + got.len(), + big.len(), + "large message must survive fragmentation" + ); assert_eq!(&got[..], &big[..]); // Reverse direction. @@ -1407,7 +1407,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc lead.put_u8(FRAG_MORE); lead.put_slice(b"leading!"); dc.write(&lead.freeze()).await.unwrap(); - dc.write(&bytes::Bytes::copy_from_slice(frame)).await.unwrap(); + dc.write(&bytes::Bytes::copy_from_slice(frame)) + .await + .unwrap(); let err = answerer .next() @@ -1472,5 +1474,4 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .await .expect("concurrent WebRTC sends did not complete in time"); } - } From a0d995f571b218ac998d7ff67f965dcd81de7a94 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 15:29:21 +0800 Subject: [PATCH 16/35] webrtc: detach teardown, bound reassembly before growing, vet the data channel Four review findings on the receive path, all verified against the vendored webrtc-rs rather than inferred: - next() awaited pc.close() on every error/EOF path while every consumer polls next() inside a select! against a 1s timer. close() latches is_closed before its first await and fires the state handler last, so losing that race left a pc no later close() could retry, a SESSIONS entry only that handler evicts, and a state_notify that never reaches Closed. close_detached() hands the teardown to the runtime; being a non-async fn, its callers have no await point to be cancelled at. The send path's timeout arm passes its logical-message permit along, so the exclusion it relies on now outlives the caller too. - the reassembly cap was checked after extend_from_slice, so the peak was the cap plus a fragment, and BytesMut's reallocate-and-copy growth held old and new buffers at once. Check before appending, and stop borrowing bytes_codec's ~1 GiB MAX_FRAME_LENGTH: that bound is only affordable for TCP because its length prefix rejects an oversize frame before buffering any of it, while this framing can only discover the overrun by accumulating it - and the answerer runs before any password check. MAX_RECV_MESSAGE (64 MiB) bounds both directions. - the EOF path claimed an empty message could never be confused with a reset. It can: webrtc-data maps the StringEmpty/BinaryEmpty PPIDs to n == 0 and dc.read() discards the flag that separates them. Both mean the same thing to us, so the handling stands - the comment and the log line now say what actually happened. - on_data_channel bound whatever the remote opened, however it opened it. Reassembly spans messages, so it is sound only on an ordered, fully-reliable channel, and webrtc-rs derives those parameters verbatim from the remote's DCEP OPEN; extra channels additionally split teardown from the channel carrying traffic and re-arm Open over a latched Closed. Refuse both. Also release the accumulator on the EOF and read-error exits, which were the only paths that left a partial message reachable through the SESSIONS clone. Regression tests for the detached teardown and the bind-once guard, both mutation-checked; the test comments state what is and is not covered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 255 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 225 insertions(+), 30 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index f3c8cce76c..61370e9227 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -64,7 +64,6 @@ use tokio::sync::{mpsc, watch, Mutex, Semaphore}; use tokio::time::{timeout, timeout_at, Instant}; use url::Url; -use crate::bytes_codec::MAX_FRAME_LENGTH; use crate::config; use crate::protobuf::Message; use crate::sodiumoxide::crypto::secretbox::Key; @@ -124,6 +123,21 @@ const RECV_BUF_SIZE: usize = 64 * 1024; const FRAG_MORE: u8 = 1; /// Fragment header byte: final (or only) fragment of a logical message. const FRAG_END: u8 = 0; +/// Largest logical message this transport will send or reassemble. +/// +/// Deliberately NOT `bytes_codec::MAX_FRAME_LENGTH` (~1 GiB), which this path borrowed: that +/// bound is affordable for TCP only because its length prefix lets the decoder reject an +/// oversize frame before buffering a byte of it. This framing carries no length, so the cap can +/// only be enforced by accumulating up to it — the cap IS the memory an unauthenticated peer can +/// make a receiver hold, and the answerer is spawned straight off a PunchHole, before any +/// password check. +/// +/// 64 MiB is ~500x the largest block RustDesk actually streams (fs.rs sends 128 KiB blocks; a +/// video frame or clipboard image is far below that), so no legitimate message can reach it, +/// while the worst case stays a bounded allocation rather than an OOM. Send and receive share +/// the constant so the two ends of a same-version pair always agree; raising it is the knob if a +/// real message ever needs more. +const MAX_RECV_MESSAGE: usize = 64 * 1024 * 1024; // use 3 public STUN servers to find out the NAT type, 2 must be the same address but different ports // https://stackoverflow.com/questions/72805316/determine-nat-mapping-behaviour-using-two-stun-servers // luckily nextcloud supports two ports for STUN @@ -424,12 +438,44 @@ impl WebRTCStream { // Register data channel creation handling let dc_open_notify = notify_tx.clone(); let stream_for_dc = stream.clone(); + // The remote may open any number of channels; only the first is ever bound. + let dc_bound = Arc::new(AtomicBool::new(false)); pc.on_data_channel(Box::new(move |dc: Arc| { let d_label = dc.label().to_owned(); let dc_open_notify2 = dc_open_notify.clone(); let stream_for_dc_clone = stream_for_dc.clone(); - log::debug!("Remote data channel {} ready", d_label); + let dc_bound = dc_bound.clone(); Box::pin(async move { + // Reassembly spans data-channel messages, so it is sound only on an ordered, + // fully-reliable channel: the 1-byte fragment header carries no sequence + // number, so a reorder splices fragments into a well-formed but wrong + // message, and a dropped fragment merges two messages instead of erroring. + // webrtc-rs derives these parameters entirely from the REMOTE's DCEP OPEN + // (`channel_type` picks ordered / max_retransmits / max_packet_life_time), + // and this pc is built from an unauthenticated offer, so the peer would + // otherwise choose our reassembly's correctness conditions for us. + if !dc.ordered() + || dc.max_retransmits().is_some() + || dc.max_packet_lifetime().is_some() + { + log::warn!( + "Rejecting WebRTC data channel {}: not ordered and fully reliable", + d_label + ); + let _ = dc.close().await; + return; + } + // Bind the first channel only. `detached` caches the first detached handle + // for the life of the stream, so rebinding would leave teardown closing a + // channel that send/recv no longer use; worse, a second channel's `on_open` + // would push Open onto the same watch and re-arm a session already latched + // Closed — and that watch gates both `send_bytes_inner` and `next()`. + if dc_bound.swap(true, Ordering::SeqCst) { + log::warn!("Ignoring extra WebRTC data channel {}", d_label); + let _ = dc.close().await; + return; + } + log::debug!("Remote data channel {} ready", d_label); let mut stream_lock = stream_for_dc_clone.lock().await; *stream_lock = dc.clone(); drop(stream_lock); @@ -741,6 +787,37 @@ impl WebRTCStream { self.pc.close().await.ok(); } + /// Tear the pc down on the runtime instead of awaiting it here, keeping `keep` alive until + /// the teardown finishes. + /// + /// Every caller polls `next()` — and often `send_bytes` — inside a `tokio::select!`, so an + /// `.await` on these paths is a cancellation point, and a competing arm (connection.rs and + /// io_loop.rs both run a 1s timer next to the read) routinely wins one. That is fatal to a + /// close: `RTCPeerConnection::close` latches `is_closed` before its first await and fires + /// the state handler only at the very end, so a cancelled close leaves a pc no later + /// `close()` can retry (they early-return on `is_closed`), whose `SESSIONS` entry — evicted + /// only from that handler — is stranded for the life of the process, and whose + /// `state_notify` never reaches `Closed`, leaving `wait_for_connect_result` reporting a live + /// connection on a dead pc. + /// + /// `keep` carries anything whose lifetime must span the teardown rather than the caller's: + /// the send path passes its logical-message permit, so no waiting clone can append to a + /// partially-written fragment sequence while the close is still in flight. + fn close_detached_with(&self, keep: T) { + let pc = self.pc.clone(); + tokio::spawn(async move { + let _keep = keep; + if let Err(err) = pc.close().await { + log::debug!("WebRTC background close failed: {}", err); + } + }); + } + + #[inline] + fn close_detached(&self) { + self.close_detached_with(()); + } + #[inline] pub fn set_raw(&mut self) { // not-supported @@ -828,7 +905,7 @@ impl WebRTCStream { // TCP-send-timeout equivalent. See the checklist entry on send backpressure. if send_timeout > 0 { let deadline = Instant::now() + Duration::from_millis(send_timeout); - let _send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { + let send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { Ok(Ok(permit)) => permit, Ok(Err(err)) => { return Err(Error::new( @@ -838,20 +915,17 @@ impl WebRTCStream { .into()); } Err(_) => { - if let Err(err) = self.pc.close().await { - log::warn!("failed to close WebRTC after send timeout: {}", err); - } + self.close_detached(); return Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()); } }; match timeout_at(deadline, self.send_bytes_inner(bytes)).await { Ok(res) => res, Err(_) => { - // Keep the logical-message permit while closing so no waiting clone can append - // a new message after a partially-written fragment sequence. - if let Err(err) = self.pc.close().await { - log::warn!("failed to close WebRTC after send timeout: {}", err); - } + // Hand the logical-message permit to the teardown so no waiting clone can + // append a new message after a partially-written fragment sequence. Holding + // it across an awaited close would drop both on cancellation. + self.close_detached_with(send_permit); Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()) } } @@ -867,7 +941,9 @@ impl WebRTCStream { } async fn send_bytes_inner(&mut self, bytes: Bytes) -> ResultType<()> { - if bytes.len() > MAX_FRAME_LENGTH { + // Same bound the receiver enforces, so we never emit a message a same-version peer + // would have to kill the connection over. + if bytes.len() > MAX_RECV_MESSAGE { return Err(Error::new(ErrorKind::InvalidInput, "Overflow").into()); } self.wait_for_connect_result().await?; @@ -896,19 +972,19 @@ impl WebRTCStream { #[inline] pub async fn next(&mut self) -> Option> { if let Err(err) = self.wait_for_connect_result().await { - self.pc.close().await.ok(); + self.close_detached(); return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); } let dc = match self.detached_dc().await { Ok(dc) => dc, Err(err) => { - self.pc.close().await.ok(); + self.close_detached(); return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); } }; // Held across the whole loop for exclusion, not for accumulator survival (see the field). - // Cancelling mid-`dc.read()` loses no data; cancelling mid-`pc.close()` on an error path - // below does leak the pc — see the checklist entry on cancel-safety. + // Cancelling mid-`dc.read()` loses no data, and every teardown below is detached rather + // than awaited, so a cancelled `next()` cannot strand the pc either. let mut st = self.recv_state.lock().await; if st.scratch.len() < RECV_BUF_SIZE { st.scratch.resize(RECV_BUF_SIZE, 0); @@ -918,7 +994,11 @@ impl WebRTCStream { let n = match dc.read(scratch.as_mut_slice()).await { Ok(n) => n, Err(err) => { - self.pc.close().await.ok(); + // Release the partial message, as the framing-violation paths below do: the + // buffer can hold up to the cap and `recv_state` outlives this call through + // the `SESSIONS` clone. + *acc = BytesMut::new(); + self.close_detached(); return Some(Err(Error::new( ErrorKind::Other, format!("data channel read error: {}", err), @@ -926,9 +1006,16 @@ impl WebRTCStream { } }; if n == 0 { - // Clean EOF: the remote reset the stream or shut its write half. An empty logical - // message is represented by a 1-byte header, so it is never confused with EOF. - self.pc.close().await.ok(); + // End of stream. Our own sender never produces this — every fragment carries at + // least its header byte — but it is NOT exclusively a reset: webrtc-data maps the + // StringEmpty/BinaryEmpty PPIDs to n == 0 as well, and `read` discards the flag + // that would separate them, so a peer can also reach here by sending one empty + // data-channel message. Both mean the same thing to us (this peer will send us + // nothing more we can frame), so treat them alike, but do not report it as a + // clean remote close: it is equally a peer that just violated the framing. + log::debug!("WebRTC data channel ended (reset or empty message)"); + *acc = BytesMut::new(); + self.close_detached(); return None; } // Two framing violations, both of which would otherwise be read as "more fragments": @@ -948,25 +1035,29 @@ impl WebRTCStream { }; if let Some(why) = bad { *acc = BytesMut::new(); - self.pc.close().await.ok(); + self.close_detached(); return Some(Err(Error::new( ErrorKind::InvalidData, format!("WebRTC {why}"), ))); } - acc.extend_from_slice(&scratch[1..n]); - // Match TCP's maximum frame size while preventing an unbounded FRAG_MORE stream from - // exhausting memory. - if acc.len() > MAX_FRAME_LENGTH { - // Release the buffer, don't just truncate it: by definition it is at the cap here, - // and `recv_state` outlives this call through the `SESSIONS` clone. + // Bound BEFORE growing, not after. This framing carries no length, so an oversize + // message can only be discovered by accumulating it — unlike the TCP codec, which + // rejects on the declared length before a single payload byte is buffered. Checking + // after the append would make the peak the cap plus a fragment, and `BytesMut` grows + // by reallocate-and-copy, so the final doubling would hold the old and new buffers at + // once: ~2x the cap in RSS for a session that has produced no message at all. + if acc.len() + (n - 1) > MAX_RECV_MESSAGE { + // Release the buffer, don't just truncate it: by definition it is near the cap + // here, and `recv_state` outlives this call through the `SESSIONS` clone. *acc = BytesMut::new(); - self.pc.close().await.ok(); + self.close_detached(); return Some(Err(Error::new( ErrorKind::InvalidData, "WebRTC reassembled message exceeded maximum frame size", ))); } + acc.extend_from_slice(&scratch[1..n]); if header == FRAG_END { let msg = std::mem::take(acc); return Some(Ok(msg)); @@ -992,8 +1083,8 @@ pub fn is_webrtc_endpoint(endpoint: &str) -> bool { mod tests { use crate::config; use crate::webrtc::WebRTCStream; - use crate::webrtc::{DEFAULT_ICE_SERVERS, FRAG_MORE}; - use bytes::{BufMut, BytesMut}; + use crate::webrtc::{DEFAULT_ICE_SERVERS, FRAG_MORE, SESSIONS}; + use bytes::{BufMut, Bytes, BytesMut}; use std::{sync::Arc, time::Duration}; use tokio::sync::Barrier; use tokio::time::timeout; @@ -1313,6 +1404,110 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc (offerer, answerer) } + // next()'s teardown paths must not be cancellable: every consumer polls next() inside a + // select! against a timer, and an awaited close that loses that race strands the pc + // (is_closed is already latched, so no later close retries) along with its SESSIONS entry, + // which only the state handler evicts. + // + // The cancellation itself is not what this test pins — `close_detached` is a non-async fn, + // so its callers have no await point to be cancelled at, and the compiler enforces that. + // What needs proving is the other half: that work handed to the runtime still runs to + // completion once the caller has walked away. + #[tokio::test] + async fn test_close_detached_completes_without_the_caller() { + let (offerer, answerer) = connect_loopback().await; + let key = format!("offer:{}", offerer.session_key()); + assert!( + SESSIONS.lock().await.contains_key(&key), + "offerer should be cached while live" + ); + + offerer.close_detached(); + drop(offerer); + + for _ in 0..200 { + if !SESSIONS.lock().await.contains_key(&key) { + answerer.close().await; + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("detached close never evicted the peer connection from SESSIONS"); + } + + // Extra channels opened after the bootstrap one must not displace it: rebinding would leave + // teardown closing a channel send/recv no longer use, and the newcomer's on_open would push + // Open onto the watch that gates both, re-arming a session already latched Closed. + // + // Scope, honestly: only the bind-once guard is exercised. The ordered+reliable check sits + // ahead of it but cannot decide anything here — `WebRTCStream::new` always creates its + // bootstrap channel first, so whatever the offerer adds afterwards is refused for being + // second regardless of its parameters. Covering that check needs a hand-built peer whose + // FIRST channel is unordered, which is more scaffolding than the guard is worth; it is a + // three-accessor test against parameters webrtc-rs derives verbatim from the remote's DCEP. + #[tokio::test] + async fn test_webrtc_answerer_binds_only_the_first_data_channel() { + use webrtc::data_channel::data_channel_init::RTCDataChannelInit; + + let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + // Replace the bootstrap channel's siblings: an unordered one and a duplicate. + let unordered = offerer + .pc + .create_data_channel( + "unordered", + Some(RTCDataChannelInit { + ordered: Some(false), + ..Default::default() + }), + ) + .await + .unwrap(); + let duplicate = offerer + .pc + .create_data_channel("duplicate", None) + .await + .unwrap(); + + let offer = offerer.get_local_endpoint_trickle().await.unwrap(); + let answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); + let answer = answerer.get_local_endpoint_trickle().await.unwrap(); + offerer.set_remote_endpoint(&answer).await.unwrap(); + + let mut off_ice = offerer.take_local_ice_rx().unwrap(); + let mut ans_ice = answerer.take_local_ice_rx().unwrap(); + let answerer_for_ice = answerer.clone(); + let offerer_for_ice = offerer.clone(); + tokio::spawn(async move { + while let Some(c) = off_ice.recv().await { + let _ = answerer_for_ice.add_remote_ice_candidate(&c).await; + } + }); + tokio::spawn(async move { + while let Some(c) = ans_ice.recv().await { + let _ = offerer_for_ice.add_remote_ice_candidate(&c).await; + } + }); + + offerer.wait_connected(20000).await.unwrap(); + let mut answerer = answerer; + answerer.wait_connected(20000).await.unwrap(); + + // The bootstrap channel still carries data: the rejected siblings did not displace it. + let payload = Bytes::from_static(b"bootstrap still bound"); + offerer.send_bytes(payload.clone()).await.unwrap(); + let got = tokio::time::timeout(Duration::from_secs(10), answerer.next()) + .await + .expect("answerer starved") + .expect("stream ended") + .expect("read failed"); + assert_eq!(&got[..], &payload[..]); + + drop(unordered); + drop(duplicate); + offerer.close().await; + answerer.close().await; + } + // One-shot callers exchange only the endpoints and never consume `take_local_ice_rx`. #[tokio::test] async fn test_webrtc_loopback_gathered_endpoints() { From dccf317b01d1d2755ea30c9f664442a06bbf6969 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Fri, 7 Aug 2026 15:44:43 +0800 Subject: [PATCH 17/35] webrtc: fix the send/cache/ICE-lifetime findings; bound log retention by volume Continues the review pass. Eight findings, each verified against the vendored webrtc-rs (0.13 / -data 0.11 / -ice 0.13) rather than inferred: - a mid-message dc.write() failure returned the error and left the pc open, so the peer kept an unterminated FRAG_MORE prefix and appended the next message to it - undetectable, since this framing carries no length or sequence number, and callers wrap sends in allow_err!. Close the stream when fragments are already on the wire. - one deadline covered the connect-wait, the gate queue and every write. A slow ICE/DTLS completion therefore ate the budget and the write timed out into a pc.close() an RTT from working; and the gate arm closed the pc from a task that never held the permit, aborting a healthy sender's fragment sequence - the exact corruption the permit exists to prevent. Connection setup gets its own budget, and only the arm holding the permit tears down. - a SESSIONS hit returned a pc built for the first caller, so a replayed offer could get an All-policy connection where the mediator had just computed Relay-only, with is_relayed() answering from the cached handle. It could also hand back a stream the state handler had already closed (it closes before it evicts). Reject both; peer_verified stays shared, being a fact about the certificate the entry is keyed by. - the ICE-candidate sender lives in the on_ice_candidate handler and close() clears no handler, so the receiver never closed and the forwarder loop this API asks callers to write parked on recv() holding a stream clone - one leaked task plus one leaked pc per connection. The terminal-state handler now drops that closure. Regression test included (fails, by 20s timeout, with the drop removed). - has_turn_server() accepted the RFC 7065 spelling `turn:host:port`, which url makes cannot-be-a-base: host_str() is None, so the server became a hostless "turn::3478" that still passed the scheme check. force_relay then built a Relay-only pc that could only time out. Parse host and port out of the path (IPv6 literals included), and make the gate require a host. - is_relayed() read the stats report's `nominated` flag, which webrtc-ice sets per checklist entry and never clears, so after a pair switch several entries carry it and HashMap order picked the answer. Read the selected pair instead - which also drops a redundant stats round trip. - the ICE-server parsing tests rewrote the process-global, on-disk `ice-servers` option while sibling loopback tests were building peer connections from it. Split parsing out of get_ice_servers so they can test it directly; the suite now passes in parallel. - log retention is a file count, so pairing it with the new 16 MiB size criterion let a flooder rotate away every file predating its own activity. Keep enough files that ~31 days survives even at full size. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/lib.rs | 8 +- src/webrtc.rs | 356 +++++++++++++++++++++++++++++++++++++------------- 2 files changed, 274 insertions(+), 90 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 15cc322a0d..695c378c05 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -464,7 +464,13 @@ pub fn init_log(_is_async: bool, _name: &str) -> Option (remote) ..."` by position. 0.17+ +//! makes the fields `pub`; switch to them and delete the parse. (It cannot use the stats +//! report's `nominated` flag instead: webrtc-ice sets that per checklist entry and never +//! clears it, so several entries carry it after a pair switch.) //! //! Then re-run the loopback tests at the bottom of this file (`cargo test --features webrtc //! webrtc::tests`). @@ -55,7 +59,6 @@ use webrtc::peer_connection::peer_connection_state::RTCPeerConnectionState; use webrtc::peer_connection::policy::ice_transport_policy::RTCIceTransportPolicy; use webrtc::peer_connection::sdp::session_description::RTCSessionDescription; use webrtc::peer_connection::RTCPeerConnection; -use webrtc::stats::StatsReportType; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use base64::Engine; @@ -125,19 +128,19 @@ const FRAG_MORE: u8 = 1; const FRAG_END: u8 = 0; /// Largest logical message this transport will send or reassemble. /// -/// Deliberately NOT `bytes_codec::MAX_FRAME_LENGTH` (~1 GiB), which this path borrowed: that -/// bound is affordable for TCP only because its length prefix lets the decoder reject an -/// oversize frame before buffering a byte of it. This framing carries no length, so the cap can -/// only be enforced by accumulating up to it — the cap IS the memory an unauthenticated peer can -/// make a receiver hold, and the answerer is spawned straight off a PunchHole, before any -/// password check. +/// Held at parity with TCP on purpose. A transport-specific ceiling would be a trap: the same +/// session moves between WebRTC and the relay, so a message under TCP's limit but over this one +/// would work on one path and kill the connection on the other — and it would bite hardest on +/// exactly the large messages (a keyframe, a big clipboard image) that a direct path exists to +/// carry. Whatever the right maximum is, both transports need the same one. /// -/// 64 MiB is ~500x the largest block RustDesk actually streams (fs.rs sends 128 KiB blocks; a -/// video frame or clipboard image is far below that), so no legitimate message can reach it, -/// while the worst case stays a bounded allocation rather than an OOM. Send and receive share -/// the constant so the two ends of a same-version pair always agree; raising it is the knob if a -/// real message ever needs more. -const MAX_RECV_MESSAGE: usize = 64 * 1024 * 1024; +/// That leaves a real exposure, shared with TCP and not created here: the cap is the memory an +/// unauthenticated peer can make a receiver hold, and the answerer runs before any password +/// check. TCP is no better off — `BytesCodec::new` leaves `max_packet_length` at `usize::MAX`, +/// so its only bound is the 30-bit length field in the frame header. Tightening it belongs in +/// one change across both paths, with a number measured against real traffic rather than +/// guessed; until then this at least holds the peak to the cap instead of twice it. +const MAX_RECV_MESSAGE: usize = crate::bytes_codec::MAX_FRAME_LENGTH; // use 3 public STUN servers to find out the NAT type, 2 must be the same address but different ports // https://stackoverflow.com/questions/72805316/determine-nat-mapping-behaviour-using-two-stun-servers // luckily nextcloud supports two ports for STUN @@ -263,30 +266,58 @@ impl WebRTCStream { #[inline] fn get_ice_server_from_url(url: &str) -> Option { - // standard url format with turn scheme: turn://user:pass@host:port - match Url::parse(url) { - Ok(u) => { - if u.scheme() == "turn" - || u.scheme() == "turns" - || u.scheme() == "stun" - || u.scheme() == "stuns" - { - Some(RTCIceServer { - urls: vec![format!( - "{}:{}:{}", - u.scheme(), - u.host_str().unwrap_or_default(), - u.port().unwrap_or(3478) - )], - username: u.username().to_string(), - credential: u.password().unwrap_or_default().to_string(), - ..Default::default() - }) - } else { - None - } - } - Err(_) => None, + let u = Url::parse(url).ok()?; + if !matches!(u.scheme(), "turn" | "turns" | "stun" | "stuns") { + return None; + } + // Two spellings are accepted, and they parse very differently: + // + // - `turn://user:pass@host:port` — non-standard, but the only form that can carry + // credentials. `url` sees an authority and fills host/port/username/password. + // - `turn:host:port` — the RFC 7065 form users actually copy from TURN docs. These + // schemes are not special and there is no `//`, so `url` makes it cannot-be-a-base: + // `host_str()` is None and the whole `host:port` lands in the path. Reading it back + // out is what makes this form work at all; it previously produced a hostless + // `turn::3478` that no ICE agent can resolve — while still satisfying + // `has_turn_server()`, so a force_relay peer connection was built against it and + // could only ever time out. + let (host, port) = match u.host_str() { + Some(host) => (host.to_owned(), u.port().unwrap_or(3478)), + None => Self::split_host_port(u.path())?, + }; + if host.is_empty() { + return None; + } + Some(RTCIceServer { + urls: vec![format!("{}:{}:{}", u.scheme(), host, port)], + username: u.username().to_string(), + credential: u.password().unwrap_or_default().to_string(), + ..Default::default() + }) + } + + /// Split `host[:port]` from an RFC 7065 URL path, defaulting the port. Bracketed IPv6 + /// literals keep their brackets, which is the form webrtc-rs re-parses. + fn split_host_port(path: &str) -> Option<(String, u16)> { + let rest = path.split(['?', '#']).next().unwrap_or_default(); + if rest.is_empty() { + return None; + } + if let Some(after_open) = rest.strip_prefix('[') { + let (host, tail) = after_open.split_once(']')?; + let port = tail + .strip_prefix(':') + .and_then(|p| p.parse().ok()) + .unwrap_or(3478); + return Some((format!("[{host}]"), port)); + } + match rest.rsplit_once(':') { + // Only a numeric tail is a port; anything else is part of the host. + Some((host, port)) if !host.is_empty() => match port.parse() { + Ok(port) => Some((host.to_owned(), port)), + Err(_) => Some((rest.to_owned(), 3478)), + }, + _ => Some((rest.to_owned(), 3478)), } } @@ -295,17 +326,30 @@ impl WebRTCStream { /// never connect — callers use this to skip building a guaranteed-dead pc. pub fn has_turn_server() -> bool { Self::get_ice_servers().iter().any(|s| { - s.urls - .iter() - .any(|u| u.starts_with("turn:") || u.starts_with("turns:")) + s.urls.iter().any(|u| { + // `scheme:host:port`, built by get_ice_server_from_url. A missing host would + // still match the scheme while being unusable, and answering `true` for one of + // those is worse than answering `false`: the caller skips its "don't build a + // guaranteed-dead Relay-only pc" guard on the strength of it. + u.strip_prefix("turn:") + .or_else(|| u.strip_prefix("turns:")) + .is_some_and(|rest| !rest.starts_with(':')) + }) }) } #[inline] fn get_ice_servers() -> Vec { - let mut ice_servers = Vec::new(); - let cfg = config::Config::get_option(config::keys::OPTION_ICE_SERVERS); + Self::parse_ice_servers(&config::Config::get_option( + config::keys::OPTION_ICE_SERVERS, + )) + } + /// Split out from `get_ice_servers` so parsing can be exercised without touching the + /// process-global, on-disk-persisted option — the tests run in parallel threads of one + /// process, so a test that rewrote it raced every peer connection another test was building. + fn parse_ice_servers(cfg: &str) -> Vec { + let mut ice_servers = Vec::new(); let mut has_stun = false; for url in cfg.split(',').map(str::trim) { @@ -356,12 +400,41 @@ impl WebRTCStream { let mut key = Self::get_key_for_sdp_json(&remote_offer)?; let start_local_offer = remote_offer.is_empty(); if !key.is_empty() { - let sessions_lock = SESSIONS.lock().await; - if let Some(cached_stream) = - sessions_lock.get(&Self::cache_key(&key, start_local_offer)) - { - log::debug!("Start webrtc with cached peer"); - return Ok(cached_stream.clone()); + let cached = { + let sessions_lock = SESSIONS.lock().await; + sessions_lock + .get(&Self::cache_key(&key, start_local_offer)) + .cloned() + }; + if let Some(cached_stream) = cached { + // A hit hands back a pc built for the FIRST caller. Two of its properties are + // this caller's to decide, so a mismatched entry must not be reused: + // + // - ICE policy. `rendezvous_mediator` recomputes relay-only per PunchHole, so a + // replayed offer asking for Relay-only would otherwise get back an All-policy + // pc, free to pick a direct pair — and `is_relayed()` would answer from the + // cached handle's own flag, describing a policy nobody asked for. + // - liveness. The state handler pushes Closed and closes the stream BEFORE it + // reaches SESSIONS to evict, so there is a window where the map still holds a + // dead pc whose `wait_for_connect_result` errors immediately. + // + // `peer_verified` is deliberately still shared: it is an identity fact about + // this DTLS certificate, not a per-caller setting, and the entry is keyed by + // that certificate's fingerprint. + let stale = matches!( + *cached_stream.state_notify.borrow(), + WebRTCConnectionState::Closed(_) + ); + if cached_stream.relay_only == force_relay && !stale { + log::debug!("Start webrtc with cached peer"); + return Ok(cached_stream); + } + log::debug!( + "Ignoring cached webrtc peer (relay_only {} != {}, or closed: {})", + cached_stream.relay_only, + force_relay, + stale + ); } } // Create a SettingEngine and enable Detach @@ -387,6 +460,14 @@ impl WebRTCStream { let (ice_tx, ice_rx) = mpsc::unbounded_channel::(); // Create a new RTCPeerConnection let pc = Arc::new(api.new_peer_connection(config).await?); + // This closure is the only owner of the ICE-candidate sender, and `on_*` handlers live + // inside the pc: nothing clears them — not `RTCPeerConnection::close`, not the ICE + // gatherer's own close — so the sender outlives `close()` and every `SESSIONS` eviction. + // The receiver therefore never sees the channel close, and the forwarder task this API + // asks callers to write (`while let Some(c) = rx.recv().await { peer.add_remote_ice(..) }` + // with a stream clone moved in) parks on `recv()` forever, holding the pc alive with it: + // one leaked task plus one leaked peer connection per connection. The terminal-state + // handler below drops this closure to break that; see the note there. let local_ice_tx = ice_tx.clone(); pc.on_ice_candidate(Box::new(move |candidate| { let local_ice_tx = local_ice_tx.clone(); @@ -516,6 +597,16 @@ impl WebRTCStream { let Some(pc_for_close2) = pc_for_close2.upgrade() else { return; }; + + // Drop the ICE-candidate handler, and with it the only sender for the + // local-candidate channel. Nothing else ever will: `close()` clears no + // handler, so the receiver would stay open forever and a caller's + // forwarder task would park on `recv()` holding a stream clone — keeping + // this pc alive past its own teardown. Replacing the handler with a + // no-op closes the channel, the forwarder's loop ends, and its clone + // goes with it. Gathering is over by this state anyway. + pc_for_close2.on_ice_candidate(Box::new(|_| Box::pin(async {}))); + let mut sessions_lock = SESSIONS.lock().await; match Self::get_key_for_peer(&pc_for_close2, start_local_offer).await { Ok(fingerprint) => { @@ -724,26 +815,26 @@ impl WebRTCStream { return Some(true); } let dtls = self.pc.sctp().transport(); - dtls.ice_transport().get_selected_candidate_pair().await?; - - // webrtc 0.13 keeps RTCIceCandidatePair's candidates private. Its stats report exposes - // the selected (nominated) pair and the corresponding candidate types instead. - let stats = self.pc.get_stats().await; - let pair = stats.reports.values().find_map(|report| match report { - StatsReportType::CandidatePair(pair) if pair.nominated => Some(pair), - _ => None, - })?; - let is_relay = |candidate_id: &str| { - matches!( - stats.reports.get(candidate_id), - Some( - StatsReportType::LocalCandidate(candidate) - | StatsReportType::RemoteCandidate(candidate) - ) if RTCIceCandidateType::from(candidate.candidate_type) - == RTCIceCandidateType::Relay - ) - }; - Some(is_relay(&pair.local_candidate_id) || is_relay(&pair.remote_candidate_id)) + let pair = dtls.ice_transport().get_selected_candidate_pair().await?; + + // Answer from the selected pair, not from the stats report's `nominated` flag: in + // webrtc-ice that flag is sticky per checklist entry (set on nomination, never cleared — + // a pair switch only clears the agent's own `nominated_pair` slot), and the report + // enumerates the whole checklist. After an ICE switch, or on a controlled agent that saw + // USE-CANDIDATE more than once, several entries carry it and `HashMap::values()` order + // decides the answer — which can differ between two calls in one session. + // + // 0.13 keeps the pair's candidates private, so read the types out of its `Display`: + // "(local) {protocol} {typ} {addr}:{port}{related} <-> (remote) ...". Positionally, not + // by substring — an address must not be able to pass for a candidate type. See the + // upgrade checklist: 0.17+ exposes the fields and this parse goes away. + let relay = RTCIceCandidateType::Relay.to_string(); + Some( + pair.to_string() + .split(" <-> ") + .filter_map(|side| side.split_whitespace().nth(2)) + .any(|typ| typ == relay), + ) } #[inline] @@ -904,6 +995,18 @@ impl WebRTCStream { // PendingQueue admits 128 KiB and inflight is cwnd/rwnd-capped — so this timeout is the // TCP-send-timeout equivalent. See the checklist entry on send backpressure. if send_timeout > 0 { + // Waiting for the connection to open is not this message's progress. Charging it to + // the send clock meant that on the first send after signaling, a slow ICE/DTLS + // completion ate most of the budget and the write inherited the remainder — then + // timed out and closed a peer connection that was an RTT from working. Give it its + // own budget and start the send clock once the channel is actually usable. + timeout( + Duration::from_millis(send_timeout), + self.wait_for_connect_result(), + ) + .await + .map_err(|_| Error::new(ErrorKind::TimedOut, "WebRTC connect timeout"))??; + let deadline = Instant::now() + Duration::from_millis(send_timeout); let send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { Ok(Ok(permit)) => permit, @@ -915,8 +1018,11 @@ impl WebRTCStream { .into()); } Err(_) => { - self.close_detached(); - return Err(Error::new(ErrorKind::TimedOut, "WebRTC send timeout").into()); + // Deliberately no teardown: this task never held the permit, so all that + // happened is that another clone is legitimately mid-message. Closing from + // here would abort a healthy sender's fragment sequence — precisely the + // corruption the permit exists to prevent. + return Err(Error::new(ErrorKind::TimedOut, "WebRTC send gate timeout").into()); } }; match timeout_at(deadline, self.send_bytes_inner(bytes)).await { @@ -953,6 +1059,7 @@ impl WebRTCStream { // Always emit at least one fragment (a lone FRAG_END header for an empty message), so a // zero-length data-channel message — which the receiver cannot distinguish from EOF — is // never sent. + let mut wrote_any = false; loop { let end = (offset + MAX_FRAGMENT_PAYLOAD).min(data.len()); let is_last = end >= data.len(); @@ -960,7 +1067,21 @@ impl WebRTCStream { let mut framed = BytesMut::with_capacity(1 + chunk.len()); framed.put_u8(if is_last { FRAG_END } else { FRAG_MORE }); framed.put_slice(chunk); - dc.write(&framed.freeze()).await?; + if let Err(err) = dc.write(&framed.freeze()).await { + if wrote_any { + // The sequence stops with fragments already on the wire, so the peer's + // accumulator holds a prefix that will never be terminated — and this framing + // carries no length or sequence number for it to notice with, so the next + // message is appended to the orphan and parsed as one corrupt frame. The + // stream cannot be made consistent again from this side; kill it. (Callers + // wrap sends in allow_err! and keep going, so returning the error alone would + // leave the corruption in place.) + log::warn!("WebRTC send failed mid-message, closing: {}", err); + self.close_detached(); + } + return Err(err.into()); + } + wrote_any = true; offset = end; if is_last { break; @@ -1139,26 +1260,57 @@ mod tests { None ); - config::Config::set_option("ice-servers".to_string(), "".to_string()); + // RFC 7065 spelling (`turn:host:port`, no authority) — what TURN docs hand out, and + // what users paste. `url` cannot-be-a-base's it, so host/port come out of the path; + // getting this wrong produced a hostless "turn::3478" that still passed the TURN gate. + for (input, expected) in [ + ("turn:example.com:3478", "turn:example.com:3478"), + ("turn:example.com", "turn:example.com:3478"), + ("turns:example.com:5349", "turns:example.com:5349"), + ("stun:example.com:19302", "stun:example.com:19302"), + ("turn:[2001:db8::1]:3478", "turn:[2001:db8::1]:3478"), + ("turn:[2001:db8::1]", "turn:[2001:db8::1]:3478"), + ( + "turn:example.com:3478?transport=udp", + "turn:example.com:3478", + ), + ] { + assert_eq!( + WebRTCStream::get_ice_server_from_url(input) + .unwrap_or_default() + .urls[0], + expected, + "parsing {input}" + ); + } + assert_eq!(WebRTCStream::get_ice_server_from_url("turn:"), None); + + // The gate must not green-light an unusable server: a hostless entry makes the caller + // skip its "don't build a guaranteed-dead Relay-only pc" guard. + assert!(!WebRTCStream::parse_ice_servers("turn:") + .iter() + .any(|s| s.urls.iter().any(|u| u.starts_with("turn:")))); + } + + // Parsing is exercised through `parse_ice_servers`, never by rewriting the global + // `ice-servers` option: `set_option` persists to the real config file and the tests share + // one process, so mutating it raced every peer connection the loopback tests were building. + #[test] + fn test_webrtc_ice_server_list() { assert_eq!( - WebRTCStream::get_ice_servers()[0].urls[0], + WebRTCStream::parse_ice_servers("")[0].urls[0], DEFAULT_ICE_SERVERS[0].to_string() ); - config::Config::set_option( - "ice-servers".to_string(), - ",stun://example.com,turn://example.com,sdf".to_string(), - ); - assert_eq!( - WebRTCStream::get_ice_servers()[0].urls[0], - "stun:example.com:3478" - ); - assert_eq!( - WebRTCStream::get_ice_servers()[1].urls[0], - "turn:example.com:3478" - ); - assert_eq!(WebRTCStream::get_ice_servers().len(), 2); - config::Config::set_option("ice-servers".to_string(), "".to_string()); + let parsed = WebRTCStream::parse_ice_servers(",stun://example.com,turn://example.com,sdf"); + assert_eq!(parsed[0].urls[0], "stun:example.com:3478"); + assert_eq!(parsed[1].urls[0], "turn:example.com:3478"); + assert_eq!(parsed.len(), 2); + + // TURN-only config still gets the default STUN servers prepended. + let turn_only = WebRTCStream::parse_ice_servers("turn:example.com:3478"); + assert_eq!(turn_only[0].urls[0], DEFAULT_ICE_SERVERS[0].to_string()); + assert_eq!(turn_only[1].urls[0], "turn:example.com:3478"); } #[test] @@ -1435,6 +1587,32 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc panic!("detached close never evicted the peer connection from SESSIONS"); } + // The local-candidate channel must close when the pc does. Its only sender lives inside the + // on_ice_candidate handler, which close() does not clear, so without the teardown the + // receiver stays open forever — and the forwarder loop this API asks callers to write holds + // a stream clone while parked on recv(), keeping the pc alive past its own close. + #[tokio::test] + async fn test_local_ice_channel_closes_with_the_peer_connection() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let mut ice_rx = offerer.take_local_ice_rx().unwrap(); + + // Exactly the loop every consumer writes, holding a clone of the stream. + let forwarder_stream = offerer.clone(); + let forwarder = tokio::spawn(async move { + while let Some(candidate) = ice_rx.recv().await { + let _ = forwarder_stream.add_remote_ice_candidate(&candidate).await; + } + }); + + offerer.close().await; + drop(offerer); + + tokio::time::timeout(Duration::from_secs(20), forwarder) + .await + .expect("forwarder task outlived the peer connection") + .unwrap(); + } + // Extra channels opened after the bootstrap one must not displace it: rebinding would leave // teardown closing a channel send/recv no longer use, and the newcomer's on_open would push // Open onto the watch that gates both, re-arming a session already latched Closed. From 6677318bd2d9db9873bbb960a6edb8ec664e5df7 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 8 Aug 2026 10:56:35 +0800 Subject: [PATCH 18/35] webrtc: make the cache guard actually apply; drop unusable ICE servers Two findings, both cases of a check that reads correct but never runs. - The SESSIONS admissibility test was applied at the lookup only. The insert-time duplicate check reads the SAME key and returned whatever it found, so an entry the lookup had just rejected came straight back - the freshly built peer connection was closed and the rejected one returned in its place. A Relay-only request could therefore be served by a cached All-policy pc (free to pick a direct pair, with is_relayed() answering for a policy nobody asked for), and a caller could be handed a pc already latched Closed. Both sites now share `is_reusable_for`, which is the only way a test on a shared key holds. - A TURN server with no credentials is not just useless: webrtc-rs validates every configured server when the peer connection is built, so one such entry fails EVERY connection, including plain non-relay ones that never wanted TURN. The RFC 7065 spelling this branch taught us to parse has nowhere to put credentials, so it produced exactly that entry - and has_turn_server() then reported TURN as available, making callers skip their "don't build a guaranteed-dead Relay-only pc" guard. Drop such entries at the source (with a log naming the spelling that does carry credentials); has_turn_server() goes back to a plain scheme test, which is sound once the constructor guarantees a host and credentials. Malformed entries are now dropped rather than repaired: an unparsable port used to be folded back into the host ("host:99999:3478") and an unbracketed IPv6 literal was split at its last colon, both of which webrtc-ice rejects - again taking every server down, not just the bad one. Also spell `&'static str` on the two associated consts (an elided lifetime there is a future hard error on the pinned 1.75 toolchain) and drop a test import left behind when the tests stopped touching config. Regression test for the cache guard; mutation-checked, as are the credential and malformed-entry paths. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 238 +++++++++++++++++++++++++++++++------------------- 1 file changed, 150 insertions(+), 88 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 18d7ddbf7b..7ddc12f9f9 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -205,8 +205,10 @@ impl WebRTCStream { // Envelope JSON key carrying the local description's ICE transport policy, alongside the // RTCSessionDescription fields (see `get_local_endpoint_trickle`). - const ICE_POLICY_KEY: &str = "ice_policy"; - const ICE_POLICY_ALL: &str = "all"; + // `'static` spelled out: an elided lifetime here is a warn-by-default future hard error on + // the 1.75 toolchain CI pins (elided_lifetimes_in_associated_constant). + const ICE_POLICY_KEY: &'static str = "ice_policy"; + const ICE_POLICY_ALL: &'static str = "all"; #[inline] fn get_key_for_sdp(sdp: &RTCSessionDescription) -> ResultType { @@ -244,6 +246,25 @@ impl WebRTCStream { ) } + /// Whether a `SESSIONS` entry may be handed to a caller asking for `force_relay`. + /// + /// A hit returns a pc built for the FIRST caller, and two of its properties belong to this + /// caller instead: the ICE policy (`rendezvous_mediator` recomputes relay-only per PunchHole, + /// so a replayed offer wanting Relay-only must not get an All-policy pc free to pick a direct + /// pair), and liveness (the state handler latches Closed and closes the stream before it + /// reaches SESSIONS to evict, so the map briefly holds dead entries). `peer_verified` stays + /// shared on purpose: it is a fact about the DTLS certificate the entry is keyed by. + /// + /// Both the lookup and the insert-time duplicate check must use this — they read the same key, + /// so a test applied at only one of them is not applied at all. + fn is_reusable_for(&self, force_relay: bool) -> bool { + self.relay_only == force_relay + && !matches!( + *self.state_notify.borrow(), + WebRTCConnectionState::Closed(_) + ) + } + #[inline] fn get_key_for_sdp_json(sdp_json: &str) -> ResultType { if sdp_json.is_empty() { @@ -288,10 +309,28 @@ impl WebRTCStream { if host.is_empty() { return None; } + let username = u.username().to_string(); + let credential = u.password().unwrap_or_default().to_string(); + // A TURN server without credentials is not merely useless: webrtc-rs validates every + // configured server in `new_peer_connection`, so one credential-less entry makes EVERY + // peer connection fail — including plain non-relay ones that never wanted TURN. The + // RFC 7065 spelling has nowhere to put credentials, so drop such entries here rather + // than let them poison the whole configuration; `turn://user:pass@host:port` carries them. + if matches!(u.scheme(), "turn" | "turns") && (username.is_empty() || credential.is_empty()) + { + log::warn!( + "Ignoring TURN server without credentials: {}:{}:{} (use {}://user:pass@host:port)", + u.scheme(), + host, + port, + u.scheme() + ); + return None; + } Some(RTCIceServer { urls: vec![format!("{}:{}:{}", u.scheme(), host, port)], - username: u.username().to_string(), - credential: u.password().unwrap_or_default().to_string(), + username, + credential, ..Default::default() }) } @@ -311,11 +350,23 @@ impl WebRTCStream { .unwrap_or(3478); return Some((format!("[{host}]"), port)); } + // An unbracketed IPv6 literal has no unambiguous split point — `2001:db8::1` would be cut + // at its last colon into host `2001:db8:` port `1` — so require the bracketed form for + // those rather than emit a host webrtc-ice cannot resolve. + if rest.matches(':').count() > 1 { + log::warn!("Ignoring ICE server {rest}: bracket IPv6 literals as [addr]:port"); + return None; + } match rest.rsplit_once(':') { - // Only a numeric tail is a port; anything else is part of the host. Some((host, port)) if !host.is_empty() => match port.parse() { Ok(port) => Some((host.to_owned(), port)), - Err(_) => Some((rest.to_owned(), 3478)), + // A port that is present but unusable is a typo, not a host: folding it back in + // would produce `host:99999:3478`, which webrtc-ice rejects outright — taking + // every peer connection down with it, not just this server. + Err(_) => { + log::warn!("Ignoring ICE server {rest}: invalid port"); + None + } }, _ => Some((rest.to_owned(), 3478)), } @@ -325,16 +376,14 @@ impl WebRTCStream { /// connection (force_relay) can only gather relay candidates, so without a TURN server it can /// never connect — callers use this to skip building a guaranteed-dead pc. pub fn has_turn_server() -> bool { + // `get_ice_server_from_url` is what makes a bare scheme test sufficient: it drops entries + // with no host and TURN entries with no credentials, i.e. exactly the ones that would + // answer `true` here while being unusable — which is worse than answering `false`, since + // the caller skips its "don't build a guaranteed-dead Relay-only pc" guard on our word. Self::get_ice_servers().iter().any(|s| { - s.urls.iter().any(|u| { - // `scheme:host:port`, built by get_ice_server_from_url. A missing host would - // still match the scheme while being unusable, and answering `true` for one of - // those is worse than answering `false`: the caller skips its "don't build a - // guaranteed-dead Relay-only pc" guard on the strength of it. - u.strip_prefix("turn:") - .or_else(|| u.strip_prefix("turns:")) - .is_some_and(|rest| !rest.starts_with(':')) - }) + s.urls + .iter() + .any(|u| u.starts_with("turn:") || u.starts_with("turns:")) }) } @@ -407,33 +456,14 @@ impl WebRTCStream { .cloned() }; if let Some(cached_stream) = cached { - // A hit hands back a pc built for the FIRST caller. Two of its properties are - // this caller's to decide, so a mismatched entry must not be reused: - // - // - ICE policy. `rendezvous_mediator` recomputes relay-only per PunchHole, so a - // replayed offer asking for Relay-only would otherwise get back an All-policy - // pc, free to pick a direct pair — and `is_relayed()` would answer from the - // cached handle's own flag, describing a policy nobody asked for. - // - liveness. The state handler pushes Closed and closes the stream BEFORE it - // reaches SESSIONS to evict, so there is a window where the map still holds a - // dead pc whose `wait_for_connect_result` errors immediately. - // - // `peer_verified` is deliberately still shared: it is an identity fact about - // this DTLS certificate, not a per-caller setting, and the entry is keyed by - // that certificate's fingerprint. - let stale = matches!( - *cached_stream.state_notify.borrow(), - WebRTCConnectionState::Closed(_) - ); - if cached_stream.relay_only == force_relay && !stale { + if cached_stream.is_reusable_for(force_relay) { log::debug!("Start webrtc with cached peer"); return Ok(cached_stream); } log::debug!( - "Ignoring cached webrtc peer (relay_only {} != {}, or closed: {})", + "Ignoring cached webrtc peer (relay_only {}, wanted {})", cached_stream.relay_only, - force_relay, - stale + force_relay ); } } @@ -706,11 +736,15 @@ impl WebRTCStream { let cache_key = Self::cache_key(&key, start_local_offer); let duplicate = { let mut final_lock = SESSIONS.lock().await; - if let Some(session) = final_lock.get(&cache_key) { - Some(session.clone()) - } else { - final_lock.insert(cache_key, webrtc_stream.clone()); - None + // Same admissibility test as the lookup above, or that lookup is dead code: an entry + // rejected there is still in the map when we get here, so returning it unconditionally + // would discard the pc we just built precisely because the cached one was unusable. + match final_lock.get(&cache_key) { + Some(session) if session.is_reusable_for(force_relay) => Some(session.clone()), + _ => { + final_lock.insert(cache_key, webrtc_stream.clone()); + None + } } }; if let Some(session) = duplicate { @@ -1202,7 +1236,6 @@ pub fn is_webrtc_endpoint(endpoint: &str) -> bool { #[cfg(test)] mod tests { - use crate::config; use crate::webrtc::WebRTCStream; use crate::webrtc::{DEFAULT_ICE_SERVERS, FRAG_MORE, SESSIONS}; use bytes::{BufMut, Bytes, BytesMut}; @@ -1213,41 +1246,37 @@ mod tests { #[test] fn test_webrtc_ice_url() { - assert_eq!( - WebRTCStream::get_ice_server_from_url("turn://example.com:3478") - .unwrap_or_default() - .urls[0], - "turn:example.com:3478" - ); + let turn = WebRTCStream::get_ice_server_from_url("turn://123:321@example.com:3478") + .expect("credentialed turn is usable"); + assert_eq!(turn.urls[0], "turn:example.com:3478"); + assert_eq!(turn.username, "123"); + assert_eq!(turn.credential, "321"); assert_eq!( - WebRTCStream::get_ice_server_from_url("turn://example.com") + WebRTCStream::get_ice_server_from_url("turn://123:321@example.com") .unwrap_or_default() .urls[0], "turn:example.com:3478" ); - assert_eq!( - WebRTCStream::get_ice_server_from_url("turn://123@example.com") - .unwrap_or_default() - .username, - "123" - ); - - assert_eq!( - WebRTCStream::get_ice_server_from_url("turn://123@example.com") - .unwrap_or_default() - .credential, - "" - ); - - assert_eq!( - WebRTCStream::get_ice_server_from_url("turn://123:321@example.com") - .unwrap_or_default() - .credential, - "321" - ); + // TURN without both halves of the credential is dropped rather than passed on: + // webrtc-rs validates every configured server when the peer connection is built, so + // one such entry fails EVERY connection, including those that never wanted TURN. + for missing in [ + "turn://example.com:3478", + "turn://example.com", + "turn://123@example.com", + "turns://example.com:5349", + "turn:example.com:3478", + ] { + assert_eq!( + WebRTCStream::get_ice_server_from_url(missing), + None, + "credential-less {missing} must not reach the configuration" + ); + } + // STUN needs no credentials, so both spellings stay usable. assert_eq!( WebRTCStream::get_ice_server_from_url("stun://example.com:3478") .unwrap_or_default() @@ -1260,19 +1289,17 @@ mod tests { None ); - // RFC 7065 spelling (`turn:host:port`, no authority) — what TURN docs hand out, and - // what users paste. `url` cannot-be-a-base's it, so host/port come out of the path; - // getting this wrong produced a hostless "turn::3478" that still passed the TURN gate. + // RFC 7065 spelling (`scheme:host:port`, no authority) — what STUN/TURN docs hand out. + // `url` cannot-be-a-base's it, so host and port come out of the path; getting this wrong + // produced a hostless "stun::3478" no ICE agent can resolve. for (input, expected) in [ - ("turn:example.com:3478", "turn:example.com:3478"), - ("turn:example.com", "turn:example.com:3478"), - ("turns:example.com:5349", "turns:example.com:5349"), ("stun:example.com:19302", "stun:example.com:19302"), - ("turn:[2001:db8::1]:3478", "turn:[2001:db8::1]:3478"), - ("turn:[2001:db8::1]", "turn:[2001:db8::1]:3478"), + ("stun:example.com", "stun:example.com:3478"), + ("stun:[2001:db8::1]:19302", "stun:[2001:db8::1]:19302"), + ("stun:[2001:db8::1]", "stun:[2001:db8::1]:3478"), ( - "turn:example.com:3478?transport=udp", - "turn:example.com:3478", + "stun:example.com:19302?transport=udp", + "stun:example.com:19302", ), ] { assert_eq!( @@ -1283,13 +1310,22 @@ mod tests { "parsing {input}" ); } - assert_eq!(WebRTCStream::get_ice_server_from_url("turn:"), None); - // The gate must not green-light an unusable server: a hostless entry makes the caller - // skip its "don't build a guaranteed-dead Relay-only pc" guard. - assert!(!WebRTCStream::parse_ice_servers("turn:") - .iter() - .any(|s| s.urls.iter().any(|u| u.starts_with("turn:")))); + // Malformed entries are dropped, never repaired into something webrtc-ice will choke + // on: a folded-in bad port ("host:99999:3478") or an unbracketed IPv6 split at its last + // colon fails peer-connection construction outright, taking every server down with it. + for bad in [ + "stun:", + "stun:example.com:99999", + "stun:example.com:abc", + "stun:2001:db8::1", + ] { + assert_eq!( + WebRTCStream::get_ice_server_from_url(bad), + None, + "malformed {bad} must be dropped" + ); + } } // Parsing is exercised through `parse_ice_servers`, never by rewriting the global @@ -1302,13 +1338,16 @@ mod tests { DEFAULT_ICE_SERVERS[0].to_string() ); - let parsed = WebRTCStream::parse_ice_servers(",stun://example.com,turn://example.com,sdf"); + // Unusable entries drop out of the list; the rest of the config still applies. + let parsed = WebRTCStream::parse_ice_servers( + ",stun://example.com,turn://u:p@example.com,turn://nocreds.example.com,sdf", + ); assert_eq!(parsed[0].urls[0], "stun:example.com:3478"); assert_eq!(parsed[1].urls[0], "turn:example.com:3478"); assert_eq!(parsed.len(), 2); // TURN-only config still gets the default STUN servers prepended. - let turn_only = WebRTCStream::parse_ice_servers("turn:example.com:3478"); + let turn_only = WebRTCStream::parse_ice_servers("turn://u:p@example.com:3478"); assert_eq!(turn_only[0].urls[0], DEFAULT_ICE_SERVERS[0].to_string()); assert_eq!(turn_only[1].urls[0], "turn:example.com:3478"); } @@ -1587,6 +1626,29 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc panic!("detached close never evicted the peer connection from SESSIONS"); } + // A replayed offer asking for a different ICE policy must not be handed the cached peer + // connection. The lookup and the insert-time duplicate check read the same key, so the test + // fails if either one stops applying `is_reusable_for` — which is how the guard was dead + // code: the lookup rejected the entry, and the insert handed back the very same one. + #[tokio::test] + async fn test_cached_peer_is_not_reused_across_ice_policies() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let offer = offerer.get_local_endpoint_trickle().await.unwrap(); + + let all_ice = WebRTCStream::new(&offer, false, 20000).await.unwrap(); + assert!(!all_ice.relay_only); + + let relay_only = WebRTCStream::new(&offer, true, 20000).await.unwrap(); + assert!( + relay_only.relay_only, + "a Relay-only request was answered with the cached All-policy peer connection" + ); + + relay_only.close().await; + all_ice.close().await; + offerer.close().await; + } + // The local-candidate channel must close when the pc does. Its only sender lives inside the // on_ice_candidate handler, which close() does not clear, so without the teardown the // receiver stays open forever — and the forwarder loop this API asks callers to write holds From 73007cb38e1fd688543d14b591ab788e0a6384a3 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 8 Aug 2026 13:10:04 +0800 Subject: [PATCH 19/35] webrtc: split the send budget, make close_webrtc uncancellable, restore log retention - send_bytes computed one deadline before acquiring the send gate and reused it for the write, so time queued behind another clone's message was charged to this message. A caller that only just lost the gate race got a few milliseconds to write in and then tore the whole peer connection down for missing them - the narrower the miss, the more certain the teardown. The write gets its own budget; the gate wait keeps the one it had, and still never closes (it never held the permit). - Stream::close_webrtc awaited WebRTCStream::close, which is exactly the form close_detached exists to avoid: most callers sit in a select! arm or a future the UI can abandon, and a cancelled close is unretryable (is_closed is latched before the first await, so later attempts early-return and the handler that evicts the session never runs). It is now a plain fn calling close_detached - with no await point there is nothing to cancel. close_detached is public and carries the runtime-teardown guard the parent repo had written separately for its Drop path, so that copy goes away. - Log retention goes back to 31 files. Raising it to 31*8 defended the flood case badly (a file count cannot outrun a flood; only the rate limits at the log sites can) while silently multiplying steady-state retention and disk for every ordinary install, on every platform. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/lib.rs | 14 +++++++------- src/stream.rs | 12 +++++++++--- src/webrtc.rs | 41 ++++++++++++++++++++++++++++++----------- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 695c378c05..3e7435e577 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -464,13 +464,13 @@ pub fn init_log(_is_async: bool, _name: &str) -> Option s.close().await, + Stream::WebRTC(s) => s.close_detached(), #[allow(unreachable_patterns)] _ => {} } diff --git a/src/webrtc.rs b/src/webrtc.rs index 7ddc12f9f9..fdb7297bef 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -928,18 +928,31 @@ impl WebRTCStream { /// `keep` carries anything whose lifetime must span the teardown rather than the caller's: /// the send path passes its logical-message permit, so no waiting clone can append to a /// partially-written fragment sequence while the close is still in flight. - fn close_detached_with(&self, keep: T) { + pub fn close_detached_with(&self, keep: T) { let pc = self.pc.clone(); - tokio::spawn(async move { - let _keep = keep; - if let Err(err) = pc.close().await { - log::debug!("WebRTC background close failed: {}", err); - } - }); + // Take the runtime handle explicitly rather than calling `tokio::spawn`: this also runs + // from `Drop` impls, which can execute during runtime teardown where a bare spawn panics. + // `Handle::spawn` can panic while the runtime is shutting down too, so catch it — a brief + // leak until process exit beats aborting the process from a destructor. + let Ok(handle) = tokio::runtime::Handle::try_current() else { + log::warn!("no tokio runtime available to close the WebRTC peer connection"); + return; + }; + let spawned = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + handle.spawn(async move { + let _keep = keep; + if let Err(err) = pc.close().await { + log::debug!("WebRTC background close failed: {}", err); + } + }); + })); + if spawned.is_err() { + log::warn!("failed to spawn the WebRTC close (runtime shutting down)"); + } } #[inline] - fn close_detached(&self) { + pub fn close_detached(&self) { self.close_detached_with(()); } @@ -1041,8 +1054,8 @@ impl WebRTCStream { .await .map_err(|_| Error::new(ErrorKind::TimedOut, "WebRTC connect timeout"))??; - let deadline = Instant::now() + Duration::from_millis(send_timeout); - let send_permit = match timeout_at(deadline, send_gate.acquire_owned()).await { + let gate_deadline = Instant::now() + Duration::from_millis(send_timeout); + let send_permit = match timeout_at(gate_deadline, send_gate.acquire_owned()).await { Ok(Ok(permit)) => permit, Ok(Err(err)) => { return Err(Error::new( @@ -1059,7 +1072,13 @@ impl WebRTCStream { return Err(Error::new(ErrorKind::TimedOut, "WebRTC send gate timeout").into()); } }; - match timeout_at(deadline, self.send_bytes_inner(bytes)).await { + // Fresh budget once the permit is held: queueing behind another clone's message is + // not this message's progress, so charging it here meant a caller that only just lost + // the gate race got a few milliseconds to write in — and then tore the whole peer + // connection down for missing them. The narrower the miss, the more certain the + // teardown, which is precisely backwards. + let write_deadline = Instant::now() + Duration::from_millis(send_timeout); + match timeout_at(write_deadline, self.send_bytes_inner(bytes)).await { Ok(res) => res, Err(_) => { // Hand the logical-message permit to the teardown so no waiting clone can From 4d1b9774054c9973a9a1911bcf3ac9ca90603da4 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 8 Aug 2026 17:59:46 +0800 Subject: [PATCH 20/35] stream: close the peer connection on drop A WebRTC peer connection outlives its handle - the session cache holds a clone, and the handler that evicts it only fires on a terminal ICE state - so it has to be closed explicitly. Making that a per-exit-path obligation meant every return, break and `?` had to remember it, and the long-lived side never did: server::connection ends its ~15 exits by dropping the stream, and the transport race drops the losing result outright. Nothing warned; a missed close leaks a pc, its ICE agent and its sockets silently, and only under WebRTC. Stream is not Clone, so dropping it is the end of the transport and there is no second owner to surprise - drop-means-close is simply what the type already meant. The explicit close_webrtc() calls stay valid (they close sooner than scope end), but they are an optimization now rather than the thing correctness rests on. Also delete get_webrtc_stream(): it had no callers and was the one API handing out an owned clone that outlives its Stream, i.e. the only way to defeat this. Regression test included; it fails with the drop body emptied. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/stream.rs | 22 +++++++++++++++------- src/webrtc.rs | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 135dbaa99c..676fd11f7a 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -203,13 +203,21 @@ impl Stream { pub fn from(stream: TcpStream, stream_addr: SocketAddr) -> Self { Self::Tcp(tcp::FramedStream::from(stream, stream_addr)) } +} - #[inline] - #[cfg(feature = "webrtc")] - pub fn get_webrtc_stream(&self) -> Option { - match self { - Self::WebRTC(s) => Some(s.clone()), - _ => None, - } +/// Owning the stream owns the transport, WebRTC included. +/// +/// A peer connection outlives its handle — the session cache holds a clone, and the handler that +/// evicts it only fires on a terminal ICE state — so it has to be closed explicitly. Doing that +/// at each exit path made it an obligation every `return`, `break` and `?` had to remember, and +/// the long-lived side never did: `server::connection` ends its ~15 exits by dropping the stream. +/// Nothing warned, because a missed close leaks silently and only under WebRTC. +/// +/// `Stream` is not `Clone`, so dropping it really is the end of the transport and there is no +/// second owner to surprise. Explicit `close_webrtc()` calls remain valid — they close sooner +/// than scope end — but they are now an optimization rather than the thing correctness rests on. +impl Drop for Stream { + fn drop(&mut self) { + self.close_webrtc(); } } diff --git a/src/webrtc.rs b/src/webrtc.rs index fdb7297bef..a81fc8612a 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1645,6 +1645,31 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc panic!("detached close never evicted the peer connection from SESSIONS"); } + // Owning the Stream owns the peer connection: dropping it must close the pc and evict the + // session, without the owner having to remember to. Every exit path used to carry that + // obligation, and the controlled side never honoured it. + #[tokio::test] + async fn dropping_the_stream_closes_the_peer_connection() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let key = format!("offer:{}", offerer.session_key()); + assert!( + SESSIONS.lock().await.contains_key(&key), + "offerer should be cached while live" + ); + + // No explicit close anywhere: the stream simply goes out of scope, as it does on the + // exits that forget. + drop(crate::Stream::WebRTC(offerer)); + + for _ in 0..200 { + if !SESSIONS.lock().await.contains_key(&key) { + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("dropping the stream left the peer connection in SESSIONS"); + } + // A replayed offer asking for a different ICE policy must not be handed the cached peer // connection. The lookup and the insert-time duplicate check read the same key, so the test // fails if either one stops applying `is_reusable_for` — which is how the guard was dead From ddea60cd3d1c4934cfd352d033ce6e5f290bdb7e Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 8 Aug 2026 18:17:37 +0800 Subject: [PATCH 21/35] webrtc: hand whole messages out of the read buffer instead of copying them A message that arrives in one fragment was read into a scratch buffer and then copied into a second, freshly allocated one - an allocation and a full copy per message, on top of SCTP's own reassembly copy, for every input event and most audio packets. The read buffer is a BytesMut now, so such a message is split straight out of it: the caller gets a slice sharing the allocation, with no copy and no allocation. Splitting consumes the buffer from the front, so it is re-initialized in chunks rather than per read, which spreads the one remaining cost (zeroing) across every small message that fits in a chunk. Slices keep their chunk alive, so this trades a bounded amount of retention for the copy. Multi-fragment messages still accumulate, unchanged - there is nothing to hand out until the last fragment arrives. Test alternates whole and fragmented messages either side of the fragment boundary for long enough to cross several refills, which is where the consumed-buffer scheme differs from the fixed one; an off-by-one in the split reddens it. The send side keeps its per-fragment copy: prepending the header byte is what forces it, and removing that needs the fragment flag to move into the SCTP PPID - another wire change, so it is deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 84 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 16 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index a81fc8612a..1892285c51 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -62,7 +62,7 @@ use webrtc::peer_connection::RTCPeerConnection; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use base64::Engine; -use bytes::{BufMut, Bytes, BytesMut}; +use bytes::{Buf, BufMut, Bytes, BytesMut}; use tokio::sync::{mpsc, watch, Mutex, Semaphore}; use tokio::time::{timeout, timeout_at, Instant}; use url::Url; @@ -94,11 +94,12 @@ pub struct WebRTCStream { // Serialize a complete logical message across clones. Each fragment is a separate SCTP // message, so serializing only individual writes would allow two large messages to interleave. send_gate: Arc, - // Receive-side reassembly state. The accumulator survives a cancelled `next()` because it - // lives here behind the Arc, not in the future. The mutex excludes a concurrent reader only - // while `next()` is actually running — a cancellation drops the guard mid-message, so it is - // the single-reader assumption, not the lock, that ultimately prevents two readers splicing - // into one `acc`. Single reader assumed, like the rest of the stream API. + // Receive-side reassembly state, behind the Arc rather than in the future so a cancelled + // `next()` does not lose the partial message. SINGLE READER ONLY: reassembly spans calls, and + // the fragment header carries no length or sequence number, so two readers splice unrelated + // fragments into one `acc` and produce a well-formed, undetectably wrong message. Every + // consumer today reads from one task; keep it that way (this is why no API hands out a clone + // that outlives its `Stream`). recv_state: Arc>, // True once the controller has completed the RustDesk identity binding (DTLS fingerprint // matched to the signed peer id, via `set_key`). DTLS always encrypts; this flag mirrors TCP's @@ -108,10 +109,14 @@ pub struct WebRTCStream { #[derive(Default)] struct RecvState { - // Accumulated payload of the logical message currently being reassembled. + // Accumulated payload of the logical message currently being reassembled. Only multi-fragment + // messages reach it; a message that arrives whole is split straight out of `scratch`. acc: BytesMut, - // Reused read scratch buffer, avoiding a per-message allocation. - scratch: Vec, + // Read buffer, refilled in `SCRATCH_REFILL` chunks rather than per read. A whole-message + // fragment is handed to the caller with `split_to`, which shares this allocation instead of + // copying, so the buffer is consumed from the front and re-initialized only when what is left + // can no longer hold one fragment — amortizing that cost over many small messages. + scratch: BytesMut, } // The SCTP data channel's 65536-byte max message size is handled by @@ -122,6 +127,11 @@ const MAX_FRAGMENT_PAYLOAD: usize = 60000; /// Receive scratch size: must be >= 1 (fragment header) + `MAX_FRAGMENT_PAYLOAD` and fit the /// negotiated SCTP max message size. const RECV_BUF_SIZE: usize = 64 * 1024; +/// How much read buffer to initialize at a time. Whole messages are split out of it without +/// copying, so it is consumed rather than reused; refilling in chunks spreads the one cost that +/// remains — zeroing — across every small message that fits. Slices handed to the caller keep the +/// whole chunk alive, so this trades a bounded amount of retention for the copy. +const SCRATCH_REFILL: usize = 4 * RECV_BUF_SIZE; /// Fragment header byte: more fragments follow for this logical message. const FRAG_MORE: u8 = 1; /// Fragment header byte: final (or only) fragment of a logical message. @@ -1156,16 +1166,18 @@ impl WebRTCStream { return Some(Err(Error::new(ErrorKind::Other, err.to_string()))); } }; - // Held across the whole loop for exclusion, not for accumulator survival (see the field). - // Cancelling mid-`dc.read()` loses no data, and every teardown below is detached rather - // than awaited, so a cancelled `next()` cannot strand the pc either. + // Held across `dc.read().await` on purpose, against the usual "no locks across await" + // rule: it is what makes the single reader this reassembly requires (see the field) an + // exclusion rather than a convention. Releasing it around the read would admit exactly + // the second reader that corrupts `acc`. The cost — a would-be second reader blocking + // until a packet arrives — is a state the design does not permit anyway. let mut st = self.recv_state.lock().await; - if st.scratch.len() < RECV_BUF_SIZE { - st.scratch.resize(RECV_BUF_SIZE, 0); - } loop { let RecvState { acc, scratch } = &mut *st; - let n = match dc.read(scratch.as_mut_slice()).await { + if scratch.len() < RECV_BUF_SIZE { + scratch.resize(SCRATCH_REFILL, 0); + } + let n = match dc.read(&mut scratch[..RECV_BUF_SIZE]).await { Ok(n) => n, Err(err) => { // Release the partial message, as the framing-violation paths below do: the @@ -1231,6 +1243,12 @@ impl WebRTCStream { "WebRTC reassembled message exceeded maximum frame size", ))); } + // A message that arrived whole is handed over as a slice of the read buffer: no + // allocation and no copy, which is every input event and most audio packets. + if header == FRAG_END && acc.is_empty() { + scratch.advance(1); + return Some(Ok(scratch.split_to(n - 1))); + } acc.extend_from_slice(&scratch[1..n]); if header == FRAG_END { let msg = std::mem::take(acc); @@ -1645,6 +1663,40 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc panic!("detached close never evicted the peer connection from SESSIONS"); } + // Whole messages are split out of the read buffer instead of copied, so the buffer is + // consumed from the front and periodically re-initialized. Alternating whole and fragmented + // messages exercises that against the accumulator path, and running well past one refill + // exercises the refill itself — a boundary the fixed-size buffer never had. + #[tokio::test] + async fn test_webrtc_mixed_message_sizes_survive_scratch_refill() { + let (mut offerer, mut answerer) = connect_loopback().await; + + // Sized either side of MAX_FRAGMENT_PAYLOAD, and enough rounds to consume several + // SCRATCH_REFILL chunks. + let sizes = [1usize, 64, 60_000, 60_001, 130_000, 7]; + for round in 0..40u8 { + for &len in &sizes { + let payload = Bytes::from(vec![round; len]); + offerer.send_bytes(payload.clone()).await.unwrap(); + let got = timeout(Duration::from_secs(10), answerer.next()) + .await + .expect("receiver starved") + .expect("stream ended") + .expect("read failed"); + assert_eq!(got.len(), len, "round {round}, len {len}"); + assert!( + got.iter().all(|&b| b == round), + "round {}, len {}: content or boundary corrupted", + round, + len + ); + } + } + + offerer.close().await; + answerer.close().await; + } + // Owning the Stream owns the peer connection: dropping it must close the pc and evict the // session, without the owner having to remember to. Every exit path used to carry that // obligation, and the controlled side never honoured it. From 2cb8d0c7d87e662abfe634545ad4d388b50d1dd7 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 8 Aug 2026 19:13:24 +0800 Subject: [PATCH 22/35] webrtc: reject data channels effectively; hand the permit to a desynced close - The on_data_channel guards called dc.close() from inside the handler, which webrtc-rs runs to completion BEFORE handle_open binds the SCTP stream. At that point close() only flips the ready state and returns, and handle_open sets it back to Open; with detached channels no read loop is spawned either. So a refused channel stayed open and undrained, and its queued bytes count against the association-wide receive window - about a megabyte written into an ignored channel stalls the one carrying the session, from an unauthenticated peer. Close from the channel's own on_open instead, where the stream exists. This is also what makes the ordered+reliable precondition an actual rejection rather than a log line. - A write that failed part-way through a fragment sequence called close_detached() and let the send permit drop, while the timeout path hands the permit to the teardown for exactly this reason: the close is in flight, state_notify is still Open, and callers wrap sends in allow_err! and keep going - so the next message could be written onto the orphaned prefix the failure left on the peer. send_bytes_inner now reports that it desynced and the caller, which holds the permit, does the teardown. - Drop the catch_unwind around Handle::spawn: release builds set panic = "abort", so it can never catch anything, and the comment claimed a mitigation that does not exist. The reachable case is having no runtime handle at all, which is now logged at debug rather than warn - Stream's Drop reaches this on every non-runtime thread. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 97 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 33 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 1892285c51..36d69281ce 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -256,6 +256,28 @@ impl WebRTCStream { ) } + /// Reject a data channel from inside `on_data_channel`, where closing it directly does not + /// work. + /// + /// webrtc-rs runs this handler to completion BEFORE `handle_open` binds the SCTP stream, so + /// at this point `close()` only flips the ready state and returns — no stream reset — and + /// `handle_open` then sets it back to Open. With detached channels no read loop is spawned + /// either, so a channel refused here would stay open and undrained, and its queued bytes + /// count against the association-wide receive window: about a megabyte written into an + /// ignored channel stalls the one carrying the session. Close from the channel's own + /// `on_open` instead, which runs once the stream exists. + fn close_unbound_channel(dc: Arc) { + let dc_for_close = dc.clone(); + dc.on_open(Box::new(move || { + let dc = dc_for_close.clone(); + Box::pin(async move { + if let Err(err) = dc.close().await { + log::debug!("failed to close rejected data channel: {}", err); + } + }) + })); + } + /// Whether a `SESSIONS` entry may be handed to a caller asking for `force_relay`. /// /// A hit returns a pc built for the FIRST caller, and two of its properties belong to this @@ -583,7 +605,7 @@ impl WebRTCStream { "Rejecting WebRTC data channel {}: not ordered and fully reliable", d_label ); - let _ = dc.close().await; + Self::close_unbound_channel(dc); return; } // Bind the first channel only. `detached` caches the first detached handle @@ -593,7 +615,7 @@ impl WebRTCStream { // Closed — and that watch gates both `send_bytes_inner` and `next()`. if dc_bound.swap(true, Ordering::SeqCst) { log::warn!("Ignoring extra WebRTC data channel {}", d_label); - let _ = dc.close().await; + Self::close_unbound_channel(dc); return; } log::debug!("Remote data channel {} ready", d_label); @@ -941,24 +963,20 @@ impl WebRTCStream { pub fn close_detached_with(&self, keep: T) { let pc = self.pc.clone(); // Take the runtime handle explicitly rather than calling `tokio::spawn`: this also runs - // from `Drop` impls, which can execute during runtime teardown where a bare spawn panics. - // `Handle::spawn` can panic while the runtime is shutting down too, so catch it — a brief - // leak until process exit beats aborting the process from a destructor. + // from `Drop`, which can execute on a thread with no runtime, where a bare spawn panics — + // and a panic in a destructor aborts the process. Without a handle the pc cannot be closed + // here at all; it is released at process exit. (Catching a panic from `Handle::spawn` + // during runtime shutdown is not an option: release builds set `panic = "abort"`.) let Ok(handle) = tokio::runtime::Handle::try_current() else { - log::warn!("no tokio runtime available to close the WebRTC peer connection"); + log::debug!("no tokio runtime available to close the WebRTC peer connection"); return; }; - let spawned = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - handle.spawn(async move { - let _keep = keep; - if let Err(err) = pc.close().await { - log::debug!("WebRTC background close failed: {}", err); - } - }); - })); - if spawned.is_err() { - log::warn!("failed to spawn the WebRTC close (runtime shutting down)"); - } + handle.spawn(async move { + let _keep = keep; + if let Err(err) = pc.close().await { + log::debug!("WebRTC background close failed: {}", err); + } + }); } #[inline] @@ -1088,8 +1106,17 @@ impl WebRTCStream { // connection down for missing them. The narrower the miss, the more certain the // teardown, which is precisely backwards. let write_deadline = Instant::now() + Duration::from_millis(send_timeout); - match timeout_at(write_deadline, self.send_bytes_inner(bytes)).await { - Ok(res) => res, + let mut desynced = false; + match timeout_at(write_deadline, self.send_bytes_inner(bytes, &mut desynced)).await { + // A write that failed part-way leaves the same orphaned prefix a timeout does, so + // it needs the same teardown — and the same handoff of the permit, or a waiting + // clone appends its message to that prefix while the close is still in flight. + Ok(res) => { + if desynced { + self.close_detached_with(send_permit); + } + res + } Err(_) => { // Hand the logical-message permit to the teardown so no waiting clone can // append a new message after a partially-written fragment sequence. Holding @@ -1099,17 +1126,24 @@ impl WebRTCStream { } } } else { - let _send_permit = send_gate.acquire_owned().await.map_err(|err| { + let send_permit = send_gate.acquire_owned().await.map_err(|err| { Error::new( ErrorKind::BrokenPipe, format!("WebRTC send gate closed: {}", err), ) })?; - self.send_bytes_inner(bytes).await + let mut desynced = false; + let res = self.send_bytes_inner(bytes, &mut desynced).await; + if desynced { + self.close_detached_with(send_permit); + } + res } } - async fn send_bytes_inner(&mut self, bytes: Bytes) -> ResultType<()> { + /// `desynced` is set when the failure left a partial fragment sequence on the wire, i.e. when + /// the stream can no longer be made consistent and the caller must close it. + async fn send_bytes_inner(&mut self, bytes: Bytes, desynced: &mut bool) -> ResultType<()> { // Same bound the receiver enforces, so we never emit a message a same-version peer // would have to kill the connection over. if bytes.len() > MAX_RECV_MESSAGE { @@ -1131,17 +1165,14 @@ impl WebRTCStream { framed.put_u8(if is_last { FRAG_END } else { FRAG_MORE }); framed.put_slice(chunk); if let Err(err) = dc.write(&framed.freeze()).await { - if wrote_any { - // The sequence stops with fragments already on the wire, so the peer's - // accumulator holds a prefix that will never be terminated — and this framing - // carries no length or sequence number for it to notice with, so the next - // message is appended to the orphan and parsed as one corrupt frame. The - // stream cannot be made consistent again from this side; kill it. (Callers - // wrap sends in allow_err! and keep going, so returning the error alone would - // leave the corruption in place.) - log::warn!("WebRTC send failed mid-message, closing: {}", err); - self.close_detached(); - } + // A sequence that stops with fragments already on the wire leaves the peer's + // accumulator holding a prefix that will never be terminated — and this framing + // carries no length or sequence number for it to notice with, so the next message + // is appended to the orphan and parsed as one corrupt frame. Report it so the + // caller can tear the stream down while still holding the send permit; callers + // wrap sends in allow_err! and keep going, so returning the error alone would + // leave the corruption in place. + *desynced = wrote_any; return Err(err.into()); } wrote_any = true; From 58970129492d5488e38bdd547e483f19a6b18e0f Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sun, 9 Aug 2026 22:38:15 +0800 Subject: [PATCH 23/35] webrtc: trim the comments to AGENTS.md length 435 comment lines to 328, and the module header from 40 to 17. What went is what the rules say does not belong in the source: past-bug narration, rejected alternatives, measurements, and restatements of the adjacent code. What stays is the why a reader cannot derive locally - the single-reader invariant on the reassembly state, the send-permit handoff, why the close must be detached and why that lock is held across an await, and the webrtc-rs behaviours this module depends on. Comments only; no code changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/webrtc.rs | 264 +++++++++++++------------------------------------- 1 file changed, 67 insertions(+), 197 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 36d69281ce..c580d7dbba 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1,43 +1,20 @@ //! WebRTC transport for RustDesk streams. //! -//! # webrtc crate upgrade checklist +//! # Bumping the webrtc crate //! -//! The webrtc crate version is MSRV-pinned in Cargo.toml (see the comment there). Beyond plain -//! API compatibility, this module relies on webrtc-rs *internals* that its public API does not -//! guarantee. All of them were verified against webrtc 0.13 (webrtc-data 0.11, webrtc-sctp 0.12); -//! re-verify each against the new crate sources when bumping: +//! This module depends on webrtc-rs internals its public API does not promise. Re-verify each +//! against the new sources (last checked: webrtc 0.13, -data 0.11, -sctp 0.12), then run +//! `cargo test --features webrtc webrtc::tests`. //! -//! - **Send backpressure is bounded**: `data::DataChannel::write` PARKS when webrtc-sctp's -//! PendingQueue is full (byte-counting semaphore, `QUEUE_BYTES_LIMIT` = 128 KiB; permits return -//! as chunks drain) and inflight data is cwnd/rwnd-capped (peer default rwnd 1 MiB). -//! `send_bytes` depends on this both for bounded memory on slow links and for its -//! send_timeout-then-close semantics. If a new version buffers unboundedly instead, video can -//! OOM a slow session and the send timeout never fires. -//! - **Max SCTP message size 65536**: `MAX_FRAGMENT_PAYLOAD` + 1 header byte must stay below it. -//! - **The successful read path is cancel-safe**: `read_sctp` dequeues synchronously and returns -//! with no `.await` after it, and `read_data_channel` adds none on the user-data path. So -//! `next_timeout`, which drops that future routinely, cannot lose a fragment — a version that -//! awaited after dequeuing would, undetectably, since the header carries no length or checksum. -//! Scope: the *read*. `read_data_channel` does await after dequeuing on its ErrShortBuffer and -//! DCEP branches, and `next()` itself awaits `pc.close()` on its error paths — and -//! `RTCPeerConnection::close` latches `is_closed` before its first await, so a cancelled close -//! silently makes every later close a no-op and leaves the pc in `SESSIONS`. -//! - **`detach()` is an idempotent Arc clone with no close-on-drop** (`detached_dc` caches it and -//! clones are shared across `WebRTCStream` clones). -//! - **`on_*` handlers are stored inside the pc**: a handler capturing a strong -//! `Arc` forms an uncollectable cycle and leaks the pc permanently — see the -//! `Arc::downgrade` in `new()`; any newly added handler must follow it. -//! - **`Disconnected` peer-connection state is transient/recoverable** (ICE consent lapse); -//! only `Failed`/`Closed` are treated as terminal by the state handler. -//! - **`RTCIceCandidatePair`'s `Display` format**: its candidates are private in 0.13, so -//! `is_relayed()` reads the candidate type out of -//! `"(local) {protocol} {typ} {addr}:{port}{related} <-> (remote) ..."` by position. 0.17+ -//! makes the fields `pub`; switch to them and delete the parse. (It cannot use the stats -//! report's `nominated` flag instead: webrtc-ice sets that per checklist entry and never -//! clears it, so several entries carry it after a pair switch.) -//! -//! Then re-run the loopback tests at the bottom of this file (`cargo test --features webrtc -//! webrtc::tests`). +//! - `DataChannel::write` parks on a full PendingQueue instead of buffering without bound. +//! - SCTP max message size is 65536: `MAX_FRAGMENT_PAYLOAD` + 1 must stay below it. +//! - `read_sctp` does not await after dequeuing, so a dropped `next()` cannot lose a fragment. +//! - `detach()` is an idempotent Arc clone that does not close on drop. +//! - `on_*` handlers live in the pc: one holding a strong `Arc` leaks it. +//! - `close()` latches `is_closed` before its first await, so a cancelled close is unretryable. +//! - `Disconnected` is transient; only `Failed`/`Closed` are terminal. +//! - `RTCIceCandidatePair`'s `Display` layout — `is_relayed()` parses it because 0.13 keeps the +//! candidates private. 0.17+ exposes them; switch and delete the parse. use std::collections::HashMap; use std::io::{Error, ErrorKind}; @@ -94,12 +71,9 @@ pub struct WebRTCStream { // Serialize a complete logical message across clones. Each fragment is a separate SCTP // message, so serializing only individual writes would allow two large messages to interleave. send_gate: Arc, - // Receive-side reassembly state, behind the Arc rather than in the future so a cancelled - // `next()` does not lose the partial message. SINGLE READER ONLY: reassembly spans calls, and - // the fragment header carries no length or sequence number, so two readers splice unrelated - // fragments into one `acc` and produce a well-formed, undetectably wrong message. Every - // consumer today reads from one task; keep it that way (this is why no API hands out a clone - // that outlives its `Stream`). + // Behind the Arc, not in the future, so a cancelled `next()` keeps the partial message. + // SINGLE READER ONLY: reassembly spans calls and the fragment header carries no length or + // sequence number, so two readers splice unrelated fragments into one wrong-but-valid message. recv_state: Arc>, // True once the controller has completed the RustDesk identity binding (DTLS fingerprint // matched to the signed peer id, via `set_key`). DTLS always encrypts; this flag mirrors TCP's @@ -136,20 +110,9 @@ const SCRATCH_REFILL: usize = 4 * RECV_BUF_SIZE; const FRAG_MORE: u8 = 1; /// Fragment header byte: final (or only) fragment of a logical message. const FRAG_END: u8 = 0; -/// Largest logical message this transport will send or reassemble. -/// -/// Held at parity with TCP on purpose. A transport-specific ceiling would be a trap: the same -/// session moves between WebRTC and the relay, so a message under TCP's limit but over this one -/// would work on one path and kill the connection on the other — and it would bite hardest on -/// exactly the large messages (a keyframe, a big clipboard image) that a direct path exists to -/// carry. Whatever the right maximum is, both transports need the same one. -/// -/// That leaves a real exposure, shared with TCP and not created here: the cap is the memory an -/// unauthenticated peer can make a receiver hold, and the answerer runs before any password -/// check. TCP is no better off — `BytesCodec::new` leaves `max_packet_length` at `usize::MAX`, -/// so its only bound is the 30-bit length field in the frame header. Tightening it belongs in -/// one change across both paths, with a number measured against real traffic rather than -/// guessed; until then this at least holds the peak to the cap instead of twice it. +/// Largest logical message this transport will send or reassemble, and the memory an +/// unauthenticated peer can make a receiver hold. Kept at parity with TCP on purpose: one session +/// moves between both paths, so a transport-specific ceiling would kill it on the other one. const MAX_RECV_MESSAGE: usize = crate::bytes_codec::MAX_FRAME_LENGTH; // use 3 public STUN servers to find out the NAT type, 2 must be the same address but different ports // https://stackoverflow.com/questions/72805316/determine-nat-mapping-behaviour-using-two-stun-servers @@ -257,15 +220,8 @@ impl WebRTCStream { } /// Reject a data channel from inside `on_data_channel`, where closing it directly does not - /// work. - /// - /// webrtc-rs runs this handler to completion BEFORE `handle_open` binds the SCTP stream, so - /// at this point `close()` only flips the ready state and returns — no stream reset — and - /// `handle_open` then sets it back to Open. With detached channels no read loop is spawned - /// either, so a channel refused here would stay open and undrained, and its queued bytes - /// count against the association-wide receive window: about a megabyte written into an - /// ignored channel stalls the one carrying the session. Close from the channel's own - /// `on_open` instead, which runs once the stream exists. + /// work: webrtc-rs runs that handler to completion BEFORE `handle_open` binds the SCTP stream, + /// so `close()` only flips the ready state and `handle_open` then sets it back to Open. fn close_unbound_channel(dc: Arc) { let dc_for_close = dc.clone(); dc.on_open(Box::new(move || { @@ -278,17 +234,9 @@ impl WebRTCStream { })); } - /// Whether a `SESSIONS` entry may be handed to a caller asking for `force_relay`. - /// - /// A hit returns a pc built for the FIRST caller, and two of its properties belong to this - /// caller instead: the ICE policy (`rendezvous_mediator` recomputes relay-only per PunchHole, - /// so a replayed offer wanting Relay-only must not get an All-policy pc free to pick a direct - /// pair), and liveness (the state handler latches Closed and closes the stream before it - /// reaches SESSIONS to evict, so the map briefly holds dead entries). `peer_verified` stays - /// shared on purpose: it is a fact about the DTLS certificate the entry is keyed by. - /// - /// Both the lookup and the insert-time duplicate check must use this — they read the same key, - /// so a test applied at only one of them is not applied at all. + /// Whether a cached `SESSIONS` entry may be handed to a caller asking for `force_relay`: it + /// was built for the first caller, so its ICE policy may not match, and the state handler + /// evicts late enough that dead entries linger. Both the lookup and the insert must use it. fn is_reusable_for(&self, force_relay: bool) -> bool { self.relay_only == force_relay && !matches!( @@ -323,17 +271,9 @@ impl WebRTCStream { if !matches!(u.scheme(), "turn" | "turns" | "stun" | "stuns") { return None; } - // Two spellings are accepted, and they parse very differently: - // - // - `turn://user:pass@host:port` — non-standard, but the only form that can carry - // credentials. `url` sees an authority and fills host/port/username/password. - // - `turn:host:port` — the RFC 7065 form users actually copy from TURN docs. These - // schemes are not special and there is no `//`, so `url` makes it cannot-be-a-base: - // `host_str()` is None and the whole `host:port` lands in the path. Reading it back - // out is what makes this form work at all; it previously produced a hostless - // `turn::3478` that no ICE agent can resolve — while still satisfying - // `has_turn_server()`, so a force_relay peer connection was built against it and - // could only ever time out. + // Two accepted spellings parse differently: `turn://user:pass@host:port` has an authority + // and is the only form that can carry credentials, while the RFC 7065 `turn:host:port` is + // cannot-be-a-base, so `host_str()` is None and the whole `host:port` lands in the path. let (host, port) = match u.host_str() { Some(host) => (host.to_owned(), u.port().unwrap_or(3478)), None => Self::split_host_port(u.path())?, @@ -522,14 +462,9 @@ impl WebRTCStream { let (ice_tx, ice_rx) = mpsc::unbounded_channel::(); // Create a new RTCPeerConnection let pc = Arc::new(api.new_peer_connection(config).await?); - // This closure is the only owner of the ICE-candidate sender, and `on_*` handlers live - // inside the pc: nothing clears them — not `RTCPeerConnection::close`, not the ICE - // gatherer's own close — so the sender outlives `close()` and every `SESSIONS` eviction. - // The receiver therefore never sees the channel close, and the forwarder task this API - // asks callers to write (`while let Some(c) = rx.recv().await { peer.add_remote_ice(..) }` - // with a stream clone moved in) parks on `recv()` forever, holding the pc alive with it: - // one leaked task plus one leaked peer connection per connection. The terminal-state - // handler below drops this closure to break that; see the note there. + // `on_*` handlers are never cleared — not by `close()`, not by `SESSIONS` eviction — so + // this sender outlives the pc and would park a caller's ICE forwarder on `recv()` forever, + // holding the pc with it. The terminal-state handler below drops this closure to break it. let local_ice_tx = ice_tx.clone(); pc.on_ice_candidate(Box::new(move |candidate| { let local_ice_tx = local_ice_tx.clone(); @@ -589,14 +524,9 @@ impl WebRTCStream { let stream_for_dc_clone = stream_for_dc.clone(); let dc_bound = dc_bound.clone(); Box::pin(async move { - // Reassembly spans data-channel messages, so it is sound only on an ordered, - // fully-reliable channel: the 1-byte fragment header carries no sequence - // number, so a reorder splices fragments into a well-formed but wrong - // message, and a dropped fragment merges two messages instead of erroring. - // webrtc-rs derives these parameters entirely from the REMOTE's DCEP OPEN - // (`channel_type` picks ordered / max_retransmits / max_packet_life_time), - // and this pc is built from an unauthenticated offer, so the peer would - // otherwise choose our reassembly's correctness conditions for us. + // Reassembly is sound only on an ordered, fully-reliable channel: the fragment + // header has no sequence number, so a reorder or a loss silently merges + // messages. webrtc-rs takes these from the unauthenticated REMOTE's DCEP OPEN. if !dc.ordered() || dc.max_retransmits().is_some() || dc.max_packet_lifetime().is_some() @@ -660,13 +590,9 @@ impl WebRTCStream { return; }; - // Drop the ICE-candidate handler, and with it the only sender for the - // local-candidate channel. Nothing else ever will: `close()` clears no - // handler, so the receiver would stay open forever and a caller's - // forwarder task would park on `recv()` holding a stream clone — keeping - // this pc alive past its own teardown. Replacing the handler with a - // no-op closes the channel, the forwarder's loop ends, and its clone - // goes with it. Gathering is over by this state anyway. + // Nothing else ever drops this sender (`close()` clears no handler), so + // replace the handler to close the channel: a caller's forwarder loop then + // ends instead of parking on `recv()` and holding this pc alive. pc_for_close2.on_ice_candidate(Box::new(|_| Box::pin(async {}))); let mut sessions_lock = SESSIONS.lock().await; @@ -712,14 +638,9 @@ impl WebRTCStream { }) })); - // process offer/answer - // - // Trickle ICE: this block is local-only work (pc construction, DTLS keygen, SDP marshal), - // no gathering wait. The controlled side awaits answer creation inline on its punch-reply - // critical path, so adding a network wait here would delay every hole punch. - // A failure below leaves a live pc whose state handler only fires on a terminal ICE state, - // so a bare `?`-drop leaks it — remotely triggerable via a crafted `type:"answer"` offer - // that passes the pre-check but fails `set_remote_description`. Close before propagating. + // Trickle ICE: local-only work, no gathering wait — the controlled side awaits answer + // creation inline on its punch-reply critical path. A failure below leaves a live pc whose + // state handler only fires on a terminal ICE state, so close before propagating. let offer_answer: ResultType = async { if start_local_offer { let sdp = pc.create_offer(None).await?; @@ -811,14 +732,9 @@ impl WebRTCStream { let sdp = if self.relay_only { serde_json::to_string(&local_desc)? } else { - // Declare the ICE transport policy inside the envelope. The receiver of an - // offer that arrives with force_relay set must know whether it may answer - // with full ICE (relay forced by the transport, e.g. WebSocket signaling) - // or must stay Relay-only + TURN-gated (relay by policy) — and the envelope - // is the offer's own property, so it rides here rather than in a proto - // field the rendezvous server would have to forward. An extra key is - // invisible to older peers: serde ignores unknown fields when parsing - // RTCSessionDescription, so absence — not an error — is the old semantics. + // Rides in the envelope, not a proto field the rendezvous server would forward, + // because it is the offer's own property: the receiver must know whether + // force_relay was policy (stay Relay-only) or transport. Older peers ignore it. let mut v = serde_json::to_value(&local_desc)?; v[Self::ICE_POLICY_KEY] = serde_json::Value::from(Self::ICE_POLICY_ALL); serde_json::to_string(&v)? @@ -883,17 +799,9 @@ impl WebRTCStream { let dtls = self.pc.sctp().transport(); let pair = dtls.ice_transport().get_selected_candidate_pair().await?; - // Answer from the selected pair, not from the stats report's `nominated` flag: in - // webrtc-ice that flag is sticky per checklist entry (set on nomination, never cleared — - // a pair switch only clears the agent's own `nominated_pair` slot), and the report - // enumerates the whole checklist. After an ICE switch, or on a controlled agent that saw - // USE-CANDIDATE more than once, several entries carry it and `HashMap::values()` order - // decides the answer — which can differ between two calls in one session. - // - // 0.13 keeps the pair's candidates private, so read the types out of its `Display`: - // "(local) {protocol} {typ} {addr}:{port}{related} <-> (remote) ...". Positionally, not - // by substring — an address must not be able to pass for a candidate type. See the - // upgrade checklist: 0.17+ exposes the fields and this parse goes away. + // Not the stats report's `nominated` flag: webrtc-ice never clears it per checklist entry, + // so after a pair switch several entries carry it and map order decides the answer. + // 0.13 keeps the candidates private, so read the types out of `Display` by position. let relay = RTCIceCandidateType::Relay.to_string(); Some( pair.to_string() @@ -944,22 +852,11 @@ impl WebRTCStream { self.pc.close().await.ok(); } - /// Tear the pc down on the runtime instead of awaiting it here, keeping `keep` alive until - /// the teardown finishes. - /// - /// Every caller polls `next()` — and often `send_bytes` — inside a `tokio::select!`, so an - /// `.await` on these paths is a cancellation point, and a competing arm (connection.rs and - /// io_loop.rs both run a 1s timer next to the read) routinely wins one. That is fatal to a - /// close: `RTCPeerConnection::close` latches `is_closed` before its first await and fires - /// the state handler only at the very end, so a cancelled close leaves a pc no later - /// `close()` can retry (they early-return on `is_closed`), whose `SESSIONS` entry — evicted - /// only from that handler — is stranded for the life of the process, and whose - /// `state_notify` never reaches `Closed`, leaving `wait_for_connect_result` reporting a live - /// connection on a dead pc. + /// Tear the pc down on the runtime, keeping `keep` alive until the teardown finishes. /// - /// `keep` carries anything whose lifetime must span the teardown rather than the caller's: - /// the send path passes its logical-message permit, so no waiting clone can append to a - /// partially-written fragment sequence while the close is still in flight. + /// Callers poll this path inside a `select!`, and `close()` latches `is_closed` before its + /// first await, so a cancelled close strands the pc: unretryable, never evicted from + /// `SESSIONS`, never notifying `Closed`. The send path passes its permit as `keep`. pub fn close_detached_with(&self, keep: T) { let pc = self.pc.clone(); // Take the runtime handle explicitly rather than calling `tokio::spawn`: this also runs @@ -1165,13 +1062,9 @@ impl WebRTCStream { framed.put_u8(if is_last { FRAG_END } else { FRAG_MORE }); framed.put_slice(chunk); if let Err(err) = dc.write(&framed.freeze()).await { - // A sequence that stops with fragments already on the wire leaves the peer's - // accumulator holding a prefix that will never be terminated — and this framing - // carries no length or sequence number for it to notice with, so the next message - // is appended to the orphan and parsed as one corrupt frame. Report it so the - // caller can tear the stream down while still holding the send permit; callers - // wrap sends in allow_err! and keep going, so returning the error alone would - // leave the corruption in place. + // Fragments already on the wire leave the peer an unterminated prefix, and this + // framing has no length for it to notice with, so the next message is appended + // to it. Callers wrap sends in `allow_err!`, so the error alone is not enough. *desynced = wrote_any; return Err(err.into()); } @@ -1223,24 +1116,17 @@ impl WebRTCStream { } }; if n == 0 { - // End of stream. Our own sender never produces this — every fragment carries at - // least its header byte — but it is NOT exclusively a reset: webrtc-data maps the - // StringEmpty/BinaryEmpty PPIDs to n == 0 as well, and `read` discards the flag - // that would separate them, so a peer can also reach here by sending one empty - // data-channel message. Both mean the same thing to us (this peer will send us - // nothing more we can frame), so treat them alike, but do not report it as a - // clean remote close: it is equally a peer that just violated the framing. + // Not exclusively a stream reset: webrtc-data maps the empty-message PPIDs to + // n == 0 as well and `read` drops the flag that would separate them. Both mean + // nothing frameable follows, so treat them alike but not as a clean remote close. log::debug!("WebRTC data channel ended (reset or empty message)"); *acc = BytesMut::new(); self.close_detached(); return None; } - // Two framing violations, both of which would otherwise be read as "more fragments": - // an unrecognized header, and a FRAG_MORE carrying no payload. The latter is the - // nastier one — it adds nothing to `acc`, so the MAX_FRAME_LENGTH cap below never - // trips and the loop spins for as long as the peer keeps sending. `send_bytes_inner` - // emits FRAG_MORE only for a full MAX_FRAGMENT_PAYLOAD chunk, so neither is reachable - // from our own sender. + // Both would otherwise read as "more fragments". The payload-less FRAG_MORE is the + // nastier one: it adds nothing to `acc`, so the cap below never trips and the loop + // spins for as long as the peer keeps sending. let header = scratch[0]; let bad = match header { FRAG_END => None, @@ -1258,12 +1144,9 @@ impl WebRTCStream { format!("WebRTC {why}"), ))); } - // Bound BEFORE growing, not after. This framing carries no length, so an oversize - // message can only be discovered by accumulating it — unlike the TCP codec, which - // rejects on the declared length before a single payload byte is buffered. Checking - // after the append would make the peak the cap plus a fragment, and `BytesMut` grows - // by reallocate-and-copy, so the final doubling would hold the old and new buffers at - // once: ~2x the cap in RSS for a session that has produced no message at all. + // Bound BEFORE growing: this framing carries no length, so an oversize message is only + // discovered by accumulating it, and checking after the append would let `BytesMut`'s + // reallocate-and-copy hold the old and the new buffer at once. if acc.len() + (n - 1) > MAX_RECV_MESSAGE { // Release the buffer, don't just truncate it: by definition it is near the cap // here, and `recv_state` outlives this call through the `SESSIONS` clone. @@ -1663,15 +1546,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc (offerer, answerer) } - // next()'s teardown paths must not be cancellable: every consumer polls next() inside a - // select! against a timer, and an awaited close that loses that race strands the pc - // (is_closed is already latched, so no later close retries) along with its SESSIONS entry, - // which only the state handler evicts. - // - // The cancellation itself is not what this test pins — `close_detached` is a non-async fn, - // so its callers have no await point to be cancelled at, and the compiler enforces that. - // What needs proving is the other half: that work handed to the runtime still runs to - // completion once the caller has walked away. + // next()'s teardown must not be cancellable: an awaited close that loses the select! race + // against a consumer's timer strands the pc (is_closed already latched) and its SESSIONS + // entry. `close_detached` is non-async, so what needs proving is that the handoff completes. #[tokio::test] async fn test_close_detached_completes_without_the_caller() { let (offerer, answerer) = connect_loopback().await; @@ -1802,16 +1679,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .unwrap(); } - // Extra channels opened after the bootstrap one must not displace it: rebinding would leave - // teardown closing a channel send/recv no longer use, and the newcomer's on_open would push - // Open onto the watch that gates both, re-arming a session already latched Closed. - // - // Scope, honestly: only the bind-once guard is exercised. The ordered+reliable check sits - // ahead of it but cannot decide anything here — `WebRTCStream::new` always creates its - // bootstrap channel first, so whatever the offerer adds afterwards is refused for being - // second regardless of its parameters. Covering that check needs a hand-built peer whose - // FIRST channel is unordered, which is more scaffolding than the guard is worth; it is a - // three-accessor test against parameters webrtc-rs derives verbatim from the remote's DCEP. + // Extra channels must not displace the bound one: the newcomer's on_open would push Open onto + // the watch that gates send and recv, re-arming a session already latched Closed. Only the + // bind-once guard is exercised; the ordered+reliable check cannot decide anything here. #[tokio::test] async fn test_webrtc_answerer_binds_only_the_first_data_channel() { use webrtc::data_channel::data_channel_init::RTCDataChannelInit; From 1f8463d720e9d9452be75d3b5e584a787ad34385 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Mon, 10 Aug 2026 18:55:37 +0800 Subject: [PATCH 24/35] config: add OPTION_ENABLE_KCP_CC to config::keys Options belong in this crate (AGENTS.md), and being here also lets a branded installer pre-set it via KEYS_SETTINGS, which a bare const in src/common.rs could not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ --- src/config.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config.rs b/src/config.rs index 7ac5fb54f6..98df0bbc8c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2996,6 +2996,7 @@ pub mod keys { pub const OPTION_ENABLE_UDP_PUNCH: &str = "enable-udp-punch"; pub const OPTION_ENABLE_IPV6_PUNCH: &str = "enable-ipv6-punch"; pub const OPTION_ENABLE_WEBRTC: &str = "enable-webrtc"; + pub const OPTION_ENABLE_KCP_CC: &str = "enable-kcp-congestion-control"; pub const OPTION_HIDE_USERNAME_ON_CARD: &str = "hide-username-on-card"; pub const OPTION_HIDE_HELP_CARDS: &str = "hide-help-cards"; pub const OPTION_DEFAULT_CONNECT_PASSWORD: &str = "default-connect-password"; @@ -3194,6 +3195,7 @@ pub mod keys { OPTION_ALLOW_INSECURE_TLS_FALLBACK, OPTION_KEEP_AWAKE_DURING_INCOMING_SESSIONS, OPTION_ALLOW_AUTO_UPDATE, + OPTION_ENABLE_KCP_CC, ]; // BUILDIN_SETTINGS From 01ee2f46ece493110510b8d8b5ca019488f33f7a Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 22 Aug 2026 21:38:25 +0800 Subject: [PATCH 25/35] webrtc: own every peer connection's I/O on a process-lifetime runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pc's UDP sockets register with the reactor, and its ICE/DTLS/SCTP pumps spawn on the runtime, that is current while `new` builds it. For a rustdesk controller that is io_loop's own `#[tokio::main(flavor = "current_thread")]` runtime, dropped the moment io_loop returns — so the pc outlived the only runtime able to drive its I/O, and the session-end close, merely spawned there, was never polled once. No DTLS close_notify left, and the peer waited out ICE decay (~25-30s in its log) where TCP delivers a FIN at once. Driving the close from somewhere else does not help: the sockets and pumps are already gone, so close() returns having swallowed every transport error without reaching the wire. Home the pc where it can outlive its caller instead. `new` runs its body on WEBRTC_RT, a lazy process-lifetime runtime, so every socket and background task belongs to it; `close_detached_with` spawns each close there as its own task — never cancelled, since a close cancelled after RTCPeerConnection::close latches `is_closed` is unretryable and strands the pc in SESSIONS, and never serialized, since close() has no deadline and one stalled pc would otherwise block every later session's teardown. Data-plane futures are still polled from caller runtimes; only the owning driver has to stay alive. A cancelled `new` needs the same care: rt.spawn keeps running when its JoinHandle is dropped, so the setup task would finish, cache the session, and hand its result to nobody — and an unanswered offerer never reaches a terminal ICE state, so it sat in SESSIONS forever. NewStreamHandoff closes an abandoned freshly-built stream, and hands a cache hit back disarmed: that one is shared with a live caller whose connection must survive this one's cancellation. Tests cover the production shapes: a session-end close from a thread with no runtime, an EOF-then-Drop double close, a close queued as the creating runtime is destroyed, and the cancelled-new handoff both ways. Two suite leaks surfaced on the way — streams dropped without close, polluting SESSIONS for every later test — and are fixed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016HV43uh1ztv6Wm5qi3Y1ne --- src/stream.rs | 4 +- src/webrtc.rs | 449 +++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 408 insertions(+), 45 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 676fd11f7a..ac1be30409 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -95,7 +95,9 @@ impl Stream { /// abandon, and an awaited close that loses that race is unretryable — `close()` latches /// `is_closed` before its first await, so every later attempt early-returns while the state /// handler that would evict the session never runs. With no await point here there is - /// nothing to cancel; the teardown finishes on the runtime. + /// nothing to cancel; the teardown runs to completion on the WebRTC I/O runtime + /// (see `webrtc::WEBRTC_RT`) that owns the pc's sockets and pump tasks, so it reaches the + /// wire even after a caller's own runtime dies on return. #[inline] pub fn close_webrtc(&self) { match self { diff --git a/src/webrtc.rs b/src/webrtc.rs index c580d7dbba..bd4ccced60 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -19,7 +19,7 @@ use std::collections::HashMap; use std::io::{Error, ErrorKind}; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; @@ -79,6 +79,28 @@ pub struct WebRTCStream { // matched to the signed peer id, via `set_key`). DTLS always encrypts; this flag mirrors TCP's // "secured after key exchange" so key-less / unbound WebRTC is not shown as peer-authenticated. peer_verified: Arc, + // Whether anyone still wants this pc — see `HandoffState`. Shared by every clone, the + // `SESSIONS` entry included, because that question is about the pc, not about one handle. + handoff: Arc, +} + +/// Whether a pc `new_inner` built is still wanted, shared by every `NewStreamHandoff` handed out +/// for it. `new_inner` caches the pc before its `new()` returns, so the builder's handoff and any +/// number of cache hits are in flight at once; a pc is abandoned only when the last of them is +/// dropped with none adopted, and closing on any single Drop would kill a live caller's session. +#[derive(Default)] +struct HandoffState { + // Handoffs handed out and neither adopted nor dropped yet. Incremented under the `SESSIONS` + // lock so a hit that is still arriving cannot be overtaken by the abandon-close below. + pending: AtomicUsize, + // Latched by the first `into_inner`: some caller owns the stream, and since `WebRTCStream` + // has no `Drop` there is nothing that could un-own it. Abandoning any later hit is then a + // no-op, which is what keeps a cached pc alive across a cancelled caller. + adopted: AtomicBool, + // Set under the `SESSIONS` lock once the abandon-close commits, so a hit that locks the map + // afterwards (eviction only happens later, from the state handler) rejects the entry instead + // of adopting a pc that is already going away. + closing: AtomicBool, } #[derive(Default)] @@ -126,6 +148,69 @@ static DEFAULT_ICE_SERVERS: [&str; 3] = [ lazy_static::lazy_static! { static ref SESSIONS: Arc::>> = Default::default(); + + // The process-lifetime runtime that owns ALL WebRTC I/O. A pc's UDP sockets register with + // the reactor — and its ICE/DTLS/SCTP pump tasks spawn on the runtime — that is current + // while `new_inner` and the handlers it installs execute; if that were a session's + // `#[tokio::main]` runtime (dropped the moment io_loop returns), the sockets and pumps + // would die with it and even a close driven elsewhere would "succeed" without ever putting + // close_notify on the wire — the peer then waits out ICE decay. So `new()` runs its body + // here, and detached closes run here too: as independent tasks (a deadline-free `close()` + // must not head-of-line block other sessions' teardown) that are never cancelled (a close + // cancelled after `RTCPeerConnection::close` latches `is_closed` is unretryable, stranding + // the pc in SESSIONS). Data-plane futures (send/recv/close) may be polled from any caller + // runtime — cross-runtime polling is fine; only the OWNING driver must stay alive. Two + // workers so one busy session cannot starve the rest. This is the one deliberate exception + // to AGENTS.md's no-runtime-in-libraries rule: a single lazily-built runtime for I/O whose + // lifetime exceeds any caller's. + static ref WEBRTC_RT: Option = tokio::runtime::Builder::new_multi_thread() + .worker_threads(2) + .thread_name("webrtc-io") + .enable_all() + .build() + .map_err(|err| log::error!("failed to build the WebRTC I/O runtime: {}", err)) + .ok(); +} + +/// Hands a stream from `new_inner` (running detached on `WEBRTC_RT`) back to its `new()` +/// caller. If that caller was cancelled while awaiting, the runtime drops the task's output — +/// this guard — and an unanswered offerer never reaches a terminal ICE state on its own, so +/// without the Drop-close its pc, sockets and pump tasks would sit in SESSIONS forever. Claims +/// are counted in the shared `HandoffState`, so only the last one dropped with the stream still +/// unadopted closes it; a hit another caller is already using is left alone. +struct NewStreamHandoff { + stream: Option, +} + +impl NewStreamHandoff { + /// Take a claim on `stream`. Call under the `SESSIONS` lock: a claim registered after the map + /// is unlocked can land behind the abandon-close of the claim that was last outstanding. + fn claim(stream: WebRTCStream) -> Self { + stream.handoff.pending.fetch_add(1, Ordering::AcqRel); + Self { + stream: Some(stream), + } + } + + fn into_inner(mut self) -> Option { + let stream = self.stream.take()?; + // Before releasing the claim, or a concurrent Drop can see the count reach zero without + // seeing that this caller took ownership. + stream.handoff.adopted.store(true, Ordering::Release); + stream.handoff.pending.fetch_sub(1, Ordering::AcqRel); + Some(stream) + } +} + +impl Drop for NewStreamHandoff { + fn drop(&mut self) { + if let Some(stream) = self.stream.take() { + let last = stream.handoff.pending.fetch_sub(1, Ordering::AcqRel) == 1; + if last && !stream.handoff.adopted.load(Ordering::Acquire) { + stream.close_if_abandoned(); + } + } + } } impl Clone for WebRTCStream { @@ -142,6 +227,7 @@ impl Clone for WebRTCStream { send_gate: self.send_gate.clone(), recv_state: self.recv_state.clone(), peer_verified: self.peer_verified.clone(), + handoff: self.handoff.clone(), } } } @@ -239,6 +325,7 @@ impl WebRTCStream { /// evicts late enough that dead entries linger. Both the lookup and the insert must use it. fn is_reusable_for(&self, force_relay: bool) -> bool { self.relay_only == force_relay + && !self.handoff.closing.load(Ordering::Acquire) && !matches!( *self.state_notify.borrow(), WebRTCConnectionState::Closed(_) @@ -401,11 +488,31 @@ impl WebRTCStream { ice_servers } + /// Built and driven on `WEBRTC_RT`: every socket and background task the pc creates must + /// belong to a runtime that outlives the session (see `WEBRTC_RT`). A caller that abandons + /// this future mid-await leaves the setup task to finish there; the abandoned result then + /// closes a freshly built pc (and leaves a shared cache hit alone) — see `NewStreamHandoff`. pub async fn new( remote_endpoint: &str, force_relay: bool, ms_timeout: u64, ) -> ResultType { + let Some(rt) = WEBRTC_RT.as_ref() else { + return Err(anyhow::anyhow!("WebRTC I/O runtime unavailable")); + }; + let remote_endpoint = remote_endpoint.to_owned(); + rt.spawn(Self::new_inner(remote_endpoint, force_relay, ms_timeout)) + .await + .map_err(|err| anyhow::anyhow!("WebRTC setup task failed: {}", err))?? + .into_inner() + .ok_or_else(|| anyhow::anyhow!("WebRTC setup handoff was empty")) + } + + async fn new_inner( + remote_endpoint: String, + force_relay: bool, + ms_timeout: u64, + ) -> ResultType { // The endpoint contains a Base64-encoded SDP with host addresses and live ICE // credentials. Log only its size so debug logs cannot disclose that information. log::debug!( @@ -415,28 +522,34 @@ impl WebRTCStream { let remote_offer = if remote_endpoint.is_empty() { "".into() } else { - Self::get_remote_offer(remote_endpoint)? + Self::get_remote_offer(&remote_endpoint)? }; let mut key = Self::get_key_for_sdp_json(&remote_offer)?; let start_local_offer = remote_offer.is_empty(); if !key.is_empty() { + // Claim inside the lock: `close_if_abandoned` commits under it too, so this hit + // either registers in time to call that close off, or finds the entry already rejected. let cached = { let sessions_lock = SESSIONS.lock().await; - sessions_lock - .get(&Self::cache_key(&key, start_local_offer)) - .cloned() - }; - if let Some(cached_stream) = cached { - if cached_stream.is_reusable_for(force_relay) { - log::debug!("Start webrtc with cached peer"); - return Ok(cached_stream); + match sessions_lock.get(&Self::cache_key(&key, start_local_offer)) { + Some(session) if session.is_reusable_for(force_relay) => { + Some(NewStreamHandoff::claim(session.clone())) + } + Some(session) => { + log::debug!( + "Ignoring cached webrtc peer (relay_only {}, wanted {})", + session.relay_only, + force_relay + ); + None + } + None => None, } - log::debug!( - "Ignoring cached webrtc peer (relay_only {}, wanted {})", - cached_stream.relay_only, - force_relay - ); + }; + if let Some(handoff) = cached { + log::debug!("Start webrtc with cached peer"); + return Ok(handoff); } } // Create a SettingEngine and enable Detach @@ -682,31 +795,38 @@ impl WebRTCStream { send_gate: Arc::new(Semaphore::new(1)), recv_state: Arc::new(Mutex::new(RecvState::default())), peer_verified: Arc::new(AtomicBool::new(false)), + handoff: Default::default(), }; // Insert into the session cache, but never `await pc.close()` while holding this lock: // `close()` fires the peer-connection-state handler inline, which itself locks SESSIONS, // self-deadlocking the whole process. Resolve any duplicate off-lock. let cache_key = Self::cache_key(&key, start_local_offer); - let duplicate = { + // Claim inside the lock, the insert included: with the pc cached but unclaimed, a hit + // could take the only claim on it and release it again, closing it under this caller. + let mut duplicate = None; + let handoff = { let mut final_lock = SESSIONS.lock().await; // Same admissibility test as the lookup above, or that lookup is dead code: an entry // rejected there is still in the map when we get here, so returning it unconditionally // would discard the pc we just built precisely because the cached one was unusable. match final_lock.get(&cache_key) { - Some(session) if session.is_reusable_for(force_relay) => Some(session.clone()), + Some(session) if session.is_reusable_for(force_relay) => { + duplicate = Some(webrtc_stream.clone()); + NewStreamHandoff::claim(session.clone()) + } _ => { final_lock.insert(cache_key, webrtc_stream.clone()); - None + NewStreamHandoff::claim(webrtc_stream) } } }; - if let Some(session) = duplicate { + if let Some(loser) = duplicate { // A concurrent `new()` already cached an equivalent stream; discard this pc's - // resources (off-lock) and return the cached one. - webrtc_stream.close().await; - return Ok(session); + // resources (on the closer thread — awaiting here would strand the pc if this + // `new()` were cancelled mid-close) and return the cached one. + loser.close_detached(); } - Ok(webrtc_stream) + Ok(handoff) } /// One-shot endpoint: waits for ICE gathering so the SDP already carries the candidates. @@ -852,27 +972,23 @@ impl WebRTCStream { self.pc.close().await.ok(); } - /// Tear the pc down on the runtime, keeping `keep` alive until the teardown finishes. - /// - /// Callers poll this path inside a `select!`, and `close()` latches `is_closed` before its - /// first await, so a cancelled close strands the pc: unretryable, never evicted from - /// `SESSIONS`, never notifying `Closed`. The send path passes its permit as `keep`. + /// Tear the pc down as an independent task on `WEBRTC_RT` — the runtime that owns the + /// pc's sockets and pump tasks, so the close can actually reach the wire no matter which + /// caller runtimes have died — keeping `keep` alive until the teardown finishes. Callable + /// from any context, `Drop` on a runtime-less thread included, and never cancelled, so + /// `close()`'s `is_closed` latch is only ever set by a close that finishes. The send path + /// passes its permit as `keep`. pub fn close_detached_with(&self, keep: T) { - let pc = self.pc.clone(); - // Take the runtime handle explicitly rather than calling `tokio::spawn`: this also runs - // from `Drop`, which can execute on a thread with no runtime, where a bare spawn panics — - // and a panic in a destructor aborts the process. Without a handle the pc cannot be closed - // here at all; it is released at process exit. (Catching a panic from `Handle::spawn` - // during runtime shutdown is not an option: release builds set `panic = "abort"`.) - let Ok(handle) = tokio::runtime::Handle::try_current() else { - log::debug!("no tokio runtime available to close the WebRTC peer connection"); + // If the runtime never built, no pc exists either (`new` fails first); nothing to close. + let Some(rt) = WEBRTC_RT.as_ref() else { return; }; - handle.spawn(async move { - let _keep = keep; + let pc = self.pc.clone(); + rt.spawn(async move { if let Err(err) = pc.close().await { - log::debug!("WebRTC background close failed: {}", err); + log::debug!("WebRTC close failed: {}", err); } + drop(keep); }); } @@ -881,6 +997,32 @@ impl WebRTCStream { self.close_detached_with(()); } + /// Close a pc no caller ever took. Spawned on `WEBRTC_RT` like `close_detached`, but the + /// decision is remade under the `SESSIONS` lock that a cache hit claims under: either the hit + /// registers first and this call stands down, or `closing` commits first and + /// `is_reusable_for` rejects the entry, which the state handler evicts only later. + fn close_if_abandoned(&self) { + let Some(rt) = WEBRTC_RT.as_ref() else { + return; + }; + let stream = self.clone(); + rt.spawn(async move { + { + let _sessions_lock = SESSIONS.lock().await; + if stream.handoff.pending.load(Ordering::Acquire) != 0 + || stream.handoff.adopted.load(Ordering::Acquire) + { + return; + } + stream.handoff.closing.store(true, Ordering::Release); + } + // Off-lock: `close()` fires the state handler inline and that handler locks SESSIONS. + if let Err(err) = stream.pc.close().await { + log::debug!("WebRTC close of an abandoned peer failed: {}", err); + } + }); + } + #[inline] pub fn set_raw(&mut self) { // not-supported @@ -1188,7 +1330,7 @@ pub fn is_webrtc_endpoint(endpoint: &str) -> bool { #[cfg(test)] mod tests { use crate::webrtc::WebRTCStream; - use crate::webrtc::{DEFAULT_ICE_SERVERS, FRAG_MORE, SESSIONS}; + use crate::webrtc::{NewStreamHandoff, DEFAULT_ICE_SERVERS, FRAG_MORE, SESSIONS, WEBRTC_RT}; use bytes::{BufMut, Bytes, BytesMut}; use std::{sync::Arc, time::Duration}; use tokio::sync::Barrier; @@ -1489,10 +1631,12 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc "invalid webrtc endpoint should error" ); - assert!( - WebRTCStream::new("", false, 10000).await.is_ok(), - "local webrtc endpoint should ok" - ); + let stream = WebRTCStream::new("", false, 10000) + .await + .expect("local webrtc endpoint should ok"); + // A raw WebRTCStream has no Drop: close it, or its session stays cached in SESSIONS + // and pollutes cross-test assertions about the cache. + stream.close().await; endpoint = "webrtc://eyJ0eXBlIjoiYW5zd2VyIiwic2RwIjoidj0wXHJcbm89LSA0MTA1NDk3NTY2NDgyMTQzODEwIDYwMzk1NzQw\ MCBJTiBJUDQgMC4wLjAuMFxyXG5zPS1cclxudD0wIDBcclxuYT1maW5nZXJwcmludDpzaGEtMjU2IDYxOjYwOjc0OjQwOjI4OkNFOjBCOjBDOjc1OjRCOj\ @@ -1515,6 +1659,7 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc let mut stream = WebRTCStream::new("", false, 100).await.unwrap(); let err = stream.wait_connected(10).await.unwrap_err(); assert!(err.to_string().contains("timeout")); + stream.close().await; // see test_webrtc_new_stream: no Drop on a raw WebRTCStream } async fn connect_loopback() -> (WebRTCStream, WebRTCStream) { @@ -1816,6 +1961,222 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .expect("webrtc loopback did not complete in time"); } + // A client session runs on its own `#[tokio::main(flavor = "current_thread")]` runtime that + // is dropped the moment io_loop returns, so a close spawned or awaited there can be killed + // before (or worse, after) `close()` latches `is_closed`. Pin the production sequence over + // `Stream::WebRTC`: `close_webrtc()` from a thread with no runtime at all, then Drop; the + // peer must still see EOF — the closer thread, not any caller runtime, delivers it. + #[tokio::test(flavor = "multi_thread")] + async fn test_session_end_close_reaches_the_peer() { + let (offerer, mut answerer) = connect_loopback().await; + let stream = crate::Stream::WebRTC(offerer); + tokio::task::spawn_blocking(move || { + stream.close_webrtc(); + drop(stream); + }) + .await + .expect("session thread"); + + match timeout(Duration::from_secs(5), answerer.next()).await { + Ok(None) | Ok(Some(Err(_))) => {} + Ok(Some(Ok(b))) => panic!("expected EOF after session close, got {} bytes", b.len()), + Err(_) => panic!("peer never observed the session-end close"), + } + answerer.close().await; + } + + // The shared count, not any one handoff, decides: with a second claim outstanding on a pc + // no one has adopted, dropping the first must leave it open for that caller, and dropping + // the last must close it — an unanswered offerer never reaches a terminal ICE state itself. + #[tokio::test(flavor = "multi_thread")] + async fn test_last_abandoned_claim_closes_an_unadopted_pc() { + let builder = WEBRTC_RT + .as_ref() + .expect("WebRTC I/O runtime") + .spawn(WebRTCStream::new_inner(String::new(), false, 20000)) + .await + .expect("setup task") + .expect("offerer builds"); + let stream = builder + .stream + .as_ref() + .expect("a fresh handoff carries its stream") + .clone(); + let key = format!("offer:{}", stream.session_key()); + let hit = NewStreamHandoff::claim(stream); + + drop(builder); + tokio::time::sleep(Duration::from_millis(300)).await; + assert!( + SESSIONS.lock().await.contains_key(&key), + "a claim was still outstanding, so the pc had to stay open" + ); + + drop(hit); + for _ in 0..200 { + if !SESSIONS.lock().await.contains_key(&key) { + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("the last abandoned claim never closed the unadopted pc"); + } + + // `new_inner` caches the pc BEFORE its own `new()` returns, so a concurrent caller can hit + // that cache and adopt the stream while the builder's handoff is still outstanding. Closing + // on the builder's Drop then tears down a connection another caller is already using, so + // abandonment has to be a property of the pc, not of one handoff. + #[tokio::test(flavor = "multi_thread")] + async fn test_abandoned_builder_handoff_spares_an_adopted_pc() { + // Exactly what `new()` does, minus the adoption: the setup task runs on WEBRTC_RT and + // hands back a fresh, still-unadopted handoff. + let builder = WEBRTC_RT + .as_ref() + .expect("WebRTC I/O runtime") + .spawn(WebRTCStream::new_inner(String::new(), false, 20000)) + .await + .expect("setup task") + .expect("offerer builds"); + let stream = builder + .stream + .as_ref() + .expect("a fresh handoff carries its stream") + .clone(); + let key = format!("offer:{}", stream.session_key()); + assert!( + SESSIONS.lock().await.contains_key(&key), + "new_inner must cache the pc it built" + ); + + // The concurrent `new()`: a cache hit that takes the stream and goes on using it. + let adopted = NewStreamHandoff::claim(stream) + .into_inner() + .expect("a cache hit hands the stream back"); + // ...and now the builder's `new()` is cancelled, so the runtime drops its handoff. + drop(builder); + + tokio::time::sleep(Duration::from_millis(500)).await; + assert!( + SESSIONS.lock().await.contains_key(&key), + "abandoning the builder's handoff closed a pc another caller had already adopted" + ); + adopted.close_detached(); + } + + // End-to-end: cancel `new()` itself after its first poll (the spawn is already in flight) + // and every session it transiently created must be closed and evicted again. Keys are + // compared as a set difference so concurrent tests' own sessions do not interfere; theirs + // clean up within the wait too. + #[tokio::test(flavor = "multi_thread")] + async fn test_cancelled_new_does_not_leak_the_pc() { + use std::collections::HashSet; + let before: HashSet = SESSIONS.lock().await.keys().cloned().collect(); + // Zero timeout: polls the future exactly once (spawning new_inner), then cancels it. + let _ = timeout(Duration::ZERO, WebRTCStream::new("", false, 20000)).await; + // Let the detached setup task finish (and insert its session) before demanding the + // difference be empty, or an early check passes vacuously while the leak forms later. + tokio::time::sleep(Duration::from_millis(500)).await; + // 30s: concurrent tests' transient sessions land in the difference too and must be + // given time to finish and evict (every test closes what it creates). + const ATTEMPTS: usize = 600; + for attempt in 1..=ATTEMPTS { + let now: HashSet = SESSIONS.lock().await.keys().cloned().collect(); + let leftover: Vec<&String> = now.difference(&before).collect(); + if leftover.is_empty() { + return; + } + // Positional, not an inline `{leftover:?}`: on edition 2018 a lone-literal `panic!` + // does not go through format_args and would print the placeholder verbatim. + assert!( + attempt < ATTEMPTS, + "cancelled new() left a pc cached in SESSIONS (leftover: {:?})", + leftover + ); + tokio::time::sleep(Duration::from_millis(50)).await; + } + } + + // The production shape of a controller session end: the offerer was created on a session's + // own current-thread runtime, the session queues its detached close and the runtime is + // destroyed immediately after (io_loop returning drops it). The pc's sockets and its + // ICE/DTLS/SCTP pump tasks must not die with that runtime, or the queued close completes + // without ever putting close_notify on the wire and the peer waits out ICE decay. Gathered + // (non-trickle) endpoints keep the cross-runtime signaling to two string handoffs. + #[tokio::test(flavor = "multi_thread")] + async fn test_close_survives_creator_runtime_destruction() { + // tokio oneshots for every handoff: their sends are synchronous, and the session side + // awaits them INSIDE block_on — a current_thread runtime only drives its tasks while + // being block_on-driven, and the offerer's SCTP/ICE pumps must stay live until the + // answerer has confirmed the channel is open. + let (offer_tx, offer_rx) = tokio::sync::oneshot::channel::(); + let (answer_tx, answer_rx) = tokio::sync::oneshot::channel::(); + let (go_tx, go_rx) = tokio::sync::oneshot::channel::<()>(); + let session = std::thread::spawn(move || { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("session runtime"); + let offerer = rt.block_on(async { + let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + offer_tx + .send(offerer.get_local_endpoint().await.unwrap()) + .unwrap(); + let answer = answer_rx.await.unwrap(); + offerer.set_remote_endpoint(&answer).await.unwrap(); + offerer.wait_connected(20000).await.unwrap(); + // Keep driving the runtime until the answerer confirms open, so the EOF + // assertion — not connection setup — is what discriminates. + go_rx.await.unwrap(); + offerer + }); + // The session ends: queue the close, then destroy the runtime the pc was created + // on — exactly what io_loop returning does. + drop(crate::Stream::WebRTC(offerer)); + drop(rt); + }); + + let offer = offer_rx.await.expect("offer from session thread"); + let mut answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); + answer_tx + .send(answerer.get_local_endpoint().await.unwrap()) + .expect("answer to session thread"); + answerer.wait_connected(20000).await.unwrap(); + go_tx.send(()).expect("release session thread"); + session.join().expect("session thread"); + + match timeout(Duration::from_secs(5), answerer.next()).await { + Ok(None) | Ok(Some(Err(_))) => {} + Ok(Some(Ok(b))) => panic!("expected EOF after session close, got {} bytes", b.len()), + Err(_) => panic!("peer never observed the close after the creator runtime died"), + } + answerer.close().await; + } + + // The peer-initiated end: `next()` hits EOF and fires its own `close_detached` before the + // session's final close (here via `Stream`'s Drop) is issued. Both land on the serialized + // closer thread, so the first runs to completion and the second is a benign no-op — the pc + // must end up closed and evicted, never stranded by one closer cancelling the other. + #[tokio::test(flavor = "multi_thread")] + async fn test_eof_close_then_drop_still_evicts() { + let (mut offerer, answerer) = connect_loopback().await; + let key = format!("offer:{}", offerer.session_key()); + answerer.close().await; + match timeout(Duration::from_secs(5), offerer.next()).await { + Ok(None) | Ok(Some(Err(_))) => {} // EOF path fired close_detached internally + Ok(Some(Ok(b))) => panic!("expected EOF after peer close, got {} bytes", b.len()), + Err(_) => panic!("offerer.next() hung after peer close"), + } + drop(crate::Stream::WebRTC(offerer)); // second close via Drop + + for _ in 0..200 { + if !SESSIONS.lock().await.contains_key(&key) { + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("pc still cached after EOF close + Drop close"); + } + // Both framing violations must end the stream. The empty-FRAG_MORE case is the one that // cannot be caught downstream: it adds nothing to the accumulator, so the MAX_FRAME_LENGTH // cap never trips and `next()` would otherwise spin for as long as the peer keeps writing. From db7723efa1021fcba6fb78e23799242d5dd1f804 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Mon, 24 Aug 2026 17:30:05 +0800 Subject: [PATCH 26/35] config: add OPTION_ENABLE_TCP_PUNCH The client gains a switch for the TCP punch alongside the UDP, IPv6 and WebRTC ones. Its "enable-" prefix gives it the usual default-on semantics; unlike its siblings it is deliberately left out of the self-hosted default-off list on the client side, since every hbbs has always supported TCP punching. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016HV43uh1ztv6Wm5qi3Y1ne --- src/config.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config.rs b/src/config.rs index 98df0bbc8c..02fabd7aec 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2993,6 +2993,7 @@ pub mod keys { "allow-command-line-settings-when-settings-disabled"; // Connection punch-through options + pub const OPTION_ENABLE_TCP_PUNCH: &str = "enable-tcp-punch"; pub const OPTION_ENABLE_UDP_PUNCH: &str = "enable-udp-punch"; pub const OPTION_ENABLE_IPV6_PUNCH: &str = "enable-ipv6-punch"; pub const OPTION_ENABLE_WEBRTC: &str = "enable-webrtc"; @@ -3126,6 +3127,7 @@ pub mod keys { OPTION_ALLOW_AUTO_RECORD_OUTGOING, OPTION_HIDE_RECORDING_BUTTON, OPTION_VIDEO_SAVE_DIRECTORY, + OPTION_ENABLE_TCP_PUNCH, OPTION_ENABLE_UDP_PUNCH, OPTION_ENABLE_IPV6_PUNCH, OPTION_ENABLE_WEBRTC, From 3e7968763a6085dffa1dfba1593f15b9d3cf4315 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 06:44:02 +0800 Subject: [PATCH 27/35] webrtc: keep ICE candidates out of the trickle offer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_local_endpoint_trickle` read `pc.local_description()`, and webrtc-rs runs `populate_local_candidates` there: it appends every candidate gathered so far. The name promised a candidate-free endpoint, the value grew with gathering, and callers read it a network round trip after `new` — long enough on a multi-homed host for host and srflx candidates to fill it in. The rendezvous server hands that blob to a UDP-registered peer inside a single `PunchHole` datagram, so it fragmented and was dropped without a trace on paths that discard fragments: no error, no log, the punch simply never answered. A parallel offer-less TCP-punch request had been covering for it, so what showed was "WebRTC never wins", not "WebRTC is broken" — until that request went away with the TCP punch switch and the connection failed outright. Take the endpoint once, at construction, and store it. Encode it before `set_local_description`, which is what starts gathering, so there is nothing to strip; `trickle_endpoint` strips `a=candidate:` and `a=end-of-candidates` anyway, making the bound a property of the value instead of the call order. What is left is the session parameters the peer needs to start ICE and DTLS — ufrag, pwd, fingerprint, setup, the sctp m-line — a fixed 673 bytes, where the candidates that follow are one small message each. `get_local_endpoint` keeps its old contract: it still waits for gathering and reads the live description, through the shared `encode_endpoint`. `UDP_ENDPOINT_BUDGET` only warns — which leg carries the endpoint is the server's to decide, and a TCP/WS route has no packet ceiling, so refusing there would cost WebRTC for no reason. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 210 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 183 insertions(+), 27 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index bd4ccced60..65c3fa9427 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -82,6 +82,10 @@ pub struct WebRTCStream { // Whether anyone still wants this pc — see `HandoffState`. Shared by every clone, the // `SESSIONS` entry included, because that question is about the pc, not about one handle. handoff: Arc, + // The offer/answer envelope as created, kept because `pc.local_description()` grows with + // every gathered candidate — see `get_local_endpoint_trickle`. Shared, not copied per clone: + // a session clones this struct several times and reads the envelope once. + local_endpoint: Arc, } /// Whether a pc `new_inner` built is still wanted, shared by every `NewStreamHandoff` handed out @@ -228,6 +232,7 @@ impl Clone for WebRTCStream { recv_state: self.recv_state.clone(), peer_verified: self.peer_verified.clone(), handoff: self.handoff.clone(), + local_endpoint: self.local_endpoint.clone(), } } } @@ -754,34 +759,58 @@ impl WebRTCStream { // Trickle ICE: local-only work, no gathering wait — the controlled side awaits answer // creation inline on its punch-reply critical path. A failure below leaves a live pc whose // state handler only fires on a terminal ICE state, so close before propagating. - let offer_answer: ResultType = async { + // Encode each endpoint before `set_local_description`, which is what starts gathering, so + // there is nothing for `trickle_endpoint` to strip in the first place. + let offer_answer: ResultType<(String, String)> = async { if start_local_offer { let sdp = pc.create_offer(None).await?; + let endpoint = Self::trickle_endpoint(&sdp, force_relay)?; pc.set_local_description(sdp.clone()).await?; // SDP carries host/srflx IPs and ICE ufrag/pwd; log only its size, not the body. - log::debug!("local offer SDP built ({} bytes)", sdp.sdp.len()); + log::debug!( + "local offer SDP built ({} bytes, endpoint {} bytes)", + sdp.sdp.len(), + endpoint.len() + ); let k = Self::get_key_for_sdp(&sdp)?; log::debug!("Start webrtc with local key: {}", k); - Ok(k) + Ok((k, endpoint)) } else { let sdp = serde_json::from_str::(&remote_offer)?; pc.set_remote_description(sdp.clone()).await?; let answer = pc.create_answer(None).await?; + let endpoint = Self::trickle_endpoint(&answer, force_relay)?; pc.set_local_description(answer).await?; - log::debug!("remote offer SDP received ({} bytes)", sdp.sdp.len()); + log::debug!( + "remote offer SDP received ({} bytes), local answer endpoint {} bytes", + sdp.sdp.len(), + endpoint.len() + ); let k = Self::get_key_for_sdp(&sdp)?; log::debug!("Start webrtc with remote key: {}", k); - Ok(k) + Ok((k, endpoint)) } } .await; - key = match offer_answer { - Ok(k) => k, + let (new_key, local_endpoint) = match offer_answer { + Ok(x) => x, Err(e) => { pc.close().await.ok(); return Err(e); } }; + // Only the UDP leg has a ceiling, and which leg carries this is the rendezvous server's + // to decide, so this reports rather than refuses: over a TCP/WS route a larger endpoint is + // delivered fine, and failing here would cost WebRTC for no reason. + if local_endpoint.len() > Self::UDP_ENDPOINT_BUDGET { + log::warn!( + "WebRTC endpoint is {} bytes, over the {} byte UDP budget; a peer reached over UDP \ + may never receive it", + local_endpoint.len(), + Self::UDP_ENDPOINT_BUDGET + ); + } + key = new_key; let webrtc_stream = Self { pc, @@ -796,6 +825,7 @@ impl WebRTCStream { recv_state: Arc::new(Mutex::new(RecvState::default())), peer_verified: Arc::new(AtomicBool::new(false)), handoff: Default::default(), + local_endpoint: Arc::new(local_endpoint), }; // Insert into the session cache, but never `await pc.close()` while holding this lock: // `close()` fires the peer-connection-state handler inline, which itself locks SESSIONS, @@ -841,29 +871,73 @@ impl WebRTCStream { // gathered host/srflx/relay candidates. let mut gather_complete = self.pc.gathering_complete_promise().await; let _gathering_channel_closed = gather_complete.recv().await; - self.get_local_endpoint_trickle().await - } - - /// Return the current local description immediately for callers that signal candidates via - /// `take_local_ice_rx`. Unlike `get_local_endpoint`, this does not wait for ICE gathering. + let Some(local_desc) = self.pc.local_description().await else { + return Err(anyhow::anyhow!("Local desc is not set")); + }; + Self::encode_endpoint(&local_desc, self.relay_only) + } + + /// The offer/answer exactly as it was created, for callers that signal candidates via + /// `take_local_ice_rx`. + /// + /// Deliberately not `pc.local_description()`: that one runs `populate_local_candidates`, so + /// it returns the SDP plus every candidate gathered up to that moment — hundreds of bytes on + /// a multi-homed host, and the caller reads it a network round trip after `new`, by which + /// time gathering has filled it in. The rendezvous server forwards this blob to the peer as a + /// single UDP datagram, and one that needs IP fragmentation is dropped outright on paths that + /// discard fragments, silently and every time. Candidates belong on the trickle channel. #[inline] pub async fn get_local_endpoint_trickle(&self) -> ResultType { - if let Some(local_desc) = self.pc.local_description().await { - let sdp = if self.relay_only { - serde_json::to_string(&local_desc)? - } else { - // Rides in the envelope, not a proto field the rendezvous server would forward, - // because it is the offer's own property: the receiver must know whether - // force_relay was policy (stay Relay-only) or transport. Older peers ignore it. - let mut v = serde_json::to_value(&local_desc)?; - v[Self::ICE_POLICY_KEY] = serde_json::Value::from(Self::ICE_POLICY_ALL); - serde_json::to_string(&v)? - }; - let endpoint = Self::sdp_to_endpoint(&sdp); - Ok(endpoint) - } else { - Err(anyhow::anyhow!("Local desc is not set")) + Ok(self.local_endpoint.as_ref().clone()) + } + + /// How large the offer/answer may be before the UDP leg of its route stops being safe. It is + /// the one signaling message that can grow: a peer registered over UDP receives it inside + /// `PunchHole` — with the mangled addresses and the permission blobs — as a single datagram, + /// and one that needs IP fragmentation is dropped without a trace on paths that discard + /// fragments. The candidates that follow are one small message each, so they never approach + /// this. Nothing enforces it: a TCP/WS leg has no packet ceiling, and `trickle_endpoint` + /// already pins the value near 700 bytes either way. It is what the warning and the tests + /// measure against, so a regression shows up as a log line instead of a silent drop. + const UDP_ENDPOINT_BUDGET: usize = 1024; + + /// The endpoint a trickling peer signals: the session parameters needed to start ICE and DTLS + /// (ice-ufrag, ice-pwd, fingerprint, setup, the sctp m-line), without the candidates — those + /// ride `take_local_ice_rx` and arrive as individual `IceCandidate` messages. + /// + /// The split is what keeps this endpoint a fixed ~700 bytes: candidates are the only part of a + /// local description that grows, and `pc.local_description()` appends every one gathered so + /// far. Stripping them here makes the bound a property of the value rather than of when it was + /// taken, so the size cannot drift with gathering however this is later refactored. + fn trickle_endpoint(local_desc: &RTCSessionDescription, relay_only: bool) -> ResultType { + let mut sdp = String::with_capacity(local_desc.sdp.len()); + for line in local_desc.sdp.lines() { + // `end-of-candidates` would tell the remote agent to stop waiting for the trickle. + if line.starts_with("a=candidate:") || line.starts_with("a=end-of-candidates") { + continue; + } + sdp.push_str(line); + sdp.push_str("\r\n"); } + let mut desc = local_desc.clone(); + desc.sdp = sdp; + Self::encode_endpoint(&desc, relay_only) + } + + /// Base64 envelope of a local description, carrying its ICE transport policy unless the pc is + /// Relay-only (see `ICE_POLICY_KEY`). + fn encode_endpoint(local_desc: &RTCSessionDescription, relay_only: bool) -> ResultType { + let sdp = if relay_only { + serde_json::to_string(local_desc)? + } else { + // Rides in the envelope, not a proto field the rendezvous server would forward, + // because it is the offer's own property: the receiver must know whether + // force_relay was policy (stay Relay-only) or transport. Older peers ignore it. + let mut v = serde_json::to_value(local_desc)?; + v[Self::ICE_POLICY_KEY] = serde_json::Value::from(Self::ICE_POLICY_ALL); + serde_json::to_string(&v)? + }; + Ok(Self::sdp_to_endpoint(&sdp)) } /// Whether the peer's endpoint declares it was built with ICE transport policy `all` @@ -1476,6 +1550,88 @@ mod tests { .expect("extra envelope key must not break RTCSessionDescription parsing"); } + /// The rendezvous server hands a trickle endpoint to the peer inside one UDP datagram, so it + /// must stay small however far gathering has got. `pc.local_description()` appends every + /// candidate gathered so far — the stored endpoint must not, or the datagram fragments and is + /// dropped without a trace on paths that discard fragments. + #[tokio::test] + async fn test_trickle_endpoint_never_grows_with_candidates() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let first = offerer.get_local_endpoint_trickle().await.unwrap(); + + let mut gather_complete = offerer.pc.gathering_complete_promise().await; + let _ = timeout(Duration::from_secs(10), gather_complete.recv()).await; + + let after = offerer.get_local_endpoint_trickle().await.unwrap(); + assert_eq!(first, after, "trickle endpoint changed while ICE gathered"); + assert!( + after.len() <= WebRTCStream::UDP_ENDPOINT_BUDGET, + "trickle endpoint is {} bytes, over the {} byte budget", + after.len(), + WebRTCStream::UDP_ENDPOINT_BUDGET + ); + assert!( + !WebRTCStream::get_remote_offer(&after) + .unwrap() + .contains("a=candidate"), + "trickle endpoint must carry no ICE candidates" + ); + + // The one-shot endpoint keeps its old contract, which is also what makes the check above + // meaningful: candidates really were gathered by now. + let gathered = offerer.get_local_endpoint().await.unwrap(); + assert!( + gathered.len() > after.len(), + "gathered endpoint {} is not larger than the trickle one {}", + gathered.len(), + after.len() + ); + offerer.close().await; + } + + /// The split itself: candidates out, everything ICE and DTLS need to start in. A stripped + /// endpoint the peer cannot consume would trade a silent drop for a silent failure. + #[tokio::test] + async fn test_trickle_endpoint_splits_candidates_off() { + let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); + let mut gather_complete = offerer.pc.gathering_complete_promise().await; + let _ = timeout(Duration::from_secs(10), gather_complete.recv()).await; + + let gathered = offerer.pc.local_description().await.unwrap(); + assert!( + gathered.sdp.contains("a=candidate:"), + "nothing to strip: the pc gathered no candidate" + ); + + let endpoint = WebRTCStream::trickle_endpoint(&gathered, false).unwrap(); + let json = WebRTCStream::get_remote_offer(&endpoint).unwrap(); + assert!(!json.contains("a=candidate:"), "candidate survived the split"); + assert!( + !json.contains("a=end-of-candidates"), + "end-of-candidates would stop the remote agent waiting for the trickle" + ); + for keep in [ + "m=application", + "a=ice-ufrag:", + "a=ice-pwd:", + "a=fingerprint:", + "a=setup:", + "a=mid:", + "a=sctp-port:", + ] { + assert!(json.contains(keep), "trickle endpoint dropped {keep}"); + } + assert!( + endpoint.len() <= WebRTCStream::UDP_ENDPOINT_BUDGET, + "stripped endpoint is {} bytes", + endpoint.len() + ); + serde_json::from_str::(&json) + .expect("a stripped endpoint must still parse as a session description"); + + offerer.close().await; + } + #[test] fn test_webrtc_session_key() { let mut sdp_str = "".to_owned(); From 748eefdf2ef10eb8d92ac9b30b7373185e94eead Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 08:38:37 +0800 Subject: [PATCH 28/35] webrtc: address review of the trickle-offer change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `local_endpoint` replaces `get_local_endpoint_trickle`. The value is taken at construction, so the getter could not fail and did not await — but it kept an `async fn -> ResultType` shape, leaving both call sites maintaining error arms that can never run, one of them commented as preventing a leak it can no longer prevent. Returning `&str` drops the future, the Result, a copy of the envelope per read, and both arms. `encode_endpoint` now takes the two fields a session description actually serializes rather than the value. `trickle_endpoint` was cloning a whole description — parsed SDP tree included — to overwrite `.sdp`, which left `parsed` holding every candidate it had just stripped: harmless only because `parsed` is `#[serde(skip)]` and the value was serialized immediately. Building the envelope from `(sdp_type, sdp)` removes both the clone and the trap. It is assembled through an explicit `serde_json::Map` because `json!` expands its values to `to_value(..).unwrap()`. Tests: the `{keep}` in an `assert!` message was a literal, not a format argument, under this crate's 2018 edition — it warned and would have named no field; deserializing the stripped endpoint proved nothing (`parsed` is skipped, so both fields are opaque strings) and now goes through `get_key_for_sdp`, which unmarshals and requires the fingerprint to have survived; the growth test compared the endpoint against itself, which an immutable field makes unfalsifiable, and now compares it against the live description that does grow; waiting out ICE gathering is replaced by polling for the first candidate, and both tests close before they assert so a failure here cannot strand a pc in SESSIONS and be reported twice. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 185 ++++++++++++++++++++++++++++---------------------- 1 file changed, 104 insertions(+), 81 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 65c3fa9427..7097c56088 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -34,6 +34,7 @@ use webrtc::ice_transport::ice_server::RTCIceServer; use webrtc::peer_connection::configuration::RTCConfiguration; use webrtc::peer_connection::peer_connection_state::RTCPeerConnectionState; use webrtc::peer_connection::policy::ice_transport_policy::RTCIceTransportPolicy; +use webrtc::peer_connection::sdp::sdp_type::RTCSdpType; use webrtc::peer_connection::sdp::session_description::RTCSessionDescription; use webrtc::peer_connection::RTCPeerConnection; @@ -82,9 +83,8 @@ pub struct WebRTCStream { // Whether anyone still wants this pc — see `HandoffState`. Shared by every clone, the // `SESSIONS` entry included, because that question is about the pc, not about one handle. handoff: Arc, - // The offer/answer envelope as created, kept because `pc.local_description()` grows with - // every gathered candidate — see `get_local_endpoint_trickle`. Shared, not copied per clone: - // a session clones this struct several times and reads the envelope once. + // Taken once at construction: `pc.local_description()` grows with every gathered candidate + // and this must not (why: `UDP_ENDPOINT_BUDGET`). Shared so cloning the struct stays cheap. local_endpoint: Arc, } @@ -268,7 +268,7 @@ impl WebRTCStream { } // Envelope JSON key carrying the local description's ICE transport policy, alongside the - // RTCSessionDescription fields (see `get_local_endpoint_trickle`). + // RTCSessionDescription fields (see `local_endpoint`). // `'static` spelled out: an elided lifetime here is a warn-by-default future hard error on // the 1.75 toolchain CI pins (elided_lifetimes_in_associated_constant). const ICE_POLICY_KEY: &'static str = "ice_policy"; @@ -874,41 +874,42 @@ impl WebRTCStream { let Some(local_desc) = self.pc.local_description().await else { return Err(anyhow::anyhow!("Local desc is not set")); }; - Self::encode_endpoint(&local_desc, self.relay_only) + Self::encode_endpoint(local_desc.sdp_type, &local_desc.sdp, self.relay_only) } /// The offer/answer exactly as it was created, for callers that signal candidates via - /// `take_local_ice_rx`. + /// `take_local_ice_rx`. Taken at construction, so this cannot fail and cannot block — unlike + /// `get_local_endpoint`, which reads the live description and waits for gathering. /// - /// Deliberately not `pc.local_description()`: that one runs `populate_local_candidates`, so - /// it returns the SDP plus every candidate gathered up to that moment — hundreds of bytes on - /// a multi-homed host, and the caller reads it a network round trip after `new`, by which - /// time gathering has filled it in. The rendezvous server forwards this blob to the peer as a - /// single UDP datagram, and one that needs IP fragmentation is dropped outright on paths that - /// discard fragments, silently and every time. Candidates belong on the trickle channel. + /// Deliberately not `pc.local_description()`, which runs `populate_local_candidates` and so + /// returns the SDP plus every candidate gathered up to that moment — and callers read this a + /// network round trip after `new`, by which time gathering has filled it in. Why the size + /// matters: `UDP_ENDPOINT_BUDGET`. #[inline] - pub async fn get_local_endpoint_trickle(&self) -> ResultType { - Ok(self.local_endpoint.as_ref().clone()) + pub fn local_endpoint(&self) -> &str { + self.local_endpoint.as_str() } - /// How large the offer/answer may be before the UDP leg of its route stops being safe. It is + /// The size a trickle endpoint must stay under for the UDP leg of its route to be safe. It is /// the one signaling message that can grow: a peer registered over UDP receives it inside /// `PunchHole` — with the mangled addresses and the permission blobs — as a single datagram, /// and one that needs IP fragmentation is dropped without a trace on paths that discard - /// fragments. The candidates that follow are one small message each, so they never approach - /// this. Nothing enforces it: a TCP/WS leg has no packet ceiling, and `trickle_endpoint` - /// already pins the value near 700 bytes either way. It is what the warning and the tests - /// measure against, so a regression shows up as a log line instead of a silent drop. + /// fragments. The candidates that follow are one small message each and never approach it. + /// + /// Not an MTU calculation: `PunchHole`'s other fields are not bounded here, so a value near + /// this number could still fragment. It is a tripwire on a quantity `trickle_endpoint` pins at + /// ~700 bytes, sized to leave that headroom while still catching a regression that lets + /// candidates back in — as a log line rather than another silent drop. Nothing enforces it: a + /// TCP/WS leg has no packet ceiling. const UDP_ENDPOINT_BUDGET: usize = 1024; /// The endpoint a trickling peer signals: the session parameters needed to start ICE and DTLS /// (ice-ufrag, ice-pwd, fingerprint, setup, the sctp m-line), without the candidates — those /// ride `take_local_ice_rx` and arrive as individual `IceCandidate` messages. /// - /// The split is what keeps this endpoint a fixed ~700 bytes: candidates are the only part of a - /// local description that grows, and `pc.local_description()` appends every one gathered so - /// far. Stripping them here makes the bound a property of the value rather than of when it was - /// taken, so the size cannot drift with gathering however this is later refactored. + /// Candidates are the only part of a local description that grows. Stripping them makes the + /// size a property of the value rather than of when it was taken, so it cannot drift with + /// gathering however this is later refactored. fn trickle_endpoint(local_desc: &RTCSessionDescription, relay_only: bool) -> ResultType { let mut sdp = String::with_capacity(local_desc.sdp.len()); for line in local_desc.sdp.lines() { @@ -919,25 +920,27 @@ impl WebRTCStream { sdp.push_str(line); sdp.push_str("\r\n"); } - let mut desc = local_desc.clone(); - desc.sdp = sdp; - Self::encode_endpoint(&desc, relay_only) + Self::encode_endpoint(local_desc.sdp_type, &sdp, relay_only) } /// Base64 envelope of a local description, carrying its ICE transport policy unless the pc is /// Relay-only (see `ICE_POLICY_KEY`). - fn encode_endpoint(local_desc: &RTCSessionDescription, relay_only: bool) -> ResultType { - let sdp = if relay_only { - serde_json::to_string(local_desc)? - } else { + /// Built from the two fields a session description serializes rather than from the value, so + /// a caller that rewrote the SDP cannot leave a stale `parsed` tree riding along with it. + fn encode_endpoint(sdp_type: RTCSdpType, sdp: &str, relay_only: bool) -> ResultType { + let mut envelope = serde_json::Map::new(); + envelope.insert("type".to_owned(), serde_json::to_value(sdp_type)?); + envelope.insert("sdp".to_owned(), serde_json::Value::from(sdp)); + if !relay_only { // Rides in the envelope, not a proto field the rendezvous server would forward, // because it is the offer's own property: the receiver must know whether // force_relay was policy (stay Relay-only) or transport. Older peers ignore it. - let mut v = serde_json::to_value(local_desc)?; - v[Self::ICE_POLICY_KEY] = serde_json::Value::from(Self::ICE_POLICY_ALL); - serde_json::to_string(&v)? - }; - Ok(Self::sdp_to_endpoint(&sdp)) + envelope.insert( + Self::ICE_POLICY_KEY.to_owned(), + serde_json::Value::from(Self::ICE_POLICY_ALL), + ); + } + Ok(Self::sdp_to_endpoint(&serde_json::to_string(&envelope)?)) } /// Whether the peer's endpoint declares it was built with ICE transport policy `all` @@ -1550,43 +1553,54 @@ mod tests { .expect("extra envelope key must not break RTCSessionDescription parsing"); } - /// The rendezvous server hands a trickle endpoint to the peer inside one UDP datagram, so it - /// must stay small however far gathering has got. `pc.local_description()` appends every - /// candidate gathered so far — the stored endpoint must not, or the datagram fragments and is - /// dropped without a trace on paths that discard fragments. + /// The live description once it has gathered something. Polls instead of awaiting + /// `gathering_complete_promise`: a stream sits in the global `SESSIONS` for the whole wait, and + /// waiting out gathering there is long enough to trip `test_cancelled_new_does_not_leak_the_pc` + /// when the suite runs its tests in parallel. One candidate is all these tests need. + /// Returns `None` on timeout rather than panicking: the caller must get to `close()` before + /// it asserts, or a stranded pc fails `test_cancelled_new_does_not_leak_the_pc` as well and + /// one root failure is reported as two. + async fn first_gathered_description(stream: &WebRTCStream) -> Option { + timeout(Duration::from_secs(10), async { + loop { + if let Some(desc) = stream.pc.local_description().await { + if desc.sdp.contains("a=candidate:") { + return desc; + } + } + tokio::time::sleep(Duration::from_millis(20)).await; + } + }) + .await + .ok() + } + + /// The signalled endpoint and the live description must diverge. `pc.local_description()` + /// appends every candidate gathered so far; the endpoint the peer is handed must not grow with + /// them, because the rendezvous server delivers it to a UDP-registered peer as one datagram + /// and a fragmented one is dropped without a trace on paths that discard fragments. #[tokio::test] - async fn test_trickle_endpoint_never_grows_with_candidates() { + async fn test_stored_endpoint_does_not_grow_with_the_live_description() { let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); - let first = offerer.get_local_endpoint_trickle().await.unwrap(); - - let mut gather_complete = offerer.pc.gathering_complete_promise().await; - let _ = timeout(Duration::from_secs(10), gather_complete.recv()).await; + let live = first_gathered_description(&offerer).await; + let endpoint = offerer.local_endpoint().to_owned(); + offerer.close().await; - let after = offerer.get_local_endpoint_trickle().await.unwrap(); - assert_eq!(first, after, "trickle endpoint changed while ICE gathered"); - assert!( - after.len() <= WebRTCStream::UDP_ENDPOINT_BUDGET, - "trickle endpoint is {} bytes, over the {} byte budget", - after.len(), - WebRTCStream::UDP_ENDPOINT_BUDGET - ); + // Some(_) is the assertion that the live description grew: the helper only returns once it + // carries a candidate, so the endpoint below really had something to grow by. + live.expect("no ICE candidate gathered within 10s"); assert!( - !WebRTCStream::get_remote_offer(&after) + !WebRTCStream::get_remote_offer(&endpoint) .unwrap() .contains("a=candidate"), - "trickle endpoint must carry no ICE candidates" + "the signalled endpoint grew candidates alongside the live description" ); - - // The one-shot endpoint keeps its old contract, which is also what makes the check above - // meaningful: candidates really were gathered by now. - let gathered = offerer.get_local_endpoint().await.unwrap(); assert!( - gathered.len() > after.len(), - "gathered endpoint {} is not larger than the trickle one {}", - gathered.len(), - after.len() + endpoint.len() <= WebRTCStream::UDP_ENDPOINT_BUDGET, + "signalled endpoint is {} bytes, over the {} byte budget", + endpoint.len(), + WebRTCStream::UDP_ENDPOINT_BUDGET ); - offerer.close().await; } /// The split itself: candidates out, everything ICE and DTLS need to start in. A stripped @@ -1594,14 +1608,9 @@ mod tests { #[tokio::test] async fn test_trickle_endpoint_splits_candidates_off() { let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); - let mut gather_complete = offerer.pc.gathering_complete_promise().await; - let _ = timeout(Duration::from_secs(10), gather_complete.recv()).await; - - let gathered = offerer.pc.local_description().await.unwrap(); - assert!( - gathered.sdp.contains("a=candidate:"), - "nothing to strip: the pc gathered no candidate" - ); + let gathered = first_gathered_description(&offerer).await; + offerer.close().await; + let gathered = gathered.expect("no ICE candidate gathered within 10s"); let endpoint = WebRTCStream::trickle_endpoint(&gathered, false).unwrap(); let json = WebRTCStream::get_remote_offer(&endpoint).unwrap(); @@ -1619,17 +1628,31 @@ mod tests { "a=mid:", "a=sctp-port:", ] { - assert!(json.contains(keep), "trickle endpoint dropped {keep}"); + assert!(json.contains(keep), "trickle endpoint dropped {}", keep); } assert!( endpoint.len() <= WebRTCStream::UDP_ENDPOINT_BUDGET, "stripped endpoint is {} bytes", endpoint.len() ); - serde_json::from_str::(&json) - .expect("a stripped endpoint must still parse as a session description"); - - offerer.close().await; + // `parsed` is #[serde(skip)], so deserializing alone would accept any string as the SDP + // body. Unmarshal it, which is also what the peer does, and require the fingerprint the + // session key is derived from to have survived. + let desc: RTCSessionDescription = + serde_json::from_str(&json).expect("a stripped endpoint must deserialize"); + WebRTCStream::get_key_for_sdp(&desc) + .expect("a stripped endpoint must still parse as SDP and keep its fingerprint"); + + // `encode_endpoint`, which `get_local_endpoint` also goes through, holds the opposite + // contract: carry whatever it is handed, candidates included. (What `get_local_endpoint` + // adds on top — the gathering wait — is covered by test_webrtc_loopback_gathered_endpoints.) + let kept = WebRTCStream::encode_endpoint(gathered.sdp_type, &gathered.sdp, false).unwrap(); + assert!( + WebRTCStream::get_remote_offer(&kept) + .unwrap() + .contains("a=candidate:"), + "encode_endpoint dropped the candidates it was handed" + ); } #[test] @@ -1820,9 +1843,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc async fn connect_loopback() -> (WebRTCStream, WebRTCStream) { let mut offerer = WebRTCStream::new("", false, 20000).await.unwrap(); - let offer = offerer.get_local_endpoint_trickle().await.unwrap(); + let offer = offerer.local_endpoint().to_owned(); let answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); - let answer = answerer.get_local_endpoint_trickle().await.unwrap(); + let answer = answerer.local_endpoint().to_owned(); offerer.set_remote_endpoint(&answer).await.unwrap(); // Bridge trickle candidates directly between the two peers, both directions. @@ -1938,7 +1961,7 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc #[tokio::test] async fn test_cached_peer_is_not_reused_across_ice_policies() { let offerer = WebRTCStream::new("", false, 20000).await.unwrap(); - let offer = offerer.get_local_endpoint_trickle().await.unwrap(); + let offer = offerer.local_endpoint().to_owned(); let all_ice = WebRTCStream::new(&offer, false, 20000).await.unwrap(); assert!(!all_ice.relay_only); @@ -2006,9 +2029,9 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc .await .unwrap(); - let offer = offerer.get_local_endpoint_trickle().await.unwrap(); + let offer = offerer.local_endpoint().to_owned(); let answerer = WebRTCStream::new(&offer, false, 20000).await.unwrap(); - let answer = answerer.get_local_endpoint_trickle().await.unwrap(); + let answer = answerer.local_endpoint().to_owned(); offerer.set_remote_endpoint(&answer).await.unwrap(); let mut off_ice = offerer.take_local_ice_rx().unwrap(); From a96ec7f77e09fe93ef5286862fae52fbaf4bbe35 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 08:49:20 +0800 Subject: [PATCH 29/35] webrtc/tests: look for a session that outlasts the window, not an idle instant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_cancelled_new_does_not_leak_the_pc` demanded an instant at which no key had appeared since its snapshot. That asks the whole suite to go quiet, which `--test-threads=2` never grants: one lane is this test for its entire 30s wait while the other keeps starting sessions, so the difference is never empty and the test fails whatever the pc it is actually watching did. Intersect the difference across samples instead. A concurrent test's session appears and is closed again, so it drops out; a leaked pc never does. The cancelled attempt's own key cannot be named here — its fingerprint is generated inside the task that was abandoned — so outlasting the window is the property available to test, and it is the one that means "leaked". This sharpens what the failure says; it does not make `--test-threads=2` pass. With the new assertion a single `offer:` key still survives all 30s there, and the test passes in 0.5s when run alone, so the entry belongs to another test rather than to the cancelled `new()`. The two that close only their answerer and leave the offerer to an indirect path — `test_session_end_close_reaches_the_peer` (Stream::close_webrtc) and `test_eof_close_then_drop_still_evicts` (Drop) — are where to look. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 7097c56088..6da108b678 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -2243,33 +2243,44 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc } // End-to-end: cancel `new()` itself after its first poll (the spawn is already in flight) - // and every session it transiently created must be closed and evicted again. Keys are - // compared as a set difference so concurrent tests' own sessions do not interfere; theirs - // clean up within the wait too. + // and every session it transiently created must be closed and evicted again. + // + // The cancelled attempt's own key is unknowable here — its DTLS fingerprint is generated + // inside the task that was abandoned — so what is watched for is a key that OUTLASTS the + // window, not an instant with no new keys at all. Concurrent tests each close what they + // create, so their keys come and go and drop out of the running intersection; a leaked pc + // never does. Waiting for an empty difference instead made this test depend on the suite + // having an idle moment, which `--test-threads=2` never gives it: one lane is this test for + // the whole wait while the other keeps starting sessions. #[tokio::test(flavor = "multi_thread")] async fn test_cancelled_new_does_not_leak_the_pc() { use std::collections::HashSet; let before: HashSet = SESSIONS.lock().await.keys().cloned().collect(); // Zero timeout: polls the future exactly once (spawning new_inner), then cancels it. let _ = timeout(Duration::ZERO, WebRTCStream::new("", false, 20000)).await; - // Let the detached setup task finish (and insert its session) before demanding the - // difference be empty, or an early check passes vacuously while the leak forms later. + // Let the detached setup task finish (and insert its session) before sampling, or the + // first sample is taken before the leak has formed and every later one intersects to + // nothing. tokio::time::sleep(Duration::from_millis(500)).await; - // 30s: concurrent tests' transient sessions land in the difference too and must be - // given time to finish and evict (every test closes what it creates). - const ATTEMPTS: usize = 600; + const ATTEMPTS: usize = 600; // 30s + let mut persisted: Option> = None; for attempt in 1..=ATTEMPTS { let now: HashSet = SESSIONS.lock().await.keys().cloned().collect(); - let leftover: Vec<&String> = now.difference(&before).collect(); - if leftover.is_empty() { + let new_keys: HashSet = now.difference(&before).cloned().collect(); + persisted = Some(match persisted { + None => new_keys, + Some(prev) => prev.intersection(&new_keys).cloned().collect(), + }); + let persisted_keys = persisted.as_ref().map_or(0, HashSet::len); + if persisted_keys == 0 { return; } - // Positional, not an inline `{leftover:?}`: on edition 2018 a lone-literal `panic!` + // Positional, not an inline `{persisted:?}`: on edition 2018 a lone-literal `panic!` // does not go through format_args and would print the placeholder verbatim. assert!( attempt < ATTEMPTS, - "cancelled new() left a pc cached in SESSIONS (leftover: {:?})", - leftover + "cancelled new() left a pc cached in SESSIONS (persisted: {:?})", + persisted ); tokio::time::sleep(Duration::from_millis(50)).await; } From cc8537c1042529e79cde185795843e6b8d342900 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 09:09:53 +0800 Subject: [PATCH 30/35] webrtc/tests: close the stream a lost cancellation hands back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_cancelled_new_does_not_leak_the_pc` cancelled `new()` with a zero timeout and discarded whatever came back. The cancellation is not guaranteed to win: the setup task runs on WEBRTC_RT, and it can finish inside the single poll the timeout allows, in which case `new()` returns a live stream. `WebRTCStream` has no `Drop`, so `let _ =` on that one strands its pc in SESSIONS — and the test then reported the leak it had just created, blaming the cancelled attempt. Fewer test threads leave more CPU for that task, so it won the race often enough that `--test-threads=2` failed every run while the default count passed; the entry that survived carried `conn=New`, `sig=HaveLocalOffer` and a `Pending` state watch, i.e. a pc nobody had ever closed. Close what a lost race hands back and retry for a real cancellation, asserting that one happened rather than testing nothing. 24 tests now pass at 1, 2, 4, 8 and default threads. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index 6da108b678..5a2bdb0e0e 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -2256,8 +2256,25 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc async fn test_cancelled_new_does_not_leak_the_pc() { use std::collections::HashSet; let before: HashSet = SESSIONS.lock().await.keys().cloned().collect(); - // Zero timeout: polls the future exactly once (spawning new_inner), then cancels it. - let _ = timeout(Duration::ZERO, WebRTCStream::new("", false, 20000)).await; + // Zero timeout: polls the future exactly once (spawning new_inner), then cancels it — + // usually. The setup task runs on its own runtime and can finish inside that single poll, + // and then `new()` hands back a live stream instead. `WebRTCStream` has no `Drop`, so + // discarding that one is itself a leak, and this test would go on to report it as the + // cancelled attempt's. Close what comes back and try for a real cancellation. Fewer test + // threads make the setup task likelier to win, which is why this surfaced under + // `--test-threads=2` and not at the default. + let mut cancelled = false; + for _ in 0..20 { + match timeout(Duration::ZERO, WebRTCStream::new("", false, 20000)).await { + Err(_) => { + cancelled = true; + break; + } + Ok(Ok(stream)) => stream.close().await, + Ok(Err(_)) => {} + } + } + assert!(cancelled, "new() never lost the race with its own cancellation"); // Let the detached setup task finish (and insert its session) before sampling, or the // first sample is taken before the leak has formed and every later one intersects to // nothing. From 6b8182ed38b4c9d800ae4e168137000c3545536f Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 09:15:52 +0800 Subject: [PATCH 31/35] webrtc: record why WebRTCStream has no Drop An absent impl is invisible in the source, and this one keeps being proposed. Closing on each clone's drop would end a live session the moment a losing race future is dropped; closing on the last clone is circular, since the cache entry is itself a clone and is removed by the state handler a close fires. Ownership is carried by `OffererGuard` and `Stream::WebRTC` instead, so say so where someone adding another holder will look. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/webrtc.rs b/src/webrtc.rs index 5a2bdb0e0e..83ea5799f1 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -57,6 +57,11 @@ enum WebRTCConnectionState { Closed(String), } +/// A shared handle, not an owner: every clone points at the same `pc`, and `SESSIONS` holds one +/// of those clones. Hence no `Drop` — closing per clone would kill a live session as soon as a +/// losing race future is dropped, and closing on the last clone would wait on the very close +/// that evicts the cache entry. Ownership is `OffererGuard` until a connection attempt adopts +/// the stream and `Stream::WebRTC` after; holding one outside those two means closing it by hand. pub struct WebRTCStream { pc: Arc, stream: Arc>>, From 7ea29baacf0b013ee1f76bb8c7c9b5ff35958c3c Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 13:59:52 +0800 Subject: [PATCH 32/35] webrtc: stop gathering link-local IPv6 host candidates fe80::/10 can only be bound together with a scope id, which the gathered address list drops before it reaches the bind, so every link-local address yields nothing but a failed bind and a warning line - seven of them per session on a macOS host. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/webrtc.rs b/src/webrtc.rs index 83ea5799f1..0de1f3501b 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -566,6 +566,13 @@ impl WebRTCStream { let mut s = SettingEngine::default(); s.detach_data_channels(); s.set_ice_multicast_dns_mode(MulticastDnsMode::Disabled); + // fe80::/10 can only be bound together with a scope id, which `IpAddr` cannot carry, so + // gathering one never yields a candidate - only a failed bind and a warning per address. + // Spelled out because `is_unicast_link_local` is not stable on our MSRV. + s.set_ip_filter(Box::new(|ip: IpAddr| match ip { + IpAddr::V6(v6) => v6.segments()[0] & 0xffc0 != 0xfe80, + IpAddr::V4(_) => true, + })); // Create the API object let api = APIBuilder::new().with_setting_engine(s).build(); From e2aa3832b2620d1923cf845709b5e5bf1651ccba Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 14:45:44 +0800 Subject: [PATCH 33/35] webrtc: report the family of the nominated ICE pair The label a session is reported under names the transport that won the race, not the family ICE ended up nominating, so a WebRTC session over IPv6 read the same as one over IPv4. Take it from the remote side of the selected pair - the address the peer is actually reached at, which the rendezvous-observed address the session is otherwise identified by cannot report. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/stream.rs | 13 +++++++++++++ src/webrtc.rs | 14 ++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/stream.rs b/src/stream.rs index ac1be30409..2df8269154 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -122,6 +122,19 @@ impl Stream { } } + /// Whether an established WebRTC transport reaches the peer over IPv6 (used to name the + /// transport in the UI). `None` for non-WebRTC transports and before ICE selects a pair — + /// every other transport already carries the family in the label it was raced under. + #[inline] + pub async fn webrtc_remote_ipv6(&self) -> Option { + match self { + #[cfg(feature = "webrtc")] + Stream::WebRTC(s) => s.is_remote_ipv6().await, + #[allow(unreachable_patterns)] + _ => None, + } + } + /// DTLS certificate fingerprint for a WebRTC stream (`local`=true for this endpoint's own /// cert, false for the peer's), used to bind the channel to the signed peer identity. /// Returns None for non-WebRTC transports, which authenticate via the secretbox key exchange. diff --git a/src/webrtc.rs b/src/webrtc.rs index 0de1f3501b..a7bda1c657 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -1020,6 +1020,20 @@ impl WebRTCStream { ) } + /// Whether the nominated pair reaches the peer over IPv6 — `None` before one is selected. + /// The remote side on purpose: it is the address the peer is actually reached at, which the + /// rendezvous-observed address the session is otherwise identified by cannot report. + pub async fn is_remote_ipv6(&self) -> Option { + let dtls = self.pc.sctp().transport(); + let pair = dtls.ice_transport().get_selected_candidate_pair().await?; + // Same `Display` parse as `is_relayed`, one token over: a side renders as + // `(remote)
:`, and `protocol` is only udp/tcp, so the + // family has to come from the address — an IPv6 literal is the only one with two colons. + let pair = pair.to_string(); + let remote = pair.split(" <-> ").nth(1)?; + Some(remote.split_whitespace().nth(3)?.matches(':').count() > 1) + } + #[inline] pub fn take_local_ice_rx(&self) -> Option> { self.local_ice_rx.lock().ok().and_then(|mut rx| rx.take()) From 2f7536527d1d2d41480ee7ca4d568005fd003eba Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 17:00:32 +0800 Subject: [PATCH 34/35] config: name the KCP congestion-control option for what it does `option2bool` reads an `enable-` option as on unless it is literally "N", while this one's accessor required a literal "Y" - the name promised default-on and the code shipped default-off. `allow-` is the prefix whose rule matches the behaviour that already ships, so the rename settles the contradiction without moving a single user's transport. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 02fabd7aec..793ad47779 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2997,7 +2997,7 @@ pub mod keys { pub const OPTION_ENABLE_UDP_PUNCH: &str = "enable-udp-punch"; pub const OPTION_ENABLE_IPV6_PUNCH: &str = "enable-ipv6-punch"; pub const OPTION_ENABLE_WEBRTC: &str = "enable-webrtc"; - pub const OPTION_ENABLE_KCP_CC: &str = "enable-kcp-congestion-control"; + pub const OPTION_ALLOW_KCP_CC: &str = "allow-kcp-congestion-control"; pub const OPTION_HIDE_USERNAME_ON_CARD: &str = "hide-username-on-card"; pub const OPTION_HIDE_HELP_CARDS: &str = "hide-help-cards"; pub const OPTION_DEFAULT_CONNECT_PASSWORD: &str = "default-connect-password"; @@ -3197,7 +3197,7 @@ pub mod keys { OPTION_ALLOW_INSECURE_TLS_FALLBACK, OPTION_KEEP_AWAKE_DURING_INCOMING_SESSIONS, OPTION_ALLOW_AUTO_UPDATE, - OPTION_ENABLE_KCP_CC, + OPTION_ALLOW_KCP_CC, ]; // BUILDIN_SETTINGS From 96933d6230090df9d73bf98fe736f6359f97a4d9 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Tue, 25 Aug 2026 17:00:45 +0800 Subject: [PATCH 35/35] webrtc: choose ICE servers by network, and expose the STUN half Two of the three entries sat on one host, so they failed together - in the same millisecond, on a peer whose route to that host was down. Spend the slots on separate networks instead: two anycast, two unicast, and a :443 for the networks that pass no other UDP port. The note about reading NAT type off two ports of one address goes with them - webrtc-ice queries each URL from its own socket, so that comparison never held, and nothing consumes the result. `stun_servers()` hands the STUN half out, so the IPv6 probe can stop keeping a second hand-written copy and an operator's OPTION_ICE_SERVERS override reaches both paths. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3 --- src/webrtc.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/webrtc.rs b/src/webrtc.rs index a7bda1c657..41adfef5d7 100644 --- a/src/webrtc.rs +++ b/src/webrtc.rs @@ -145,13 +145,14 @@ const FRAG_END: u8 = 0; /// unauthenticated peer can make a receiver hold. Kept at parity with TCP on purpose: one session /// moves between both paths, so a transport-specific ceiling would kill it on the other one. const MAX_RECV_MESSAGE: usize = crate::bytes_codec::MAX_FRAME_LENGTH; -// use 3 public STUN servers to find out the NAT type, 2 must be the same address but different ports -// https://stackoverflow.com/questions/72805316/determine-nat-mapping-behaviour-using-two-stun-servers -// luckily nextcloud supports two ports for STUN -// unluckily webrtc-rs does not use the same port to do the STUN request -static DEFAULT_ICE_SERVERS: [&str; 3] = [ +// Four networks, not four names: webrtc-ice queries each URL from its own socket, so entries +// sharing a host buy no redundancy - the two this list used to carry failed together, in the same +// millisecond, on a peer whose route to that one host was down. Two anycast, two unicast; the 443 +// entry is for networks that pass no other UDP port. +static DEFAULT_ICE_SERVERS: [&str; 4] = [ "stun:stun.cloudflare.com:3478", - "stun:stun.nextcloud.com:3478", + "stun:stun.l.google.com:19302", + "stun:stun.antisip.com:3478", "stun:stun.nextcloud.com:443", ]; @@ -463,6 +464,14 @@ impl WebRTCStream { )) } + /// The default UDP STUN servers as bare `host:port`, for application-level address probes. + pub fn default_stun_servers() -> Vec { + DEFAULT_ICE_SERVERS + .iter() + .filter_map(|url| url.strip_prefix("stun:").map(str::to_owned)) + .collect() + } + /// Split out from `get_ice_servers` so parsing can be exercised without touching the /// process-global, on-disk-persisted option — the tests run in parallel threads of one /// process, so a test that rewrote it raced every peer connection another test was building.