fix(cliprdr): release outgoing locks before initiating a file copy (#1375)

Author: ymarcus93

crates/ironrdp-cliprdr/src/lib.rs

@@ -1027,6 +1027,47 @@ impl<R: Role> Cliprdr<R> {
         // 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<SvcMessage> {
+        if self.outgoing_locks.is_empty() {
+            return Vec::new();
+        }
+
+        let cleared: Vec<u32> = 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<LockDataId> = 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<R: Role> Cliprdr<R> {
         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())
     }
 }
 

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<SvcMessage> = 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<SvcMessage> = 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:?}"
+    );
+}