diff --git a/CHANGELOG.md b/CHANGELOG.md index b342fb6fc..cf39852a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +- **Fixed** In cached tasks on Linux and macOS, programs that pass `-1` as a directory descriptor to calls such as `openat` no longer abort. For example, `ps -p ` from procps-ng 4.0.6 or later aborted when the process had already exited ([#862](https://github.com/voidzero-dev/vite-task/pull/862)). - **Fixed** `vp run` no longer panics before running a script that contains non-ASCII characters, such as `echo こんにちは`, and no longer cuts off such commands where it displays them ([#861](https://github.com/voidzero-dev/vite-task/pull/861)). - **Fixed** On Linux, concurrent system calls in tracked programs, such as `pnpm dlx`, no longer cause a stack overflow ([#854](https://github.com/voidzero-dev/vite-task/pull/854)). - **Fixed** Cached tasks can now run scripts whose shebang names an interpreter path longer than 126 bytes. Before, the path was cut short and the script failed to start or ran the wrong program. On macOS, text after a `#` in the shebang line is now ignored, as it is when the script runs on its own ([#788](https://github.com/voidzero-dev/vite-task/pull/788)). diff --git a/crates/fspy/tests/invalid_dirfd.rs b/crates/fspy/tests/invalid_dirfd.rs new file mode 100644 index 000000000..6dfc802a5 --- /dev/null +++ b/crates/fspy/tests/invalid_dirfd.rs @@ -0,0 +1,193 @@ +//! A tracked program may pass `-1` as a directory descriptor, typically a +//! failed `open` result that it does not check. With a relative path the call +//! fails with `EBADF`; with an absolute path the descriptor is ignored. The +//! interposers must forward both cases instead of aborting the program. +//! +//! procps-ng's `ps -p ` does this when the process has exited: it opens +//! `/proc/`, then calls `openat(-1, "status", ...)`. +//! +//! Directory descriptors exist only on Unix; Windows has no equivalent calls. +#![cfg(unix)] + +mod test_utils; + +use fspy::PathAccess; +use test_log::test; +use test_utils::{assert_contains, assert_not_contains}; + +fn errno() -> libc::c_int { + std::io::Error::last_os_error().raw_os_error().unwrap() +} + +/// `STATX_BASIC_STATS`. The libc crate does not define it for every Linux +/// target, such as musl without `musl_v1_2_3`. +#[cfg(target_os = "linux")] +const STATX_BASIC_STATS: libc::c_uint = 0x7ff; + +/// Storage for the kernel's 256-byte `struct statx`. The libc crate does not +/// define the struct for every Linux target. +#[cfg(target_os = "linux")] +#[repr(C, align(8))] +struct StatxBuf([u8; 256]); + +/// Calls `statx` through `syscall`, which fspy intercepts on glibc. +/// +/// The libc `statx` wrapper is not called directly: glibc added it in 2.28, +/// so a test built for an older glibc fails to link. +#[cfg(target_os = "linux")] +fn syscall_statx( + dirfd: libc::c_int, + path: *const libc::c_char, + flags: libc::c_int, +) -> libc::c_long { + let mut buf = StatxBuf([0; 256]); + // SAFETY: `path` is null or a valid C string, and `buf` is writable + // storage of the size that the kernel writes. + unsafe { + libc::syscall( + libc::SYS_statx, + dirfd, + path, + flags, + STATX_BASIC_STATS, + core::ptr::from_mut(&mut buf), + ) + } +} + +/// Calls the glibc `statx` wrapper, resolved at run time. With `LD_PRELOAD`, +/// the lookup finds the fspy interposer first. +/// +/// # Panics +/// +/// Panics if glibc has no `statx` (added in 2.28). +#[cfg(all(target_os = "linux", target_env = "gnu"))] +fn glibc_statx(dirfd: libc::c_int, path: *const libc::c_char, flags: libc::c_int) -> libc::c_int { + type Statx = unsafe extern "C" fn( + libc::c_int, + *const libc::c_char, + libc::c_int, + libc::c_uint, + *mut StatxBuf, + ) -> libc::c_int; + // SAFETY: the name is a valid C string. + let symbol = unsafe { libc::dlsym(libc::RTLD_DEFAULT, c"statx".as_ptr()) }; + assert!(!symbol.is_null(), "glibc has no statx"); + // SAFETY: glibc's `statx` has this signature. + let statx = unsafe { core::mem::transmute::<*mut libc::c_void, Statx>(symbol) }; + let mut buf = StatxBuf([0; 256]); + // SAFETY: `path` is null or a valid C string, and `buf` is writable + // storage of the size that the kernel writes. + unsafe { statx(dirfd, path, flags, STATX_BASIC_STATS, core::ptr::from_mut(&mut buf)) } +} + +/// Calls the glibc `execveat` wrapper, resolved at run time for the same +/// reason as [`glibc_statx`]. +/// +/// # Panics +/// +/// Panics if glibc has no `execveat` (added in 2.34). +#[cfg(all(target_os = "linux", target_env = "gnu"))] +fn glibc_execveat(dirfd: libc::c_int, path: &core::ffi::CStr) -> libc::c_int { + type Execveat = unsafe extern "C" fn( + libc::c_int, + *const libc::c_char, + *const *const libc::c_char, + *const *const libc::c_char, + libc::c_int, + ) -> libc::c_int; + // SAFETY: the name is a valid C string. + let symbol = unsafe { libc::dlsym(libc::RTLD_DEFAULT, c"execveat".as_ptr()) }; + assert!(!symbol.is_null(), "glibc has no execveat"); + // SAFETY: glibc's `execveat` has this signature. + let execveat = unsafe { core::mem::transmute::<*mut libc::c_void, Execveat>(symbol) }; + let argv = [path.as_ptr(), core::ptr::null()]; + let envp = [core::ptr::null()]; + // SAFETY: `path` is a valid C string, and `argv` and `envp` are + // null-terminated arrays of valid C strings. + unsafe { execveat(dirfd, path.as_ptr(), argv.as_ptr(), envp.as_ptr(), 0) } +} + +#[test(tokio::test)] +async fn relative_path_at_invalid_dirfd() -> anyhow::Result<()> { + let accesses = track_fn!((), |(): ()| { + let path = c"hello"; + // SAFETY: every call below receives a valid C string and writable + // output storage; the kernel rejects the descriptor. + unsafe { + assert_eq!(libc::openat(-1, path.as_ptr(), libc::O_RDONLY), -1); + assert_eq!(errno(), libc::EBADF); + + let mut stat = std::mem::MaybeUninit::::uninit(); + assert_eq!(libc::fstatat(-1, path.as_ptr(), stat.as_mut_ptr(), 0), -1); + assert_eq!(errno(), libc::EBADF); + + assert_eq!(libc::faccessat(-1, path.as_ptr(), libc::F_OK, 0), -1); + assert_eq!(errno(), libc::EBADF); + + assert!(libc::fdopendir(-1).is_null()); + assert_eq!(errno(), libc::EBADF); + } + #[cfg(target_os = "linux")] + { + assert_eq!(syscall_statx(-1, path.as_ptr(), 0), -1); + assert_eq!(errno(), libc::EBADF); + } + #[cfg(all(target_os = "linux", target_env = "gnu"))] + { + assert_eq!(glibc_statx(-1, path.as_ptr(), 0), -1); + assert_eq!(errno(), libc::EBADF); + + assert_eq!(glibc_execveat(-1, path), -1); + assert_eq!(errno(), libc::EBADF); + } + }) + .await?; + // `-1` is not the current directory. The child inherits the current + // directory of the test. + assert_not_contains(&accesses, &std::env::current_dir()?.join("hello")); + Ok(()) +} + +/// `statx` with a null path and `AT_EMPTY_PATH` uses only the descriptor. +#[cfg(target_os = "linux")] +#[test(tokio::test)] +async fn empty_path_at_invalid_dirfd() -> anyhow::Result<()> { + track_fn!((), |(): ()| { + // Linux 6.11 and later accept a null path with `AT_EMPTY_PATH` and + // reject the descriptor. Earlier kernels reject the null path. + let rejected = [libc::EBADF, libc::EFAULT]; + + assert_eq!(syscall_statx(-1, core::ptr::null(), libc::AT_EMPTY_PATH), -1); + assert!(rejected.contains(&errno()), "unexpected errno {}", errno()); + + #[cfg(target_env = "gnu")] + { + assert_eq!(glibc_statx(-1, core::ptr::null(), libc::AT_EMPTY_PATH), -1); + assert!(rejected.contains(&errno()), "unexpected errno {}", errno()); + } + }) + .await?; + Ok(()) +} + +#[test(tokio::test)] +async fn absolute_path_at_invalid_dirfd() -> anyhow::Result<()> { + let tmp_dir = tempfile::tempdir()?; + let tmp_path = std::fs::canonicalize(tmp_dir.path())?.join("hello"); + std::fs::write(&tmp_path, "")?; + + let accesses = track_fn!(tmp_path.to_str().unwrap().to_owned(), |tmp_path: String| { + let path = std::ffi::CString::new(tmp_path).unwrap(); + // SAFETY: `path` is a valid C string; an absolute path makes the + // kernel ignore the descriptor. + let fd = unsafe { libc::openat(-1, path.as_ptr(), libc::O_RDONLY) }; + assert!(fd >= 0, "openat failed with errno {}", errno()); + // SAFETY: `fd` was opened above and is closed once. + unsafe { libc::close(fd) }; + }) + .await?; + assert_contains(&accesses, &tmp_path, PathAccess::Read); + + Ok(()) +} diff --git a/crates/fspy/tests/test_utils/mod.rs b/crates/fspy/tests/test_utils/mod.rs index 0ae779051..4ffa8f6d1 100644 --- a/crates/fspy/tests/test_utils/mod.rs +++ b/crates/fspy/tests/test_utils/mod.rs @@ -57,6 +57,36 @@ pub fn assert_contains<'a>( ); } +/// Asserts that `unexpected_path` was not accessed in any way. +/// +/// # Panics +/// +/// Panics if the path was accessed. +#[expect( + clippy::allow_attributes, + reason = "allow attribute required for conditionally-used helper" +)] +#[allow(dead_code, reason = "not all test files use this helper")] +#[track_caller] +pub fn assert_not_contains(accesses: &PathAccessIterable, unexpected_path: &Path) { + let matching = accesses + .iter() + .filter(|access| { + access.paths().any(|path| { + path.strip_path_prefix(unexpected_path, |strip_result| { + strip_result.is_ok_and(|stripped| stripped.as_os_str().is_empty()) + }) + }) + }) + .collect::>(); + assert!( + matching.is_empty(), + "Expected no accesses to {}, found: {:?}", + unexpected_path.display(), + matching + ); +} + /// Spawns a subprocess that executes the given function with file access tracking. /// /// - $arg: The argument to pass to the function diff --git a/crates/fspy_client_unix/src/convert.rs b/crates/fspy_client_unix/src/convert.rs index 53c3db99c..d8f28b6a7 100644 --- a/crates/fspy_client_unix/src/convert.rs +++ b/crates/fspy_client_unix/src/convert.rs @@ -107,19 +107,38 @@ impl ToAbsolutePath for BorrowedFd<'_> { } } -pub struct PathAt<'fd, 'path>(pub BorrowedFd<'fd>, pub fspy_nostd::CStr<'path, fspy_nostd::Thin>); +impl ToAbsolutePath for Option { + fn to_absolute_path<'a, A: Allocator>( + self, + allocator: &'a A, + ) -> nix::Result>> + where + Self: 'a, + { + self.map_or(Ok(None), |path| path.to_absolute_path(allocator)) + } +} + +/// A pathname resolved against a directory descriptor, as in `openat`. +/// +/// The descriptor is `None` when the caller passed `-1`. The kernel ignores +/// it for an absolute pathname and fails a relative one with `EBADF`. +pub struct PathAt<'fd, 'path>( + pub Option>, + pub fspy_nostd::CStr<'path, fspy_nostd::Thin>, +); impl PathAt<'_, '_> { /// Borrows raw directory-descriptor and pathname arguments. /// /// # Safety /// - /// `fd` must remain valid while the returned value is used, and `path` - /// must point to a valid NUL-terminated string. + /// `fd` must be `-1` or remain valid while the returned value is used, + /// and `path` must point to a valid NUL-terminated string. #[must_use] pub const unsafe fn borrow_raw(fd: c_int, path: *const c_char) -> Self { // SAFETY: both invariants are upheld by the caller. - Self(unsafe { BorrowedFd::borrow_raw(fd) }, unsafe { + Self(unsafe { BorrowedFd::try_borrow_raw(fd) }, unsafe { fspy_nostd::CStr::from_ptr(path.cast()) }) } @@ -140,7 +159,11 @@ impl ToAbsolutePath for PathAt<'_, '_> { // Already absolute, and already NUL-terminated by the caller. Ok(Some(counted)) } else { - let Some(mut base) = get_fd_path(allocator, self.0)? else { + // A relative pathname at `-1` accesses nothing: the call fails. + let Some(dirfd) = self.0 else { + return Ok(None); + }; + let Some(mut base) = get_fd_path(allocator, dirfd)? else { return Ok(None); }; if !pathname.is_empty() { @@ -167,7 +190,7 @@ impl ToAbsolutePath for fspy_nostd::CStr<'_, fspy_nostd::Thin> { where Self: 'a, { - PathAt(CWD, self).to_absolute_path(allocator) + PathAt(Some(CWD), self).to_absolute_path(allocator) } } diff --git a/crates/fspy_nostd/src/fd.rs b/crates/fspy_nostd/src/fd.rs index 202411033..c7b894850 100644 --- a/crates/fspy_nostd/src/fd.rs +++ b/crates/fspy_nostd/src/fd.rs @@ -36,6 +36,27 @@ impl BorrowedFd<'_> { Self { fd, lifetime: PhantomData } } + /// Borrows a descriptor argument passed by a caller, or returns `None` + /// when it is `-1`. + /// + /// Callers can pass `-1` unchecked from a failed call such as `open`. + /// The kernel rejects it with `EBADF` wherever it uses the descriptor, + /// so it is not a caller error to report here. + /// + /// # Safety + /// + /// Any value other than `-1` must satisfy [`Self::borrow_raw`]. + #[must_use] + pub const unsafe fn try_borrow_raw(fd: RawFd) -> Option { + if fd == -1 { + None + } else { + // SAFETY: `fd` is not `-1`, and the caller upholds the + // remaining requirements of `borrow_raw`. + Some(unsafe { Self::borrow_raw(fd) }) + } + } + /// Returns the raw descriptor without transferring ownership. #[must_use] pub const fn as_raw_fd(self) -> RawFd { diff --git a/crates/fspy_preload_unix/src/interceptions/access.rs b/crates/fspy_preload_unix/src/interceptions/access.rs index c856a2bbf..db220a314 100644 --- a/crates/fspy_preload_unix/src/interceptions/access.rs +++ b/crates/fspy_preload_unix/src/interceptions/access.rs @@ -23,7 +23,7 @@ unsafe extern "C" fn faccessat( mode: c_int, flags: c_int, ) -> c_int { - // SAFETY: dirfd and pathname are valid arguments provided by the caller of the interposed function + // SAFETY: dirfd is -1 or a valid file descriptor, and pathname is a valid C string, provided by the caller of the interposed function unsafe { handle_open(PathAt::borrow_raw(dirfd, pathname), AccessMode::Read); } diff --git a/crates/fspy_preload_unix/src/interceptions/dirent.rs b/crates/fspy_preload_unix/src/interceptions/dirent.rs index e4d7c781e..048f8f447 100644 --- a/crates/fspy_preload_unix/src/interceptions/dirent.rs +++ b/crates/fspy_preload_unix/src/interceptions/dirent.rs @@ -51,8 +51,8 @@ mod macos_only { buf_len: usize, basep: *mut i64, ) -> isize { - // SAFETY: fd is a valid file descriptor provided by the caller of __getdirentries64 - unsafe { handle_open(BorrowedFd::borrow_raw(fd), AccessMode::ReadDir) }; + // SAFETY: fd is -1 or a valid file descriptor provided by the caller of __getdirentries64 + unsafe { handle_open(BorrowedFd::try_borrow_raw(fd), AccessMode::ReadDir) }; // SAFETY: calling the original libc __getdirentries64() with the same arguments forwarded from the interposed function unsafe { __getdirentries64::original()(fd, buf, buf_len, basep) } } @@ -65,16 +65,16 @@ unsafe extern "C" fn getdirentries( nbytes: c_int, basep: *mut c_long, ) -> c_int { - // SAFETY: fd is a valid file descriptor provided by the caller of the interposed function - unsafe { handle_open(BorrowedFd::borrow_raw(fd), AccessMode::ReadDir) }; + // SAFETY: fd is -1 or a valid file descriptor provided by the caller of the interposed function + unsafe { handle_open(BorrowedFd::try_borrow_raw(fd), AccessMode::ReadDir) }; // SAFETY: calling the original libc getdirentries() with the same arguments forwarded from the interposed function unsafe { getdirentries::original()(fd, buf, nbytes, basep) } } intercept!(fdopendir(64): unsafe extern "C" fn (fd: c_int) -> *mut DIR); unsafe extern "C" fn fdopendir(fd: c_int) -> *mut DIR { - // SAFETY: fd is a valid file descriptor provided by the caller of the interposed function - unsafe { handle_open(BorrowedFd::borrow_raw(fd), AccessMode::ReadDir) }; + // SAFETY: fd is -1 or a valid file descriptor provided by the caller of the interposed function + unsafe { handle_open(BorrowedFd::try_borrow_raw(fd), AccessMode::ReadDir) }; // SAFETY: calling the original libc fdopendir() with the same arguments forwarded from the interposed function unsafe { fdopendir::original()(fd) } } diff --git a/crates/fspy_preload_unix/src/interceptions/linux_syscall.rs b/crates/fspy_preload_unix/src/interceptions/linux_syscall.rs index a9d7750c0..f351ca1fa 100644 --- a/crates/fspy_preload_unix/src/interceptions/linux_syscall.rs +++ b/crates/fspy_preload_unix/src/interceptions/linux_syscall.rs @@ -38,11 +38,11 @@ unsafe extern "C" fn syscall(syscall_no: c_long, mut args: ...) -> c_long { let flags = a2 as c_int; if pathname.is_null() { if flags & libc::AT_EMPTY_PATH != 0 { - // SAFETY: dirfd is provided by the statx syscall caller. - unsafe { handle_open(BorrowedFd::borrow_raw(dirfd), AccessMode::Read) }; + // SAFETY: dirfd is -1 or a valid file descriptor provided by the statx syscall caller. + unsafe { handle_open(BorrowedFd::try_borrow_raw(dirfd), AccessMode::Read) }; } } else { - // SAFETY: pathname is a non-null C string pointer provided by the statx syscall caller. + // SAFETY: dirfd is -1 or a valid file descriptor, and pathname is a non-null C string pointer, provided by the statx syscall caller. unsafe { handle_open(PathAt::borrow_raw(dirfd, pathname), AccessMode::Read); }; diff --git a/crates/fspy_preload_unix/src/interceptions/open.rs b/crates/fspy_preload_unix/src/interceptions/open.rs index e4e6138a2..cb8ac5f3b 100644 --- a/crates/fspy_preload_unix/src/interceptions/open.rs +++ b/crates/fspy_preload_unix/src/interceptions/open.rs @@ -47,7 +47,7 @@ unsafe extern "C" fn openat( flags: c_int, mut args: ... ) -> c_int { - // SAFETY: dirfd and path are valid arguments provided by the caller of the interposed function + // 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)) }; if has_mode_arg(flags) { @@ -88,7 +88,7 @@ unsafe extern "C" fn openat_nocancel( flags: c_int, mut args: ... ) -> c_int { - // SAFETY: dirfd and path are valid arguments provided by the caller of openat$NOCANCEL + // 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)) }; if has_mode_arg(flags) { // SAFETY: O_CREAT requires a mode argument, matching the openat$NOCANCEL contract diff --git a/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs b/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs index 729162d02..6aa15e4aa 100644 --- a/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs +++ b/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs @@ -240,7 +240,7 @@ mod linux_only { let _unused = execveat::original; let arena = fspy_nostd_alloc::pooled_bump(); - // SAFETY: dirfd and pathname are valid arguments from the interposed execveat call. + // SAFETY: dirfd is -1 or a valid file descriptor, and pathname is a valid C string, from the interposed execveat call. let path = unsafe { PathAt::borrow_raw(dirfd, pathname) }; let abs_path = match path.to_absolute_path(&arena) { Ok(None) => { diff --git a/crates/fspy_preload_unix/src/interceptions/stat.rs b/crates/fspy_preload_unix/src/interceptions/stat.rs index 050288500..a386de6b5 100644 --- a/crates/fspy_preload_unix/src/interceptions/stat.rs +++ b/crates/fspy_preload_unix/src/interceptions/stat.rs @@ -36,7 +36,7 @@ unsafe extern "C" fn fstatat( buf: *mut stat_struct, flags: c_int, ) -> c_int { - // SAFETY: dirfd and pathname are valid arguments provided by the caller of the interposed function + // SAFETY: dirfd is -1 or a valid file descriptor, and pathname is a valid C string, provided by the caller of the interposed function unsafe { handle_open(PathAt::borrow_raw(dirfd, pathname), AccessMode::Read); } @@ -70,11 +70,11 @@ unsafe extern "C" fn statx( if pathname.is_null() { if flags & libc::AT_EMPTY_PATH != 0 { - // SAFETY: dirfd is provided by the statx caller. - unsafe { handle_open(BorrowedFd::borrow_raw(dirfd), AccessMode::Read) }; + // SAFETY: dirfd is -1 or a valid file descriptor provided by the statx caller. + unsafe { handle_open(BorrowedFd::try_borrow_raw(dirfd), AccessMode::Read) }; } } else { - // SAFETY: pathname is a non-null C string pointer provided by the statx caller. + // SAFETY: dirfd is -1 or a valid file descriptor, and pathname is a non-null C string pointer, provided by the statx caller. unsafe { handle_open(PathAt::borrow_raw(dirfd, pathname), AccessMode::Read) }; } // SAFETY: calling the original libc statx() with the same arguments forwarded from the interposed function