fix(clipboard): drop read-time refresh, idle expiry and rebuild reuse; tighten checks

Remove the read-time recency and the 5-minute idle expiry on both owners,
and the unix reuse of an unchanged rebuild (has_same_files). They covered
corner cases, added state, and each brought regressions of its own. Kept
lists are again evicted oldest first (unix) or least recently sent
(Windows), 4 at most; a transfer that outlives 4 newer lists fails cleanly.

Windows: serve kept lists after the existing ownership check again, so a
request carrying a list id no longer bypasses it while the host clipboard
holds files from another session. Refuse request types other than size or
range before allocating.

unix: a retired list's file must still match the size and modified time it
was listed with, checked on the handle the read uses; a file replaced at
its path before its read now fails instead of being served.

Tests: drop the four tests of the removed behavior, add
a_retired_list_refuses_a_file_replaced_before_its_read.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

libs/clipboard/src/platform/unix/serv_files.rs

@@ -13,11 +13,8 @@ use parking_lot::Mutex;
 use std::{
     collections::VecDeque,
     path::PathBuf,
-    sync::{
-        atomic::{AtomicU64, Ordering},
-        Arc,
-    },
-    time::{Duration, Instant, SystemTime},
+    sync::{atomic::Ordering, Arc},
+    time::SystemTime,
     usize,
 };
 
@@ -38,11 +35,6 @@ lazy_static::lazy_static! {
 }
 
 const MAX_RETIRED_CLIP_FILES: usize = 4;
-// Source of `ClipFiles::last_used`.
-static RETIRED_USE_SEQ: AtomicU64 = AtomicU64::new(0);
-// A retired list nobody has read for this long is dropped: its transfer is over, or its peer is
-// gone. The current list is kept, as before.
-const RETIRED_CLIP_FILES_TTL: Duration = Duration::from_secs(5 * 60);
 
 // The first descriptor's `clsid`, inside the 32 reserved bytes every peer skips. A random
 // nonce there makes each copy's list id distinct, even when two copies have identical
@@ -100,11 +92,6 @@ struct ClipFiles {
     id: i32,
     // Connections that were sent `files_pdu`.
     served_to: Vec<i32>,
-    // When a retired list was retired or last read; the lowest is evicted first. Kept apart
-    // from the queue order, which is the order lists were sent and picks a peer's list.
-    last_used: u64,
-    // When a retired list was retired or last read, for `RETIRED_CLIP_FILES_TTL`.
-    last_used_at: Option<Instant>,
 }
 
 impl ClipFiles {
@@ -152,16 +139,6 @@ impl ClipFiles {
         Ok(())
     }
 
-    // The same files with the same descriptors, whatever the nonce.
-    fn has_same_files(&self, other: &ClipFiles) -> bool {
-        let (a, b) = (&self.files_pdu, &other.files_pdu);
-        self.files == other.files
-            && a.len() == b.len()
-            && a.len() >= LIST_NONCE.end
-            && a[..LIST_NONCE.start] == b[..LIST_NONCE.start]
-            && a[LIST_NONCE.end..] == b[LIST_NONCE.end..]
-    }
-
     // Close files that are not being read, such as the preloaded next file; `read_exact_at`
     // reopens them on the next read. A file part-way through keeps its handle and offset, so
     // its remaining bytes come from the same file even if its path is replaced meanwhile.
@@ -173,6 +150,36 @@ impl ClipFiles {
         }
     }
 
