diff --git a/libs/clipboard/src/platform/unix/serv_files.rs b/libs/clipboard/src/platform/unix/serv_files.rs index 3c106a22a..f2cd46c26 100644 --- a/libs/clipboard/src/platform/unix/serv_files.rs +++ b/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, - // 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, } 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 { if let FileContentsRequest::Range { file_idx, offset, .. @@ -340,34 +347,19 @@ fn select_clip_files<'a>( conn_id: i32, clip_data_id: Option, ) -> 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, 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 = (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(); } diff --git a/libs/clipboard/src/windows/wf_cliprdr.c b/libs/clipboard/src/windows/wf_cliprdr.c index 721141d7e..1e3bcbb13 100644 --- a/libs/clipboard/src/windows/wf_cliprdr.c +++ b/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);