From b845e062416cf6c6ab6e5dff39041613dfd96e5a Mon Sep 17 00:00:00 2001 From: wan9chi Date: Wed, 11 Nov 2026 15:23:26 +0800 Subject: [PATCH 1/2] fix(fspy): don't treat opening a file as a folder as reading it Since Node 23, Node's `fs.rmSync` on macOS is libc++'s `std::filesystem::remove_all`, which opens every path it removes with `O_DIRECTORY | O_NOFOLLOW` and unlinks the path when that open fails with `ENOTDIR` or `ELOOP`. fspy recorded the failed open as a read, so a task that cleans and rebuilds its output folder had each output that existed beforehand as an input, and missed the cache once a later step changed one. An `O_DIRECTORY` open for reading, without `O_CREAT`, of a path that is there and isn't a folder is no longer recorded, in the preload library and in the Linux seccomp handler. An open of a folder, or of a path that isn't there, is still recorded, since tools such as Go's `os.ReadDir` list folders that way. Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 2 +- crates/fspy/src/unix/syscall_handler/mod.rs | 53 ++++++++++++++----- crates/fspy/tests/rust_std.rs | 34 ++++++++++++ crates/fspy/tests/static_executable.rs | 13 ++++- crates/fspy_nostd/src/fs/linux.rs | 32 ++++++++++- crates/fspy_nostd/src/fs/mac.rs | 26 ++++++++- crates/fspy_nostd/src/fs/unix.rs | 22 +++++++- .../src/interceptions/open.rs | 32 +++++++++-- crates/fspy_shared_unix/src/lib.rs | 1 + crates/fspy_shared_unix/src/open_flags.rs | 48 +++++++++++++++++ crates/fspy_test_bin/src/main.rs | 4 ++ crates/vt_bin/src/vtt/fs.rs | 31 +++++++++-- 12 files changed, 272 insertions(+), 26 deletions(-) create mode 100644 crates/fspy_shared_unix/src/open_flags.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 505c1af30..97e9db455 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ - **Fixed** `vp run` now exits with code 1 when a task's cache can't be checked, for example because the cache database can't be read. It already stopped there, but exited 0 ([#960](https://github.com/voidzero-dev/vite-task/pull/960)). - **Changed** The helper files `vp run` writes to the system temp directory, such as the library that tracks file accesses and the JSON Schema that `vp run --json` points to, now share one folder that only the current user can write to, like `/tmp/vp_run-1000` on Linux, instead of a folder for each kind of file ([#974](https://github.com/voidzero-dev/vite-task/pull/974)). - **Perf** When several tasks become ready at once, `vp run` now checks their cache entries at the same time instead of one after another, so cache hits start sooner. Hashing a task's input files for its cache lookup no longer holds up other tasks ([#982](https://github.com/voidzero-dev/vite-task/pull/982)). -- **Fixed** On Windows, the files a task deletes or moves, such as a build cleaning its output folder before writing it again, are no longer among its inputs, as on Linux and macOS. Before, when a later step changed one of those files, for example by stamping a version into the rebuilt output, the next run missed the cache with a reason like `'dist/index.js' modified`, though the task never read the file ([#980](https://github.com/voidzero-dev/vite-task/pull/980)). +- **Fixed** On Windows and macOS, the files a task deletes or moves, such as a build cleaning its output folder before writing it again, are no longer among its inputs, as on Linux. On macOS, this affected the files Node's `fs.rmSync` deletes from Node 23 on, since it first tries to open each one as a folder. Before, when a later step changed one of those files, for example by stamping a version into the rebuilt output, the next run missed the cache with a reason like `'dist/index.js' modified`, though the task never read the file ([#980](https://github.com/voidzero-dev/vite-task/pull/980), [#991](https://github.com/voidzero-dev/vite-task/pull/991)). - **Changed** In `@voidzero-dev/vite-task-client`, a `getEnvs` query that matches every env name, `{ prefix: '' }` or `*`, now returns nothing instead of every environment variable, and no longer adds them to the cache key ([#976](https://github.com/voidzero-dev/vite-task/pull/976)). - **Added** `vp run --json` prints every package and task in the workspace, with each task's `dependsOn` resolved to task IDs, as one JSON document for scripts, editors and coding agents. It runs nothing and, like `vp run` without a task, loads every package's config. It prints the same document from any directory in the workspace: indented in a terminal, on one line when captured. Its `$schema` field points to a JSON Schema that describes each field, which `vp run` writes to the system temp directory ([#948](https://github.com/voidzero-dev/vite-task/pull/948)). - **Fixed** On Linux, where every user shares the temp folder `/tmp`, the helper files `vp run` writes there, such as the library that tracks file accesses, now go in a folder only the current user can write to, like `/tmp/fspy-1000`. Before, they went in a folder such as `/tmp/fspy` that belonged to whoever ran `vp run` first: running a different version as another user failed, and the same version used files that user could replace ([#947](https://github.com/voidzero-dev/vite-task/pull/947)). diff --git a/crates/fspy/src/unix/syscall_handler/mod.rs b/crates/fspy/src/unix/syscall_handler/mod.rs index d55ce8fbb..2c37b13b5 100644 --- a/crates/fspy/src/unix/syscall_handler/mod.rs +++ b/crates/fspy/src/unix/syscall_handler/mod.rs @@ -7,7 +7,7 @@ mod stat; use std::{ borrow::Cow, ffi::{OsStr, c_int}, - io, + fs, io, os::unix::ffi::OsStrExt, path::{Path, PathBuf}, sync::Arc, @@ -18,6 +18,7 @@ use fspy_seccomp_unotify::{ supervisor::handler::arg::{CStrPtr, Caller, Fd}, }; use fspy_shared::ipc::{AccessMode, PathAccess, channel::Sender}; +use fspy_shared_unix::open_flags::fails_without_reading; const PATH_MAX: usize = libc::PATH_MAX as usize; @@ -48,7 +49,18 @@ impl SyscallHandler { libc::O_WRONLY => AccessMode::Write, _ => AccessMode::Read, }; - self.handle_path(caller, dir_fd, path_ptr, mode) + let Some(path) = read_path(&mut self.path_read_buf, caller, dir_fd, path_ptr)? else { + return Ok(()); + }; + let skipped = fails_without_reading(flags, |follow_symlinks| { + let metadata = + if follow_symlinks { fs::metadata(&path) } else { fs::symlink_metadata(&path) }; + metadata.ok().map(|metadata| metadata.is_dir()) + }); + if !skipped { + self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); + } + Ok(()) } fn handle_path( @@ -58,19 +70,9 @@ impl SyscallHandler { path_ptr: CStrPtr, mode: AccessMode, ) -> io::Result<()> { - let Some(path_len) = path_ptr.read(caller, &mut self.path_read_buf)? else { - // Ignore paths that are too long to fit in PATH_MAX - return Ok(()); - }; - let mut path = Cow::Borrowed(Path::new(OsStr::from_bytes(&self.path_read_buf[..path_len]))); - if !path.is_absolute() { - let mut resolved_path = PathBuf::from(dir_fd.get_path(caller)?); - if !nix::NixPath::is_empty(path.as_ref()) { - resolved_path.push(&path); - } - path = Cow::Owned(resolved_path); + if let Some(path) = read_path(&mut self.path_read_buf, caller, dir_fd, path_ptr)? { + self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); } - self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); Ok(()) } @@ -85,6 +87,29 @@ impl SyscallHandler { } } +/// Reads the path at `path_ptr` in the caller into `buf` and resolves it +/// against `dir_fd`, or returns `None` for a path too long to fit in +/// `PATH_MAX`, which isn't recorded. +fn read_path<'buf>( + buf: &'buf mut [u8; PATH_MAX], + caller: Caller, + dir_fd: Fd, + path_ptr: CStrPtr, +) -> io::Result>> { + let Some(path_len) = path_ptr.read(caller, buf)? else { + return Ok(None); + }; + let path = Path::new(OsStr::from_bytes(&buf[..path_len])); + if path.is_absolute() { + return Ok(Some(Cow::Borrowed(path))); + } + let mut resolved_path = PathBuf::from(dir_fd.get_path(caller)?); + if !nix::NixPath::is_empty(path) { + resolved_path.push(path); + } + Ok(Some(Cow::Owned(resolved_path))) +} + impl_handler!( SyscallHandler: diff --git a/crates/fspy/tests/rust_std.rs b/crates/fspy/tests/rust_std.rs index 9bbbc37b4..a0b94f2c0 100644 --- a/crates/fspy/tests/rust_std.rs +++ b/crates/fspy/tests/rust_std.rs @@ -35,6 +35,40 @@ async fn open_write() -> anyhow::Result<()> { Ok(()) } +/// An `O_DIRECTORY` open of a file fails without reading it, as does one of a +/// symlink with `O_NOFOLLOW`, so neither is recorded. libc++'s +/// `std::filesystem::remove_all` opens every path it removes that way. An +/// open of a folder, or of a path that isn't there, is still recorded. +#[cfg(unix)] +#[test(tokio::test)] +async fn open_folder_only() -> anyhow::Result<()> { + let tmp_dir = tempfile::tempdir()?; + let tmp_path = tmp_dir.path().canonicalize()?; + File::create(tmp_path.join("file"))?; + fs::create_dir(tmp_path.join("folder"))?; + std::os::unix::fs::symlink("folder", tmp_path.join("link"))?; + let tmp_path_str = tmp_path.to_str().unwrap().to_owned(); + let accesses = track_fn!(tmp_path_str, |tmp_path_str: String| { + use std::os::unix::fs::OpenOptionsExt as _; + let tmp_path = std::path::Path::new(&tmp_path_str); + for (name, flags) in + [("file", 0), ("link", libc::O_NOFOLLOW), ("folder", 0), ("missing", 0)] + { + let _ = OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY | flags) + .open(tmp_path.join(name)); + } + }) + .await?; + test_utils::assert_not_contains(&accesses, &tmp_path.join("file")); + test_utils::assert_not_contains(&accesses, &tmp_path.join("link")); + assert_contains(&accesses, &tmp_path.join("folder"), PathAccess::Read); + assert_contains(&accesses, &tmp_path.join("missing"), PathAccess::Read); + + Ok(()) +} + #[test(tokio::test)] async fn rename_destination() -> anyhow::Result<()> { let tmp_dir = tempfile::tempdir()?; diff --git a/crates/fspy/tests/static_executable.rs b/crates/fspy/tests/static_executable.rs index 9de887a03..103228055 100644 --- a/crates/fspy/tests/static_executable.rs +++ b/crates/fspy/tests/static_executable.rs @@ -8,7 +8,7 @@ use std::path::Path; use fspy::PathAccessIterable; use test_log::test; -use crate::test_utils::{assert_contains, test_bin_path}; +use crate::test_utils::{assert_contains, assert_not_contains, test_bin_path}; mod test_utils; @@ -44,6 +44,17 @@ async fn open_readwrite() { assert_contains(&accesses, Path::new("/hello"), fspy::PathAccess::ReadWrite); } +#[test(tokio::test)] +async fn open_folder_only() { + // The open of a file fails without reading it, so it isn't recorded. + let file = tempfile::NamedTempFile::new().unwrap(); + let accesses = track_test_bin(&["open_folder_only", file.path().to_str().unwrap()], None).await; + assert_not_contains(&accesses, file.path()); + + let accesses = track_test_bin(&["open_folder_only", "/hello"], None).await; + assert_contains(&accesses, Path::new("/hello"), fspy::PathAccess::Read); +} + #[test(tokio::test)] async fn openat2_read() { let accesses = track_test_bin(&["openat2_read", "/hello"], None).await; diff --git a/crates/fspy_nostd/src/fs/linux.rs b/crates/fspy_nostd/src/fs/linux.rs index 259133da2..5b3fff8a3 100644 --- a/crates/fspy_nostd/src/fs/linux.rs +++ b/crates/fspy_nostd/src/fs/linux.rs @@ -13,6 +13,9 @@ pub(super) const O_CREAT: u32 = linux_raw_sys::general::O_CREAT; pub(super) const O_EXCL: u32 = linux_raw_sys::general::O_EXCL; pub(super) const O_CLOEXEC: u32 = linux_raw_sys::general::O_CLOEXEC; pub(super) const AT_REMOVEDIR: u32 = linux_raw_sys::general::AT_REMOVEDIR; +pub(super) const AT_SYMLINK_NOFOLLOW: u32 = linux_raw_sys::general::AT_SYMLINK_NOFOLLOW; +pub(super) const S_IFMT: u32 = linux_raw_sys::general::S_IFMT; +pub(super) const S_IFDIR: u32 = linux_raw_sys::general::S_IFDIR; pub(super) type ModeBits = u32; pub(super) const S_IRUSR: ModeBits = linux_raw_sys::general::S_IRUSR; pub(super) const S_IWUSR: ModeBits = linux_raw_sys::general::S_IWUSR; @@ -69,7 +72,34 @@ pub(super) fn fstat(fd: BorrowedFd<'_>) -> Result { .map_err(Error::from)?; // SAFETY: a successful `fstat` initialized the complete structure. let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size }) + Ok(Stat { st_size: raw.st_size, st_mode: raw.st_mode }) +} + +#[expect(clippy::needless_pass_by_value, reason = "CStr is a borrowed value type")] +pub(super) fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { + // x86_64 names the syscall `newfstatat`. + #[cfg(target_arch = "x86_64")] + const FSTATAT: syscalls::Sysno = syscalls::Sysno::newfstatat; + #[cfg(not(target_arch = "x86_64"))] + const FSTATAT: syscalls::Sysno = syscalls::Sysno::fstatat; + + let mut raw = core::mem::MaybeUninit::::zeroed(); + // SAFETY: `dirfd` remains borrowed, `path` is NUL-terminated, and `raw` + // points to writable storage for the kernel's architecture-specific + // `stat` structure. + unsafe { + syscalls::syscall!( + FSTATAT, + dirfd.as_raw_fd(), + path.as_ptr(), + raw.as_mut_ptr(), + flags.bits() + ) + } + .map_err(Error::from)?; + // SAFETY: a successful `fstatat` initialized the complete structure. + let raw = unsafe { raw.assume_init() }; + Ok(Stat { st_size: raw.st_size, st_mode: raw.st_mode }) } pub(super) fn ftruncate(fd: BorrowedFd<'_>, len: u64) -> Result<()> { diff --git a/crates/fspy_nostd/src/fs/mac.rs b/crates/fspy_nostd/src/fs/mac.rs index 943dfd417..6e76005b1 100644 --- a/crates/fspy_nostd/src/fs/mac.rs +++ b/crates/fspy_nostd/src/fs/mac.rs @@ -14,6 +14,9 @@ pub(super) const O_CREAT: u32 = libc::O_CREAT.cast_unsigned(); pub(super) const O_EXCL: u32 = libc::O_EXCL.cast_unsigned(); pub(super) const O_CLOEXEC: u32 = libc::O_CLOEXEC.cast_unsigned(); pub(super) const AT_REMOVEDIR: u32 = libc::AT_REMOVEDIR.cast_unsigned(); +pub(super) const AT_SYMLINK_NOFOLLOW: u32 = libc::AT_SYMLINK_NOFOLLOW.cast_unsigned(); +pub(super) const S_IFMT: u32 = libc::S_IFMT as u32; +pub(super) const S_IFDIR: u32 = libc::S_IFDIR as u32; pub(super) type ModeBits = libc::mode_t; pub(super) const S_IRUSR: ModeBits = libc::S_IRUSR; pub(super) const S_IWUSR: ModeBits = libc::S_IWUSR; @@ -61,7 +64,28 @@ pub(super) fn fstat(fd: BorrowedFd<'_>) -> Result { } // SAFETY: a successful `fstat` initialized the complete structure. let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size }) + Ok(Stat { st_size: raw.st_size, st_mode: u32::from(raw.st_mode) }) +} + +#[expect(clippy::needless_pass_by_value, reason = "CStr is a borrowed value type")] +pub(super) fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { + let mut raw = core::mem::MaybeUninit::::zeroed(); + // SAFETY: `dirfd` remains borrowed, `path` is NUL-terminated, and `raw` + // is writable for the call. + if unsafe { + libc::fstatat( + dirfd.as_raw_fd(), + path.as_ptr().cast(), + raw.as_mut_ptr(), + flags.bits().cast_signed(), + ) + } == -1 + { + return Err(Error::last_os_error()); + } + // SAFETY: a successful `fstatat` initialized the complete structure. + let raw = unsafe { raw.assume_init() }; + Ok(Stat { st_size: raw.st_size, st_mode: u32::from(raw.st_mode) }) } pub(super) fn ftruncate(fd: BorrowedFd<'_>, len: u64) -> Result<()> { diff --git a/crates/fspy_nostd/src/fs/unix.rs b/crates/fspy_nostd/src/fs/unix.rs index 36c7ceeec..31c14ac02 100644 --- a/crates/fspy_nostd/src/fs/unix.rs +++ b/crates/fspy_nostd/src/fs/unix.rs @@ -44,13 +44,24 @@ bitflags! { pub struct AtFlags: u32 { /// Remove a directory rather than a non-directory entry. const REMOVEDIR = imp::AT_REMOVEDIR; + /// Report on a symlink itself rather than on what it points to. + const SYMLINK_NOFOLLOW = imp::AT_SYMLINK_NOFOLLOW; } } -/// Metadata used by fspy's shared-memory backing files. +/// File metadata. #[derive(Clone, Copy, Debug)] pub struct Stat { pub st_size: i64, + pub st_mode: u32, +} + +impl Stat { + /// Whether the file is a directory. + #[must_use] + pub const fn is_dir(&self) -> bool { + self.st_mode & imp::S_IFMT == imp::S_IFDIR + } } /// The platform's maximum pathname size, including the terminating NUL. @@ -88,6 +99,15 @@ pub fn fstat(fd: BorrowedFd<'_>) -> Result { imp::fstat(fd) } +/// Returns metadata for `path` relative to `dirfd`. +/// +/// # Errors +/// +/// Returns the error reported by `fstatat`. +pub fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { + imp::fstatat(dirfd, path, flags) +} + /// Sets the length of an open file. /// /// # Errors diff --git a/crates/fspy_preload_unix/src/interceptions/open.rs b/crates/fspy_preload_unix/src/interceptions/open.rs index cb8ac5f3b..7dad63bdf 100644 --- a/crates/fspy_preload_unix/src/interceptions/open.rs +++ b/crates/fspy_preload_unix/src/interceptions/open.rs @@ -1,3 +1,5 @@ +use fspy_nostd::{CWD, fs::AtFlags}; +use fspy_shared_unix::open_flags::fails_without_reading; use libc::FILE; use crate::{ @@ -9,6 +11,28 @@ use crate::{ macros::intercept, }; +/// Records an open of `path` with `flags`, unless it fails without reading +/// the path. +/// +/// # Safety +/// +/// `path` must satisfy the contract of [`PathAt::borrow_raw`]. +unsafe fn handle_open_at(path: PathAt<'_, '_>, flags: c_int) { + let PathAt(dirfd, pathname) = path; + let skipped = fails_without_reading(flags, |follow_symlinks| { + let flags = if follow_symlinks { AtFlags::empty() } else { AtFlags::SYMLINK_NOFOLLOW }; + // The kernel ignores the descriptor for an absolute path, and a + // relative path at `-1` isn't recorded anyway. + fspy_nostd::fs::fstatat(dirfd.unwrap_or(CWD), pathname, flags) + .ok() + .map(|stat| stat.is_dir()) + }); + if !skipped { + // SAFETY: path is valid, as the caller guarantees. + unsafe { handle_open(path, OpenFlags(flags)) }; + } +} + const fn has_mode_arg(o_flags: c_int) -> bool { if o_flags & libc::O_CREAT != 0 { return true; @@ -28,7 +52,7 @@ type Mode = c_int; intercept!(open(64): unsafe extern "C" fn(*const c_char, c_int, args: ...) -> c_int); unsafe extern "C" fn open(path: *const c_char, flags: c_int, mut args: ...) -> c_int { // SAFETY: path is a valid C string pointer provided by the caller of the interposed function - unsafe { handle_open(fspy_nostd::CStr::from_ptr(path.cast()), OpenFlags(flags)) }; + unsafe { handle_open_at(PathAt(Some(CWD), fspy_nostd::CStr::from_ptr(path.cast())), flags) }; if has_mode_arg(flags) { // SAFETY: when O_CREAT or O_TMPFILE is set, a mode_t argument is required by the open() contract let mode: Mode = unsafe { args.next_arg() }; @@ -48,7 +72,7 @@ unsafe extern "C" fn openat( mut args: ... ) -> c_int { // SAFETY: dirfd is -1 or a valid file descriptor, and path is a valid C string, provided by the caller of the interposed function - unsafe { handle_open(PathAt::borrow_raw(dirfd, path), OpenFlags(flags)) }; + unsafe { handle_open_at(PathAt::borrow_raw(dirfd, path), flags) }; if has_mode_arg(flags) { // https://github.com/tailhook/openat/issues/21#issuecomment-535914957 @@ -67,7 +91,7 @@ intercept!(open_nocancel: unsafe extern "C" fn(*const c_char, c_int, ...) -> c_i #[cfg(target_os = "macos")] unsafe extern "C" fn open_nocancel(path: *const c_char, flags: c_int, mut args: ...) -> c_int { // SAFETY: path is a valid C string pointer provided by the caller of open$NOCANCEL - unsafe { handle_open(fspy_nostd::CStr::from_ptr(path.cast()), OpenFlags(flags)) }; + unsafe { handle_open_at(PathAt(Some(CWD), fspy_nostd::CStr::from_ptr(path.cast())), flags) }; if has_mode_arg(flags) { // SAFETY: O_CREAT requires a mode argument, matching the open$NOCANCEL contract let mode: Mode = unsafe { args.next_arg() }; @@ -89,7 +113,7 @@ unsafe extern "C" fn openat_nocancel( mut args: ... ) -> c_int { // SAFETY: dirfd is -1 or a valid file descriptor, and path is a valid C string, provided by the caller of openat$NOCANCEL - unsafe { handle_open(PathAt::borrow_raw(dirfd, path), OpenFlags(flags)) }; + unsafe { handle_open_at(PathAt::borrow_raw(dirfd, path), flags) }; if has_mode_arg(flags) { // SAFETY: O_CREAT requires a mode argument, matching the openat$NOCANCEL contract let mode: Mode = unsafe { args.next_arg() }; diff --git a/crates/fspy_shared_unix/src/lib.rs b/crates/fspy_shared_unix/src/lib.rs index 5437a3efc..d15ae1e54 100644 --- a/crates/fspy_shared_unix/src/lib.rs +++ b/crates/fspy_shared_unix/src/lib.rs @@ -2,6 +2,7 @@ pub mod exec; pub(crate) mod open_exec; +pub mod open_flags; pub mod payload; pub mod spawn; diff --git a/crates/fspy_shared_unix/src/open_flags.rs b/crates/fspy_shared_unix/src/open_flags.rs new file mode 100644 index 000000000..d639cea44 --- /dev/null +++ b/crates/fspy_shared_unix/src/open_flags.rs @@ -0,0 +1,48 @@ +use nix::libc::{O_ACCMODE, O_CREAT, O_DIRECTORY, O_NOFOLLOW, O_RDONLY, c_int}; + +/// Whether an open with `flags` fails without reading the path, so it isn't +/// recorded. +/// +/// An `O_DIRECTORY` open for reading, without `O_CREAT`, opens only a +/// folder. For a path that is there and isn't one, it fails with `ENOTDIR`, +/// or with `ELOOP` for a symlink when `O_NOFOLLOW` is set. libc++'s +/// `std::filesystem::remove_all`, which Node's `fs.rmSync` uses on macOS, +/// opens every path it removes that way, and removes the path as a file when +/// the open fails. A folder, or a path that isn't there, is still recorded: +/// a task that lists a folder that way, as Go's `os.ReadDir` does, finds out +/// whether it's there. +/// +/// `is_folder` reports whether the path is a folder, following a final +/// symlink when its argument is `true`, or `None` when that can't be told, +/// as when the path isn't there. +pub fn fails_without_reading(flags: c_int, is_folder: impl FnOnce(bool) -> Option) -> bool { + flags & O_DIRECTORY != 0 + && flags & O_ACCMODE == O_RDONLY + && flags & O_CREAT == 0 + && is_folder(flags & O_NOFOLLOW == 0) == Some(false) +} + +#[cfg(test)] +mod tests { + use nix::libc::{O_CLOEXEC, O_RDWR, O_WRONLY}; + + use super::*; + + #[test] + fn folder_only_opens_of_non_folders_fail_without_reading() { + let not_a_folder = |_| Some(false); + // libc++'s `remove_all`. + assert!(fails_without_reading( + O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC, + not_a_folder + )); + assert!(fails_without_reading(O_RDONLY | O_DIRECTORY, not_a_folder)); + assert!(!fails_without_reading(O_RDONLY, not_a_folder)); + assert!(!fails_without_reading(O_WRONLY | O_DIRECTORY, not_a_folder)); + assert!(!fails_without_reading(O_RDWR | O_DIRECTORY, not_a_folder)); + assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY | O_CREAT, not_a_folder)); + + assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY, |_| Some(true))); + assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY, |_| None)); + } +} diff --git a/crates/fspy_test_bin/src/main.rs b/crates/fspy_test_bin/src/main.rs index de88d8ece..4d343d04c 100644 --- a/crates/fspy_test_bin/src/main.rs +++ b/crates/fspy_test_bin/src/main.rs @@ -29,6 +29,10 @@ fn main() { "open_readwrite" => { let _ = File::options().read(true).write(true).open(path); } + "open_folder_only" => { + use std::os::unix::fs::OpenOptionsExt as _; + let _ = File::options().read(true).custom_flags(nix::libc::O_DIRECTORY).open(path); + } "openat2_read" => { let _ = openat2(AT_FDCWD, path, OpenHow::new().flags(OFlag::O_RDONLY)); } diff --git a/crates/vt_bin/src/vtt/fs.rs b/crates/vt_bin/src/vtt/fs.rs index 37c065778..5131ee182 100644 --- a/crates/vt_bin/src/vtt/fs.rs +++ b/crates/vt_bin/src/vtt/fs.rs @@ -23,7 +23,9 @@ const USAGE: &str = "Usage: vtt fs ... [ ...]..."; /// missing folder isn't an error. /// - `rm `: remove the file, or the folder and everything in it, like /// `rm -r`. A missing path isn't an error, like a tool cleaning its output -/// folder. +/// folder. On Linux and macOS, it opens each path as a folder first and +/// removes the path as a file when it isn't one, like Node's `fs.rmSync` +/// on macOS, which uses libc++'s `std::filesystem::remove_all`. /// - `mkdir `: create the folder and any missing parents, like /// `mkdir -p`. /// - `rename `: move the file or folder `` to ``, like @@ -46,7 +48,7 @@ pub fn run(args: &[String]) -> Result<(), Box> { ["stat", path, rest @ ..] => (allow_missing(fs::metadata(path)), rest), ["list", dir, rest @ ..] => (allow_missing(list(dir)), rest), ["walk", dir, rest @ ..] => (allow_missing(walk(Path::new(dir))), rest), - ["rm", path, rest @ ..] => (allow_missing(remove(path)), rest), + ["rm", path, rest @ ..] => (allow_missing(remove(Path::new(path))), rest), ["mkdir", dir, rest @ ..] => (fs::create_dir_all(dir), rest), ["rename", from, to, rest @ ..] => (fs::rename(from, to), rest), _ => return Err(format!("{USAGE}\nunexpected arguments: {}", ops.join(" ")).into()), @@ -72,7 +74,30 @@ fn walk(dir: &Path) -> io::Result<()> { Ok(()) } -fn remove(path: &str) -> io::Result<()> { +#[cfg(unix)] +fn remove(path: &Path) -> io::Result<()> { + use std::os::unix::fs::OpenOptionsExt as _; + + match fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(path) + { + Ok(_) => { + for entry in fs::read_dir(path)? { + remove(&entry?.path())?; + } + fs::remove_dir(path) + } + Err(err) if matches!(err.raw_os_error(), Some(libc::ENOTDIR | libc::ELOOP)) => { + fs::remove_file(path) + } + Err(err) => Err(err), + } +} + +#[cfg(windows)] +fn remove(path: &Path) -> io::Result<()> { if fs::symlink_metadata(path)?.is_dir() { fs::remove_dir_all(path) } else { From 21fa93728e22c6fc6262d98ef85da5e9446edadc Mon Sep 17 00:00:00 2001 From: wan9chi Date: Wed, 11 Nov 2026 15:42:45 +0800 Subject: [PATCH 2/2] fix(fspy): ignore every O_DIRECTORY open Drop the fstatat check: an O_DIRECTORY open reads nothing, whether it opens a folder or fails on a file, and listing the folder is recorded on its own. Co-authored-by: Claude Opus 5.5 --- crates/fspy/src/unix/syscall_handler/mod.rs | 58 +++++------------- crates/fspy/tests/rust_std.rs | 25 +++----- crates/fspy/tests/static_executable.rs | 9 +-- crates/fspy_client_unix/src/convert.rs | 24 ++++---- crates/fspy_client_unix/src/lib.rs | 4 +- crates/fspy_nostd/src/fs/linux.rs | 32 +--------- crates/fspy_nostd/src/fs/mac.rs | 26 +------- crates/fspy_nostd/src/fs/unix.rs | 22 +------ .../src/interceptions/open.rs | 32 ++-------- crates/fspy_shared_unix/src/open_flags.rs | 61 ++++++------------- crates/fspy_test_bin/src/main.rs | 2 +- 11 files changed, 70 insertions(+), 225 deletions(-) diff --git a/crates/fspy/src/unix/syscall_handler/mod.rs b/crates/fspy/src/unix/syscall_handler/mod.rs index 2c37b13b5..31bc3f0f7 100644 --- a/crates/fspy/src/unix/syscall_handler/mod.rs +++ b/crates/fspy/src/unix/syscall_handler/mod.rs @@ -7,7 +7,7 @@ mod stat; use std::{ borrow::Cow, ffi::{OsStr, c_int}, - fs, io, + io, os::unix::ffi::OsStrExt, path::{Path, PathBuf}, sync::Arc, @@ -18,7 +18,7 @@ use fspy_seccomp_unotify::{ supervisor::handler::arg::{CStrPtr, Caller, Fd}, }; use fspy_shared::ipc::{AccessMode, PathAccess, channel::Sender}; -use fspy_shared_unix::open_flags::fails_without_reading; +use fspy_shared_unix::open_flags::open_access_mode; const PATH_MAX: usize = libc::PATH_MAX as usize; @@ -44,23 +44,10 @@ impl SyscallHandler { path_ptr: CStrPtr, flags: c_int, ) -> io::Result<()> { - let mode = match flags & libc::O_ACCMODE { - libc::O_RDWR => AccessMode::ReadWrite, - libc::O_WRONLY => AccessMode::Write, - _ => AccessMode::Read, - }; - let Some(path) = read_path(&mut self.path_read_buf, caller, dir_fd, path_ptr)? else { + let Some(mode) = open_access_mode(flags) else { return Ok(()); }; - let skipped = fails_without_reading(flags, |follow_symlinks| { - let metadata = - if follow_symlinks { fs::metadata(&path) } else { fs::symlink_metadata(&path) }; - metadata.ok().map(|metadata| metadata.is_dir()) - }); - if !skipped { - self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); - } - Ok(()) + self.handle_path(caller, dir_fd, path_ptr, mode) } fn handle_path( @@ -70,9 +57,19 @@ impl SyscallHandler { path_ptr: CStrPtr, mode: AccessMode, ) -> io::Result<()> { - if let Some(path) = read_path(&mut self.path_read_buf, caller, dir_fd, path_ptr)? { - self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); + let Some(path_len) = path_ptr.read(caller, &mut self.path_read_buf)? else { + // Ignore paths that are too long to fit in PATH_MAX + return Ok(()); + }; + let mut path = Cow::Borrowed(Path::new(OsStr::from_bytes(&self.path_read_buf[..path_len]))); + if !path.is_absolute() { + let mut resolved_path = PathBuf::from(dir_fd.get_path(caller)?); + if !nix::NixPath::is_empty(path.as_ref()) { + resolved_path.push(&path); + } + path = Cow::Owned(resolved_path); } + self.ipc_sender.send(&mode.with_path(path.as_os_str().into())); Ok(()) } @@ -87,29 +84,6 @@ impl SyscallHandler { } } -/// Reads the path at `path_ptr` in the caller into `buf` and resolves it -/// against `dir_fd`, or returns `None` for a path too long to fit in -/// `PATH_MAX`, which isn't recorded. -fn read_path<'buf>( - buf: &'buf mut [u8; PATH_MAX], - caller: Caller, - dir_fd: Fd, - path_ptr: CStrPtr, -) -> io::Result>> { - let Some(path_len) = path_ptr.read(caller, buf)? else { - return Ok(None); - }; - let path = Path::new(OsStr::from_bytes(&buf[..path_len])); - if path.is_absolute() { - return Ok(Some(Cow::Borrowed(path))); - } - let mut resolved_path = PathBuf::from(dir_fd.get_path(caller)?); - if !nix::NixPath::is_empty(path) { - resolved_path.push(path); - } - Ok(Some(Cow::Owned(resolved_path))) -} - impl_handler!( SyscallHandler: diff --git a/crates/fspy/tests/rust_std.rs b/crates/fspy/tests/rust_std.rs index a0b94f2c0..0a93b897b 100644 --- a/crates/fspy/tests/rust_std.rs +++ b/crates/fspy/tests/rust_std.rs @@ -35,36 +35,31 @@ async fn open_write() -> anyhow::Result<()> { Ok(()) } -/// An `O_DIRECTORY` open of a file fails without reading it, as does one of a -/// symlink with `O_NOFOLLOW`, so neither is recorded. libc++'s -/// `std::filesystem::remove_all` opens every path it removes that way. An -/// open of a folder, or of a path that isn't there, is still recorded. +/// An `O_DIRECTORY` open reads nothing, so it isn't recorded, whether it +/// fails on a file, as when libc++'s `std::filesystem::remove_all` opens each +/// path it removes, or opens a folder. Reading the folder's entries is +/// recorded on its own (see `readdir`). #[cfg(unix)] #[test(tokio::test)] -async fn open_folder_only() -> anyhow::Result<()> { +async fn open_directory() -> anyhow::Result<()> { let tmp_dir = tempfile::tempdir()?; let tmp_path = tmp_dir.path().canonicalize()?; File::create(tmp_path.join("file"))?; fs::create_dir(tmp_path.join("folder"))?; - std::os::unix::fs::symlink("folder", tmp_path.join("link"))?; let tmp_path_str = tmp_path.to_str().unwrap().to_owned(); let accesses = track_fn!(tmp_path_str, |tmp_path_str: String| { use std::os::unix::fs::OpenOptionsExt as _; let tmp_path = std::path::Path::new(&tmp_path_str); - for (name, flags) in - [("file", 0), ("link", libc::O_NOFOLLOW), ("folder", 0), ("missing", 0)] - { + for name in ["file", "folder"] { let _ = OpenOptions::new() .read(true) - .custom_flags(libc::O_DIRECTORY | flags) + .custom_flags(libc::O_DIRECTORY) .open(tmp_path.join(name)); } }) .await?; test_utils::assert_not_contains(&accesses, &tmp_path.join("file")); - test_utils::assert_not_contains(&accesses, &tmp_path.join("link")); - assert_contains(&accesses, &tmp_path.join("folder"), PathAccess::Read); - assert_contains(&accesses, &tmp_path.join("missing"), PathAccess::Read); + test_utils::assert_not_contains(&accesses, &tmp_path.join("folder")); Ok(()) } @@ -283,9 +278,9 @@ async fn readdir() -> anyhow::Result<()> { // and if that fails, not read dir call (NtQueryDirectoryFile/NtQueryDirectoryFileEx) is made. // - macOS/Linux: // - opendir results in a read_dir access. This call is directly made without trying to open the directory as a fd first. - // - open + fopendir results in READ access, because open would fail with ENOENT, and fopendir is not called. + // - open + fdopendir results in no access, because the `O_DIRECTORY` open isn't recorded, and it fails with ENOENT, so fdopendir is not called. // - // This difference is acceptable because both will result in a "not found" fingerprint in vite-task. + // READ and read_dir both result in a "not found" fingerprint in vite-task. // To keep the test consistent across platforms, we create the directory first. std::fs::create_dir(tmpdir.path().join("hello_dir"))?; diff --git a/crates/fspy/tests/static_executable.rs b/crates/fspy/tests/static_executable.rs index 103228055..c2db10a4e 100644 --- a/crates/fspy/tests/static_executable.rs +++ b/crates/fspy/tests/static_executable.rs @@ -45,14 +45,11 @@ async fn open_readwrite() { } #[test(tokio::test)] -async fn open_folder_only() { - // The open of a file fails without reading it, so it isn't recorded. +async fn open_directory() { + // The open fails on a file without reading it. let file = tempfile::NamedTempFile::new().unwrap(); - let accesses = track_test_bin(&["open_folder_only", file.path().to_str().unwrap()], None).await; + let accesses = track_test_bin(&["open_directory", file.path().to_str().unwrap()], None).await; assert_not_contains(&accesses, file.path()); - - let accesses = track_test_bin(&["open_folder_only", "/hello"], None).await; - assert_contains(&accesses, Path::new("/hello"), fspy::PathAccess::Read); } #[test(tokio::test)] diff --git a/crates/fspy_client_unix/src/convert.rs b/crates/fspy_client_unix/src/convert.rs index 6b3e72c93..9b21214aa 100644 --- a/crates/fspy_client_unix/src/convert.rs +++ b/crates/fspy_client_unix/src/convert.rs @@ -4,6 +4,7 @@ use allocator_api2::{alloc::Allocator, vec::Vec}; use bstr::ByteSlice; use fspy_nostd::{BorrowedFd, CWD}; use fspy_shared::ipc::AccessMode; +use fspy_shared_unix::open_flags::open_access_mode; use libc::{c_char, c_int}; #[cfg(target_os = "linux")] @@ -196,44 +197,41 @@ impl ToAbsolutePath for fspy_nostd::CStr<'_, fspy_nostd::Thin> { } pub trait ToAccessMode { - /// Converts the intercepted operation's mode into an access mode. + /// Converts the intercepted operation's mode into the access to record, + /// or `None` for one that isn't recorded. /// /// # Safety /// /// Implementations backed by raw process pointers require those pointers /// to remain valid for the conversion. - unsafe fn to_access_mode(self) -> AccessMode; + unsafe fn to_access_mode(self) -> Option; } impl ToAccessMode for AccessMode { - unsafe fn to_access_mode(self) -> AccessMode { - self + unsafe fn to_access_mode(self) -> Option { + Some(self) } } pub struct OpenFlags(pub c_int); impl ToAccessMode for OpenFlags { - unsafe fn to_access_mode(self) -> AccessMode { - match self.0 & libc::O_ACCMODE { - libc::O_RDWR => AccessMode::ReadWrite, - libc::O_WRONLY => AccessMode::Write, - _ => AccessMode::Read, - } + unsafe fn to_access_mode(self) -> Option { + open_access_mode(self.0) } } pub struct ModeStr(pub *const c_char); impl ToAccessMode for ModeStr { - unsafe fn to_access_mode(self) -> AccessMode { + unsafe fn to_access_mode(self) -> Option { // SAFETY: self.0 is a non-null pointer to a valid null-terminated C // string, as guaranteed by the libc calling convention. let mode_str = unsafe { CStr::from_ptr(self.0) }.to_bytes().as_bstr(); let has_read = mode_str.contains(&b'r'); let has_write = mode_str.contains(&b'w') || mode_str.contains(&b'a'); - match (has_read, has_write) { + Some(match (has_read, has_write) { (false, true) => AccessMode::Write, (true, true) => AccessMode::ReadWrite, _ => AccessMode::Read, - } + }) } } diff --git a/crates/fspy_client_unix/src/lib.rs b/crates/fspy_client_unix/src/lib.rs index d51c90f10..d6a68564e 100644 --- a/crates/fspy_client_unix/src/lib.rs +++ b/crates/fspy_client_unix/src/lib.rs @@ -143,7 +143,9 @@ impl<'a> Client<'a> { ) -> anyhow::Result<()> { // SAFETY: mode contains a valid pointer (if ModeStr) or a plain value, // as provided by the caller. - let mode = unsafe { mode.to_access_mode() }; + let Some(mode) = (unsafe { mode.to_access_mode() }) else { + return Ok(()); + }; let Some(abs_path) = path.to_absolute_path(&allocator)? else { return Ok(()); }; diff --git a/crates/fspy_nostd/src/fs/linux.rs b/crates/fspy_nostd/src/fs/linux.rs index 5b3fff8a3..259133da2 100644 --- a/crates/fspy_nostd/src/fs/linux.rs +++ b/crates/fspy_nostd/src/fs/linux.rs @@ -13,9 +13,6 @@ pub(super) const O_CREAT: u32 = linux_raw_sys::general::O_CREAT; pub(super) const O_EXCL: u32 = linux_raw_sys::general::O_EXCL; pub(super) const O_CLOEXEC: u32 = linux_raw_sys::general::O_CLOEXEC; pub(super) const AT_REMOVEDIR: u32 = linux_raw_sys::general::AT_REMOVEDIR; -pub(super) const AT_SYMLINK_NOFOLLOW: u32 = linux_raw_sys::general::AT_SYMLINK_NOFOLLOW; -pub(super) const S_IFMT: u32 = linux_raw_sys::general::S_IFMT; -pub(super) const S_IFDIR: u32 = linux_raw_sys::general::S_IFDIR; pub(super) type ModeBits = u32; pub(super) const S_IRUSR: ModeBits = linux_raw_sys::general::S_IRUSR; pub(super) const S_IWUSR: ModeBits = linux_raw_sys::general::S_IWUSR; @@ -72,34 +69,7 @@ pub(super) fn fstat(fd: BorrowedFd<'_>) -> Result { .map_err(Error::from)?; // SAFETY: a successful `fstat` initialized the complete structure. let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size, st_mode: raw.st_mode }) -} - -#[expect(clippy::needless_pass_by_value, reason = "CStr is a borrowed value type")] -pub(super) fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { - // x86_64 names the syscall `newfstatat`. - #[cfg(target_arch = "x86_64")] - const FSTATAT: syscalls::Sysno = syscalls::Sysno::newfstatat; - #[cfg(not(target_arch = "x86_64"))] - const FSTATAT: syscalls::Sysno = syscalls::Sysno::fstatat; - - let mut raw = core::mem::MaybeUninit::::zeroed(); - // SAFETY: `dirfd` remains borrowed, `path` is NUL-terminated, and `raw` - // points to writable storage for the kernel's architecture-specific - // `stat` structure. - unsafe { - syscalls::syscall!( - FSTATAT, - dirfd.as_raw_fd(), - path.as_ptr(), - raw.as_mut_ptr(), - flags.bits() - ) - } - .map_err(Error::from)?; - // SAFETY: a successful `fstatat` initialized the complete structure. - let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size, st_mode: raw.st_mode }) + Ok(Stat { st_size: raw.st_size }) } pub(super) fn ftruncate(fd: BorrowedFd<'_>, len: u64) -> Result<()> { diff --git a/crates/fspy_nostd/src/fs/mac.rs b/crates/fspy_nostd/src/fs/mac.rs index 6e76005b1..943dfd417 100644 --- a/crates/fspy_nostd/src/fs/mac.rs +++ b/crates/fspy_nostd/src/fs/mac.rs @@ -14,9 +14,6 @@ pub(super) const O_CREAT: u32 = libc::O_CREAT.cast_unsigned(); pub(super) const O_EXCL: u32 = libc::O_EXCL.cast_unsigned(); pub(super) const O_CLOEXEC: u32 = libc::O_CLOEXEC.cast_unsigned(); pub(super) const AT_REMOVEDIR: u32 = libc::AT_REMOVEDIR.cast_unsigned(); -pub(super) const AT_SYMLINK_NOFOLLOW: u32 = libc::AT_SYMLINK_NOFOLLOW.cast_unsigned(); -pub(super) const S_IFMT: u32 = libc::S_IFMT as u32; -pub(super) const S_IFDIR: u32 = libc::S_IFDIR as u32; pub(super) type ModeBits = libc::mode_t; pub(super) const S_IRUSR: ModeBits = libc::S_IRUSR; pub(super) const S_IWUSR: ModeBits = libc::S_IWUSR; @@ -64,28 +61,7 @@ pub(super) fn fstat(fd: BorrowedFd<'_>) -> Result { } // SAFETY: a successful `fstat` initialized the complete structure. let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size, st_mode: u32::from(raw.st_mode) }) -} - -#[expect(clippy::needless_pass_by_value, reason = "CStr is a borrowed value type")] -pub(super) fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { - let mut raw = core::mem::MaybeUninit::::zeroed(); - // SAFETY: `dirfd` remains borrowed, `path` is NUL-terminated, and `raw` - // is writable for the call. - if unsafe { - libc::fstatat( - dirfd.as_raw_fd(), - path.as_ptr().cast(), - raw.as_mut_ptr(), - flags.bits().cast_signed(), - ) - } == -1 - { - return Err(Error::last_os_error()); - } - // SAFETY: a successful `fstatat` initialized the complete structure. - let raw = unsafe { raw.assume_init() }; - Ok(Stat { st_size: raw.st_size, st_mode: u32::from(raw.st_mode) }) + Ok(Stat { st_size: raw.st_size }) } pub(super) fn ftruncate(fd: BorrowedFd<'_>, len: u64) -> Result<()> { diff --git a/crates/fspy_nostd/src/fs/unix.rs b/crates/fspy_nostd/src/fs/unix.rs index 31c14ac02..36c7ceeec 100644 --- a/crates/fspy_nostd/src/fs/unix.rs +++ b/crates/fspy_nostd/src/fs/unix.rs @@ -44,24 +44,13 @@ bitflags! { pub struct AtFlags: u32 { /// Remove a directory rather than a non-directory entry. const REMOVEDIR = imp::AT_REMOVEDIR; - /// Report on a symlink itself rather than on what it points to. - const SYMLINK_NOFOLLOW = imp::AT_SYMLINK_NOFOLLOW; } } -/// File metadata. +/// Metadata used by fspy's shared-memory backing files. #[derive(Clone, Copy, Debug)] pub struct Stat { pub st_size: i64, - pub st_mode: u32, -} - -impl Stat { - /// Whether the file is a directory. - #[must_use] - pub const fn is_dir(&self) -> bool { - self.st_mode & imp::S_IFMT == imp::S_IFDIR - } } /// The platform's maximum pathname size, including the terminating NUL. @@ -99,15 +88,6 @@ pub fn fstat(fd: BorrowedFd<'_>) -> Result { imp::fstat(fd) } -/// Returns metadata for `path` relative to `dirfd`. -/// -/// # Errors -/// -/// Returns the error reported by `fstatat`. -pub fn fstatat(dirfd: BorrowedFd<'_>, path: CStr<'_, R>, flags: AtFlags) -> Result { - imp::fstatat(dirfd, path, flags) -} - /// Sets the length of an open file. /// /// # Errors diff --git a/crates/fspy_preload_unix/src/interceptions/open.rs b/crates/fspy_preload_unix/src/interceptions/open.rs index 7dad63bdf..cb8ac5f3b 100644 --- a/crates/fspy_preload_unix/src/interceptions/open.rs +++ b/crates/fspy_preload_unix/src/interceptions/open.rs @@ -1,5 +1,3 @@ -use fspy_nostd::{CWD, fs::AtFlags}; -use fspy_shared_unix::open_flags::fails_without_reading; use libc::FILE; use crate::{ @@ -11,28 +9,6 @@ use crate::{ macros::intercept, }; -/// Records an open of `path` with `flags`, unless it fails without reading -/// the path. -/// -/// # Safety -/// -/// `path` must satisfy the contract of [`PathAt::borrow_raw`]. -unsafe fn handle_open_at(path: PathAt<'_, '_>, flags: c_int) { - let PathAt(dirfd, pathname) = path; - let skipped = fails_without_reading(flags, |follow_symlinks| { - let flags = if follow_symlinks { AtFlags::empty() } else { AtFlags::SYMLINK_NOFOLLOW }; - // The kernel ignores the descriptor for an absolute path, and a - // relative path at `-1` isn't recorded anyway. - fspy_nostd::fs::fstatat(dirfd.unwrap_or(CWD), pathname, flags) - .ok() - .map(|stat| stat.is_dir()) - }); - if !skipped { - // SAFETY: path is valid, as the caller guarantees. - unsafe { handle_open(path, OpenFlags(flags)) }; - } -} - const fn has_mode_arg(o_flags: c_int) -> bool { if o_flags & libc::O_CREAT != 0 { return true; @@ -52,7 +28,7 @@ type Mode = c_int; intercept!(open(64): unsafe extern "C" fn(*const c_char, c_int, args: ...) -> c_int); unsafe extern "C" fn open(path: *const c_char, flags: c_int, mut args: ...) -> c_int { // SAFETY: path is a valid C string pointer provided by the caller of the interposed function - unsafe { handle_open_at(PathAt(Some(CWD), fspy_nostd::CStr::from_ptr(path.cast())), flags) }; + unsafe { handle_open(fspy_nostd::CStr::from_ptr(path.cast()), OpenFlags(flags)) }; if has_mode_arg(flags) { // SAFETY: when O_CREAT or O_TMPFILE is set, a mode_t argument is required by the open() contract let mode: Mode = unsafe { args.next_arg() }; @@ -72,7 +48,7 @@ unsafe extern "C" fn openat( mut args: ... ) -> c_int { // SAFETY: dirfd is -1 or a valid file descriptor, and path is a valid C string, provided by the caller of the interposed function - unsafe { handle_open_at(PathAt::borrow_raw(dirfd, path), flags) }; + unsafe { handle_open(PathAt::borrow_raw(dirfd, path), OpenFlags(flags)) }; if has_mode_arg(flags) { // https://github.com/tailhook/openat/issues/21#issuecomment-535914957 @@ -91,7 +67,7 @@ intercept!(open_nocancel: unsafe extern "C" fn(*const c_char, c_int, ...) -> c_i #[cfg(target_os = "macos")] unsafe extern "C" fn open_nocancel(path: *const c_char, flags: c_int, mut args: ...) -> c_int { // SAFETY: path is a valid C string pointer provided by the caller of open$NOCANCEL - unsafe { handle_open_at(PathAt(Some(CWD), fspy_nostd::CStr::from_ptr(path.cast())), flags) }; + unsafe { handle_open(fspy_nostd::CStr::from_ptr(path.cast()), OpenFlags(flags)) }; if has_mode_arg(flags) { // SAFETY: O_CREAT requires a mode argument, matching the open$NOCANCEL contract let mode: Mode = unsafe { args.next_arg() }; @@ -113,7 +89,7 @@ unsafe extern "C" fn openat_nocancel( mut args: ... ) -> c_int { // SAFETY: dirfd is -1 or a valid file descriptor, and path is a valid C string, provided by the caller of openat$NOCANCEL - unsafe { handle_open_at(PathAt::borrow_raw(dirfd, path), flags) }; + unsafe { handle_open(PathAt::borrow_raw(dirfd, path), OpenFlags(flags)) }; if has_mode_arg(flags) { // SAFETY: O_CREAT requires a mode argument, matching the openat$NOCANCEL contract let mode: Mode = unsafe { args.next_arg() }; diff --git a/crates/fspy_shared_unix/src/open_flags.rs b/crates/fspy_shared_unix/src/open_flags.rs index d639cea44..d9c15a778 100644 --- a/crates/fspy_shared_unix/src/open_flags.rs +++ b/crates/fspy_shared_unix/src/open_flags.rs @@ -1,48 +1,25 @@ -use nix::libc::{O_ACCMODE, O_CREAT, O_DIRECTORY, O_NOFOLLOW, O_RDONLY, c_int}; +use fspy_shared::ipc::AccessMode; +use nix::libc::{O_ACCMODE, O_DIRECTORY, O_RDWR, O_WRONLY, c_int}; -/// Whether an open with `flags` fails without reading the path, so it isn't +/// The access an open with `flags` is recorded as, or `None` if it isn't /// recorded. /// -/// An `O_DIRECTORY` open for reading, without `O_CREAT`, opens only a -/// folder. For a path that is there and isn't one, it fails with `ENOTDIR`, -/// or with `ELOOP` for a symlink when `O_NOFOLLOW` is set. libc++'s +/// An `O_DIRECTORY` open reads nothing: it opens only a folder, and reading +/// the folder's entries is recorded on its own. libc++'s /// `std::filesystem::remove_all`, which Node's `fs.rmSync` uses on macOS, -/// opens every path it removes that way, and removes the path as a file when -/// the open fails. A folder, or a path that isn't there, is still recorded: -/// a task that lists a folder that way, as Go's `os.ReadDir` does, finds out -/// whether it's there. -/// -/// `is_folder` reports whether the path is a folder, following a final -/// symlink when its argument is `true`, or `None` when that can't be told, -/// as when the path isn't there. -pub fn fails_without_reading(flags: c_int, is_folder: impl FnOnce(bool) -> Option) -> bool { - flags & O_DIRECTORY != 0 - && flags & O_ACCMODE == O_RDONLY - && flags & O_CREAT == 0 - && is_folder(flags & O_NOFOLLOW == 0) == Some(false) -} - -#[cfg(test)] -mod tests { - use nix::libc::{O_CLOEXEC, O_RDWR, O_WRONLY}; - - use super::*; - - #[test] - fn folder_only_opens_of_non_folders_fail_without_reading() { - let not_a_folder = |_| Some(false); - // libc++'s `remove_all`. - assert!(fails_without_reading( - O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC, - not_a_folder - )); - assert!(fails_without_reading(O_RDONLY | O_DIRECTORY, not_a_folder)); - assert!(!fails_without_reading(O_RDONLY, not_a_folder)); - assert!(!fails_without_reading(O_WRONLY | O_DIRECTORY, not_a_folder)); - assert!(!fails_without_reading(O_RDWR | O_DIRECTORY, not_a_folder)); - assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY | O_CREAT, not_a_folder)); - - assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY, |_| Some(true))); - assert!(!fails_without_reading(O_RDONLY | O_DIRECTORY, |_| None)); +/// opens every path it removes that way and removes the path as a file when +/// the open fails, so recording the open would make every file it removes an +/// input. On Linux, `O_TMPFILE` includes `O_DIRECTORY`; such an open makes a +/// file without a name, and giving it one with `linkat` is recorded as a +/// write. +#[must_use] +pub const fn open_access_mode(flags: c_int) -> Option { + if flags & O_DIRECTORY != 0 { + return None; } + Some(match flags & O_ACCMODE { + O_RDWR => AccessMode::ReadWrite, + O_WRONLY => AccessMode::Write, + _ => AccessMode::Read, + }) } diff --git a/crates/fspy_test_bin/src/main.rs b/crates/fspy_test_bin/src/main.rs index 4d343d04c..68d4cd78a 100644 --- a/crates/fspy_test_bin/src/main.rs +++ b/crates/fspy_test_bin/src/main.rs @@ -29,7 +29,7 @@ fn main() { "open_readwrite" => { let _ = File::options().read(true).write(true).open(path); } - "open_folder_only" => { + "open_directory" => { use std::os::unix::fs::OpenOptionsExt as _; let _ = File::options().read(true).custom_flags(nix::libc::O_DIRECTORY).open(path); }