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..31bc3f0f7 100644 --- a/crates/fspy/src/unix/syscall_handler/mod.rs +++ b/crates/fspy/src/unix/syscall_handler/mod.rs @@ -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::open_access_mode; const PATH_MAX: usize = libc::PATH_MAX as usize; @@ -43,10 +44,8 @@ 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(mode) = open_access_mode(flags) else { + return Ok(()); }; self.handle_path(caller, dir_fd, path_ptr, mode) } diff --git a/crates/fspy/tests/rust_std.rs b/crates/fspy/tests/rust_std.rs index 9bbbc37b4..0a93b897b 100644 --- a/crates/fspy/tests/rust_std.rs +++ b/crates/fspy/tests/rust_std.rs @@ -35,6 +35,35 @@ async fn open_write() -> anyhow::Result<()> { Ok(()) } +/// 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_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"))?; + 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 in ["file", "folder"] { + let _ = OpenOptions::new() + .read(true) + .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("folder")); + + Ok(()) +} + #[test(tokio::test)] async fn rename_destination() -> anyhow::Result<()> { let tmp_dir = tempfile::tempdir()?; @@ -249,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 9de887a03..c2db10a4e 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,14 @@ async fn open_readwrite() { assert_contains(&accesses, Path::new("/hello"), fspy::PathAccess::ReadWrite); } +#[test(tokio::test)] +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_directory", file.path().to_str().unwrap()], None).await; + assert_not_contains(&accesses, file.path()); +} + #[test(tokio::test)] async fn openat2_read() { let accesses = track_test_bin(&["openat2_read", "/hello"], None).await; 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_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..d9c15a778 --- /dev/null +++ b/crates/fspy_shared_unix/src/open_flags.rs @@ -0,0 +1,25 @@ +use fspy_shared::ipc::AccessMode; +use nix::libc::{O_ACCMODE, O_DIRECTORY, O_RDWR, O_WRONLY, c_int}; + +/// The access an open with `flags` is recorded as, or `None` if it isn't +/// recorded. +/// +/// 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, 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 de88d8ece..68d4cd78a 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_directory" => { + 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 {