diff --git a/crates/ironrdp-cliprdr/src/lib.rs b/crates/ironrdp-cliprdr/src/lib.rs index 2fa0df009..30fad8c1c 100644 --- a/crates/ironrdp-cliprdr/src/lib.rs +++ b/crates/ironrdp-cliprdr/src/lib.rs @@ -1027,6 +1027,47 @@ impl Cliprdr { // Backend notification deferred until actual cleanup } + /// Immediately sends `Unlock` PDUs for — and drops — every outgoing clipboard lock. + /// + /// Outgoing locks are created when we download files from the remote: each asks the + /// Shared Clipboard Owner to retain File Stream data so we can keep pulling it even + /// after the clipboard changes ([MS-RDPECLIP] 2.2.4.1). They are normally released + /// lazily by the inactivity sweep in [`Self::drive_timeouts`], so concurrent downloads + /// that outlive a *remote* clipboard change aren't aborted. + /// + /// When the **local** side takes clipboard ownership itself (initiating a file copy), + /// those locks point at data we are replacing. Leaving them held while we advertise a + /// fresh `FormatList` makes the server track a lock for a download that will never + /// finish; some servers (notably Windows `rdpclip.exe`) react badly to that overlap. + /// Releasing them up front keeps the lock/ownership state consistent. + /// + /// Returns the `Unlock` PDUs to send (these must precede the new `FormatList` on the + /// wire); empty when no locks are held. + fn release_outgoing_locks(&mut self) -> Vec { + if self.outgoing_locks.is_empty() { + return Vec::new(); + } + + let cleared: Vec = self.outgoing_locks.keys().copied().collect(); + self.outgoing_locks.clear(); + self.current_lock_id = None; + + info!( + count = cleared.len(), + "Releasing outgoing locks before taking clipboard ownership" + ); + + let messages = cleared + .iter() + .map(|id| into_cliprdr_message(ClipboardPdu::UnlockData(LockDataId(*id)))) + .collect(); + + let lock_ids: Vec = cleared.iter().map(|id| LockDataId(*id)).collect(); + self.backend.on_outgoing_locks_cleared(&lock_ids); + + messages + } + /// Lazily runs periodic cleanup during normal API activity. /// /// Cleans up expired locks, stale file contents requests, and inactive @@ -1537,7 +1578,15 @@ impl Cliprdr { let format_list = self.build_format_list(&formats).map_err(|e| encode_err!(e))?; let pdu = ClipboardPdu::FormatList(format_list); - Ok(vec![into_cliprdr_message(pdu)].into()) + // Release any outgoing download locks BEFORE advertising our file list. By + // initiating a file copy we take clipboard ownership, so locks placed for + // downloads from the previous owner are now stale; sending a new FormatList while + // they're still held desyncs the server's lock state. + // The Unlock PDUs must precede the FormatList on the wire. + let mut messages = self.release_outgoing_locks(); + messages.push(into_cliprdr_message(pdu)); + + Ok(messages.into()) } } diff --git a/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs b/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs index c3d5e8b79..aecab1cbb 100644 --- a/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs +++ b/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs @@ -562,3 +562,77 @@ fn on_outgoing_locks_expired_callback_invoked() { // Cleared callback should NOT have fired yet (cleanup hasn't run) assert!(cleared_ids.lock().unwrap().is_empty()); } + +// -- Taking clipboard ownership releases stale download locks -------- + +/// When the local side initiates a file copy (an upload), it takes clipboard +/// ownership — so the outgoing locks placed for downloads from the previous owner are +/// now stale. They must be released with `Unlock` PDUs that PRECEDE our `FormatList` on +/// the wire; otherwise the server keeps tracking a lock for a download that will never +/// complete, which desyncs its clipboard state. +#[test] +fn initiate_file_copy_releases_outgoing_download_locks() { + let mut cliprdr = ready_locking_client(); + + // Two remote file lists => two outgoing download locks (e.g. two in-flight downloads). + let lock1 = process_file_format_list(&mut cliprdr); + let lock2 = process_file_format_list(&mut cliprdr); + assert_eq!(cliprdr.__test_outgoing_locks().len(), 2); + + let messages: Vec = cliprdr + .initiate_file_copy(vec![FileDescriptor::new("upload.txt")]) + .unwrap() + .into(); + + // Every outgoing lock is released and the current lock id is cleared. + assert!( + cliprdr.__test_outgoing_locks().is_empty(), + "outgoing locks must be released when taking clipboard ownership" + ); + assert_eq!(cliprdr.__test_current_lock_id(), None); + + // Wire order: an Unlock for each held lock, THEN our FormatList last. + assert_eq!(messages.len(), 3, "expected 2 Unlock PDUs + 1 FormatList"); + + decode_pdu!(messages[0] => _b0, pdu0); + let id0 = match pdu0 { + ClipboardPdu::UnlockData(id) => id.0, + other => panic!("expected UnlockData first, got {other:?}"), + }; + decode_pdu!(messages[1] => _b1, pdu1); + let id1 = match pdu1 { + ClipboardPdu::UnlockData(id) => id.0, + other => panic!("expected UnlockData second, got {other:?}"), + }; + let mut unlocked = [id0, id1]; + unlocked.sort_unstable(); + let mut expected = [lock1, lock2]; + expected.sort_unstable(); + assert_eq!(unlocked, expected, "both download locks must be unlocked"); + + decode_pdu!(messages[2] => _b2, last_pdu); + assert!( + matches!(last_pdu, ClipboardPdu::FormatList(_)), + "FormatList must come after the Unlock PDUs, got {last_pdu:?}" + ); +} + +/// With no outgoing locks held, `initiate_file_copy` sends only the `FormatList` +/// (no spurious `Unlock`). +#[test] +fn initiate_file_copy_without_locks_sends_only_format_list() { + let mut cliprdr = ready_locking_client(); + assert!(cliprdr.__test_outgoing_locks().is_empty()); + + let messages: Vec = cliprdr + .initiate_file_copy(vec![FileDescriptor::new("upload.txt")]) + .unwrap() + .into(); + + assert_eq!(messages.len(), 1, "expected only a FormatList when no locks are held"); + decode_pdu!(messages[0] => _b0, pdu); + assert!( + matches!(pdu, ClipboardPdu::FormatList(_)), + "expected FormatList, got {pdu:?}" + ); +}