From e1bd10a4a2e5e04756b286e1d5d819f4320e7069 Mon Sep 17 00:00:00 2001 From: John Millikin Date: Sat, 15 Aug 2020 12:46:47 +0900 Subject: [PATCH] Stabilize `FuseHandlers::listxattr()` issue: https://github.com/jmillikin/rust-fuse/issues/9 --- fuse/Cargo.toml | 1 - fuse/src/fuse_handlers.rs | 2 - fuse/src/fuse_server.rs | 1 - fuse/src/protocol/getxattr/getxattr.rs | 12 +- fuse/src/protocol/getxattr/getxattr_test.rs | 11 +- fuse/src/protocol/listxattr/listxattr.rs | 215 +++++++++++++----- fuse/src/protocol/listxattr/listxattr_test.rs | 177 ++++++++------ fuse/src/protocol/mod.rs | 2 +- fuse/src/protocol/readdir/readdir.rs | 6 +- 9 files changed, 280 insertions(+), 147 deletions(-) diff --git a/fuse/Cargo.toml b/fuse/Cargo.toml index 8b3e618f..e997ea6b 100644 --- a/fuse/Cargo.toml +++ b/fuse/Cargo.toml @@ -19,7 +19,6 @@ unstable_fuse_fsyncdir = [] unstable_fuse_getlk = [] unstable_fuse_ioctl = [] unstable_fuse_link = [] -unstable_fuse_listxattr = [] unstable_fuse_lseek = [] unstable_fuse_mkdir = [] unstable_fuse_mknod = [] diff --git a/fuse/src/fuse_handlers.rs b/fuse/src/fuse_handlers.rs index 68d105e7..b091972a 100644 --- a/fuse/src/fuse_handlers.rs +++ b/fuse/src/fuse_handlers.rs @@ -196,8 +196,6 @@ pub trait FuseHandlers { respond.err(errors::ENOSYS); } - #[cfg(any(doc, feature = "unstable_fuse_listxattr"))] - #[cfg_attr(doc, doc(cfg(feature = "unstable_fuse_listxattr")))] fn listxattr( &self, ctx: server::ServerContext, diff --git a/fuse/src/fuse_server.rs b/fuse/src/fuse_server.rs index 369f8b2d..f3f00fc1 100644 --- a/fuse/src/fuse_server.rs +++ b/fuse/src/fuse_server.rs @@ -314,7 +314,6 @@ fn fuse_request_dispatch( fuse_kernel::FUSE_IOCTL => do_dispatch!(ioctl), #[cfg(feature = "unstable_fuse_link")] fuse_kernel::FUSE_LINK => do_dispatch!(link), - #[cfg(feature = "unstable_fuse_listxattr")] fuse_kernel::FUSE_LISTXATTR => do_dispatch!(listxattr), fuse_kernel::FUSE_LOOKUP => do_dispatch!(lookup), #[cfg(feature = "unstable_fuse_lseek")] diff --git a/fuse/src/protocol/getxattr/getxattr.rs b/fuse/src/protocol/getxattr/getxattr.rs index 90dd683c..90340551 100644 --- a/fuse/src/protocol/getxattr/getxattr.rs +++ b/fuse/src/protocol/getxattr/getxattr.rs @@ -84,9 +84,7 @@ pub struct GetxattrResponse<'a> { } impl<'a> GetxattrResponse<'a> { - pub fn for_request_size( - request_size: Option, - ) -> Self { + pub fn new(request_size: Option) -> Self { Self { request_size, raw: Default::default(), @@ -106,10 +104,8 @@ impl<'a> GetxattrResponse<'a> { self.try_set_value(value).unwrap() } - pub fn try_set_value( - &mut self, - value: &'a [u8], - ) -> Option<()> { // TODO: Result + pub fn try_set_value(&mut self, value: &'a [u8]) -> Option<()> { + // TODO: Result if value.len() > crate::XATTR_SIZE_MAX { return None; // ERANGE } @@ -124,7 +120,7 @@ impl<'a> GetxattrResponse<'a> { return None; // ERANGE } self.value = value; - } + }, } return Some(()); } diff --git a/fuse/src/protocol/getxattr/getxattr_test.rs b/fuse/src/protocol/getxattr/getxattr_test.rs index 53e0f874..872b6a5b 100644 --- a/fuse/src/protocol/getxattr/getxattr_test.rs +++ b/fuse/src/protocol/getxattr/getxattr_test.rs @@ -84,7 +84,7 @@ fn request_impl_debug() { #[test] fn response_sized() { let request_size = num::NonZeroU32::new(10); - let mut resp = GetxattrResponse::for_request_size(request_size); + let mut resp = GetxattrResponse::new(request_size); assert_eq!(resp.request_size(), request_size); // value must fit in kernel buffer @@ -109,11 +109,14 @@ fn response_sized() { #[test] fn response_unsized() { - let mut resp = GetxattrResponse::for_request_size(None); + let mut resp = GetxattrResponse::new(None); assert_eq!(resp.request_size(), None); // set_value() doesn't allow value sizes larger than XATTR_SIZE_MAX - assert!(resp.try_set_value(&[0; crate::XATTR_SIZE_MAX + 1]).is_none()); + assert!( + resp.try_set_value(&[0; crate::XATTR_SIZE_MAX + 1]) + .is_none() + ); assert!(resp.value.is_empty()); assert_eq!(resp.raw.size, 0); @@ -144,7 +147,7 @@ fn response_unsized() { #[test] fn response_impl_debug() { let request_size = num::NonZeroU32::new(10); - let mut response = GetxattrResponse::for_request_size(request_size); + let mut response = GetxattrResponse::new(request_size); response.set_value(b"some\x00bytes"); assert_eq!( diff --git a/fuse/src/protocol/listxattr/listxattr.rs b/fuse/src/protocol/listxattr/listxattr.rs index 198a7e8b..58cada2d 100644 --- a/fuse/src/protocol/listxattr/listxattr.rs +++ b/fuse/src/protocol/listxattr/listxattr.rs @@ -21,21 +21,31 @@ mod listxattr_test; // ListxattrRequest {{{ +/// Request type for [`FuseHandlers::listxattr`]. +/// +/// [`FuseHandlers::listxattr`]: ../trait.FuseHandlers.html#method.listxattr pub struct ListxattrRequest<'a> { - header: &'a fuse_kernel::fuse_in_header, - size: u32, + phantom: PhantomData<&'a ()>, + node_id: NodeId, + size: Option, } impl ListxattrRequest<'_> { - pub fn node_id(&self) -> u64 { - self.header.nodeid + pub fn node_id(&self) -> NodeId { + self.node_id } - pub fn size(&self) -> Option { - if self.size == 0 { - None - } else { - Some(self.size) - } + + pub fn size(&self) -> Option { + self.size + } +} + +impl fmt::Debug for ListxattrRequest<'_> { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + fmt.debug_struct("ListxattrRequest") + .field("node_id", &self.node_id) + .field("size", &format_args!("{:?}", &self.size)) + .finish() } } @@ -48,8 +58,9 @@ impl<'a> fuse_io::DecodeRequest<'a> for ListxattrRequest<'a> { let raw: &fuse_kernel::fuse_getxattr_in = dec.next_sized()?; Ok(Self { - header, - size: raw.size, + phantom: PhantomData, + node_id: try_node_id(header.nodeid)?, + size: num::NonZeroU32::new(raw.size), }) } } @@ -58,71 +69,153 @@ impl<'a> fuse_io::DecodeRequest<'a> for ListxattrRequest<'a> { // ListxattrResponse {{{ +/// Response type for [`FuseHandlers::listxattr`]. +/// +/// [`FuseHandlers::listxattr`]: ../trait.FuseHandlers.html#method.listxattr pub struct ListxattrResponse<'a> { - phantom: PhantomData<&'a ()>, - request_size: u32, + request_size: Option, raw: fuse_kernel::fuse_getxattr_out, - buf: Vec, + buf: ListxattrBuf<'a>, } impl ListxattrResponse<'_> { - // TODO: fix construction API - pub fn new(request: &ListxattrRequest) -> Self { - // Clamp the maximum response size to avoid caring about u32 overflow. - // This limit is far higher than existing kernel implementations support. - ListxattrResponse { - phantom: PhantomData, - request_size: cmp::min(request.size, 1 << 30), + pub fn new(request_size: Option) -> Self { + Self { + request_size, + raw: Default::default(), + buf: ListxattrBuf::Owned { cap: Vec::new() }, + } + } +} + +impl<'a> ListxattrResponse<'a> { + pub fn with_capacity( + request_size: Option, + capacity: &'a mut [u8], + ) -> Self { + Self { + request_size, raw: Default::default(), - buf: Vec::new(), + buf: ListxattrBuf::Borrowed { + cap: capacity, + size: 0, + }, } } +} - pub fn set_size(&mut self, size: u32) { - self.raw.size = size; +impl ListxattrResponse<'_> { + pub fn request_size(&self) -> Option { + self.request_size } - pub fn push<'a>(&mut self, name: &'a CStr) -> io::Result<()> { - let bytes = name.to_bytes_with_nul(); - if bytes.len() > u32::MAX as usize { - return Err(io::Error::from_raw_os_error( - errors::ERANGE.get() as i32 - )); - } - let bytes_len = bytes.len() as u32; + pub fn add_name(&mut self, name: &XattrName) { + self.try_add_name(name).unwrap() + } - let old_buf_size: u32; - if self.request_size == 0 { - old_buf_size = self.raw.size; - } else { - old_buf_size = self.buf.len() as u32; - } + pub fn try_add_name(&mut self, name: &XattrName) -> Option<()> { + use crate::XATTR_LIST_MAX; + + let name = name.as_bytes(); + let name_len = name.len() as usize; + let name_buf_len = name_len + 1; // includes NUL terminator - let new_buf_size = match old_buf_size.checked_add(bytes_len) { - Some(x) => Ok(x), + let mut request_size = match self.request_size { None => { - Err(io::Error::from_raw_os_error(errors::ERANGE.get() as i32)) + // Don't actually copy any bytes around, just keep track of the + // response size. + if self.raw.size as usize + name_buf_len > XATTR_LIST_MAX { + return None; + } + self.raw.size += name_buf_len as u32; + return Some(()); }, - }?; + Some(x) => x.get() as usize, + }; - if self.request_size == 0 { - self.raw.size = new_buf_size; - return Ok(()); + if request_size > XATTR_LIST_MAX { + request_size = XATTR_LIST_MAX; } - if new_buf_size > self.request_size { - return Err(io::Error::from_raw_os_error( - errors::ERANGE.get() as i32 - )); - } - self.buf.extend_from_slice(bytes); - Ok(()) + let name_buf = match &mut self.buf { + ListxattrBuf::Borrowed { + cap, + size: size_ref, + } => { + let current_size = *size_ref; + let new_size = current_size + name_buf_len; + if new_size > cap.len() || new_size > request_size { + return None; + } + let (_, remaining_cap) = cap.split_at_mut(current_size); + let (name_buf, _) = remaining_cap.split_at_mut(name_buf_len); + *size_ref = new_size; + name_buf + }, + ListxattrBuf::Owned { cap } => { + let current_size = cap.len(); + let new_size = current_size + name_buf_len; + if new_size > request_size { + return None; + } + cap.resize(new_size, 0u8); + let (_, entry_buf) = cap.split_at_mut(current_size); + entry_buf + }, + }; + + debug_assert!(name_buf.len() == name_buf_len); + + let (name_no_nul, name_nul) = name_buf.split_at_mut(name_len); + name_no_nul.copy_from_slice(name); + name_nul[0] = 0; + Some(()) } } +enum ListxattrBuf<'a> { + Owned { cap: Vec }, + Borrowed { cap: &'a mut [u8], size: usize }, +} + impl fmt::Debug for ListxattrResponse<'_> { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - fmt.debug_struct("ListxattrResponse").finish() + let mut out = fmt.debug_struct("ListxattrResponse"); + out.field("request_size", &format_args!("{:?}", &self.request_size)); + match self.request_size { + None => { + out.field("size", &self.raw.size); + let names: &[u8] = &[]; + out.field("names", &names); + }, + Some(_) => match &self.buf { + ListxattrBuf::Owned { cap } => { + out.field("size", &cap.len()); + out.field("names", &DebugListxattrNames(&cap)); + }, + ListxattrBuf::Borrowed { cap, size } => { + let (bytes, _) = cap.split_at(*size); + out.field("size", size); + out.field("names", &DebugListxattrNames(&bytes)); + }, + }, + } + + out.finish() + } +} + +struct DebugListxattrNames<'a>(&'a [u8]); + +impl fmt::Debug for DebugListxattrNames<'_> { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + let mut out = fmt.debug_list(); + for chunk in self.0.split(|&b| b == 0) { + if chunk.len() > 0 { + out.entry(&DebugBytesAsString(chunk)); + } + } + out.finish() } } @@ -131,10 +224,16 @@ impl fuse_io::EncodeResponse for ListxattrResponse<'_> { &'a self, enc: fuse_io::ResponseEncoder, ) -> std::io::Result<()> { - if self.raw.size != 0 { - enc.encode_sized(&self.raw) - } else { - enc.encode_bytes(&self.buf) + if self.request_size.is_none() { + return enc.encode_sized(&self.raw); + } + + match &self.buf { + ListxattrBuf::Owned { cap } => enc.encode_bytes(&cap), + ListxattrBuf::Borrowed { cap, size } => { + let (bytes, _) = cap.split_at(*size); + enc.encode_bytes(bytes) + }, } } } diff --git a/fuse/src/protocol/listxattr/listxattr_test.rs b/fuse/src/protocol/listxattr/listxattr_test.rs index 4fa1d843..a7c6097f 100644 --- a/fuse/src/protocol/listxattr/listxattr_test.rs +++ b/fuse/src/protocol/listxattr/listxattr_test.rs @@ -22,7 +22,10 @@ use super::{ListxattrRequest, ListxattrResponse}; #[test] fn request_sized() { let buf = MessageBuilder::new() - .set_opcode(fuse_kernel::FUSE_LISTXATTR) + .set_header(|h| { + h.opcode = fuse_kernel::FUSE_LISTXATTR; + h.nodeid = 123; + }) .push_sized(&fuse_kernel::fuse_getxattr_in { size: 10, ..Default::default() @@ -31,13 +34,16 @@ fn request_sized() { let req: ListxattrRequest = decode_request!(buf); - assert_eq!(req.size(), Some(10)); + assert_eq!(req.size(), Some(num::NonZeroU32::new(10).unwrap())); } #[test] fn request_unsized() { let buf = MessageBuilder::new() - .set_opcode(fuse_kernel::FUSE_LISTXATTR) + .set_header(|h| { + h.opcode = fuse_kernel::FUSE_LISTXATTR; + h.nodeid = 123; + }) .push_sized(&fuse_kernel::fuse_getxattr_in { size: 0, ..Default::default() @@ -50,38 +56,51 @@ fn request_unsized() { } #[test] -fn response_sized() { - let mut resp: ListxattrResponse<'static> = todo!(); - /* - let mut resp = ListxattrRequest { - header: &HEADER, - size: 10, - }.new_response(); - */ - assert_eq!(resp.request_size, 10); - - // value must fit in kernel buffer - { - let cstring = CString::new("12345678901").unwrap(); - let err = resp.push(&cstring).unwrap_err(); - assert_eq!(err.raw_os_error().unwrap(), errors::ERANGE.get() as i32); +fn request_impl_debug() { + let request = &ListxattrRequest { + phantom: PhantomData, + node_id: crate::ROOT_ID, + size: num::NonZeroU32::new(11), + }; - assert!(resp.buf.is_empty()); - assert_eq!(resp.raw.size, 0); - } + assert_eq!( + format!("{:#?}", request), + concat!( + "ListxattrRequest {\n", + " node_id: 1,\n", + " size: Some(11),\n", + "}", + ), + ); +} - // pushes append null-terminated xattr names - { - let cstring = CString::new("123").unwrap(); - resp.push(&cstring).unwrap(); - assert_eq!(resp.buf, vec![49, 50, 51, 0]); - } +#[test] +fn response_sized_heap() { + let request_size = num::NonZeroU32::new(10); + let mut resp = ListxattrResponse::new(request_size); + response_sized_test_impl(&mut resp); +} + +#[test] +fn response_sized_stack() { + let request_size = num::NonZeroU32::new(10); + let mut buf = [0u8; 1024]; + let mut resp = ListxattrResponse::with_capacity(request_size, &mut buf); + response_sized_test_impl(&mut resp); +} + +fn response_sized_test_impl(resp: &mut ListxattrResponse) { + assert_eq!(resp.request_size(), num::NonZeroU32::new(10)); + + // response must fit in kernel buffer { - let cstring = CString::new("456").unwrap(); - resp.push(&cstring).unwrap(); - assert_eq!(resp.buf, vec![49, 50, 51, 0, 52, 53, 54, 0]); + let name = XattrName::from_bytes(b"12345678901").unwrap(); + assert!(resp.try_add_name(name).is_none()); } - assert_eq!(resp.raw.size, 0); + + // xattr names are NUL-terminated + resp.add_name(XattrName::from_bytes(b"123").unwrap()); + resp.add_name(XattrName::from_bytes(b"456").unwrap()); let encoded = encode_response!(resp); @@ -100,32 +119,15 @@ fn response_sized() { #[test] fn response_unsized() { - let mut resp: ListxattrResponse<'static> = todo!(); - /* - let mut resp = ListxattrRequest { - header: &HEADER, - size: 0, - }.new_response(); - */ - assert_eq!(resp.request_size, 0); + let mut resp = ListxattrResponse::new(None); + assert_eq!(resp.request_size(), None); // set_value() doesn't store value bytes for unsized responses - { - let cstring = CString::new("123").unwrap(); - resp.push(&cstring).unwrap(); - assert!(resp.buf.is_empty()); - assert_eq!(resp.raw.size, 4); - } - { - let cstring = CString::new("456").unwrap(); - resp.push(&cstring).unwrap(); - assert!(resp.buf.is_empty()); - assert_eq!(resp.raw.size, 8); - } + resp.add_name(XattrName::from_bytes(b"123").unwrap()); + assert_eq!(resp.raw.size, 4); - // size can also be set directly - resp.set_size(30); - assert_eq!(resp.raw.size, 30); + resp.add_name(XattrName::from_bytes(b"456").unwrap()); + assert_eq!(resp.raw.size, 8); let encoded = encode_response!(resp); @@ -139,7 +141,7 @@ fn response_unsized() { unique: 0, }) .push_sized(&fuse_kernel::fuse_getxattr_out { - size: 30, + size: 8, padding: 0, }) .build() @@ -147,18 +149,55 @@ fn response_unsized() { } #[test] -fn response_detect_overflow() { - let mut resp: ListxattrResponse<'static> = todo!(); - /* - let mut resp = ListxattrRequest { - header: &HEADER, - size: 10, - }.new_response(); - */ - let big_buf = - unsafe { slice::from_raw_parts(0 as *const u8, u32::MAX as usize + 1) }; - let big_cstr = unsafe { CStr::from_bytes_with_nul_unchecked(big_buf) }; - - let err = resp.push(&big_cstr).unwrap_err(); - assert_eq!(err.raw_os_error().unwrap(), errors::ERANGE.get() as i32); +fn response_size_limit() { + // listxattr response size can't exceed XATTR_LIST_MAX + let mut resp = ListxattrResponse::new(None); + let name = XattrName::from_bytes(&[b'a'; 250]).unwrap(); + for _ in 0..261 { + resp.add_name(name); + } + assert_eq!(resp.raw.size, 65511); + assert!(resp.try_add_name(name).is_none()); +} + +#[test] +fn response_sized_impl_debug() { + let request_size = num::NonZeroU32::new(10); + let mut response = ListxattrResponse::new(request_size); + + response.add_name(XattrName::from_bytes(b"123").unwrap()); + response.add_name(XattrName::from_bytes(b"456").unwrap()); + + assert_eq!( + format!("{:#?}", response), + concat!( + "ListxattrResponse {\n", + " request_size: Some(10),\n", + " size: 8,\n", + " names: [\n", + " \"123\",\n", + " \"456\",\n", + " ],\n", + "}", + ), + ); +} + +#[test] +fn response_unsized_impl_debug() { + let mut response = ListxattrResponse::new(None); + + response.add_name(XattrName::from_bytes(b"123").unwrap()); + response.add_name(XattrName::from_bytes(b"456").unwrap()); + + assert_eq!( + format!("{:#?}", response), + concat!( + "ListxattrResponse {\n", + " request_size: None,\n", + " size: 8,\n", + " names: [],\n", + "}", + ), + ); } diff --git a/fuse/src/protocol/mod.rs b/fuse/src/protocol/mod.rs index bdb7f379..ba689ab2 100644 --- a/fuse/src/protocol/mod.rs +++ b/fuse/src/protocol/mod.rs @@ -52,7 +52,7 @@ op_mod!(getlk, "getlk/getlk.rs", "unstable_fuse_getlk"); op_mod!(getxattr, "getxattr/getxattr.rs"); op_mod!(ioctl, "ioctl/ioctl.rs", "unstable_fuse_ioctl"); op_mod!(link, "link/link.rs", "unstable_fuse_link"); -op_mod!(listxattr, "listxattr/listxattr.rs", "unstable_fuse_listxattr"); +op_mod!(listxattr, "listxattr/listxattr.rs"); op_mod!(lookup, "lookup/lookup.rs"); op_mod!(lseek, "lseek/lseek.rs", "unstable_fuse_lseek"); op_mod!(mkdir, "mkdir/mkdir.rs", "unstable_fuse_mkdir"); diff --git a/fuse/src/protocol/readdir/readdir.rs b/fuse/src/protocol/readdir/readdir.rs index 3ced666d..724b2a32 100644 --- a/fuse/src/protocol/readdir/readdir.rs +++ b/fuse/src/protocol/readdir/readdir.rs @@ -260,16 +260,16 @@ impl ReaddirBuf<'_> { let entry_buf = match self { ReaddirBuf::Borrowed { cap, - size: size_mut, + size: size_ref, } => { - let current_size: usize = *size_mut; + let current_size: usize = *size_ref; let new_size = current_size.checked_add(entry_size)?; if new_size > cap.len() { return None; } let (_, remaining_cap) = cap.split_at_mut(current_size); let (entry_buf, _) = remaining_cap.split_at_mut(entry_size); - *size_mut = new_size; + *size_ref = new_size; entry_buf },