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 6aa15e4aa..51ed0f942 100644 --- a/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs +++ b/crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs @@ -1,9 +1,8 @@ -mod with_argv; +use std::{ffi::VaList, ptr::null}; -use allocator_api2::alloc::Allocator; +use allocator_api2::{alloc::Allocator, vec::Vec}; use fspy_shared_unix::exec::ExecResolveConfig; use libc::{c_char, c_int}; -use with_argv::with_argv; use crate::{ client::{global_client, raw_exec::RawExec}, @@ -25,6 +24,32 @@ pub unsafe fn environ() -> *const *const c_char { unsafe { environ } } +/// Collects `arg` and the variadic arguments after it into the +/// NULL-terminated argv array that `execl`, `execlp` and `execle` take as a +/// list. The array lives in `allocator`, not libc's heap: exec may run in +/// the child of `fork()`, where malloc's lock may be held by a thread that +/// no longer exists. `va` is left past the terminator, where `execle` finds +/// `envp`. +/// +/// # Safety +/// +/// Unless `arg` is NULL, `va` must hold `*const c_char` arguments up to a +/// NULL pointer. +unsafe fn collect_argv( + allocator: A, + mut arg: *const c_char, + va: &mut VaList<'_>, +) -> Vec<*const c_char, A> { + let mut argv = Vec::new_in(allocator); + while !arg.is_null() { + argv.push(arg); + // SAFETY: the caller guarantees the arguments continue up to a NULL pointer. + arg = unsafe { va.next_arg() }; + } + argv.push(null()); + argv +} + fn handle_exec( allocator: impl Allocator, config: ExecResolveConfig, @@ -77,67 +102,48 @@ unsafe extern "C" fn execve( } intercept!(execl(64): unsafe extern "C" fn(path: *const c_char, arg0: *const c_char, ...) -> c_int); -unsafe extern "C" fn execl(path: *const c_char, arg0: *const c_char, valist: ...) -> c_int { +unsafe extern "C" fn execl(path: *const c_char, arg: *const c_char, mut valist: ...) -> c_int { #[expect( clippy::no_effect_underscore_binding, reason = "suppresses unused warning on *::original" )] let _unused = execl::original; - // SAFETY: valist and arg0 are valid variadic arguments forwarded from the interposed execl function - unsafe { - with_argv(valist, arg0, |args, _remaining| { - handle_exec( - fspy_nostd_alloc::pooled_bump(), - ExecResolveConfig::search_path_disabled(), - path, - args.as_ptr(), - environ(), - ) - }) - } + let arena = fspy_nostd_alloc::pooled_bump(); + // SAFETY: valist holds execl's NULL-terminated argument list after arg, + // and environ() returns the valid process environment pointer + let (argv, envp) = unsafe { (collect_argv(&arena, arg, &mut valist), environ()) }; + handle_exec(&arena, ExecResolveConfig::search_path_disabled(), path, argv.as_ptr(), envp) } intercept!(execlp(64): unsafe extern "C" fn(path: *const c_char, arg0: *const c_char, ...) -> c_int); -unsafe extern "C" fn execlp(path: *const c_char, arg0: *const c_char, valist: ...) -> c_int { +unsafe extern "C" fn execlp(path: *const c_char, arg: *const c_char, mut valist: ...) -> c_int { #[expect( clippy::no_effect_underscore_binding, reason = "suppresses unused warning on *::original" )] let _unused = execlp::original; - // SAFETY: valist and arg0 are valid variadic arguments forwarded from the interposed execlp function - unsafe { - with_argv(valist, arg0, |args, _remaining| { - handle_exec( - fspy_nostd_alloc::pooled_bump(), - ExecResolveConfig::search_path_enabled(None), - path, - args.as_ptr(), - environ(), - ) - }) - } + let arena = fspy_nostd_alloc::pooled_bump(); + // SAFETY: valist holds execlp's NULL-terminated argument list after arg, + // and environ() returns the valid process environment pointer + let (argv, envp) = unsafe { (collect_argv(&arena, arg, &mut valist), environ()) }; + handle_exec(&arena, ExecResolveConfig::search_path_enabled(None), path, argv.as_ptr(), envp) } intercept!(execle(64): unsafe extern "C" fn(path: *const c_char, arg0: *const c_char, ...) -> c_int); -unsafe extern "C" fn execle(path: *const c_char, arg0: *const c_char, valist: ...) -> c_int { +unsafe extern "C" fn execle(path: *const c_char, arg: *const c_char, mut valist: ...) -> c_int { #[expect( clippy::no_effect_underscore_binding, reason = "suppresses unused warning on *::original" )] let _unused = execle::original; - // SAFETY: valist and arg0 are valid variadic arguments forwarded from the interposed execle function - unsafe { - with_argv(valist, arg0, |args, mut remaining| { - let envp = remaining.next_arg::<*const *const c_char>(); - handle_exec( - fspy_nostd_alloc::pooled_bump(), - ExecResolveConfig::search_path_disabled(), - path, - args.as_ptr(), - envp, - ) - }) - } + let arena = fspy_nostd_alloc::pooled_bump(); + // SAFETY: valist holds execle's NULL-terminated argument list after arg, + // followed by envp + let (argv, envp) = unsafe { + let argv = collect_argv(&arena, arg, &mut valist); + (argv, valist.next_arg::<*const *const c_char>()) + }; + handle_exec(&arena, ExecResolveConfig::search_path_disabled(), path, argv.as_ptr(), envp) } intercept!(execv(64): unsafe extern "C" fn(path: *const c_char, argv: *const *const c_char) -> c_int); diff --git a/crates/fspy_preload_unix/src/interceptions/spawn/exec/with_argv.rs b/crates/fspy_preload_unix/src/interceptions/spawn/exec/with_argv.rs deleted file mode 100644 index 8f1971a95..000000000 --- a/crates/fspy_preload_unix/src/interceptions/spawn/exec/with_argv.rs +++ /dev/null @@ -1,66 +0,0 @@ -use std::{ - ffi::VaList, - mem::{self, MaybeUninit}, - slice, -}; - -use libc::{c_char, c_int}; -use nix::Error; - -// https://github.com/redox-os/relibc/blob/710911febb07a43716a6236cc9e5b864e227e36e/src/header/unistd/mod.rs#L1094 -#[expect(clippy::similar_names, reason = "arg0 and argc are standard C naming conventions")] -pub unsafe fn with_argv( - mut va: VaList, - arg0: *const c_char, - f: impl FnOnce(&[*const c_char], VaList) -> c_int, -) -> c_int { - let argc = 1 + { - let mut va = va.clone(); - // Safety: argv is guaranteed to be NULL-terminated - core::iter::from_fn(|| Some(unsafe { va.next_arg::<*const c_char>() })) - .position(|s| { - // Find the NULL terminator - s.is_null() - }) - .unwrap() - }; - - let mut stack: [MaybeUninit<*const c_char>; 32] = [MaybeUninit::uninit(); 32]; - - let out = if argc < 32 { - stack.as_mut_slice() - } else if argc < 4096 { - // TODO: Use ARG_MAX, not this hardcoded constant - // SAFETY: requesting a heap allocation of the correct size for argc + 1 pointers, including the NULL terminator - let ptr = unsafe { libc::malloc((argc + 1) * mem::size_of::<*const c_char>()) }; - if ptr.is_null() { - Error::ENOMEM.set(); - return -1; - } - // SAFETY: ptr is non-null (checked above), properly aligned, and points to argc + 1 elements worth of allocated memory - unsafe { slice::from_raw_parts_mut(ptr.cast::>(), argc + 1) } - } else { - Error::E2BIG.set(); - return -1; - }; - out[0].write(arg0); - - for item in out.iter_mut().take(argc).skip(1) { - // SAFETY: extracting the next *const c_char argument from the va_list; the count was pre-validated - item.write(unsafe { va.next_arg::<*const c_char>() }); - } - out[argc].write(core::ptr::null()); - // SAFETY: consuming the NULL terminator from the va_list to advance past it - unsafe { va.next_arg::<*const c_char>() }; - - // Safety: MaybeUninit<*const c_char> has the same layout as *const c_char, - // and all elements have been initialized via write() above. - f(unsafe { &*(&raw const *out as *const [*const c_char]) }, va); - - // f only returns if it fails - if argc >= 32 { - // SAFETY: out was allocated with libc::malloc above (argc >= 32 branch), so it must be freed with libc::free - unsafe { libc::free(out.as_mut_ptr().cast()) }; - } - -1 -}