From 151550242bbabca94d27481d7972fd3230ec2258 Mon Sep 17 00:00:00 2001 From: Surma Date: Fri, 23 Oct 2026 00:49:46 +0100 Subject: [PATCH] sausage-vfs: review fixes (fallible growth, symlink flags, stream cancel, sized reads, timestamps, error kinds, linker shadowing) API: Layer::stat(path, follow), Layer::readlink, Open::follow, Stat::symlink and Entry::symlink. Process::idle documents the shutdown order: abort, await the task, await idle, release locks. --- vfs/Cargo.toml | 1 + vfs/src/adapter.rs | 93 +++++++++++++----- vfs/src/generated.rs | 13 ++- vfs/src/host.rs | 90 ++++++++++++++--- vfs/src/lib.rs | 16 +++- vfs/src/memory.rs | 133 +++++++++++++++++++------- vfs/src/mounts.rs | 21 +++- vfs/src/process.rs | 5 +- vfs/src/streams.rs | 58 ++++++++++- vfs/tests/guests/files/src/main.rs | 123 ++++++++++++++++++++++++ vfs/tests/layers.proptest-regressions | 7 ++ vfs/tests/layers.rs | 51 ++++++++-- vfs/tests/regressions.rs | 92 ++++++++++++++++++ vfs/tests/wasi.rs | 67 ++++++++++++- 14 files changed, 673 insertions(+), 97 deletions(-) create mode 100644 vfs/tests/layers.proptest-regressions create mode 100644 vfs/tests/regressions.rs diff --git a/vfs/Cargo.toml b/vfs/Cargo.toml index 7aea8d6..27cdc51 100644 --- a/vfs/Cargo.toml +++ b/vfs/Cargo.toml @@ -7,6 +7,7 @@ description = "A composable virtual filesystem for WASI guests in Wasmtime." [dependencies] async-trait = "0.1" cap-std = "4" +cap-fs-ext = "4" wasmtime = "49" wasmtime-wasi = "49" bytes = "1" diff --git a/vfs/src/adapter.rs b/vfs/src/adapter.rs index 1fdce88..8bfdc69 100644 --- a/vfs/src/adapter.rs +++ b/vfs/src/adapter.rs @@ -3,6 +3,8 @@ //! Directory descriptors retain their absolute virtual path after a directory rename. //! Relative operations through those descriptors use the old path, not the moved folder. //! Reopen the moved folder before further relative operations. +//! Object comparisons and metadata hashes use paths, not inode identity (review F12). +//! Hard links remain unsupported, including tempfile persist_noclobber. use crate::streams::{Reader, Writer}; use crate::*; use wasmtime::component::{HasData, Linker, Resource, ResourceTable}; @@ -43,8 +45,12 @@ pub fn add_to_linker( view: fn(&mut T) -> View<'_>, ) -> wasmtime::Result<()> { linker.allow_shadowing(true); - types::add_to_linker::(linker, view)?; - preopens::add_to_linker::(linker, view)?; + let result = (|| { + types::add_to_linker::(linker, view)?; + preopens::add_to_linker::(linker, view) + })(); + linker.allow_shadowing(false); + result?; Ok(()) } #[derive(Clone)] @@ -63,6 +69,8 @@ fn code(e: &io::Error) -> ErrorCode { ReadOnlyFilesystem => ErrorCode::ReadOnly, CrossesDevices => ErrorCode::CrossDevice, Unsupported => ErrorCode::Unsupported, + FileTooLarge => ErrorCode::FileTooLarge, + ResourceBusy => ErrorCode::Busy, _ => ErrorCode::from(e), } } @@ -78,7 +86,9 @@ fn stat(s: Stat) -> types::DescriptorStat { }) } types::DescriptorStat { - type_: if s.directory { + type_: if s.symlink { + types::DescriptorType::SymbolicLink + } else if s.directory { types::DescriptorType::Directory } else { types::DescriptorType::RegularFile @@ -90,12 +100,19 @@ fn stat(s: Stat) -> types::DescriptorStat { status_change_timestamp: None, } } -fn time(t: types::NewTimestamp) -> Option { +fn time(t: types::NewTimestamp) -> FsResult> { match t { - types::NewTimestamp::NoChange => None, - types::NewTimestamp::Now => Some(SystemTime::now()), + types::NewTimestamp::NoChange => Ok(None), + types::NewTimestamp::Now => Ok(Some(SystemTime::now())), types::NewTimestamp::Timestamp(t) => { - Some(SystemTime::UNIX_EPOCH + std::time::Duration::new(t.seconds, t.nanoseconds)) + let duration = std::time::Duration::from_secs(t.seconds) + .checked_add(std::time::Duration::from_nanos(t.nanoseconds as u64)) + .ok_or(ErrorCode::Overflow)?; + Ok(Some( + SystemTime::UNIX_EPOCH + .checked_add(duration) + .ok_or(ErrorCode::Overflow)?, + )) } } } @@ -140,6 +157,7 @@ impl File for Root { async fn stat(&self) -> Result { Ok(Stat { directory: true, + symlink: false, size: 0, accessed: None, modified: None, @@ -211,7 +229,7 @@ impl HostDescriptor for View<'_> { ) -> FsResult<()> { self.fd(&fd)? .file - .set_times(time(atim), time(mtim)) + .set_times(time(atim)?, time(mtim)?) .await .map_err(errno) } @@ -257,7 +275,9 @@ impl HostDescriptor for View<'_> { .into_iter() .map(|e| DirectoryEntry { name: e.name, - type_: if e.directory { + type_: if e.symlink { + types::DescriptorType::SymbolicLink + } else if e.directory { types::DescriptorType::Directory } else { types::DescriptorType::RegularFile @@ -288,9 +308,11 @@ impl HostDescriptor for View<'_> { path_flags: types::PathFlags, path: String, ) -> FsResult { - let _ = path_flags; + let follow = path_flags.contains(types::PathFlags::SYMLINK_FOLLOW); let p = self.path(&fd, &path)?; - Ok(stat(self.adapter.mounts.stat(&p).await.map_err(errno)?)) + Ok(stat( + self.adapter.mounts.stat(&p, follow).await.map_err(errno)?, + )) } async fn set_times_at( &mut self, @@ -300,20 +322,14 @@ impl HostDescriptor for View<'_> { atim: types::NewTimestamp, mtim: types::NewTimestamp, ) -> FsResult<()> { - let _ = path_flags; + let follow = path_flags.contains(types::PathFlags::SYMLINK_FOLLOW); let p = self.path(&fd, &path)?; self.adapter .mounts - .open( - &p, - Open { - write: true, - ..Open::default() - }, - ) + .metadata_file(&p, follow) .await .map_err(errno)? - .set_times(time(atim), time(mtim)) + .set_times(time(atim)?, time(mtim)?) .await .map_err(errno) } @@ -336,7 +352,7 @@ impl HostDescriptor for View<'_> { oflags: OpenFlags, flags: DescriptorFlags, ) -> FsResult> { - let _ = path_flags; + let follow = path_flags.contains(types::PathFlags::SYMLINK_FOLLOW); let path = self.path(&fd, &path)?; let file = self .adapter @@ -344,6 +360,7 @@ impl HostDescriptor for View<'_> { .open( &path, Open { + follow, write: flags.contains(DescriptorFlags::WRITE), create: oflags.contains(OpenFlags::CREATE), exclusive: oflags.contains(OpenFlags::EXCLUSIVE), @@ -365,8 +382,8 @@ impl HostDescriptor for View<'_> { fd: Resource, path: String, ) -> FsResult { - let _ = (fd, path); - Err(ErrorCode::Unsupported.into()) + let p = self.path(&fd, &path)?; + self.adapter.mounts.readlink(&p).await.map_err(errno) } async fn remove_directory_at( &mut self, @@ -449,9 +466,9 @@ impl HostDescriptor for View<'_> { path_flags: types::PathFlags, path: String, ) -> FsResult { - let _ = path_flags; + let follow = path_flags.contains(types::PathFlags::SYMLINK_FOLLOW); let p = self.path(&fd, &path)?; - self.adapter.mounts.stat(&p).await.map_err(errno)?; + self.adapter.mounts.stat(&p, follow).await.map_err(errno)?; Ok(hash(&p)) } } @@ -472,3 +489,29 @@ impl HostDirectoryEntryStream for View<'_> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + /// Timestamp arithmetic returns overflow instead of panicking on either addition. + #[test] + fn timestamp_overflow() { + for nanoseconds in [0, u32::MAX] { + let error = time(types::NewTimestamp::Timestamp(types::Datetime { + seconds: u64::MAX, + nanoseconds, + })) + .unwrap_err(); + assert_eq!(error.downcast().unwrap(), ErrorCode::Overflow); + } + } + /// Portable memory errors preserve their WASI error codes. + #[test] + fn portable_error_codes() { + assert_eq!( + code(&error(io::ErrorKind::FileTooLarge)), + ErrorCode::FileTooLarge + ); + assert_eq!(code(&error(io::ErrorKind::ResourceBusy)), ErrorCode::Busy); + } +} diff --git a/vfs/src/generated.rs b/vfs/src/generated.rs index 66c3565..4fc569d 100644 --- a/vfs/src/generated.rs +++ b/vfs/src/generated.rs @@ -2,7 +2,8 @@ use crate::*; use std::{collections::BTreeMap, io::ErrorKind as E}; type Generator = Arc Result> + Send + Sync>; -/// A tree of generated files. Parent folders are implicit. +/// A static tree of generated files. Parent folders are implicit. +/// Generators must be cheap and must not block. Stat can invoke them as well as open. #[derive(Default)] pub struct Generated { files: BTreeMap, @@ -29,7 +30,7 @@ fn readonly() -> Result { } #[async_trait] impl Layer for Generated { - async fn stat(&self, path: &str) -> Result { + async fn stat(&self, path: &str, _follow: bool) -> Result { if let Some(f) = self.files.get(path) { return Snapshot(f()?).stat().await; } @@ -38,6 +39,7 @@ impl Layer for Generated { } Ok(Stat { directory: true, + symlink: false, size: 0, accessed: None, modified: None, @@ -67,7 +69,11 @@ impl Layer for Generated { } Ok(entries .into_iter() - .map(|(name, directory)| Entry { name, directory }) + .map(|(name, directory)| Entry { + name, + directory, + symlink: false, + }) .collect()) } async fn open(&self, path: &str, o: Open) -> Result> { @@ -103,6 +109,7 @@ impl File for Snapshot { async fn stat(&self) -> Result { Ok(Stat { directory: false, + symlink: false, size: self.0.len() as u64, accessed: None, modified: None, diff --git a/vfs/src/host.rs b/vfs/src/host.rs index 9a4d849..52186a6 100644 --- a/vfs/src/host.rs +++ b/vfs/src/host.rs @@ -1,11 +1,10 @@ //! Capability-sandboxed host files. Every system call runs on the blocking pool. +//! Writable files currently open a second append descriptor by path. A concurrent rename +//! between those opens can make append refer to a different file (review F9). use crate::*; -use cap_std::fs::{Dir, OpenOptions}; -use std::{ - io::{Read, Seek, SeekFrom, Write}, - path::Path, - sync::Mutex, -}; +use cap_fs_ext::{FollowSymlinks, OpenOptionsFollowExt}; +use cap_std::fs::{Dir, FileExt, OpenOptions}; +use std::{io::Write, path::Path, sync::Mutex}; /// A host directory bound to one process tracker. #[derive(Clone)] @@ -36,6 +35,7 @@ fn rel(path: &str) -> String { fn stat(m: cap_std::fs::Metadata) -> Stat { Stat { directory: m.is_dir(), + symlink: m.file_type().is_symlink(), size: m.len(), accessed: m.accessed().ok().map(|t| t.into_std()), modified: m.modified().ok().map(|t| t.into_std()), @@ -48,9 +48,24 @@ struct Handle { } #[async_trait] impl Layer for Host { - async fn stat(&self, path: &str) -> Result { + async fn stat(&self, path: &str, follow: bool) -> Result { let (d, p) = (self.dir.clone(), rel(path)); - self.process.blocking(move || d.metadata(p).map(stat)).await + self.process + .blocking(move || { + if follow { + d.metadata(p) + } else { + d.symlink_metadata(p) + } + .map(stat) + }) + .await + } + async fn readlink(&self, path: &str) -> Result { + let (d, p) = (self.dir.clone(), rel(path)); + self.process + .blocking(move || d.read_link(p).map(|p| p.to_string_lossy().into_owned())) + .await } async fn list(&self, path: &str) -> Result> { let (d, p) = (self.dir.clone(), rel(path)); @@ -63,6 +78,7 @@ impl Layer for Host { Ok(Entry { name: e.file_name().to_string_lossy().into_owned(), directory: e.file_type()?.is_dir(), + symlink: e.file_type()?.is_symlink(), }) }) .collect::>>()?; @@ -83,13 +99,26 @@ impl Layer for Host { .create(o.create) .create_new(o.create && o.exclusive) .truncate(o.truncate); + options.follow(if o.follow { + FollowSymlinks::Yes + } else { + FollowSymlinks::No + }); let f = d.open_with(&p, &options)?; let directory = f.metadata()?.is_dir(); if o.directory && !directory { return Err(error(io::ErrorKind::NotADirectory)); } let append = if o.write && !directory { - d.open_with(&p, OpenOptions::new().append(true))? + { + let mut append = OpenOptions::new(); + append.append(true).follow(if o.follow { + FollowSymlinks::Yes + } else { + FollowSymlinks::No + }); + d.open_with(&p, &append)? + } } else { f.try_clone()? }; @@ -131,10 +160,14 @@ impl File for Handle { let f = self.file.clone(); self.process .blocking(move || { - let mut f = f.lock().unwrap(); - f.seek(SeekFrom::Start(offset))?; + let f = f.lock().unwrap(); let mut bytes = Vec::new(); - (&mut *f).take(length as u64).read_to_end(&mut bytes)?; + bytes + .try_reserve(length) + .map_err(|_| error(io::ErrorKind::FileTooLarge))?; + bytes.resize(length, 0); + let n = f.read_at(&mut bytes, offset)?; + bytes.truncate(n); Ok(bytes) }) .await @@ -143,9 +176,8 @@ impl File for Handle { let f = self.file.clone(); self.process .blocking(move || { - let mut f = f.lock().unwrap(); - f.seek(SeekFrom::Start(offset))?; - f.write(&bytes) + let f = f.lock().unwrap(); + f.write_at(&bytes, offset) }) .await } @@ -188,3 +220,31 @@ impl File for Handle { .await } } + +#[cfg(test)] +mod tests { + use super::*; + use std::io::{Seek, SeekFrom}; + /// Positional reads and writes do not change the descriptor's cursor. + #[tokio::test] + async fn positional_io_keeps_cursor() { + let temp = tempfile::tempdir().unwrap(); + std::fs::write(temp.path().join("file"), b"abcd").unwrap(); + let host = Host::new(temp.path(), Process::default()).unwrap(); + let file = host + .dir + .open_with("file", OpenOptions::new().read(true).write(true)) + .unwrap(); + let append = file.try_clone().unwrap(); + let file = Arc::new(Mutex::new(file)); + file.lock().unwrap().seek(SeekFrom::Start(3)).unwrap(); + let handle = Handle { + file: file.clone(), + append: Arc::new(Mutex::new(append)), + process: Process::default(), + }; + assert_eq!(handle.read(0, 1).await.unwrap(), b"a"); + handle.write(1, b"Z".to_vec()).await.unwrap(); + assert_eq!(file.lock().unwrap().stream_position().unwrap(), 3); + } +} diff --git a/vfs/src/lib.rs b/vfs/src/lib.rs index b9722cf..324a83c 100644 --- a/vfs/src/lib.rs +++ b/vfs/src/lib.rs @@ -23,6 +23,7 @@ pub type Result = io::Result; #[derive(Clone, Debug, PartialEq, Eq)] pub struct Stat { pub directory: bool, + pub symlink: bool, pub size: u64, pub accessed: Option, pub modified: Option, @@ -32,10 +33,13 @@ pub struct Stat { pub struct Entry { pub name: String, pub directory: bool, + pub symlink: bool, } /// Options for an open operation. Exclusive applies only with create. #[derive(Clone, Copy, Debug, Default)] pub struct Open { + /// Follow the final symbolic link when true. + pub follow: bool, pub write: bool, pub create: bool, pub exclusive: bool, @@ -61,7 +65,11 @@ pub trait File: Send + Sync { /// Implementations must retain open file identity and return portable IO errors. #[async_trait] pub trait Layer: Send + Sync { - async fn stat(&self, path: &str) -> Result; + async fn stat(&self, path: &str, follow: bool) -> Result; + /// Return the target of a symbolic link, or InvalidInput for another node. + async fn readlink(&self, _path: &str) -> Result { + Err(error(io::ErrorKind::InvalidInput)) + } async fn list(&self, path: &str) -> Result>; async fn open(&self, path: &str, options: Open) -> Result>; async fn mkdir(&self, path: &str) -> Result<()>; @@ -88,5 +96,9 @@ pub fn resolve(base: &str, path: &str) -> String { p => parts.push(p), } } - format!("/{}", parts.join("/")) + let mut result = format!("/{}", parts.join("/")); + if path.ends_with('/') && result != "/" { + result.push('/'); + } + result } diff --git a/vfs/src/memory.rs b/vfs/src/memory.rs index 1de68e7..ccc4908 100644 --- a/vfs/src/memory.rs +++ b/vfs/src/memory.rs @@ -4,9 +4,10 @@ use std::{collections::BTreeMap, io::ErrorKind as E, sync::Mutex}; #[derive(Clone)] enum Node { - Dir, + Dir(Arc>), File(Arc>), } +#[derive(Default)] struct Data { bytes: Vec, accessed: Option, @@ -20,7 +21,10 @@ pub struct Memory { impl Default for Memory { fn default() -> Self { Self { - nodes: Arc::new(Mutex::new(BTreeMap::from([("/".into(), Node::Dir)]))), + nodes: Arc::new(Mutex::new(BTreeMap::from([( + "/".into(), + Node::Dir(Arc::default()), + )]))), } } } @@ -29,25 +33,54 @@ fn parent(path: &str) -> &str { .map(|(p, _)| if p.is_empty() { "/" } else { p }) .unwrap_or("/") } +fn validate(nodes: &BTreeMap, path: &str) -> Result<()> { + let mut prefix = String::new(); + let parts: Vec<_> = path + .trim_end_matches('/') + .split('/') + .filter(|p| !p.is_empty()) + .collect(); + for part in parts.iter().take(parts.len().saturating_sub(1)) { + prefix.push('/'); + prefix.push_str(part); + directory(nodes, &prefix)?; + } + if path.ends_with('/') && path != "/" { + directory(nodes, path.trim_end_matches('/'))?; + } + Ok(()) +} +fn clean(path: &str) -> &str { + if path == "/" { + path + } else { + path.trim_end_matches('/') + } +} fn directory(nodes: &BTreeMap, path: &str) -> Result<()> { match nodes.get(path) { - Some(Node::Dir) => Ok(()), + Some(Node::Dir(_)) => Ok(()), Some(_) => Err(error(E::NotADirectory)), None => Err(error(E::NotFound)), } } fn metadata(node: &Node) -> Stat { match node { - Node::Dir => Stat { - directory: true, - size: 0, - accessed: None, - modified: None, - }, + Node::Dir(data) => { + let data = data.lock().unwrap(); + Stat { + directory: true, + symlink: false, + size: 0, + accessed: data.accessed, + modified: data.modified, + } + } Node::File(data) => { let d = data.lock().unwrap(); Stat { directory: false, + symlink: false, size: d.bytes.len() as u64, accessed: d.accessed, modified: d.modified, @@ -61,28 +94,34 @@ struct Handle { } #[async_trait] impl Layer for Memory { - async fn stat(&self, path: &str) -> Result { - self.nodes - .lock() - .unwrap() - .get(path) + async fn stat(&self, path: &str, follow: bool) -> Result { + let _ = follow; + let nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + nodes + .get(clean(path)) .map(metadata) .ok_or_else(|| error(E::NotFound)) } async fn list(&self, path: &str) -> Result> { let nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + let path = clean(path); directory(&nodes, path)?; Ok(nodes .iter() .filter(|(p, _)| p.as_str() != path && parent(p) == path) .map(|(p, n)| Entry { name: p.rsplit('/').next().unwrap().into(), - directory: matches!(n, Node::Dir), + directory: matches!(n, Node::Dir(_)), + symlink: false, }) .collect()) } async fn open(&self, path: &str, o: Open) -> Result> { let mut nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + let path = clean(path); if o.truncate && !o.write { return Err(error(E::InvalidInput)); } @@ -105,7 +144,7 @@ impl Layer for Memory { } let node = nodes[path].clone(); match &node { - Node::Dir if o.write || o.truncate => return Err(error(E::IsADirectory)), + Node::Dir(_) if o.write || o.truncate => return Err(error(E::IsADirectory)), Node::File(_) if o.directory => return Err(error(E::NotADirectory)), Node::File(d) if o.truncate => d.lock().unwrap().bytes.clear(), _ => {} @@ -117,18 +156,22 @@ impl Layer for Memory { } async fn mkdir(&self, path: &str) -> Result<()> { let mut nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + let path = clean(path); if nodes.contains_key(path) { return Err(error(E::AlreadyExists)); } directory(&nodes, parent(path))?; - nodes.insert(path.into(), Node::Dir); + nodes.insert(path.into(), Node::Dir(Arc::default())); Ok(()) } async fn remove_file(&self, path: &str) -> Result<()> { let mut nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + let path = clean(path); match nodes.get(path) { None => return Err(error(E::NotFound)), - Some(Node::Dir) => return Err(error(E::IsADirectory)), + Some(Node::Dir(_)) => return Err(error(E::IsADirectory)), _ => {} } nodes.remove(path); @@ -136,6 +179,8 @@ impl Layer for Memory { } async fn remove_dir(&self, path: &str) -> Result<()> { let mut nodes = self.nodes.lock().unwrap(); + validate(&nodes, path)?; + let path = clean(path); directory(&nodes, path)?; if path == "/" { return Err(error(E::ResourceBusy)); @@ -148,6 +193,10 @@ impl Layer for Memory { } async fn rename(&self, from: &str, to: &str) -> Result<()> { let mut nodes = self.nodes.lock().unwrap(); + validate(&nodes, from)?; + validate(&nodes, to)?; + let from = clean(from); + let to = clean(to); let node = nodes.get(from).cloned().ok_or_else(|| error(E::NotFound))?; directory(&nodes, parent(to))?; if from == to { @@ -161,9 +210,14 @@ impl Layer for Memory { } if let Some(target) = nodes.get(to) { match (&node, target) { - (Node::Dir, Node::File(_)) => return Err(error(E::NotADirectory)), - (Node::File(_), Node::Dir) => return Err(error(E::IsADirectory)), - (Node::Dir, Node::Dir) if nodes.keys().any(|p| p != to && parent(p) == to) => { + (Node::Dir(_), Node::File(_)) => return Err(error(E::NotADirectory)), + (Node::File(_), Node::Dir(_)) if from.starts_with(&format!("{to}/")) => { + return Err(error(E::DirectoryNotEmpty)); + } + (Node::File(_), Node::Dir(_)) => return Err(error(E::IsADirectory)), + (Node::Dir(_), Node::Dir(_)) + if nodes.keys().any(|p| p != to && parent(p) == to) => + { return Err(error(E::DirectoryNotEmpty)); } _ => {} @@ -210,31 +264,41 @@ impl File for Handle { } async fn write(&self, offset: u64, bytes: Vec) -> Result { let mut d = self.writable()?.lock().unwrap(); - let start = usize::try_from(offset).map_err(|_| error(E::InvalidInput))?; + let start = usize::try_from(offset).map_err(|_| error(E::FileTooLarge))?; let end = start .checked_add(bytes.len()) - .ok_or_else(|| error(E::InvalidInput))?; + .ok_or_else(|| error(E::FileTooLarge))?; if !bytes.is_empty() { if end > d.bytes.len() { + let additional = end - d.bytes.len(); + d.bytes + .try_reserve(additional) + .map_err(|_| error(E::FileTooLarge))?; d.bytes.resize(end, 0); } d.bytes[start..end].copy_from_slice(&bytes); + d.modified = Some(SystemTime::now()); } Ok(bytes.len()) } async fn append(&self, bytes: Vec) -> Result { - self.writable()? - .lock() - .unwrap() - .bytes - .extend_from_slice(&bytes); + let mut d = self.writable()?.lock().unwrap(); + d.bytes + .try_reserve(bytes.len()) + .map_err(|_| error(E::FileTooLarge))?; + d.bytes.extend_from_slice(&bytes); + d.modified = Some(SystemTime::now()); Ok(bytes.len()) } async fn set_size(&self, size: u64) -> Result<()> { - self.writable()?.lock().unwrap().bytes.resize( - usize::try_from(size).map_err(|_| error(E::InvalidInput))?, - 0, - ); + let mut d = self.writable()?.lock().unwrap(); + let size = usize::try_from(size).map_err(|_| error(E::FileTooLarge))?; + let additional = size.saturating_sub(d.bytes.len()); + d.bytes + .try_reserve(additional) + .map_err(|_| error(E::FileTooLarge))?; + d.bytes.resize(size, 0); + d.modified = Some(SystemTime::now()); Ok(()) } async fn sync(&self) -> Result<()> { @@ -245,7 +309,10 @@ impl File for Handle { accessed: Option, modified: Option, ) -> Result<()> { - let mut d = self.writable()?.lock().unwrap(); + let data = match &self.node { + Node::Dir(d) | Node::File(d) => d, + }; + let mut d = data.lock().unwrap(); if accessed.is_some() { d.accessed = accessed; } diff --git a/vfs/src/mounts.rs b/vfs/src/mounts.rs index 3c474b5..c31207c 100644 --- a/vfs/src/mounts.rs +++ b/vfs/src/mounts.rs @@ -16,6 +16,18 @@ impl Mounts { self.mounts .insert(resolve("/", path), Mount { layer, readonly }); } + pub(crate) async fn metadata_file(&self, path: &str, follow: bool) -> Result> { + let (m, p) = self.route(path, true)?; + m.layer + .open( + &p, + Open { + follow, + ..Open::default() + }, + ) + .await + } fn route(&self, path: &str, write: bool) -> Result<(&Mount, String)> { let path = resolve("/", path); let (base, m) = self @@ -37,9 +49,13 @@ impl Mounts { } #[async_trait] impl Layer for Mounts { - async fn stat(&self, path: &str) -> Result { + async fn stat(&self, path: &str, follow: bool) -> Result { let (m, p) = self.route(path, false)?; - m.layer.stat(&p).await + m.layer.stat(&p, follow).await + } + async fn readlink(&self, path: &str) -> Result { + let (m, p) = self.route(path, false)?; + m.layer.readlink(&p).await } async fn list(&self, path: &str) -> Result> { let path = resolve("/", path); @@ -59,6 +75,7 @@ impl Layer for Mounts { entries.push(Entry { name: name.into(), directory: true, + symlink: false, }); } } diff --git a/vfs/src/process.rs b/vfs/src/process.rs index c71e4d0..2cffbd8 100644 --- a/vfs/src/process.rs +++ b/vfs/src/process.rs @@ -36,7 +36,10 @@ impl Process { .await .map_err(std::io::Error::other)? } - /// Wait for all outstanding system calls. The caller must prevent new operations. + /// Wait for all outstanding system calls. + /// First abort the guest task. Then await its JoinHandle, or drop its store. + /// Only then await idle before releasing locks. Abort alone does not stop a running poll. + /// This ordering also prevents later memory writes, which need no blocking guard. pub async fn idle(&self) { loop { let notified = self.0.idle.notified(); diff --git a/vfs/src/streams.rs b/vfs/src/streams.rs index 8c2495b..6a051e5 100644 --- a/vfs/src/streams.rs +++ b/vfs/src/streams.rs @@ -26,18 +26,20 @@ impl Reader { error: None, } } - fn start(&mut self) { + fn start(&mut self, size: usize) { if self.pending.is_none() && self.data.is_empty() && !self.eof { let f = self.file.clone(); let offset = self.offset; - self.pending = Some(Box::pin(async move { f.read(offset, 65536).await })); + self.pending = Some(Box::pin( + async move { f.read(offset, size.min(65536)).await }, + )); } } } #[async_trait] impl Pollable for Reader { async fn ready(&mut self) { - self.start(); + self.start(4096); if let Some(task) = &mut self.pending { match task.await { Ok(data) => { @@ -53,6 +55,14 @@ impl Pollable for Reader { } #[async_trait] impl InputStream for Reader { + async fn blocking_read(&mut self, size: usize) -> StreamResult { + if size == 0 { + return Ok(Bytes::new()); + } + self.start(size); + self.ready().await; + self.read(size) + } fn read(&mut self, size: usize) -> StreamResult { if let Some(e) = self.error.take() { return Err(failure(e)); @@ -62,7 +72,9 @@ impl InputStream for Reader { } let n = size.min(self.data.len()); let data = self.data.split_to(n); - self.start(); + if data.is_empty() && size != 0 { + self.start(size); + } Ok(data) } } @@ -100,6 +112,9 @@ impl Pollable for Writer { } #[async_trait] impl OutputStream for Writer { + async fn cancel(&mut self) { + self.ready().await; + } fn write(&mut self, bytes: Bytes) -> StreamResult<()> { if self.check_write()? == 0 { return Err(StreamError::trap("write without a permit")); @@ -132,3 +147,38 @@ impl OutputStream for Writer { Ok(if self.pending.is_some() { 0 } else { 65536 }) } } + +#[cfg(test)] +mod tests { + use super::*; + /// Both read APIs request only the bytes needed instead of caching later mutations. + #[tokio::test] + async fn small_reads() { + for blocking in [true, false] { + let memory = Memory::default(); + let file = memory + .open( + "/file", + Open { + write: true, + create: true, + ..Open::default() + }, + ) + .await + .unwrap(); + file.write(0, b"abcd".to_vec()).await.unwrap(); + let mut reader = Reader::new(file.clone(), 0); + let first = if blocking { + reader.blocking_read(1).await.unwrap() + } else { + assert!(reader.read(1).unwrap().is_empty()); + reader.ready().await; + reader.read(1).unwrap() + }; + assert_eq!(first.as_ref(), b"a"); + file.write(1, b"Z".to_vec()).await.unwrap(); + assert_eq!(reader.blocking_read(1).await.unwrap().as_ref(), b"Z"); + } + } +} diff --git a/vfs/tests/guests/files/src/main.rs b/vfs/tests/guests/files/src/main.rs index 95055ea..6607b7f 100644 --- a/vfs/tests/guests/files/src/main.rs +++ b/vfs/tests/guests/files/src/main.rs @@ -9,6 +9,11 @@ use std::{ fn main() { match std::env::var("VFS_TOPIC").unwrap().as_str() { "basic" => basic(), + "review_streams" => review_streams(), + "review_times" => review_times(), + "review_paths" => review_paths(), + "review_links" => review_links(), + "review_preopen" => review_preopen(), "trees" => trees(), "options" => options(), "rename" => rename(), @@ -255,3 +260,121 @@ fn temporary() { assert!(!original.exists()); assert_eq!(fs::read("/persisted").unwrap(), b"persisted"); } + +/// Dropped output streams finish writes and small reads do not cache later bytes. +fn review_streams() { + use wasip2::filesystem::{ + preopens, + types::{DescriptorFlags, OpenFlags, PathFlags}, + }; + let root = preopens::get_directories().remove(0).0; + let file = root + .open_at( + PathFlags::empty(), + "stream-review", + OpenFlags::CREATE, + DescriptorFlags::READ | DescriptorFlags::WRITE, + ) + .unwrap(); + let output = file.write_via_stream(0).unwrap(); + assert!(output.check_write().unwrap() > 0); + output.write(b"abcd").unwrap(); + drop(output); + assert_eq!(fs::read("/stream-review").unwrap(), b"abcd"); + let input = file.read_via_stream(0).unwrap(); + assert_eq!(input.blocking_read(1).unwrap(), b"a"); + file.write_via_stream(1) + .unwrap() + .blocking_write_and_flush(b"Z") + .unwrap(); + assert_eq!(input.blocking_read(1).unwrap(), b"Z"); +} +/// Timestamp operations accept directories and return overflow rather than panicking. +fn review_times() { + use wasip2::clocks::wall_clock::Datetime; + use wasip2::filesystem::{ + preopens, + types::{ErrorCode, NewTimestamp, PathFlags}, + }; + let root = preopens::get_directories().remove(0).0; + fs::create_dir("/times").unwrap(); + root.set_times_at( + PathFlags::SYMLINK_FOLLOW, + "times", + NewTimestamp::NoChange, + NewTimestamp::Timestamp(Datetime { + seconds: 123, + nanoseconds: 0, + }), + ) + .unwrap(); + assert_eq!( + fs::metadata("/times").unwrap().modified().unwrap(), + std::time::UNIX_EPOCH + std::time::Duration::from_secs(123) + ); + assert_eq!( + root.set_times( + NewTimestamp::Timestamp(Datetime { + seconds: u64::MAX, + nanoseconds: u32::MAX + }), + NewTimestamp::NoChange + ), + Err(ErrorCode::Overflow) + ); +} +/// Trailing slashes require folders and readlink rejects ordinary files with invalid. +fn review_paths() { + use wasip2::filesystem::{preopens, types::ErrorCode}; + fs::write("/file", b"x").unwrap(); + assert_eq!( + fs::read("/file/").unwrap_err().kind(), + ErrorKind::NotADirectory + ); + let root = preopens::get_directories().remove(0).0; + assert_eq!(root.readlink_at("file"), Err(ErrorCode::Invalid)); + assert_eq!(root.remove_directory_at("run"), Err(ErrorCode::Busy)); +} +/// Removing an internal link leaves its target intact and external links cannot escape. +fn review_links() { + use wasip2::filesystem::{ + preopens, + types::{DescriptorFlags, DescriptorType, OpenFlags, PathFlags}, + }; + let root = preopens::get_directories().remove(0).0; + assert_eq!( + root.stat_at(PathFlags::empty(), "link").unwrap().type_, + DescriptorType::SymbolicLink + ); + assert_eq!(root.readlink_at("link").unwrap(), "target"); + assert!( + root.open_at( + PathFlags::empty(), + "link", + OpenFlags::DIRECTORY, + DescriptorFlags::READ + ) + .is_err() + ); + assert_eq!( + root.open_at( + PathFlags::SYMLINK_FOLLOW, + "link", + OpenFlags::DIRECTORY, + DescriptorFlags::READ + ) + .unwrap() + .get_type() + .unwrap(), + DescriptorType::Directory + ); + fs::remove_dir_all("/link").unwrap(); + assert_eq!(fs::read("/target/keep").unwrap(), b"keep"); + assert!(fs::read("/outside-link/secret").is_err()); +} +/// The component sees exactly one virtual preopen rather than native WASI preopens. +fn review_preopen() { + let dirs = wasip2::filesystem::preopens::get_directories(); + assert_eq!(dirs.len(), 1); + assert_eq!(dirs[0].1, "/"); +} diff --git a/vfs/tests/layers.proptest-regressions b/vfs/tests/layers.proptest-regressions new file mode 100644 index 0000000..b4cc36d --- /dev/null +++ b/vfs/tests/layers.proptest-regressions @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 86f33ba33c9e3b009f9a580dbcd9a669c783a070e032d497bd480de556b48220 # shrinks to ops = [Operation { kind: 0, from: 3, to: 0, bytes: [] }, Operation { kind: 4, from: 3, to: 1, bytes: [] }] diff --git a/vfs/tests/layers.rs b/vfs/tests/layers.rs index 12b2d12..a63aba3 100644 --- a/vfs/tests/layers.rs +++ b/vfs/tests/layers.rs @@ -283,7 +283,7 @@ async fn apply_layer(layer: &dyn Layer, op: &Operation) -> std::io::Result<()> { 2 => layer.mkdir(path).await, 3 => layer.remove_dir(path).await, 4 => layer.rename(path, PATHS[op.to]).await, - 8 => layer.stat(path).await.map(|_| ()), + 8 => layer.stat(path, true).await.map(|_| ()), _ => { let file = layer .open( @@ -330,7 +330,7 @@ fn apply_std(root: &std::path::Path, op: &Operation) -> std::io::Result<()> { } } async fn compare_tree( - host: &Host, + host: &dyn Layer, root: &std::path::Path, ) -> std::result::Result<(), TestCaseError> { let mut pending = vec!["/".to_string()]; @@ -343,6 +343,7 @@ async fn compare_tree( Entry { name: entry.file_name().to_str().unwrap().into(), directory: entry.file_type().unwrap().is_dir(), + symlink: false, } }) .collect::>(); @@ -350,7 +351,7 @@ async fn compare_tree( prop_assert_eq!(host.list(&path).await.unwrap(), entries.clone()); for entry in entries { let child = resolve(&path, &entry.name); - let stat = host.stat(&child).await.unwrap(); + let stat = host.stat(&child, true).await.unwrap(); prop_assert_eq!(stat.directory, entry.directory); if entry.directory { pending.push(child); @@ -371,25 +372,53 @@ async fn compare_tree( } Ok(()) } -async fn check_host_differential(ops: Vec) -> std::result::Result<(), TestCaseError> { +async fn check_differential( + ops: Vec, + memory: bool, +) -> std::result::Result<(), TestCaseError> { let actual = tempfile::tempdir().unwrap(); let expected = tempfile::tempdir().unwrap(); - let host = Host::new(actual.path(), Process::default()).unwrap(); - for root in [actual.path(), expected.path()] { + let host: Arc = if memory { + Arc::new(Memory::default()) + } else { + Arc::new(Host::new(actual.path(), Process::default()).unwrap()) + }; + { + let root = expected.path(); std::fs::create_dir_all(root.join("a/nested")).unwrap(); std::fs::create_dir_all(root.join("b/nested")).unwrap(); std::fs::write(root.join("a/child"), b"source").unwrap(); std::fs::write(root.join("target"), b"destination").unwrap(); } + host.mkdir("/a").await.unwrap(); + host.mkdir("/a/nested").await.unwrap(); + host.mkdir("/b").await.unwrap(); + host.mkdir("/b/nested").await.unwrap(); + host.open("/a/child", rw()) + .await + .unwrap() + .write(0, b"source".to_vec()) + .await + .unwrap(); + host.open("/target", rw()) + .await + .unwrap() + .write(0, b"destination".to_vec()) + .await + .unwrap(); for op in ops { - let actual = apply_layer(&host, &op).await.map_err(|e| e.kind()); + let actual = apply_layer(host.as_ref(), &op).await.map_err(|e| e.kind()); let expected_result = apply_std(expected.path(), &op).map_err(|e| e.kind()); prop_assert_eq!(actual, expected_result, "operation: {:?}", op); - compare_tree(&host, expected.path()).await?; + compare_tree(host.as_ref(), expected.path()).await?; } Ok(()) } +async fn check_host_differential(ops: Vec) -> std::result::Result<(), TestCaseError> { + check_differential(ops, false).await +} + /// Nested renames replace existing files and empty folders just like std::fs. #[tokio::test] async fn nested_rename_replacements() { @@ -416,6 +445,12 @@ proptest! { tokio::runtime::Runtime::new().unwrap().block_on(check_memory_model(ops))?; } + /// Random memory namespace operations match std::fs and the complete tree. + #[test] + fn memory_differential(ops in operations()) { + tokio::runtime::Runtime::new().unwrap().block_on(check_differential(ops, true))?; + } + /// Random nested operations match std::fs results and the complete tree. #[test] fn host_differential(ops in operations()) { diff --git a/vfs/tests/regressions.rs b/vfs/tests/regressions.rs new file mode 100644 index 0000000..13e2056 --- /dev/null +++ b/vfs/tests/regressions.rs @@ -0,0 +1,92 @@ +//! Regression tests for review findings about memory files and process shutdown. +use sausage_vfs::*; +use std::{ + io::ErrorKind, + time::{Duration, SystemTime}, +}; +fn rw() -> Open { + Open { + write: true, + create: true, + ..Open::default() + } +} +/// A capacity overflow returns an error without poisoning the shared memory tree. +#[tokio::test] +async fn memory_capacity_failure() { + let memory = Memory::default(); + let file = memory.open("/file", rw()).await.unwrap(); + let copy = file.clone(); + let result = + tokio::spawn(async move { copy.write(i64::MAX as u64 - 10, vec![0; 32]).await }).await; + assert_eq!(result.unwrap().unwrap_err().kind(), ErrorKind::FileTooLarge); + assert_eq!( + file.write(1u64 << 62, vec![1]).await.unwrap_err().kind(), + ErrorKind::FileTooLarge + ); + assert_eq!( + file.set_size(1u64 << 62).await.unwrap_err().kind(), + ErrorKind::FileTooLarge + ); + file.write(0, b"usable".to_vec()).await.unwrap(); + assert_eq!(memory.stat("/file", true).await.unwrap().size, 6); + memory.mkdir("/still-usable").await.unwrap(); +} +/// Memory metadata distinguishes a missing parent from a regular-file parent. +#[tokio::test] +async fn memory_parent_errors() { + let memory = Memory::default(); + memory.open("/file", rw()).await.unwrap(); + assert_eq!( + memory.stat("/file/child", true).await.unwrap_err().kind(), + ErrorKind::NotADirectory + ); + assert_eq!( + memory + .open("/file/child", Open::default()) + .await + .err() + .unwrap() + .kind(), + ErrorKind::NotADirectory + ); +} +/// Memory writes update modification time and read handles can set timestamps. +#[tokio::test] +async fn memory_timestamps() { + let memory = Memory::default(); + let file = memory.open("/file", rw()).await.unwrap(); + file.write(0, vec![1]).await.unwrap(); + assert!(file.stat().await.unwrap().modified.is_some()); + let read = memory.open("/file", Open::default()).await.unwrap(); + let time = SystemTime::UNIX_EPOCH + Duration::from_secs(123); + read.set_times(None, Some(time)).await.unwrap(); + assert_eq!(read.stat().await.unwrap().modified, Some(time)); +} +/// Awaiting an aborted writer before idle prevents writes after lock release. +#[tokio::test(flavor = "multi_thread")] +async fn abort_join_then_idle() { + let temp = tempfile::tempdir().unwrap(); + let process = Process::default(); + let host = Host::new(temp.path(), process.clone()).unwrap(); + let file = host.open("/file", rw()).await.unwrap(); + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let copy = file.clone(); + let task = tokio::spawn(async move { + copy.append(vec![1]).await.unwrap(); + started_tx.send(()).unwrap(); + loop { + copy.append(vec![1]).await.unwrap(); + } + }); + started_rx.await.unwrap(); + task.abort(); + assert!(task.await.unwrap_err().is_cancelled()); + process.idle().await; + let before = file.stat().await.unwrap().size; + assert!(before >= 1); + for _ in 0..100 { + tokio::task::yield_now().await; + assert_eq!(file.stat().await.unwrap().size, before); + } +} diff --git a/vfs/tests/wasi.rs b/vfs/tests/wasi.rs index 9fff96e..34f8cde 100644 --- a/vfs/tests/wasi.rs +++ b/vfs/tests/wasi.rs @@ -55,6 +55,14 @@ async fn run(topic: &str, shared: Arc) { let (engine, component) = guest(); let world = tempfile::tempdir().unwrap(); std::fs::create_dir(world.path().join("tmp")).unwrap(); + let outside = tempfile::tempdir().unwrap(); + if topic == "review_links" { + std::fs::create_dir(world.path().join("target")).unwrap(); + std::fs::write(world.path().join("target/keep"), b"keep").unwrap(); + std::fs::write(outside.path().join("secret"), b"secret").unwrap(); + std::os::unix::fs::symlink("target", world.path().join("link")).unwrap(); + std::os::unix::fs::symlink(outside.path(), world.path().join("outside-link")).unwrap(); + } let mut mounts = Mounts::default(); mounts.mount( "/", @@ -73,13 +81,20 @@ async fn run(topic: &str, shared: Arc) { table: &mut s.table, }) .unwrap(); + let mut wasi = WasiCtxBuilder::new(); + wasi.inherit_stdio().env("VFS_TOPIC", topic); + if topic == "review_preopen" { + wasi.preopened_dir( + world.path(), + "/native", + wasmtime_wasi::filesystem::FsPerms::ReadWrite, + ) + .unwrap(); + } let mut store = Store::new( engine, State { - wasi: WasiCtxBuilder::new() - .inherit_stdio() - .env("VFS_TOPIC", topic) - .build(), + wasi: wasi.build(), table: ResourceTable::new(), fs: Adapter::new(mounts), }, @@ -150,3 +165,47 @@ async fn sqlite_transactions() { async fn named_temporary_files() { isolated("tempfile").await; } + +/// Stream drop preserves writes and small reads observe later mutations. +#[tokio::test(flavor = "multi_thread")] +async fn review_streams() { + isolated("review_streams").await; +} + +/// Timestamp conversion reports overflow and accepts directories. +#[tokio::test(flavor = "multi_thread")] +async fn review_times() { + isolated("review_times").await; +} + +/// Path errors distinguish trailing slashes, busy roots, and ordinary files. +#[tokio::test(flavor = "multi_thread")] +async fn review_paths() { + isolated("review_paths").await; +} + +/// Symlink traversal does not delete targets or escape the host root. +#[tokio::test(flavor = "multi_thread")] +async fn review_links() { + isolated("review_links").await; +} + +/// The guest sees only the virtual root preopen. +#[tokio::test(flavor = "multi_thread")] +async fn review_preopen() { + isolated("review_preopen").await; +} + +/// Adding the adapter restores duplicate-import errors on the caller's linker. +#[test] +fn linker_shadowing_is_reset() { + let engine = Engine::default(); + let mut linker = Linker::::new(&engine); + wasmtime_wasi::p2::add_to_linker_async(&mut linker).unwrap(); + adapter::add_to_linker(&mut linker, |s| View { + adapter: &mut s.fs, + table: &mut s.table, + }) + .unwrap(); + assert!(wasmtime_wasi::p2::add_to_linker_async(&mut linker).is_err()); +}