From 4d1b9774054c9973a9a1911bcf3ac9ca90603da4 Mon Sep 17 00:00:00 2001
From: rustdesk <info@rustdesk.com>
Date: Sat, 8 Aug 2026 17:59:46 +0800
Subject: [PATCH] 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) <noreply@anthropic.com>
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 135dbaa99..676fd11f7 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<webrtc::WebRTCStream> {
-        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 fdb7297be..a81fc8612 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