diff --git a/src/api/server.rs b/src/api/server.rs index c690597c0..daff41b4b 100644 --- a/src/api/server.rs +++ b/src/api/server.rs @@ -571,7 +571,7 @@ impl Server { }; w.write_all(out.as_slice()).map_err(Error::EncodeMessage)?; - w.commit(&[&data_writer.0]).map_err(Error::IoError)?; + w.commit(&[&data_writer.0]).map_err(Error::EncodeMessage)?; Ok(out.len as usize) } Err(e) => reply_error(e, in_header.unique, w), @@ -976,7 +976,7 @@ impl Server { }; w.write_all(out.as_slice()).map_err(Error::EncodeMessage)?; - w.commit(&[&cursor]).map_err(Error::IoError)?; + w.commit(&[&cursor]).map_err(Error::EncodeMessage)?; Ok(out.len as usize) } } @@ -1394,10 +1394,23 @@ fn reply_error(err: io::Error, unique: u64, mut w: Writer) -> Result { .map_err(Error::EncodeMessage) } -fn bytes_to_cstr(buf: &[u8]) -> Result<&CStr> { - // Convert to a `CStr` first so that we can drop the '\0' byte at the end - // and make sure there are no interior '\0' bytes. - CStr::from_bytes_with_nul(buf).map_err(Error::InvalidCString) +/// trim all trailing nul terminators. +pub fn bytes_to_cstr(buf: &[u8]) -> Result<&CStr> { + // There might be multiple 0s at the end of buf, find & use the first one and trim other zeros. + let len = buf.len(); + let mut n: usize = 0; + for c in buf.iter() { + if c == &0 { + break; + } + n += 1; + } + let nul_pos = if n + 1 >= len { len } else { n + 1 }; + let newbuf = &buf[0..nul_pos]; + + // Convert to a `CStr` so that we can drop the '\0' byte at the end and make sure there are no + // interior '\0' bytes. + CStr::from_bytes_with_nul(newbuf).map_err(Error::InvalidCString) } fn add_dirent( diff --git a/src/api/vfs.rs b/src/api/vfs.rs index 1c616e5ed..b59ed6ec5 100644 --- a/src/api/vfs.rs +++ b/src/api/vfs.rs @@ -431,7 +431,7 @@ impl FileSystem for Vfs { ) -> Result<(libc::stat64, Duration)> { match self.get_real_rootfs(inode)? { (Left(fs), idata) => fs.getattr(ctx, idata.ino, handle), - (Right(fs), idata) => fs.getattr(ctx, idata.ino, handle.map(|h| h)), + (Right(fs), idata) => fs.getattr(ctx, idata.ino, handle), } } @@ -445,7 +445,7 @@ impl FileSystem for Vfs { ) -> Result<(libc::stat64, Duration)> { match self.get_real_rootfs(inode)? { (Left(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle, valid), - (Right(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle.map(|h| h), valid), + (Right(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle, valid), } } @@ -587,7 +587,7 @@ impl FileSystem for Vfs { fs.create(ctx, idata.ino, name, mode, flags, umask) .map(|(mut a, b, c)| { a.inode = self.hash_inode(idata.super_index, a.inode)?; - Ok((a, b.map(|h| h), c)) + Ok((a, b, c)) })? } } diff --git a/src/lib.rs b/src/lib.rs index 96dedc124..23780517d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,8 +75,6 @@ pub enum Error { /// The `size` field of the `SetxattrIn` message does not match the length /// of the decoded value. InvalidXattrSize((u32, usize)), - /// An IO related error has happened. - IoError(io::Error), } impl error::Error for Error {} @@ -96,7 +94,6 @@ impl fmt::Display for Error { decoded value: size = {}, value.len() = {}", size, len ), - IoError(err) => write!(f, "fail to handle request: {}", err), } } } diff --git a/src/passthrough/fs.rs b/src/passthrough/fs.rs index 877a5f146..997353a5f 100644 --- a/src/passthrough/fs.rs +++ b/src/passthrough/fs.rs @@ -28,6 +28,7 @@ use crate::api::filesystem::{ Context, DirEntry, Entry, FileSystem, FsOptions, GetxattrReply, ListxattrReply, OpenOptions, SetattrValid, ZeroCopyReader, ZeroCopyWriter, }; +use crate::api::server; use crate::api::{BackendFileSystem, VFS_MAX_INO}; #[cfg(feature = "vhost-user-fs")] use crate::transport::FsCacheReqHandler; @@ -199,7 +200,7 @@ impl FromStr for CachePolicy { fn from_str(s: &str) -> Result { match s { - "never" | "Never" | "NEVER" => Ok(CachePolicy::Never), + "never" | "Never" | "NEVER" | "none" | "None" | "NONE" => Ok(CachePolicy::Never), "auto" | "Auto" | "AUTO" => Ok(CachePolicy::Auto), "always" | "Always" | "ALWAYS" => Ok(CachePolicy::Always), _ => Err("invalid cache policy"), @@ -561,6 +562,7 @@ impl PassthroughFs { } let mut rem = &buf[..]; + let orig_rem_len = rem.len(); while !rem.is_empty() { // We only use debug asserts here because these values are coming from the kernel and we // trust them implicitly. @@ -583,6 +585,20 @@ impl PassthroughFs { // break the loop so return `Ok` with a non-zero value instead. Ok(1) } else { + // The Sys_getdents64 in kernel will pad the name with '\0' + // bytes up to 8-byte alignment, so @name may contain a few null + // terminators. This causes an extra lookup from fuse when + // called by readdirplus, because kernel path walking only takes + // name without null terminators, the dentry with more than 1 + // null terminators added by readdirplus doesn't satisfy the + // path walking. + let name = server::bytes_to_cstr(name) + .map_err(|e| { + error!("do_readdir: {:?}", e); + io::Error::from_raw_os_error(libc::EINVAL) + })? + .to_bytes(); + add_entry(DirEntry { ino: dirent64.d_ino, offset: dirent64.d_off as u64, @@ -599,7 +615,12 @@ impl PassthroughFs { match res { Ok(0) => break, Ok(_) => rem = &rem[dirent64.d_reclen as usize..], - Err(e) => return Err(e), + // If there's an error, we can only signal it if we haven't + // stored any entries yet - otherwise we'd end up with wrong + // lookup counts for the entries that are already in the + // buffer. So we return what we've collected until that point. + Err(e) if rem.len() == orig_rem_len => return Err(e), + Err(_) => return Ok(()), } } @@ -902,13 +923,14 @@ impl FileSystem for PassthroughFs { add_entry: &mut dyn FnMut(DirEntry, Entry) -> io::Result, ) -> io::Result<()> { self.do_readdir(inode, handle, size, offset, &mut |dir_entry| { - // Safe because the kernel guarantees that the buffer is nul-terminated. Additionally, - // the kernel will pad the name with '\0' bytes up to 8-byte alignment and there's no - // way for us to know exactly how many padding bytes there are. This would cause - // `CStr::from_bytes_with_nul` to return an error because it would think there are - // interior '\0' bytes. We trust the kernel to provide us with properly formatted data - // so we'll just skip the checks here. - let name = unsafe { CStr::from_bytes_with_nul_unchecked(dir_entry.name) }; + // Safe because do_readdir() has ensured dir_entry.name is a + // valid [u8] generated by CStr::to_bytes(). + let name = unsafe { + CStr::from_bytes_with_nul_unchecked(std::slice::from_raw_parts( + &dir_entry.name[0], + dir_entry.name.len() + 1, + )) + }; let entry = self.do_lookup(inode, name)?; let ino = entry.inode; diff --git a/src/transport/fusedev/mod.rs b/src/transport/fusedev/mod.rs index c7f616ae8..f99a4d420 100644 --- a/src/transport/fusedev/mod.rs +++ b/src/transport/fusedev/mod.rs @@ -163,7 +163,9 @@ impl<'a> Writer<'a> { if !buf.is_empty() { return writev(self.fd, buf.as_slice()).map_err(|e| { error! {"fail to write to fuse device on commit: {}", e}; - io::Error::new(io::ErrorKind::Other, format!("{}", e)) + e.as_errno() + .map(|r| io::Error::from_raw_os_error(r as i32)) + .unwrap_or_else(|| io::Error::new(io::ErrorKind::Other, format!("{}", e))) }); } Ok(0)