From e6612112b2055e96e0fd6705766ef8e7532d4587 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Wed, 31 Oct 2018 14:31:38 -0400 Subject: [PATCH 1/3] Prepare to support setattr on deleted files In order to implement unlink and rmdir, which will come later, we must prepare to deal with the case of applying updates to a delete node via still-open handles. To do this, avoid doing a getattr immediately after a setattr (because it may not be possible once the underlying file is gone), and instead compute what the updated attributes should be. This is not completely accurate because of ctime, but it's good enough for now. Also, make the input path to setattr optional so that we can ask this function to only update the in-memory properties. --- Cargo.toml | 2 +- src/lib.rs | 7 ++ src/nodes/conv.rs | 20 ++++++ src/nodes/dir.rs | 12 ++-- src/nodes/file.rs | 4 +- src/nodes/mod.rs | 165 ++++++++++++++++++++++++++++++++----------- src/nodes/symlink.rs | 4 +- 7 files changed, 165 insertions(+), 49 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aa1379d..556239a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ rev = "4b23d3962c1baff6483caabf1ca15068216a7506" [dependencies.nix] # TODO(jmmv): Replace this with 0.12 once released. git = "https://github.com/nix-rust/nix.git" -rev = "eef3a432d57e8f830e05fede6e3099dcb689aa6b" +rev = "8c3e43ccd4fc83583c16848a35410022f5a8efc9" [dev-dependencies] tempdir = "0.3" diff --git a/src/lib.rs b/src/lib.rs index e5bccc2..c1d31e7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,13 @@ // License for the specific language governing permissions and limitations // under the License. +// For portability reasons, we need to be able to cast integer values to system-level opaque +// types such as "mode_t". Because we don't know the size of those integers on the platform we +// are building for, sometimes the casts do widen the values but other times they are no-ops. +#![cfg_attr(feature = "cargo-clippy", allow(identity_conversion))] + +// We construct complex structures in multiple places, and allowing for redundant field names +// increases readability. #![cfg_attr(feature = "cargo-clippy", allow(redundant_field_names))] #[macro_use] extern crate failure; diff --git a/src/nodes/conv.rs b/src/nodes/conv.rs index cc6bd34..cbbabed 100644 --- a/src/nodes/conv.rs +++ b/src/nodes/conv.rs @@ -60,6 +60,18 @@ pub fn timespec_to_timeval(spec: Timespec) -> sys::time::TimeVal { TimeVal::seconds(spec.sec) + TimeVal::nanoseconds(spec.nsec.into()) } +/// Converts a `sys::time::TimeVal` object into a `time::Timespec`. +// TODO(jmmv): Consider upstreaming this function as a TimeVal method. +pub fn timeval_to_timespec(val: sys::time::TimeVal) -> Timespec { + let usec = if val.tv_usec() > sys::time::suseconds_t::from(std::i32::MAX) { + warn!("Cannot represent too-long usec quantity {} in timespec; using 0", val.tv_usec()); + 0 + } else { + val.tv_usec() as i32 + }; + Timespec::new(val.tv_sec() as sys::time::time_t, usec) +} + /// Converts a file type as returned by the file system to a FUSE file type. /// /// `path` is the file from which the file type was originally extracted and is only for debugging @@ -206,6 +218,14 @@ mod tests { assert_eq!(45, val.tv_usec()); } + #[test] + fn test_timeval_to_timespec() { + let val = sys::time::TimeVal::seconds(654) + sys::time::TimeVal::nanoseconds(123_456); + let spec = timeval_to_timespec(val); + assert_eq!(654, spec.sec); + assert_eq!(123, spec.nsec); + } + #[test] fn test_system_time_to_timespec_ok() { let sys_time = SystemTime::UNIX_EPOCH + Duration::new(12345, 6789); diff --git a/src/nodes/dir.rs b/src/nodes/dir.rs index f0a1edf..5fb3fe6 100644 --- a/src/nodes/dir.rs +++ b/src/nodes/dir.rs @@ -496,10 +496,14 @@ impl Node for Dir { fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - if let Some(path) = &state.underlying_path { - setattr(path, &state.attr, delta)?; - } - Dir::getattr_locked(self.inode, &mut state) + + // setattr only gets called on writable nodes, which must have an underlying path. This + // assertion will go away when we have the ability to delete nodes as those may still + // exist in memory but have no corresponding underlying path. + debug_assert!(state.underlying_path.is_some()); + + state.attr = setattr(state.underlying_path.as_ref(), &state.attr, delta)?; + Ok(state.attr) } fn symlink(&self, name: &OsStr, link: &Path, ids: &IdGenerator, cache: &Cache) diff --git a/src/nodes/file.rs b/src/nodes/file.rs index 8901ede..9b19f0d 100644 --- a/src/nodes/file.rs +++ b/src/nodes/file.rs @@ -135,7 +135,7 @@ impl Node for File { fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - setattr(&state.underlying_path, &state.attr, delta)?; - File::getattr_locked(self.inode, &mut state) + state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?; + Ok(state.attr) } } diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 6526f4f..127119a 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -88,57 +88,142 @@ pub struct AttrDelta { /// Generic result type for of all node operations. pub type NodeResult = Result; -/// Updates the metadata of a file given a delta of attributes. -/// -/// This is a helper function to implement `Node::setattr` for the various node types. The caller -/// is responsible for obtaining the updated metadata of the underlying file to return it to the -/// kernel, as computing the updated metadata here would be inaccurate. The reason is that we don't -/// track ctimes ourselves so any modifications to the file cause the backing ctime to be updated -/// and we need to obtain it. -/// TODO(https://github.com/bazelbuild/sandboxfs/issues/43): Compute the modified fuse::FileAttr and -/// return it once we track ctimes. -/// -/// This tries to apply as many properties as possible in case of errors. When errors occur, -/// returns the first that was encountered. -pub fn setattr(path: &Path, attr: &fuse::FileAttr, delta: &AttrDelta) -> Result<(), nix::Error> { - let mut result = Ok(()); +/// Applies an operation to a path if present, or otherwise returns Ok. +fn try_path nix::Result<()>>(path: Option<&PathBuf>, op: O) -> nix::Result<()> { + match path { + Some(path) => op(path), + None => Ok(()), + } +} - if let Some(mode) = delta.mode { - result = result.and( - sys::stat::fchmodat(None, path, mode, sys::stat::FchmodatFlags::NoFollowSymlink)); +/// Helper function for `setattr` to apply only the mode changes. +fn setattr_mode(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, mode: Option) + -> Result<(), nix::Error> { + if mode.is_none() { + return Ok(()) } + let mode = mode.unwrap(); - if delta.uid.is_some() || delta.gid.is_some() { - result = result.and( - unistd::fchownat(None, path, delta.uid, delta.gid, - unistd::FchownatFlags::NoFollowSymlink)); + if mode.bits() > sys::stat::mode_t::from(std::u16::MAX) { + warn!("Got setattr with mode {:?} for {:?} (inode {}), which is too large; ignoring", + mode, path, attr.ino); + return Err(nix::Error::from_errno(Errno::EIO)); } + let perm = mode.bits() as u16; - if delta.atime.is_some() || delta.mtime.is_some() { - let atime = delta.atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); - let mtime = delta.mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); - if attr.kind == fuse::FileType::Symlink { - // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is - // not available on macOS El Capitan, which we currently require for CI builds. - warn!("Asked to update atime/mtime for symlink {} but following symlink instead", - path.display()); - } - result = result.and(sys::stat::utimes(path, &atime, &mtime)); + let result = try_path(path, |p| + sys::stat::fchmodat(None, p, mode, sys::stat::FchmodatFlags::NoFollowSymlink)); + if result.is_ok() { + attr.perm = perm; } + result +} - if let Some(size) = delta.size { +/// Helper function for `setattr` to apply only the UID and GID changes. +fn setattr_owners(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, uid: Option, + gid: Option) -> Result<(), nix::Error> { + if uid.is_none() && gid.is_none() { + return Ok(()) + } + + let result = try_path(path, |p| + unistd::fchownat(None, p, uid, gid, unistd::FchownatFlags::NoFollowSymlink)); + if result.is_ok() { + attr.uid = uid.map_or(attr.uid, u32::from); + attr.gid = uid.map_or(attr.gid, u32::from); + } + result +} + +/// Helper function for `setattr` to apply only the atime and mtime changes. +fn setattr_times(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, + atime: Option, mtime: Option) + -> Result<(), nix::Error> { + if atime.is_none() && mtime.is_none() { + return Ok(()); + } + + let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); + let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); + if attr.kind == fuse::FileType::Symlink { + // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is + // not available on macOS El Capitan, which we currently require for CI builds. + warn!("Asked to update atime/mtime for symlink {:?} but following symlink instead", path); + } + let result = try_path(path, |p| sys::stat::utimes(p, &atime, &mtime)); + if result.is_ok() { + attr.atime = conv::timeval_to_timespec(atime); + attr.mtime = conv::timeval_to_timespec(mtime); + } + result +} + +/// Helper function for `setattr` to apply only the size changes. +fn setattr_size(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, size: Option) + -> Result<(), nix::Error> { + if size.is_none() { + return Ok(()); + } + let size = size.unwrap(); + + let result = if size > ::std::i64::MAX as u64 { + warn!("truncate request got size {}, which is too large (exceeds i64's MAX)", size); + Err(nix::Error::invalid_argument()) + } else { + try_path(path, |p| unistd::truncate(p, size as i64)) + }; + if result.is_ok() { + attr.size = size; + } + result +} + +/// Updates the metadata of a file given a delta of attributes. +/// +/// This is a helper function to implement `Node::setattr` for the various node types. +/// +/// This tries to apply as many properties as possible in case of errors. When errors occur, +/// returns the first that was encountered. +pub fn setattr(path: Option<&PathBuf>, attr: &fuse::FileAttr, delta: &AttrDelta) + -> Result { + // Compute the potential new ctime for these updates. We want to avoid picking a ctime that is + // larger than what the operations below can result in (so as to prevent a future getattr from + // moving the ctime back) which is tricky because we don't know the time resolution of the + // underlying file system. Some, like HFS+, only have 1-second resolution... so pick that in + // the worst case. + // + // This is not perfectly accurate because we don't actually know what ctime the underlying file + // has gotten and thus a future getattr on it will cause the ctime to change. But as long as we + // avoid going back on time, we can afford to do this -- which is a requirement for supporting + // setting attributes on deleted files. + // + // TODO(https://github.com/bazelbuild/sandboxfs/issues/43): Revisit this when we track + // ctimes purely on our own. + let updated_ctime = { + let mut now = time::now().to_timespec(); + now.nsec = 0; + if attr.ctime > now { + attr.ctime + } else { + now + } + }; + + let mut new_attr = *attr; + // Intentionally using and() instead of and_then() to try to apply all changes but to only + // keep the first error result. + let result = Ok(()) + .and(setattr_mode(&mut new_attr, path, delta.mode)) + .and(setattr_owners(&mut new_attr, path, delta.uid, delta.gid)) + .and(setattr_times(&mut new_attr, path, delta.atime, delta.mtime)) // Updating the size only makes sense on files, but handling it here is much simpler than // doing so on a node type basis. Plus, who knows, if the kernel asked us to change the // size of anything other than a file, maybe we have to obey and try to do it. - if size > ::std::i64::MAX as u64 { - warn!("truncate request got size {}, which is too large (exceeds i64's MAX)", size); - result = result.and(Err(nix::Error::invalid_argument())); - } else { - result = result.and(unistd::truncate(path, size as i64)); - } + .and(setattr_size(&mut new_attr, path, delta.size)); + if *attr != new_attr { + new_attr.ctime = updated_ctime; } - - result + result.and(Ok(new_attr)) } /// Abstract representation of an open file handle. diff --git a/src/nodes/symlink.rs b/src/nodes/symlink.rs index 19d77e5..0e58eae 100644 --- a/src/nodes/symlink.rs +++ b/src/nodes/symlink.rs @@ -99,7 +99,7 @@ impl Node for Symlink { fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - setattr(&state.underlying_path, &state.attr, delta)?; - Symlink::getattr_locked(self.inode, &mut state) + state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?; + Ok(state.attr) } } From ec035e0cf217ca22c90b16266a4f671f0c5af767 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Wed, 31 Oct 2018 16:24:41 -0400 Subject: [PATCH 2/3] Implement rmdir and unlink To support operations on deleted nodes, this changes File and Symlink so that their underlying path is optional (Dir already had this). When None we treat these nodes as deleted, which means that some operations become impossible on them and others, like setattr, only have affect in-memory node data. --- admin/travis-build.sh | 6 ------ src/lib.rs | 28 +++++++++++++++++++------ src/nodes/dir.rs | 48 +++++++++++++++++++++++++++++++++++-------- src/nodes/file.rs | 34 ++++++++++++++++++++---------- src/nodes/mod.rs | 16 +++++++++++++++ src/nodes/symlink.rs | 34 ++++++++++++++++++++---------- 6 files changed, 123 insertions(+), 43 deletions(-) diff --git a/admin/travis-build.sh b/admin/travis-build.sh index d1a03a0..5e86176 100755 --- a/admin/travis-build.sh +++ b/admin/travis-build.sh @@ -91,16 +91,10 @@ do_rust() { TestProfiling_FileProfiles TestProfiling_BadConfiguration TestReadOnly_Attributes - TestReadWrite_Remove TestReadWrite_InodeReassignedAfterRecreation - TestReadWrite_FstatOnDeletedNode - TestReadWrite_FtruncateOnDeletedFile TestReadWrite_NestedMappingsInheritDirectoryProperties TestReadWrite_RenameFile TestReadWrite_MoveFile - TestReadWrite_FchmodOnDeletedNode - TestReadWrite_FchownOnDeletedNode - TestReadWrite_FutimesOnDeletedNode TestReconfiguration_Streams TestReconfiguration_Steps TestReconfiguration_Unmap diff --git a/src/lib.rs b/src/lib.rs index c1d31e7..dd04f9c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -489,9 +489,17 @@ impl fuse::Filesystem for SandboxFS { panic!("Required RW operation not yet implemented"); } - fn rmdir(&mut self, _req: &fuse::Request, _parent: u64, _name: &OsStr, - _reply: fuse::ReplyEmpty) { - panic!("Required RW operation not yet implemented"); + fn rmdir(&mut self, _req: &fuse::Request, parent: u64, name: &OsStr, reply: fuse::ReplyEmpty) { + let dir_node = self.find_node(parent); + if !dir_node.writable() { + reply.error(Errno::EPERM as i32); + return; + } + + match dir_node.rmdir(name) { + Ok(_) => reply.ok(), + Err(e) => reply.error(e.errno_as_i32()), + } } fn setattr(&mut self, _req: &fuse::Request, inode: u64, mode: Option, uid: Option, @@ -545,9 +553,17 @@ impl fuse::Filesystem for SandboxFS { } } - fn unlink(&mut self, _req: &fuse::Request, _parent: u64, _name: &OsStr, - _reply: fuse::ReplyEmpty) { - panic!("Required RW operation not yet implemented"); + fn unlink(&mut self, _req: &fuse::Request, parent: u64, name: &OsStr, reply: fuse::ReplyEmpty) { + let dir_node = self.find_node(parent); + if !dir_node.writable() { + reply.error(Errno::EPERM as i32); + return; + } + + match dir_node.unlink(name) { + Ok(_) => reply.ok(), + Err(e) => reply.error(e.errno_as_i32()), + } } fn write(&mut self, _req: &fuse::Request, _inode: u64, fh: u64, offset: i64, data: &[u8], diff --git a/src/nodes/dir.rs b/src/nodes/dir.rs index 5fb3fe6..8f37d08 100644 --- a/src/nodes/dir.rs +++ b/src/nodes/dir.rs @@ -254,12 +254,12 @@ impl Dir { if let Some(path) = &state.underlying_path { let fs_attr = fs::symlink_metadata(path)?; if !fs_attr.is_dir() { - warn!("Path {:?} backing a directory node is no longer a directory; got {:?}", - path, fs_attr.file_type()); + warn!("Path {} backing a directory node is no longer a directory; got {:?}", + path.display(), fs_attr.file_type()); return Err(KernelError::from_errno(errno::Errno::EIO)); } state.attr = conv::attr_fs_to_fuse(path, inode, &fs_attr); - }; + } Ok(state.attr) } @@ -348,6 +348,22 @@ impl Dir { } } } + + /// Common implementation for the `rmdir` and `unlink` operations. + /// + /// The behavior of these operations differs only in the syscall we invoke to delete the + /// underlying entry, which is passed in as the `remove` parameter. + fn remove_any(&self, name: &OsStr, remove: R) -> NodeResult<()> + where R: Fn(&PathBuf) -> io::Result<()> { + let mut state = self.state.lock().unwrap(); + let path = Dir::get_writable_path(&mut state, name)?; + + remove(&path)?; + + state.children[name].node.delete(); + state.children.remove(name); + Ok(()) + } } impl Node for Dir { @@ -363,6 +379,14 @@ impl Node for Dir { fuse::FileType::Directory } + fn delete(&self) { + let mut state = self.state.lock().unwrap(); + assert!( + state.underlying_path.is_some(), + "Delete already called or trying to delete an explicit mapping"); + state.underlying_path = None; + } + fn map(&self, components: &[Component], underlying_path: &Path, writable: bool, ids: &IdGenerator, cache: &Cache) -> Result<(), Error> { let (name, remainder) = split_components(components); @@ -494,14 +518,14 @@ impl Node for Dir { })) } + fn rmdir(&self, name: &OsStr) -> NodeResult<()> { + // TODO(jmmv): Figure out how to remove the redundant closure. + #[cfg_attr(feature = "cargo-clippy", allow(redundant_closure))] + self.remove_any(name, |p| fs::remove_dir(p)) + } + fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - - // setattr only gets called on writable nodes, which must have an underlying path. This - // assertion will go away when we have the ability to delete nodes as those may still - // exist in memory but have no corresponding underlying path. - debug_assert!(state.underlying_path.is_some()); - state.attr = setattr(state.underlying_path.as_ref(), &state.attr, delta)?; Ok(state.attr) } @@ -515,4 +539,10 @@ impl Node for Dir { Dir::post_create_lookup(self.writable, &mut state, &path, name, fuse::FileType::Symlink, ids, cache) } + + fn unlink(&self, name: &OsStr) -> NodeResult<()> { + // TODO(jmmv): Figure out how to remove the redundant closure. + #[cfg_attr(feature = "cargo-clippy", allow(redundant_closure))] + self.remove_any(name, |p| fs::remove_file(p)) + } } diff --git a/src/nodes/file.rs b/src/nodes/file.rs index 9b19f0d..deb1676 100644 --- a/src/nodes/file.rs +++ b/src/nodes/file.rs @@ -58,7 +58,7 @@ pub struct File { /// Holds the mutable data of a file node. struct MutableFile { - underlying_path: PathBuf, + underlying_path: Option, attr: fuse::FileAttr, } @@ -85,8 +85,8 @@ impl File { let attr = conv::attr_fs_to_fuse(underlying_path, inode, &fs_attr); let state = MutableFile { - underlying_path: PathBuf::from(underlying_path), - attr, + underlying_path: Some(PathBuf::from(underlying_path)), + attr: attr, }; Arc::new(File { inode, writable, state: Mutex::from(state) }) @@ -94,13 +94,15 @@ impl File { /// Same as `getattr` but with the node already locked. fn getattr_locked(inode: u64, state: &mut MutableFile) -> NodeResult { - let fs_attr = fs::symlink_metadata(&state.underlying_path)?; - if !File::supports_type(fs_attr.file_type()) { - warn!("Path {:?} backing a file node is no longer a file; got {:?}", - &state.underlying_path, fs_attr.file_type()); - return Err(KernelError::from_errno(errno::Errno::EIO)); + if let Some(path) = &state.underlying_path { + let fs_attr = fs::symlink_metadata(path)?; + if !File::supports_type(fs_attr.file_type()) { + warn!("Path {} backing a file node is no longer a file; got {:?}", + path.display(), fs_attr.file_type()); + return Err(KernelError::from_errno(errno::Errno::EIO)); + } + state.attr = conv::attr_fs_to_fuse(path, inode, &fs_attr); } - state.attr = conv::attr_fs_to_fuse(&state.underlying_path, inode, &fs_attr); Ok(state.attr) } @@ -120,6 +122,14 @@ impl Node for File { state.attr.kind } + fn delete(&self) { + let mut state = self.state.lock().unwrap(); + assert!( + state.underlying_path.is_some(), + "Delete already called or trying to delete an explicit mapping"); + state.underlying_path = None; + } + fn getattr(&self) -> NodeResult { let mut state = self.state.lock().unwrap(); File::getattr_locked(self.inode, &mut state) @@ -129,13 +139,15 @@ impl Node for File { let state = self.state.lock().unwrap(); let options = conv::flags_to_openoptions(flags, self.writable)?; - let file = options.open(&state.underlying_path)?; + let path = state.underlying_path.as_ref().expect( + "Don't know how to handle a request to reopen a deleted file"); + let file = options.open(path)?; Ok(Arc::from(file)) } fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?; + state.attr = setattr(state.underlying_path.as_ref(), &state.attr, delta)?; Ok(state.attr) } } diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 127119a..1264dcb 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -289,6 +289,12 @@ pub trait Node { /// `Symlink`s (on which we don't allow type changes at all), it's OK. fn file_type_cached(&self) -> fuse::FileType; + /// Marks the node as deleted (though the in-memory representation remains). + /// + /// The implication of this is that a node loses its backing underlying path once this method + /// is called. + fn delete(&self); + /// Maps a path onto a node and creates intermediate components as immutable directories. /// /// `_components` is the path to map, broken down into components, and relative to the current @@ -365,6 +371,11 @@ pub trait Node { panic!("Not implemented"); } + /// Deletes the empty directory `_name`. + fn rmdir(&self, _name: &OsStr) -> NodeResult<()> { + panic!("Not implemented"); + } + /// Sets one or more properties of the node's metadata. fn setattr(&self, _delta: &AttrDelta) -> NodeResult; @@ -379,4 +390,9 @@ pub trait Node { -> NodeResult<(Arc, fuse::FileAttr)> { panic!("Not implemented") } + + /// Deletes the file `_name`. + fn unlink(&self, _name: &OsStr) -> NodeResult<()> { + panic!("Not implemented"); + } } diff --git a/src/nodes/symlink.rs b/src/nodes/symlink.rs index 0e58eae..199c91d 100644 --- a/src/nodes/symlink.rs +++ b/src/nodes/symlink.rs @@ -30,7 +30,7 @@ pub struct Symlink { /// Holds the mutable data of a symlink node. struct MutableSymlink { - underlying_path: PathBuf, + underlying_path: Option, attr: fuse::FileAttr, } @@ -52,8 +52,8 @@ impl Symlink { let attr = conv::attr_fs_to_fuse(underlying_path, inode, &fs_attr); let state = MutableSymlink { - underlying_path: PathBuf::from(underlying_path), - attr, + underlying_path: Some(PathBuf::from(underlying_path)), + attr: attr, }; Arc::new(Symlink { inode, writable, state: Mutex::from(state) }) @@ -61,13 +61,15 @@ impl Symlink { /// Same as `getattr` but with the node already locked. fn getattr_locked(inode: u64, state: &mut MutableSymlink) -> NodeResult { - let fs_attr = fs::symlink_metadata(&state.underlying_path)?; - if !fs_attr.file_type().is_symlink() { - warn!("Path {:?} backing a symlink node is no longer a symlink; got {:?}", - &state.underlying_path, fs_attr.file_type()); - return Err(KernelError::from_errno(errno::Errno::EIO)); + if let Some(path) = &state.underlying_path { + let fs_attr = fs::symlink_metadata(path)?; + if !fs_attr.file_type().is_symlink() { + warn!("Path {} backing a symlink node is no longer a symlink; got {:?}", + path.display(), fs_attr.file_type()); + return Err(KernelError::from_errno(errno::Errno::EIO)); + } + state.attr = conv::attr_fs_to_fuse(path, inode, &fs_attr); } - state.attr = conv::attr_fs_to_fuse(&state.underlying_path, inode, &fs_attr); Ok(state.attr) } @@ -86,6 +88,14 @@ impl Node for Symlink { fuse::FileType::Symlink } + fn delete(&self) { + let mut state = self.state.lock().unwrap(); + assert!( + state.underlying_path.is_some(), + "Delete already called or trying to delete an explicit mapping"); + state.underlying_path = None; + } + fn getattr(&self) -> NodeResult { let mut state = self.state.lock().unwrap(); Symlink::getattr_locked(self.inode, &mut state) @@ -94,12 +104,14 @@ impl Node for Symlink { fn readlink(&self) -> NodeResult { let state = self.state.lock().unwrap(); - Ok(fs::read_link(&state.underlying_path)?) + let path = state.underlying_path.as_ref().expect( + "There is no known API to get the target of a deleted symlink"); + Ok(fs::read_link(path)?) } fn setattr(&self, delta: &AttrDelta) -> NodeResult { let mut state = self.state.lock().unwrap(); - state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?; + state.attr = setattr(state.underlying_path.as_ref(), &state.attr, delta)?; Ok(state.attr) } } From 03e6bd9f6b4bad563b63dca7ede6720a9af9a0bb Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Mon, 5 Nov 2018 06:21:50 -0500 Subject: [PATCH 3/3] Return EOPNOTSUPP when trying to setattr the times of a symlink The setattr hook should be using futimensat to update the timestamps of the node so that we could support the case of modifying those of a symlink (without following it). Unfortunately, we currently cannot do so because our testing environment does not have this system call available in all platforms... so just return EOPNOTSUPP for now (instead of doing the wrong thing and following the symlink). --- WORKSPACE | 2 +- integration/read_write_test.go | 58 +++++++++++----------------------- internal/sandbox/node.go | 7 ++++ src/nodes/mod.rs | 11 ++++--- 4 files changed, 32 insertions(+), 46 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 77f1c9e..8187155 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -57,7 +57,7 @@ go_repository( go_repository( name = "org_golang_x_sys", importpath = "golang.org/x/sys", - commit = "4b45465282a4624cf39876842a017334f13b8aff", + commit = "9b800f95dbbc54abff0acf7ee32d88ba4e328c89", ) gazelle_dependencies() diff --git a/integration/read_write_test.go b/integration/read_write_test.go index ca7d743..f1c064f 100644 --- a/integration/read_write_test.go +++ b/integration/read_write_test.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "golang.org/x/sys/unix" + "github.com/bazelbuild/sandboxfs/integration/utils" "github.com/bazelbuild/sandboxfs/internal/sandbox" ) @@ -565,20 +567,11 @@ func TestReadWrite_Chmod(t *testing.T) { } }) - t.Run("DanglingSymlink", func(t *testing.T) { - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) - - path := state.MountPath("dangling-symlink") - if err := os.Chmod(path, 0555); err == nil { - t.Errorf("Want chmod to fail on dangling link, got success") - } - }) - - t.Run("GoodSymlink", func(t *testing.T) { + t.Run("Symlink", func(t *testing.T) { utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) + utils.MustSymlink(t, "target", state.RootPath("symlink")) - path := state.MountPath("good-symlink") + path := state.MountPath("symlink") linkFileInfo, err := os.Lstat(path) if err != nil { t.Fatalf("Failed to stat %s: %v", path, err) @@ -588,11 +581,11 @@ func TestReadWrite_Chmod(t *testing.T) { t.Fatalf("Failed to chmod %s: %v", path, err) } - if err := checkPerm("good-symlink", linkFileInfo.Mode()&os.ModePerm); err != nil { + if err := checkPerm("symlink", linkFileInfo.Mode()&os.ModePerm); err != nil { t.Error(err) } if err := checkPerm("target", 0200); err != nil { - t.Error(err) + t.Errorf("Mode of symlink target was modified but shouldn't have been: %v", err) } }) } @@ -664,9 +657,8 @@ func TestReadWrite_Chown(t *testing.T) { utils.MustMkdirAll(t, state.RootPath("dir"), 0755) utils.MustWriteFile(t, state.RootPath("file"), 0644, "new content") - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) + utils.MustSymlink(t, "target", state.RootPath("symlink")) targetFileInfo, err := os.Lstat(state.RootPath("target")) if err != nil { @@ -683,8 +675,7 @@ func TestReadWrite_Chown(t *testing.T) { }{ {"Dir", "dir", 1, 2}, {"File", "file", 3, 4}, - {"DanglingSymlink", "dangling-symlink", 5, 6}, - {"GoodSymlink", "good-symlink", 7, 8}, + {"Symlink", "symlink", 7, 8}, } for _, d := range testData { t.Run(d.name, func(t *testing.T) { @@ -821,35 +812,22 @@ func TestReadWrite_Chtimes(t *testing.T) { } }) - t.Run("DanglingSymlink", func(t *testing.T) { - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) - - if _, err := chtimes("dangling-symlink", time.Unix(0, 0), time.Unix(0, 0)); err == nil { - t.Errorf("Want chtimes to fail on dangling link, got success") - } - }) - - t.Run("GoodSymlink", func(t *testing.T) { + t.Run("Symlink", func(t *testing.T) { utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) - path := state.MountPath("good-symlink") + utils.MustSymlink(t, "target", state.RootPath("symlink")) + path := state.MountPath("symlink") - linkFileInfo, err := os.Lstat(path) + atimeTimespec, err := unix.TimeToTimespec(someAtime) if err != nil { - t.Fatalf("Failed to stat %s: %v", path, err) + t.Fatalf("Failed to convert %v to a timespec: %v", someAtime, err) } - linkStat := linkFileInfo.Sys().(*syscall.Stat_t) - - wantMinCtime, err := chtimes(path, someAtime, someMtime) + mtimeTimespec, err := unix.TimeToTimespec(someMtime) if err != nil { - t.Fatal(err) + t.Fatalf("Failed to convert %v to a timespec: %v", someMtime, err) } - if err := checkTimes("good-symlink", time.Unix(0, 0), linkFileInfo.ModTime(), sandbox.Ctime(linkStat)); err != nil { - t.Error(err) - } - if err := checkTimes("target", someAtime, someMtime, wantMinCtime); err != nil { - t.Error(err) + if err = unix.UtimesNanoAt(unix.AT_FDCWD, path, []unix.Timespec{atimeTimespec, mtimeTimespec}, unix.AT_SYMLINK_NOFOLLOW); err == nil || err != syscall.EOPNOTSUPP { + t.Fatalf("Expected EOPNOTSUPP changing the times of a symlink; got %v", err) } }) } diff --git a/internal/sandbox/node.go b/internal/sandbox/node.go index ed80fdb..0b28617 100644 --- a/internal/sandbox/node.go +++ b/internal/sandbox/node.go @@ -342,6 +342,13 @@ func (n *BaseNode) setattrTimes(req *fuse.SetattrRequest) error { } if underlyingPath, isMapped := n.UnderlyingPath(); isMapped && !n.deleted { + if n.attr.Mode&os.ModeType == os.ModeSymlink { + // TODO(https://github.com/bazelbuild/sandboxfs/issues/46): Should use + // futimensat to support changing the times of a symlink if requested to + // do so. + return fuse.Errno(syscall.EOPNOTSUPP) + } + if err := os.Chtimes(underlyingPath, atime, mtime); err != nil { return err } diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 1264dcb..565c97d 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -143,13 +143,14 @@ fn setattr_times(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, return Ok(()); } - let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); - let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); if attr.kind == fuse::FileType::Symlink { - // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is - // not available on macOS El Capitan, which we currently require for CI builds. - warn!("Asked to update atime/mtime for symlink {:?} but following symlink instead", path); + // TODO(https://github.com/bazelbuild/sandboxfs/issues/46): Should use futimensat to support + // changing the times of a symlink if requested to do so. + return Err(nix::Error::from_errno(Errno::EOPNOTSUPP)); } + + let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); + let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); let result = try_path(path, |p| sys::stat::utimes(p, &atime, &mtime)); if result.is_ok() { attr.atime = conv::timeval_to_timespec(atime);