From a319d771cf2d610a4d259b87bbc8d63cc5076658 Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Fri, 18 Sep 2026 14:47:15 -0300 Subject: [PATCH 1/4] fix(server): drive clipboard channel timeouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Cliprdr::drive_timeouts` is documented as something the embedder must call from a periodic timer: it is what sends `Unlock` PDUs for expired clipboard-data locks, answers abandoned file contents requests with synthetic errors, and drops inactive locked file list snapshots. `ironrdp-client` (rdp.rs) and `ironrdp-web` (session.rs) both drive it. `ironrdp-server` never did, so on the server role: - locks created by `send_lock` on a file FormatList were expired by a later FormatList but never released — no `Unlock` PDU was ever sent; - `outgoing_locks` grew monotonically until `MAX_OUTGOING_LOCKS` (100) was reached, after which `send_lock` silently skipped locking for the rest of the session; - file contents requests abandoned by the peer were never failed, leaving backends waiting on a response that could not arrive. Add a `drive_cliprdr_timeouts` arm to `client_loop`, mirroring the existing `send_heartbeats` / `refresh_auto_reconnect_cookie` timers, and swallow sweep failures: best-effort maintenance must not disconnect a healthy session. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironrdp-server/src/server.rs | 256 ++++++++++++++++++++++++++++ 1 file changed, 256 insertions(+) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 5c9151b443..880accd072 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -72,6 +72,16 @@ use ironrdp_rdpeusb::{InterfaceAlloc, server::UrbdrcControlServer, server::Urbdr const LISTENER_BACKLOG: u32 = 1024; const AUTO_RECONNECT_COOKIE_UPDATE_INTERVAL: Duration = Duration::from_secs(60 * 60); +/// How often the server drives [`CliprdrServer::drive_timeouts`]. +/// +/// The clipboard channel tracks clipboard-data locks, in-flight file contents +/// requests and locked file list snapshots against wall-clock deadlines, but it +/// has no timer of its own: the docs on `drive_timeouts` require the embedder to +/// call it periodically. `ironrdp-client` and `ironrdp-web` already do; without +/// this the server role never sends `Unlock` PDUs, never expires locks and never +/// answers abandoned file contents requests. +const CLIPRDR_DRIVE_TIMEOUTS_INTERVAL: Duration = Duration::from_secs(5); + /// How long a single [`ironrdp_acceptor::accept_finalize`] pass may take before /// the connection is dropped. /// @@ -1648,6 +1658,44 @@ impl RdpServer { Ok(()) } + /// Drives the clipboard channel's time-based cleanup. + /// + /// See [`CLIPRDR_DRIVE_TIMEOUTS_INTERVAL`]. A cleanup failure is logged and + /// swallowed: the sweep is best-effort maintenance, and failing it must not + /// disconnect an otherwise healthy session. + async fn drive_cliprdr_timeouts( + &mut self, + writer: &mut impl FramedWrite, + user_channel_id: u16, + ) -> ServerResult<()> { + let Some(cliprdr) = self.get_svc_processor::() else { + return Ok(()); + }; + + let msgs = match cliprdr.drive_timeouts() { + Ok(msgs) => Vec::from(msgs), + Err(error) => { + warn!(%error, "Clipboard timeout cleanup failed"); + return Ok(()); + } + }; + + if msgs.is_empty() { + return Ok(()); + } + + let channel_id = self + .get_channel_id_by_type::() + .ok_or_else(|| ServerError::channel("SVC channel not found"))?; + let data = server_encode_svc_messages(msgs, channel_id, user_channel_id).map_err(ServerError::encode)?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; + + Ok(()) + } + async fn update_auto_reconnect_cookie( &mut self, cookie: Option, @@ -3206,6 +3254,7 @@ impl RdpServer { let mut event_writer = writer.clone(); let mut auto_reconnect_writer = writer.clone(); let mut heartbeat_writer = writer.clone(); + let mut cliprdr_writer = writer.clone(); let write_counter = writer.write_counter(); let ev_receiver = Arc::clone(&self.ev_receiver); let s = Rc::new(Mutex::new(self)); @@ -3403,12 +3452,30 @@ impl RdpServer { } }; + let this = Rc::clone(&s); + let drive_cliprdr_timeouts = async move { + let mut interval = tokio::time::interval(CLIPRDR_DRIVE_TIMEOUTS_INTERVAL); + // A stalled write can hold this future past several tick deadlines; + // Burst (the default) would then fire the missed ticks back-to-back + // for a sweep that is idempotent anyway. + interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + interval.tick().await; // first tick completes immediately + + loop { + interval.tick().await; + let mut this = this.lock().await; + this.drive_cliprdr_timeouts(&mut cliprdr_writer, user_channel_id) + .await?; + } + }; + let state = tokio::select!( state = dispatch_pdu => state, state = dispatch_display => state, state = dispatch_events => state, state = refresh_auto_reconnect_cookie => state, state = send_heartbeats => state, + state = drive_cliprdr_timeouts => state, ); debug!("End of client loop: {state:?}"); @@ -4840,3 +4907,192 @@ mod tests { ); } } + +/// The server role must drive [`CliprdrServer::drive_timeouts`] on a timer. +/// +/// Before [`CLIPRDR_DRIVE_TIMEOUTS_INTERVAL`] existed nothing in this crate ever +/// called it, so on the server role clipboard-data locks were created and +/// expired but never released: no `Unlock` PDU was ever sent, abandoned file +/// contents requests were never answered, and `outgoing_locks` grew until +/// `MAX_OUTGOING_LOCKS` silently disabled further locking for the session. +#[cfg(test)] +mod cliprdr_timeout_tests { + use core::any::TypeId; + use core::net::Ipv4Addr; + + use ironrdp_cliprdr::Cliprdr; + use ironrdp_cliprdr::backend::CliprdrBackend; + use ironrdp_cliprdr::pdu::{ + Capabilities, ClipboardFormat, ClipboardFormatId, ClipboardFormatName, ClipboardGeneralCapabilityFlags, + ClipboardPdu, ClipboardProtocolVersion, FileContentsRequest, FileContentsResponse, FormatDataRequest, + FormatDataResponse, FormatList, LockDataId, + }; + use ironrdp_core::{Encode as _, WriteCursor, impl_as_any}; + + use super::*; + + /// The clipboard channel dates locks from [`CliprdrBackend::now_ms`], so the + /// test owns the clock rather than sleeping. + #[derive(Debug)] + struct ClockBackend(Arc); + + impl_as_any!(ClockBackend); + + impl CliprdrBackend for ClockBackend { + fn temporary_directory(&self) -> &str { + "." + } + + fn client_capabilities(&self) -> ClipboardGeneralCapabilityFlags { + ClipboardGeneralCapabilityFlags::CAN_LOCK_CLIPDATA + | ClipboardGeneralCapabilityFlags::STREAM_FILECLIP_ENABLED + | ClipboardGeneralCapabilityFlags::USE_LONG_FORMAT_NAMES + } + + fn on_ready(&mut self) {} + fn on_request_format_list(&mut self) {} + fn on_process_negotiated_capabilities(&mut self, _: ClipboardGeneralCapabilityFlags) {} + fn on_remote_copy(&mut self, _: &[ClipboardFormat]) {} + fn on_format_data_request(&mut self, _: FormatDataRequest) {} + fn on_format_data_response(&mut self, _: FormatDataResponse<'_>) {} + fn on_file_contents_request(&mut self, _: FileContentsRequest) {} + fn on_file_contents_response(&mut self, _: FileContentsResponse<'_>) {} + fn on_lock(&mut self, _: LockDataId) {} + fn on_unlock(&mut self, _: LockDataId) {} + + fn now_ms(&self) -> u64 { + self.0.load(Ordering::SeqCst) + } + } + + #[derive(Default)] + struct CapturingWriter(Vec); + + impl FramedWrite for CapturingWriter { + type WriteAllFut<'write> + = core::future::Ready> + where + Self: 'write; + + fn write_all<'a>(&'a mut self, buf: &'a [u8]) -> Self::WriteAllFut<'a> { + self.0.extend_from_slice(buf); + core::future::ready(Ok(())) + } + } + + fn encode_pdu(pdu: &ClipboardPdu<'_>) -> Vec { + let mut buf = vec![0u8; pdu.size()]; + pdu.encode(&mut WriteCursor::new(&mut buf)).unwrap(); + buf + } + + fn capabilities_buf() -> Vec { + encode_pdu(&ClipboardPdu::Capabilities(Capabilities::new( + ClipboardProtocolVersion::V2, + ClipboardGeneralCapabilityFlags::CAN_LOCK_CLIPDATA + | ClipboardGeneralCapabilityFlags::STREAM_FILECLIP_ENABLED + | ClipboardGeneralCapabilityFlags::USE_LONG_FORMAT_NAMES, + ))) + } + + fn format_list_buf(formats: &[ClipboardFormat]) -> Vec { + encode_pdu(&ClipboardPdu::FormatList( + FormatList::new_unicode(formats, true).unwrap(), + )) + } + + fn file_format_list_buf() -> Vec { + format_list_buf(&[ClipboardFormat { + id: ClipboardFormatId(49171), + name: Some(ClipboardFormatName::new("FileGroupDescriptorW")), + }]) + } + + fn text_format_list_buf() -> Vec { + format_list_buf(&[ClipboardFormat { + id: ClipboardFormatId::CF_UNICODETEXT, + name: None, + }]) + } + + fn server_with_cliprdr(clock: &Arc) -> RdpServer { + let cliprdr: CliprdrServer = Cliprdr::with_lock_timeouts( + Box::new(ClockBackend(Arc::clone(clock))), + Duration::from_millis(100), + Duration::from_secs(60 * 60), + ); + + let mut server = RdpServer::builder() + .with_addr((Ipv4Addr::LOCALHOST, 0)) + .with_no_security() + .with_no_input() + .with_no_display() + .build(); + + server.static_channels.insert(cliprdr); + // Returns the *previous* id, so `None` is the expected first attach. + server + .static_channels + .attach_channel_id(TypeId::of::(), 1004); + + server + } + + /// An expired lock is released once its inactivity timeout elapses -- but + /// only because something drove the sweep. + #[tokio::test] + async fn an_expired_lock_is_released_when_the_sweep_is_driven() { + let clock = Arc::new(AtomicU64::new(0)); + let mut server = server_with_cliprdr(&clock); + + let cliprdr = server.get_svc_processor::().unwrap(); + cliprdr.process(&capabilities_buf()).unwrap(); + // Files on the remote clipboard make the channel lock it (2.2.4.1). + let messages = cliprdr.process(&file_format_list_buf()).unwrap(); + assert!( + messages.len() >= 2, + "a file FormatList must answer FormatListResponse AND send LockData, got {} message(s)", + messages.len() + ); + + // The remote clipboard changes: the lock expires but is deliberately + // NOT released yet, because downloads from it may still be in flight. + server + .get_svc_processor::() + .unwrap() + .process(&text_format_list_buf()) + .unwrap(); + + let mut writer = CapturingWriter::default(); + + server.drive_cliprdr_timeouts(&mut writer, 1002).await.unwrap(); + assert!( + writer.0.is_empty(), + "a lock that expired 0ms ago is still inside its inactivity window" + ); + + clock.store(500, Ordering::SeqCst); + server.drive_cliprdr_timeouts(&mut writer, 1002).await.unwrap(); + assert!( + !writer.0.is_empty(), + "past the inactivity timeout the sweep must emit an Unlock PDU; \ + an empty write means the server leaks the lock for the rest of the session" + ); + } + + /// A session without a clipboard channel must not be an error path: the + /// timer fires for every connection. + #[tokio::test] + async fn a_session_without_a_clipboard_channel_sweeps_quietly() { + let mut server = RdpServer::builder() + .with_addr((Ipv4Addr::LOCALHOST, 0)) + .with_no_security() + .with_no_input() + .with_no_display() + .build(); + + let mut writer = CapturingWriter::default(); + server.drive_cliprdr_timeouts(&mut writer, 1002).await.unwrap(); + assert!(writer.0.is_empty()); + } +} From 9eca237cbf24d261dd49f7d1efd6c82e66a3afc4 Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Mon, 21 Sep 2026 16:04:30 -0300 Subject: [PATCH 2/4] docs(server): note the backend callbacks the cliprdr sweep triggers The doc comment on `CLIPRDR_DRIVE_TIMEOUTS_INTERVAL` listed the three sweeps `drive_timeouts` performs but not the two that call into the backend without a PDU from the peer behind them: a file contents request left pending past the transfer timeout is answered with a synthetic error through `on_file_contents_response`, and an inactive locked file list snapshot is dropped with `on_unlock`. Both already ran for the client and web roles; a server backend that read `on_unlock` as "the peer sent UnlockData" starts seeing it fire on a timeout once the server drives the sweep. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironrdp-server/src/server.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 880accd072..b6c4724f92 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -80,6 +80,16 @@ const AUTO_RECONNECT_COOKIE_UPDATE_INTERVAL: Duration = Duration::from_secs(60 * /// call it periodically. `ironrdp-client` and `ironrdp-web` already do; without /// this the server role never sends `Unlock` PDUs, never expires locks and never /// answers abandoned file contents requests. +/// +/// Two of those sweeps reach the backend with no PDU from the peer behind them: +/// a file contents request left pending past the transfer timeout is answered +/// with a synthetic error through [`on_file_contents_response`], and a locked +/// file list snapshot left inactive for that same window is dropped with +/// [`on_unlock`]. Both already run for the client and web roles; a server +/// backend starts seeing them once this timer does. +/// +/// [`on_file_contents_response`]: ironrdp_cliprdr::backend::CliprdrBackend::on_file_contents_response +/// [`on_unlock`]: ironrdp_cliprdr::backend::CliprdrBackend::on_unlock const CLIPRDR_DRIVE_TIMEOUTS_INTERVAL: Duration = Duration::from_secs(5); /// How long a single [`ironrdp_acceptor::accept_finalize`] pass may take before From aa32627b6503fd20de7bd780dfc528a3010bb936 Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Wed, 30 Sep 2026 13:59:42 -0300 Subject: [PATCH 3/4] fix(server): skip the cliprdr sweep when the channel was not joined A configured clipboard channel the client never joined has no channel ID. Treat that like client_accepted does and skip it instead of returning a channel error from a best-effort maintenance sweep. Also trim the test module doc to a cross-reference to the constant's doc. Co-Authored-By: Claude Opus 5.5 --- crates/ironrdp-server/src/server.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index a526a1b3b4..5bff98676b 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -1829,9 +1829,12 @@ impl RdpServer { return Ok(()); } - let channel_id = self - .get_channel_id_by_type::() - .ok_or_else(|| ServerError::channel("SVC channel not found"))?; + // A configured channel the client never joined has no ID. Skip it like + // `client_accepted` does rather than end the session over it. + let Some(channel_id) = self.get_channel_id_by_type::() else { + warn!("Clipboard channel not joined, dropping timeout cleanup messages"); + return Ok(()); + }; let data = server_encode_svc_messages(msgs, channel_id, user_channel_id).map_err(ServerError::encode)?; writer .write_all(&data) @@ -5924,13 +5927,8 @@ mod cliprdr_error_tests { } } -/// The server role must drive [`CliprdrServer::drive_timeouts`] on a timer. -/// -/// Before [`CLIPRDR_DRIVE_TIMEOUTS_INTERVAL`] existed nothing in this crate ever -/// called it, so on the server role clipboard-data locks were created and -/// expired but never released: no `Unlock` PDU was ever sent, abandoned file -/// contents requests were never answered, and `outgoing_locks` grew until -/// `MAX_OUTGOING_LOCKS` silently disabled further locking for the session. +/// The server role must drive [`CliprdrServer::drive_timeouts`] on a timer; see +/// [`CLIPRDR_DRIVE_TIMEOUTS_INTERVAL`]. #[cfg(test)] mod cliprdr_timeout_tests { use core::any::TypeId; From 1c5790c5cac1c9523eb516ad25b49470a8f43361 Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Thu, 1 Oct 2026 08:53:38 -0300 Subject: [PATCH 4/4] test(server): share the capturing writer between the cliprdr test modules Co-Authored-By: Claude Opus 5.5 --- crates/ironrdp-server/src/server.rs | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 5bff98676b..f7ccd22a62 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -5870,7 +5870,7 @@ mod cliprdr_error_tests { } #[derive(Default)] - struct CapturingWriter(Vec); + pub(super) struct CapturingWriter(pub(super) Vec); impl FramedWrite for CapturingWriter { type WriteAllFut<'write> @@ -5943,6 +5943,7 @@ mod cliprdr_timeout_tests { }; use ironrdp_core::{Encode as _, WriteCursor, impl_as_any}; + use super::cliprdr_error_tests::CapturingWriter; use super::*; /// The clipboard channel dates locks from [`CliprdrBackend::now_ms`], so the @@ -5979,21 +5980,6 @@ mod cliprdr_timeout_tests { } } - #[derive(Default)] - struct CapturingWriter(Vec); - - impl FramedWrite for CapturingWriter { - type WriteAllFut<'write> - = core::future::Ready> - where - Self: 'write; - - fn write_all<'a>(&'a mut self, buf: &'a [u8]) -> Self::WriteAllFut<'a> { - self.0.extend_from_slice(buf); - core::future::ready(Ok(())) - } - } - fn encode_pdu(pdu: &ClipboardPdu<'_>) -> Vec { let mut buf = vec![0u8; pdu.size()]; pdu.encode(&mut WriteCursor::new(&mut buf)).unwrap();