+    // A retired list's file is reopened by path once its handle was closed, and the path may now
+    // name a replacement. Check size and modified time on the handle the read will use, and refuse
+    // a file that no longer matches the list.
+    fn check_retired_file(&mut self, request: &FileContentsRequest) -> Result<(), CliprdrError> {
+        let FileContentsRequest::Range { file_idx, .. } = request else {
+            return Ok(());
+        };
+        let Some(file) = self.file_list.get_mut(*file_idx) else {
+            return Ok(());
+        };
+        if file.is_dir {
+            return Ok(());
+        }
+        file.load_handle()?;
+        let unchanged = file
+            .handle
+            .as_ref()
+            .and_then(|handle| handle.get_ref().metadata().ok())
+            .map_or(false, |md| {
+                md.len() == file.size && md.modified().ok() == Some(file.last_write_time)
+            });
+        if unchanged {
+            Ok(())
+        } else {
+            Err(CliprdrError::InvalidRequest {
+                description: format!("file {} changed since its list was sent", file.name),
+            })
+        }
+    }
+
     fn get_files_for_audit(&self, request: &FileContentsRequest) -> Option<ClipboardFile> {
         if let FileContentsRequest::Range {
             file_idx, offset, ..
@@ -340,34 +347,19 @@ fn select_clip_files<'a>(
     conn_id: i32,
     clip_data_id: Option<i32>,
 ) -> Option<&'a mut ClipFiles> {
-    let files = match clip_data_id {
-        Some(id) if current.id == id => return Some(current),
+    match clip_data_id {
+        Some(id) if current.id == id => Some(current),
         Some(id) => retired
             .iter_mut()
             .rev()
-            .find(|files| files.id == id && files.served_to.contains(&conn_id))?,
-        None if current.served_to.contains(&conn_id) => return Some(current),
-        None => match retired
+            .find(|files| files.id == id && files.served_to.contains(&conn_id)),
+        None if current.served_to.contains(&conn_id) => Some(current),
+        None => retired
             .iter_mut()
             .rev()
             .find(|files| files.served_to.contains(&conn_id))
-        {
-            Some(files) => files,
-            None => return Some(current),
-        },
-    };
-    files.last_used = RETIRED_USE_SEQ.fetch_add(1, Ordering::Relaxed) + 1;
-    files.last_used_at = Some(Instant::now());
-    Some(files)
-}
-
-// `retain` keeps the queue in the order lists were sent.
-fn expire_retired(retired: &mut VecDeque<ClipFiles>, now: Instant) {
-    retired.retain(|files| {
-        files.last_used_at.map_or(true, |at| {
-            now.saturating_duration_since(at) < RETIRED_CLIP_FILES_TTL
-        })
-    });
+            .or(Some(current)),
+    }
 }
 
 // Keep a list a peer was sent, so its streams do not read the new copy at the same indexes.
@@ -376,16 +368,9 @@ fn retire_if_served(mut replaced: ClipFiles) {
         return;
     }
     replaced.release_handles();
-    replaced.last_used = RETIRED_USE_SEQ.fetch_add(1, Ordering::Relaxed) + 1;
-    replaced.last_used_at = Some(Instant::now());
     let mut retired = RETIRED_CLIP_FILES.lock();
-    expire_retired(&mut retired, Instant::now());
     if retired.len() == MAX_RETIRED_CLIP_FILES {
-        // The list retired or read longest ago, so a transfer still reading keeps its list.
-        let oldest = (0..retired.len()).min_by_key(|&i| retired[i].last_used);
-        if let Some(i) = oldest {
-            retired.remove(i);
-        }
+        retired.pop_front();
     }
     retired.push_back(replaced);
 }
