Skip to content

Commit 1f8f298

Browse files
wan9chiclaude
andcommitted
fix(fspy): keep execveat's resolved path alive across the exec, in the per-call arena
The arena's first user moves from the fd-relative path join to execveat, for two reasons. The join was the wrong showcase: the benchmark suite added for it showed the arena costing ~3% on that lane, because the code it replaced is nearly free — getcwd() already returns a buffer with room to append, so PathBuf::push rarely allocates at all. That code comes back unchanged. execveat is the right one. Its resolved absolute path must be copied into fresh NUL-terminated storage, exec runs in the child of fork() in multithreaded programs (posix_spawn does exactly that) where malloc can deadlock on a lock held by a vanished thread — and the previous code had a use-after-free: it built a CString, moved it into a match arm that extracted the raw pointer, dropped it at the arm's end, and passed the dangling pointer to the real exec. The arena copy is borrowed from a binding that provably outlives the exec call, and dropping CString::new also drops its interior-NUL panic path from an interception. execveat currently has no test coverage — which is how the dangling pointer survived. That gap is left for a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7f63a93 commit 1f8f298

2 files changed

Lines changed: 25 additions & 23 deletions

File tree

crates/fspy_preload_unix/src/client/convert.rs

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
#[cfg(target_os = "linux")]
22
use std::ffi::CString;
33
use std::{
4-
ffi::CStr,
4+
ffi::{CStr, OsStr},
55
os::{fd::RawFd, unix::ffi::OsStrExt as _},
66
path::PathBuf,
77
};
@@ -71,27 +71,13 @@ impl ToAbsolutePath for PathAt {
7171
if pathname.first().copied() == Some(b'/') {
7272
f(pathname.into())
7373
} else {
74-
let Some(dir) = get_fd_path(self.0)? else {
74+
let Some(mut abs_path) = get_fd_path(self.0)? else {
7575
return f(None);
7676
};
77-
// Join `dir` and the relative `pathname` in a per-call bump
78-
// arena instead of `PathBuf::push` on the global allocator. This
79-
// runs on every fd-relative open/stat, including inside signal
80-
// handlers and fork children, where the global allocator's locks
81-
// are unsafe to take. The joined path only lives for the `f`
82-
// call — nothing escapes the arena.
83-
let arena = sigsafe::alloc::arena();
84-
let mut joined = allocator_api2::vec::Vec::new_in(&arena);
85-
joined.extend_from_slice(dir.as_os_str().as_bytes());
8677
if !pathname.is_empty() {
87-
// Mirror `PathBuf::push`: exactly one separator between the
88-
// (absolute, non-empty) dir and the relative pathname.
89-
if joined.last() != Some(&b'/') {
90-
joined.push(b'/');
91-
}
92-
joined.extend_from_slice(pathname);
78+
abs_path.push(OsStr::from_bytes(pathname));
9379
}
94-
f(Some(joined.as_bstr()))
80+
f(Some(abs_path.as_os_str().as_bytes().as_bstr()))
9581
}
9682
}
9783
}

crates/fspy_preload_unix/src/interceptions/spawn/exec/mod.rs

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
mod with_argv;
22

3-
#[cfg(target_os = "linux")]
4-
use std::ffi::CString;
5-
63
use fspy_shared_unix::exec::ExecResolveConfig;
74
use libc::{c_char, c_int};
85
use with_argv::with_argv;
@@ -195,21 +192,40 @@ mod linux_only {
195192
reason = "suppresses unused warning on *::original"
196193
)]
197194
let _unused = execveat::original;
195+
// One bump arena for this intercepted call. The NUL-terminated copy
196+
// of the resolved path made below must not go through libc malloc:
197+
// programs exec from the child of `fork()` in multithreaded
198+
// processes (posix_spawn does exactly that), where malloc's lock may
199+
// be held by a thread that no longer exists.
200+
let arena = sigsafe::alloc::arena();
198201
// SAFETY: PathAt wraps a valid dirfd and pathname pointer from the interposed execveat call
199202
let abs_path_result = unsafe {
200203
PathAt(dirfd, pathname).to_absolute_path(|path| {
201204
let Some(path) = path else {
202205
return Ok(None);
203206
};
204-
Ok(Some(CString::new(&**path).unwrap()))
207+
// The resolved path plus a NUL terminator, allocated in the
208+
// arena so it stays valid past this callback. Interior NULs
209+
// cannot occur: the bytes come from NUL-terminated C strings
210+
// and fd symlink targets.
211+
let mut abs_path =
212+
allocator_api2::vec::Vec::with_capacity_in(path.len() + 1, &arena);
213+
abs_path.extend_from_slice(path);
214+
abs_path.push(0);
215+
Ok(Some(abs_path))
205216
})
206217
};
207218
let abs_path = match abs_path_result {
208219
Ok(None) => {
209220
// SAFETY: forwarding the original arguments to the real execveat syscall
210221
return unsafe { execveat::original()(dirfd, pathname, argv, envp, flags) };
211222
}
212-
Ok(Some(path)) => path.as_ptr(),
223+
// Borrowed out of `abs_path_result`, which lives until the end
224+
// of the function — past the `handle_exec` below that reads the
225+
// pointer. (The previous version built a `CString`, moved it
226+
// into this match arm, and dropped it here — `handle_exec` then
227+
// read freed memory.)
228+
Ok(Some(ref path)) => path.as_ptr().cast(),
213229
Err(errno) => {
214230
errno.set();
215231
return -1;

0 commit comments

Comments
 (0)