From db62a0122496620955e6bfc6642f8525f41f5ec4 Mon Sep 17 00:00:00 2001 From: ClSlaid Date: Fri, 20 Oct 2023 22:27:39 +0800 Subject: [PATCH] patch: fix dead lock in file transfer Signed-off-by: ClSlaid --- libs/clipboard/src/lib.rs | 18 +-- libs/clipboard/src/platform/fuse.rs | 119 ++++++++---------- .../src/platform/linux/local_file.rs | 12 +- libs/clipboard/src/platform/linux/mod.rs | 70 ++++++++--- libs/clipboard/src/platform/linux/x11.rs | 4 +- 5 files changed, 121 insertions(+), 102 deletions(-) diff --git a/libs/clipboard/src/lib.rs b/libs/clipboard/src/lib.rs index c5177426f..a69349989 100644 --- a/libs/clipboard/src/lib.rs +++ b/libs/clipboard/src/lib.rs @@ -119,19 +119,19 @@ lazy_static::lazy_static! { impl ClipboardFile { pub fn is_stopping_allowed(&self) -> bool { - match self { + matches!( + self, ClipboardFile::MonitorReady - | ClipboardFile::FormatList { .. } - | ClipboardFile::FormatDataRequest { .. } => true, - _ => false, - } + | ClipboardFile::FormatList { .. } + | ClipboardFile::FormatDataRequest { .. } + ) } pub fn is_stopping_allowed_from_peer(&self) -> bool { - match self { - ClipboardFile::MonitorReady | ClipboardFile::FormatList { .. } => true, - _ => false, - } + matches!( + self, + ClipboardFile::MonitorReady | ClipboardFile::FormatList { .. } + ) } } diff --git a/libs/clipboard/src/platform/fuse.rs b/libs/clipboard/src/platform/fuse.rs index 060049420..a52881be2 100644 --- a/libs/clipboard/src/platform/fuse.rs +++ b/libs/clipboard/src/platform/fuse.rs @@ -167,23 +167,23 @@ pub(crate) struct FuseServer { // timeout timeout: Duration, // file read reply channel - tx: Sender, - // file read reply channel rx: Receiver, } impl FuseServer { /// create a new fuse server - pub fn new(timeout: Duration) -> Self { + pub fn new(timeout: Duration) -> (Self, Sender) { let (tx, rx) = std::sync::mpsc::channel(); - Self { - generation: AtomicU64::new(0), - files: Vec::new(), - file_handle_counter: AtomicU64::new(0), - timeout, - rx, + ( + Self { + generation: AtomicU64::new(0), + files: Vec::new(), + file_handle_counter: AtomicU64::new(0), + timeout, + rx, + }, tx, - } + ) } pub fn client(server: Arc>) -> FuseClient { @@ -191,16 +191,6 @@ impl FuseServer { } } -impl FuseServer { - pub fn serve(&self, reply: ClipboardFile) -> Result<(), CliprdrError> { - self.tx.send(reply).map_err(|e| { - log::error!("failed to serve cliprdr reply from endpoint: {:?}", e); - CliprdrError::ClipboardInternalError - })?; - Ok(()) - } -} - impl FuseServer { pub fn load_file_list(&mut self, files: Vec) -> Result<(), CliprdrError> { let tree = FuseNode::build_tree(files)?; @@ -273,14 +263,13 @@ impl fuser::Filesystem for FuseServer { // error reply.error(libc::ENOENT); log::debug!("fuse: child not found"); - return; } fn opendir( &mut self, _req: &fuser::Request<'_>, ino: u64, - flags: i32, + _flags: i32, reply: fuser::ReplyOpen, ) { let files = &self.files; @@ -304,7 +293,6 @@ impl fuser::Filesystem for FuseServer { let fh = self.alloc_fd(); entry.add_handler(fh); reply.opened(fh, 0); - return; } fn readdir( @@ -357,7 +345,6 @@ impl fuser::Filesystem for FuseServer { } reply.ok(); - return; } fn releasedir( @@ -387,10 +374,9 @@ impl fuser::Filesystem for FuseServer { let _ = entry.unregister_handler(fh); reply.ok(); - return; } - fn open(&mut self, _req: &fuser::Request<'_>, ino: u64, flags: i32, reply: fuser::ReplyOpen) { + fn open(&mut self, _req: &fuser::Request<'_>, ino: u64, _flags: i32, reply: fuser::ReplyOpen) { let files = &self.files; let Some(entry) = files.get(ino as usize - 1) else { reply.error(libc::ENOENT); @@ -415,7 +401,6 @@ impl fuser::Filesystem for FuseServer { let fh = self.alloc_fd(); entry.add_handler(fh); reply.opened(fh, 0); - return; } fn read( @@ -481,13 +466,12 @@ impl fuser::Filesystem for FuseServer { return; }; - if let Err(_) = entry.unregister_handler(fh) { + if entry.unregister_handler(fh).is_err() { reply.error(libc::EBADF); log::error!("fuse: release: entry has no such handler"); return; } reply.ok(); - return; } fn getattr(&mut self, _req: &fuser::Request<'_>, ino: u64, reply: fuser::ReplyAttr) { @@ -545,7 +529,7 @@ impl FuseServer { ((offset >> 32) as i32, (offset & (u32::MAX as i64)) as i32); let request = ClipboardFile::FileContentsRequest { stream_id: node.stream_id, - list_index: node.inode as i32 - 2, + list_index: node.index as i32, dw_flags: 2, n_position_low, n_position_high, @@ -562,36 +546,36 @@ impl FuseServer { node.stream_id ); - let reply = self.rx.recv_timeout(self.timeout).map_err(|e| { - log::error!("failed to receive file list from channel: {:?}", e); - std::io::Error::new(std::io::ErrorKind::TimedOut, e) - })?; + loop { + let reply = self.rx.recv_timeout(self.timeout).map_err(|e| { + log::error!("failed to receive file list from channel: {:?}", e); + std::io::Error::new(std::io::ErrorKind::TimedOut, e) + })?; - match reply { - ClipboardFile::FileContentsResponse { - msg_flags, - stream_id, - requested_data, - } => { - if stream_id != node.stream_id { + match reply { + ClipboardFile::FileContentsResponse { + msg_flags, + stream_id, + requested_data, + } => { + if stream_id != node.stream_id { + log::debug!("stream id mismatch, ignore"); + continue; + } + if msg_flags & 1 == 0 { + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "failure request", + )); + } + return Ok(requested_data); + } + _ => { return Err(std::io::Error::new( std::io::ErrorKind::Other, - "stream id mismatch", - )); + "invalid reply", + )) } - if msg_flags & 1 == 0 { - return Err(std::io::Error::new( - std::io::ErrorKind::Other, - "failure request", - )); - } - Ok(requested_data) - } - _ => { - return Err(std::io::Error::new( - std::io::ErrorKind::Other, - "invalid reply", - )); } } } @@ -761,7 +745,10 @@ struct FuseNode { /// stream id pub stream_id: i32, - pub inode: u64, + /// file index in peer's file list + /// NOTE: + /// it is NOT the same as inode, this is the index in the file list + pub index: usize, /// parent inode pub parent: Option, @@ -778,11 +765,11 @@ struct FuseNode { } impl FuseNode { - pub fn from_description(inode: Inode, desc: FileDescription) -> Self { + pub fn from_description(index: usize, inode: Inode, desc: FileDescription) -> Self { Self { conn_id: desc.conn_id, stream_id: rand::random(), - inode, + index, name: desc.name.to_str().unwrap().to_owned(), parent: None, attributes: InodeAttributes::from_description(inode, desc), @@ -795,7 +782,7 @@ impl FuseNode { Self { conn_id: 0, stream_id: rand::random(), - inode: 1, + index: 0, name: String::from("/"), parent: None, attributes: InodeAttributes::new_root(), @@ -844,7 +831,7 @@ impl FuseNode { sub_root_map.insert(Path::new("/").to_path_buf(), FUSE_ROOT_ID); sub_root_map.insert(Path::new("").to_path_buf(), FUSE_ROOT_ID); - for file in files.into_iter() { + for (index, file) in files.into_iter().enumerate() { let name = file.name.clone(); let ancestors = name .ancestors() @@ -882,7 +869,7 @@ impl FuseNode { } })?; - let mut desc = if ancestor != &file.name { + let mut desc = if ancestor != file.name { FileDescription { conn_id: 0, name: ancestor.to_path_buf(), @@ -903,7 +890,7 @@ impl FuseNode { if desc.kind == FileType::Directory { sub_root_map.insert(desc.name.clone(), inode); } - let mut fuse_node = FuseNode::from_description(inode, desc); + let mut fuse_node = FuseNode::from_description(index, inode, desc); fuse_node.parent = Some(parent_inode); tree_list.push(fuse_node); } @@ -1095,7 +1082,7 @@ mod fuse_test { perm: 0, } } - let (d0_path, f0_path, f1_path, d1_path, f2_path) = if prefix == "" { + let (d0_path, f0_path, f1_path, d1_path, f2_path) = if prefix.is_empty() { ( "folder0".to_string(), "folder0/file0".to_string(), @@ -1149,6 +1136,10 @@ mod fuse_test { assert!(strip_list[3].children.is_empty()); assert_eq!(strip_list[4].children, vec![e + 6]); assert!(strip_list[5].children.is_empty()); + + for (idx, node) in strip_list.iter().skip(1).enumerate() { + assert_eq!(idx, node.index) + } } #[test] diff --git a/libs/clipboard/src/platform/linux/local_file.rs b/libs/clipboard/src/platform/linux/local_file.rs index 4bc2abc0a..eddde75d6 100644 --- a/libs/clipboard/src/platform/linux/local_file.rs +++ b/libs/clipboard/src/platform/linux/local_file.rs @@ -50,11 +50,7 @@ impl LocalFile { let is_dir = mt.is_dir(); let read_only = mt.permissions().readonly(); let system = false; - let hidden = if path.to_string_lossy().starts_with('.') { - true - } else { - false - }; + let hidden = path.to_string_lossy().starts_with('.'); let archive = false; let normal = !(is_dir || read_only || system || hidden || archive); let last_write_time = mt.modified().unwrap_or(SystemTime::UNIX_EPOCH); @@ -246,9 +242,9 @@ mod file_list_test { let p = prefix; - let (r_path, a_path, b_path, c_path) = if "" != prefix { + let (r_path, a_path, b_path, c_path) = if !prefix.is_empty() { ( - format!("{}", p), + p.to_string(), format!("{}/a.txt", p), format!("{}/b", p), format!("{}/b/c.txt", p), @@ -281,7 +277,7 @@ mod file_list_test { let parsed = FileDescription::parse_file_descriptors(pdu.to_vec(), 0)?; assert_eq!(parsed.len(), 4); - if "" != prefix { + if !prefix.is_empty() { assert_eq!(parsed[0].name.to_str().unwrap(), format!("{}", prefix)); assert_eq!( parsed[1].name.to_str().unwrap(), diff --git a/libs/clipboard/src/platform/linux/mod.rs b/libs/clipboard/src/platform/linux/mod.rs index 9d83fe429..67655f6f3 100644 --- a/libs/clipboard/src/platform/linux/mod.rs +++ b/libs/clipboard/src/platform/linux/mod.rs @@ -1,4 +1,9 @@ -use std::{os::unix::prelude::FileExt, path::PathBuf, sync::Arc, time::Duration}; +use std::{ + os::unix::prelude::FileExt, + path::PathBuf, + sync::{mpsc::Sender, Arc}, + time::Duration, +}; use dashmap::DashMap; use fuser::MountOption; @@ -92,8 +97,11 @@ enum FileContentsRequest { pub struct ClipboardContext { pub fuse_mount_point: PathBuf, + /// stores fuse background session handle fuse_handle: Mutex>, + /// a sender of clipboard file contents pdu to fuse server + fuse_tx: Sender, fuse_server: Arc>, clipboard: Arc, @@ -107,7 +115,9 @@ impl ClipboardContext { CliprdrError::CliprdrInit })?; - let fuse_server = Arc::new(Mutex::new(FuseServer::new(timeout))); + let (fuse_server, fuse_tx) = FuseServer::new(timeout); + + let fuse_server = Arc::new(Mutex::new(fuse_server)); let clipboard = get_sys_clipboard(&fuse_mount_point)?; let clipboard = Arc::from(clipboard) as Arc<_>; @@ -115,6 +125,7 @@ impl ClipboardContext { Ok(Self { fuse_mount_point, fuse_server, + fuse_tx, fuse_handle: Mutex::new(None), clipboard, }) @@ -175,12 +186,12 @@ impl ClipboardContext { conn_id: i32, request: FileContentsRequest, ) -> Result<(), CliprdrError> { - log::debug!("file contents (range) requested from conn: {}", conn_id); let file_contents_req = match request { FileContentsRequest::Size { stream_id, file_idx, } => { + log::debug!("file contents (size) requested from conn: {}", conn_id); let file_list = self.clipboard.get_file_list()?; let Some(file) = file_list.get(file_idx) else { log::error!( @@ -198,7 +209,12 @@ impl ClipboardContext { }); }; - log::debug!("conn {} requested file {}", conn_id, file.name); + log::debug!( + "conn {} requested file-{}: {}", + conn_id, + file_idx, + file.name + ); let size = file.size; ClipboardFile::FileContentsResponse { @@ -213,6 +229,12 @@ impl ClipboardContext { offset, length, } => { + log::debug!( + "file contents (range from {} length {}) request from conn: {}", + offset, + length, + conn_id + ); let file_list = self.clipboard.get_file_list()?; let Some(file) = file_list.get(file_idx) else { log::error!( @@ -228,7 +250,12 @@ impl ClipboardContext { ), }); }; - log::debug!("conn {} requested file {}", conn_id, file.name); + log::debug!( + "conn {} requested file-{}: {}", + conn_id, + file_idx, + file.name + ); let Some(handle) = &file.handle else { log::error!( @@ -409,7 +436,7 @@ impl ClipboardContext { log::debug!("parsing file descriptors"); // this must be a file descriptor format data - let files = FileDescription::parse_file_descriptors(format_data.into(), conn_id)?; + let files = FileDescription::parse_file_descriptors(format_data, conn_id)?; let paths = { let mut fuse_guard = self.fuse_server.lock(); @@ -425,7 +452,10 @@ impl ClipboardContext { ClipboardFile::FileContentsResponse { .. } => { log::debug!("server_file_contents_response called"); // we don't know its corresponding request, no resend can be performed - self.fuse_server.lock().serve(msg)?; + self.fuse_tx.send(msg).map_err(|e| { + log::error!("failed to send file contents response to fuse: {:?}", e); + CliprdrError::ClipboardInternalError + })?; Ok(()) } ClipboardFile::FileContentsRequest { @@ -466,9 +496,8 @@ impl ClipboardContext { fn send_file_list(&self, conn_id: i32) -> Result<(), CliprdrError> { let file_list = self.clipboard.get_file_list()?; - let paths = file_list.into_iter().map(|lf| lf.path).collect(); - send_file_list(paths, conn_id) + send_file_list(&file_list, conn_id) } } @@ -518,21 +547,24 @@ fn send_format_list(conn_id: i32) -> Result<(), CliprdrError> { Ok(()) } -fn send_file_list(paths: Vec, conn_id: i32) -> Result<(), CliprdrError> { - log::debug!( - "send file list to remote, conn={}, list={:?}", - conn_id, - paths - ); - let files = construct_file_list(paths.as_slice())?; - +fn build_file_list_pdu(files: &[LocalFile]) -> Vec { let mut data = BytesMut::with_capacity(4 + 592 * files.len()); - data.put_u32_le(paths.len() as u32); + data.put_u32_le(files.len() as u32); for file in files.iter() { data.put(file.as_bin().as_slice()); } - let format_data = data.to_vec(); + data.to_vec() +} + +fn send_file_list(files: &[LocalFile], conn_id: i32) -> Result<(), CliprdrError> { + log::debug!( + "send file list to remote, conn={}, list={:?}", + conn_id, + files.iter().map(|f| f.path.display()).collect::>() + ); + + let format_data = build_file_list_pdu(files); send_data( conn_id, diff --git a/libs/clipboard/src/platform/linux/x11.rs b/libs/clipboard/src/platform/linux/x11.rs index b263bf7b5..d2543d31d 100644 --- a/libs/clipboard/src/platform/linux/x11.rs +++ b/libs/clipboard/src/platform/linux/x11.rs @@ -96,10 +96,10 @@ impl SysClipboard for X11Clipboard { fn set_file_list(&self, paths: &[PathBuf]) -> Result<(), CliprdrError> { *self.former_file_list.lock() = paths.to_vec(); - let uri_list: Vec = paths.iter().map(|pb| encode_path_to_uri(pb)).collect(); + let uri_list: Vec = paths.iter().map(encode_path_to_uri).collect(); let uri_list = uri_list.join("\n"); let text_uri_list_data = uri_list.as_bytes().to_vec(); - let gnome_copied_files_data = vec!["copy\n".as_bytes(), uri_list.as_bytes()].concat(); + let gnome_copied_files_data = ["copy\n".as_bytes(), uri_list.as_bytes()].concat(); let batch = vec![ (self.text_uri_list, text_uri_list_data), (self.gnome_copied_files, gnome_copied_files_data),