@@ -426,7 +411,7 @@ pub fn read_file_contents(
 
     let mut current = CLIP_FILES.lock();
     let mut retired = RETIRED_CLIP_FILES.lock();
-    expire_retired(&mut retired, Instant::now());
+    let current_ptr: *const ClipFiles = &*current;
     let Some(clip_files) = select_clip_files(&mut current, &mut retired, conn_id, clip_data_id)
     else {
         return vec![Err(CliprdrError::InvalidRequest {
@@ -436,6 +421,11 @@ pub fn read_file_contents(
             ),
         })];
     };
+    if !std::ptr::eq(&*clip_files, current_ptr) {
+        if let Err(e) = clip_files.check_retired_file(&fcr) {
+            return vec![Err(e)];
+        }
+    }
     let mut res = vec![];
     if let Some(files_res) = clip_files.get_files_for_audit(&fcr) {
         res.push(Ok(files_res));
@@ -459,11 +449,6 @@ pub fn sync_files(files: &[String]) -> Result<(), CliprdrError> {
     let mut next = ClipFiles::default();
     next.sync_files(files, current)?;
     next.build_file_list_pdu()?;
-    if next.has_same_files(&files_lock) {
-        // Nothing changed (a selection with a directory always rebuilds): keep the list, and
-        // the id its peers hold, instead of spending a retired slot on a duplicate.
-        return Ok(());
-    }
     let replaced = std::mem::replace(&mut *files_lock, next);
     retire_if_served(replaced);
     Ok(())
@@ -786,32 +771,6 @@ mod sig_test {
         clear_files();
     }
 
-    #[test]
-    fn identical_recopy_keeps_its_peers_on_the_current_list() {
-        let tmp = TmpDir::new("same_recopy");
-        let dir = tmp.join("dir");
-        fs::create_dir_all(&dir).unwrap();
-        fs::write(dir.join("file.bin"), b"AAAAAAAA").unwrap();
-
-        let _guard = lock_clip_files();
-        clear_files();
-        sync_files(&[path_str(&dir)]).unwrap();
-        let id = file_list_id(&get_file_list_pdu(1));
-        let idx = CLIP_FILES.lock().first_file_index as i32;
-
-        // A selection with a directory is rebuilt on every copy; an unchanged rebuild must
-        // not use up the slots that lists still being read depend on.
-        for _ in 0..=MAX_RETIRED_CLIP_FILES {
-            sync_files(&[path_str(&dir)]).unwrap();
-        }
-        assert!(RETIRED_CLIP_FILES.lock().is_empty());
-        let read = |id| range_data(read_file_contents(1, 7, idx, 0x2, 0, 0, 4, id));
-        assert_eq!(read(Some(id)), b"AAAA");
-        assert_eq!(read(None), b"AAAA");
-
-        clear_files();
-    }
-
     fn file_index(path: &PathBuf) -> i32 {
         let files = CLIP_FILES.lock();
         files
@@ -929,94 +888,33 @@ mod sig_test {
     }
 
     #[test]
-    fn a_list_being_read_outlives_newer_copies() {
-        let tmp = TmpDir::new("read_keeps_list");
-        let files: Vec<PathBuf> = (0..=MAX_RETIRED_CLIP_FILES + 1)
-            .map(|i| {
-                let file = tmp.join(&format!("{i}.bin"));
-                fs::write(&file, format!("{i}{i}{i}{i}{i}{i}{i}{i}")).unwrap();
-                file
-            })
-            .collect();
-
-        let _guard = lock_clip_files();
-        clear_files();
-        sync_files(&[path_str(&files[0])]).unwrap();
-        let id = file_list_id(&get_file_list_pdu(1));
-        let idx = CLIP_FILES.lock().first_file_index as i32;
-        let read = || range_data(read_file_contents(1, 7, idx, 0x2, 0, 0, 4, Some(id)));
-
-        // A unix peer fetches every new list as soon as it is copied, so each copy retires the
-        // one before it. Reads in between keep the list being transferred from being evicted.
-        for file in &files[1..] {
-            sync_files(&[path_str(file)]).unwrap();
-            get_file_list_pdu(2);
-            assert_eq!(read(), b"0000");
-        }
-
-        clear_files();
-    }
-
-    #[test]
-    fn another_conns_read_does_not_change_a_legacy_peers_list() {
-        let tmp = TmpDir::new("legacy_order");
-        let [a, b, c] = ["a", "b", "c"].map(|name| {
-            let file = tmp.join(&format!("{name}.bin"));
-            fs::write(&file, name.repeat(8).to_uppercase()).unwrap();
-            file
-        });
-
-        let _guard = lock_clip_files();
-        clear_files();
-        sync_files(&[path_str(&a)]).unwrap();
-        get_file_list_pdu(1);
-        get_file_list_pdu(2);
-        let idx = CLIP_FILES.lock().first_file_index as i32;
-        sync_files(&[path_str(&b)]).unwrap();
-        get_file_list_pdu(1);
-        // Neither connection fetches C's list.
-        sync_files(&[path_str(&c)]).unwrap();
-
-        // Peers without ids: 1 is reading B, the list last sent to it; 2 is reading A.
-        let read = |conn_id, offset| {
-            range_data(read_file_contents(conn_id, 7, idx, 0x2, offset, 0, 4, None))
-        };
-        assert_eq!(read(1, 0), b"BBBB");
-        assert_eq!(read(2, 0), b"AAAA");
-        assert_eq!(read(1, 4), b"BBBB");
-
-        clear_files();
-    }
-
-    #[test]
-    fn an_idle_retired_list_expires() {
-        let tmp = TmpDir::new("idle_list");
+    fn a_retired_list_refuses_a_file_replaced_before_its_read() {
+        let tmp = TmpDir::new("replaced_unread");
         let first = tmp.join("first.bin");
         fs::write(&first, b"AAAAAAAA").unwrap();
         let second = tmp.join("second.bin");
         fs::write(&second, b"BBBBBBBB").unwrap();
+        let other = tmp.join("other.bin");
+        fs::write(&other, b"EEEEEEEE").unwrap();
 
         let _guard = lock_clip_files();
         clear_files();
-        sync_files(&[path_str(&first)]).unwrap();
+        sync_files(&[path_str(&first), path_str(&second)]).unwrap();
         let id = file_list_id(&get_file_list_pdu(1));
-        let idx = CLIP_FILES.lock().first_file_index as i32;
-        let read = |id| read_file_contents(1, 7, idx, 0x2, 0, 0, 4, id);
-        assert_eq!(range_data(read(Some(id))), b"AAAA");
-        sync_files(&[path_str(&second)]).unwrap();
+        let (first_idx, second_idx) = (file_index(&first), file_index(&second));
+        let read = |idx, offset| read_file_contents(1, 7, idx, 0x2, offset, 0, 4, Some(id));
+        assert_eq!(range_data(read(first_idx, 0)), b"AAAA");
+
+        // Retiring closes `second`, which has not been read yet; it is then replaced.
+        sync_files(&[path_str(&other)]).unwrap();
+        replace_atomically(&second, b"CCCCCCCCCC");
 
-        // Expiry runs with a later `now` rather than a backdated timestamp: an `Instant` cannot
-        // go back past the clock's start, which is only minutes away on a fresh machine.
-        let now = Instant::now();
-        expire_retired(&mut RETIRED_CLIP_FILES.lock(), now);
-        assert_eq!(RETIRED_CLIP_FILES.lock().len(), 1);
-        let later = now + RETIRED_CLIP_FILES_TTL + Duration::from_secs(1);
-        expire_retired(&mut RETIRED_CLIP_FILES.lock(), later);
-        assert!(RETIRED_CLIP_FILES.lock().is_empty());
         assert!(matches!(
-            read(Some(id)).last(),
+            read(second_idx, 0).last(),
             Some(Err(CliprdrError::InvalidRequest { .. }))
         ));
+        // The file part-way through is unaffected.
+        assert_eq!(range_data(read(first_idx, 4)), b"AAAA");
 
         clear_files();
     }

libs/clipboard/src/windows/wf_cliprdr.c

@@ -60,9 +60,6 @@
 /* Largest range served in one request, here and by the unix file server (MAX_RANGE_READ in
  * serv_files.rs). Larger IStream reads are split into requests of this size. */
 #define WF_CLIPRDR_MAX_RANGE_READ (16u * 1024u * 1024u)
-/* An earlier file list nobody has read for this long is dropped: its transfer is over, or its
- * peer is gone. The current list is kept, as before. */
-#define WF_CLIPRDR_SERVED_FILE_LIST_TTL_MS (5ull * 60 * 1000)
 static const WCHAR WF_CLIPRDR_SUPERSCRIPT_DIGITS[] = L"\x00B9\x00B2\x00B3";
 static const WCHAR WF_CLIPRDR_INVALID_FILE_NAME_CHARS[] = L"<>:\"|?*";
 
@@ -408,7 +405,6 @@ struct wf_served_file_list
 	UINT32 *connIDs; // connections the list was sent to
 	size_t nConnIDs;
 	UINT64 last_served; // served_file_list_seq when last sent; the lowest is evicted first
-	ULONGLONG last_used_tick; // GetTickCount64() when last sent or read
 	size_t nFiles;
 	size_t first_file_index;
 	WCHAR **file_names;
@@ -2607,36 +2603,6 @@ static BOOL wf_cliprdr_copy_file_list(wfClipboard *clipboard, wfServedFileList *
 	return TRUE;
 }
 
-static void wf_cliprdr_touch_served_file_list(wfClipboard *clipboard, UINT32 id)
-{
-	size_t i;
-
-	for (i = 0; i < WF_CLIPRDR_SERVED_FILE_LISTS; i++)
-	{
-		wfServedFileList *list = &clipboard->served_file_lists[i];
-		if (list->file_names && list->id == id)
-		{
-			list->last_served = ++clipboard->served_file_list_seq;
-			list->last_used_tick = GetTickCount64();
-			return;
-		}
-	}
-}
-
-static void wf_cliprdr_expire_served_file_lists(wfClipboard *clipboard)
-{
-	ULONGLONG now = GetTickCount64();
-	size_t i;
-
-	for (i = 0; i < WF_CLIPRDR_SERVED_FILE_LISTS; i++)
-	{
-		wfServedFileList *list = &clipboard->served_file_lists[i];
-		if (list->file_names && list->id != clipboard->file_list_id &&
-			now - list->last_used_tick > WF_CLIPRDR_SERVED_FILE_LIST_TTL_MS)
-			wf_cliprdr_free_served_file_list(list);
-	}
-}
-
 /* One slot per list, shared by every connection it was sent to. A full table evicts the list
  * sent least recently, so a list a transfer has just started on is kept. */
 static void wf_cliprdr_remember_served_file_list(wfClipboard *clipboard, UINT32 connID,
@@ -2646,7 +2612,6 @@ static void wf_cliprdr_remember_served_file_list(wfClipboard *clipboard, UINT32
 	UINT32 *connIDs;
 	size_t i;
 
-	wf_cliprdr_expire_served_file_lists(clipboard);
 	for (i = 0; i < WF_CLIPRDR_SERVED_FILE_LISTS; i++)
 	{
 		if (clipboard->served_file_lists[i].file_names &&
@@ -2674,7 +2639,6 @@ static void wf_cliprdr_remember_served_file_list(wfClipboard *clipboard, UINT32
 		}
 	}
 	list->last_served = ++clipboard->served_file_list_seq;
-	list->last_used_tick = GetTickCount64();
 
 	if (wf_cliprdr_served_to(list, connID))
 		return;
@@ -2733,7 +2697,6 @@ static BOOL wf_cliprdr_read_served_file_list(wfClipboard *clipboard,
 	wfServedFileList *list = NULL;
 	size_t i;
 
-	wf_cliprdr_expire_served_file_lists(clipboard);
 	for (i = 0; i < WF_CLIPRDR_SERVED_FILE_LISTS; i++)
 	{
 		if (clipboard->served_file_lists[i].file_names &&
@@ -2746,9 +2709,6 @@ static BOOL wf_cliprdr_read_served_file_list(wfClipboard *clipboard,
 	}
 	if (!list || request->listIndex >= list->nFiles)
 		return FALSE;
-	// A list being read is not the next one evicted.
-	list->last_served = ++clipboard->served_file_list_seq;
-	list->last_used_tick = GetTickCount64();
 
 	if (request->dwFlags == FILECONTENTS_SIZE)
 	{
@@ -3729,42 +3689,18 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context,
 		return ERROR_INTERNAL_ERROR;
 	}
 
-	// Refuse a range over the cap before allocating for it. Clients of this version split larger
-	// reads; an older Windows client reading more in one IStream::Read fails, as with unix owners.
-	if (fileContentsRequest->dwFlags == FILECONTENTS_RANGE &&
-		fileContentsRequest->cbRequested > WF_CLIPRDR_MAX_RANGE_READ)
+	// Refuse an unknown request type, or a range over the cap, before allocating for it. Clients
+	// of this version split larger reads; an older Windows client reading more in one
+	// IStream::Read fails, as with unix owners.
+	if ((fileContentsRequest->dwFlags != FILECONTENTS_SIZE &&
+		 fileContentsRequest->dwFlags != FILECONTENTS_RANGE) ||
+		(fileContentsRequest->dwFlags == FILECONTENTS_RANGE &&
+		 fileContentsRequest->cbRequested > WF_CLIPRDR_MAX_RANGE_READ))
 	{
 		ZeroMemory(&vStgMedium, sizeof(STGMEDIUM)); // read at exit
 		goto exit;
 	}
 
-	// A stream whose list is no longer the one on this clipboard, because a newer list was sent
-	// or the clipboard now holds files from a peer, reads by path from the list it came from.
-	// Only lists sent to this connection are served, so the check below still stops a paste
-	// from getting files it was never offered.
-	if (fileContentsRequest->haveClipDataId &&
-		(fileContentsRequest->clipDataId != clipboard->file_list_id ||
-		 is_set_by_instance(clipboard) || is_file_descriptor_from_remote()))
-	{
-		ZeroMemory(&vStgMedium, sizeof(STGMEDIUM)); // read at exit
-		if (fileContentsRequest->dwFlags != FILECONTENTS_SIZE &&
-			fileContentsRequest->dwFlags != FILECONTENTS_RANGE)
-			goto exit;
-		cbRequested = fileContentsRequest->dwFlags == FILECONTENTS_SIZE
-						  ? sizeof(UINT64)
-						  : fileContentsRequest->cbRequested;
-		pData = (BYTE *)calloc(1, cbRequested);
-		if (pData && wf_cliprdr_read_served_file_list(clipboard, fileContentsRequest, pData,
-													  cbRequested, &uSize))
-			rc = CHANNEL_RC_OK;
-		goto exit;
-	}
-
-	// The current list is read through the existing path below; keep its kept copy from being
-	// evicted or expiring while it is read, so the transfer can go on after a newer list is sent.
-	if (fileContentsRequest->haveClipDataId)
-		wf_cliprdr_touch_served_file_list(clipboard, fileContentsRequest->clipDataId);
-
 	// If the clipboard is set by the instance, or the file descriptor is from remote,
 	// we should not process the request.
 	// Because this may be the following cases:
@@ -3790,6 +3726,22 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context,
 		goto exit;
 	}
 
+	// A stream whose list is no longer the current one reads by path from the list it came from.
+	// Only lists sent to this connection are served.
+	if (fileContentsRequest->haveClipDataId &&
+		fileContentsRequest->clipDataId != clipboard->file_list_id)
+	{
+		ZeroMemory(&vStgMedium, sizeof(STGMEDIUM)); // read at exit
+		cbRequested = fileContentsRequest->dwFlags == FILECONTENTS_SIZE
+						  ? sizeof(UINT64)
+						  : fileContentsRequest->cbRequested;
+		pData = (BYTE *)calloc(1, cbRequested);
+		if (pData && wf_cliprdr_read_served_file_list(clipboard, fileContentsRequest, pData,
+													  cbRequested, &uSize))
+			rc = CHANNEL_RC_OK;
+		goto exit;
+	}
+
 	cbRequested = fileContentsRequest->cbRequested;
 	if (fileContentsRequest->dwFlags == FILECONTENTS_SIZE)
 		cbRequested = sizeof(UINT64);