diff --git a/Cargo.lock b/Cargo.lock index 08b5c7a6..3e141b1a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1121,7 +1121,6 @@ dependencies = [ "tracing", "uuid", "winapi", - "winsafe 0.0.24", ] [[package]] diff --git a/crates/fspy/tests/rust_std.rs b/crates/fspy/tests/rust_std.rs index ea4a7f16..2ff9e0a0 100644 --- a/crates/fspy/tests/rust_std.rs +++ b/crates/fspy/tests/rust_std.rs @@ -47,7 +47,7 @@ async fn readdir() -> anyhow::Result<()> { } #[test(tokio::test)] -async fn subprocess() -> anyhow::Result<()> { +async fn read_in_subprocess() -> anyhow::Result<()> { let accesses = track_child!((), |(): ()| { let mut command = if cfg!(windows) { let mut command = std::process::Command::new("cmd"); @@ -72,3 +72,18 @@ async fn subprocess() -> anyhow::Result<()> { Ok(()) } + +#[test(tokio::test)] +async fn read_program() -> anyhow::Result<()> { + let accesses = track_child!((), |(): ()| { + let _ = std::process::Command::new("./not_exist.exe").spawn(); + }) + .await?; + assert_contains( + &accesses, + current_dir().unwrap().join("not_exist.exe").as_path(), + AccessMode::READ, + ); + + Ok(()) +} diff --git a/crates/fspy/tests/test_utils.rs b/crates/fspy/tests/test_utils.rs index 3aaa0edb..f13ab520 100644 --- a/crates/fspy/tests/test_utils.rs +++ b/crates/fspy/tests/test_utils.rs @@ -11,25 +11,34 @@ pub fn assert_contains( expected_path: &Path, expected_mode: AccessMode, ) { - let found = accesses.iter().any(|access| { + let mut actual_mode: AccessMode = AccessMode::empty(); + for access in accesses.iter() { let Ok(stripped) = access.path.strip_path_prefix::<_, Result, _>( expected_path, |strip_result| strip_result.map(Path::to_path_buf), ) else { - return false; + continue; }; - stripped.as_os_str().is_empty() && access.mode == expected_mode - }); - if !found { - panic!( - "Expected to find access to path {:?} with mode {:?}, but it was not found in: {:?}", - expected_path, - expected_mode, - accesses.iter().collect::>() - ); + if stripped.as_os_str().is_empty() { + actual_mode.insert(access.mode); + } } + + if actual_mode.contains(AccessMode::READ_DIR) { + // READ_DIR already implies READ. + actual_mode.remove(AccessMode::READ); + } + + assert_eq!( + expected_mode, + actual_mode, + "Expected to find access to path {:?} with mode {:?}, but it was not found in: {:?}", + expected_path, + expected_mode, + accesses.iter().collect::>() + ); } #[macro_export] diff --git a/crates/fspy/tests/winapi.rs b/crates/fspy/tests/winapi.rs new file mode 100644 index 00000000..232df7bd --- /dev/null +++ b/crates/fspy/tests/winapi.rs @@ -0,0 +1,66 @@ +#![cfg(windows)] + +mod test_utils; + +use std::{ffi::OsStr, os::windows::ffi::OsStrExt, path::Path, ptr::null_mut}; + +use fspy::AccessMode; +use test_log::test; +use test_utils::assert_contains; +use winapi::um::processthreadsapi::{ + CreateProcessA, CreateProcessW, PROCESS_INFORMATION, STARTUPINFOA, STARTUPINFOW, +}; + +#[test(tokio::test)] +async fn create_process_a() -> anyhow::Result<()> { + let accesses = track_child!((), |(): ()| { + let mut si: STARTUPINFOA = unsafe { std::mem::zeroed() }; + let mut pi: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; + unsafe { + CreateProcessA( + c"C:\\fspy_test_not_exist_program.exe".as_ptr().cast(), + null_mut(), + null_mut(), + null_mut(), + 0, + 0, + null_mut(), + null_mut(), + &mut si, + &mut pi, + ) + }; + }) + .await?; + assert_contains(&accesses, Path::new("C:\\fspy_test_not_exist_program.exe"), AccessMode::READ); + + Ok(()) +} + +#[test(tokio::test)] +async fn create_process_w() -> anyhow::Result<()> { + let accesses = track_child!((), |(): ()| { + let mut si: STARTUPINFOW = unsafe { std::mem::zeroed() }; + let mut pi: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; + let program = + OsStr::new("C:\\fspy_test_not_exist_program.exe\0").encode_wide().collect::>(); + unsafe { + CreateProcessW( + program.as_ptr(), + null_mut(), + null_mut(), + null_mut(), + 0, + 0, + null_mut(), + null_mut(), + &mut si, + &mut pi, + ) + }; + }) + .await?; + assert_contains(&accesses, Path::new("C:\\fspy_test_not_exist_program.exe"), AccessMode::READ); + + Ok(()) +} diff --git a/crates/fspy_preload_windows/src/lib.rs b/crates/fspy_preload_windows/src/lib.rs index 7360e457..04957cd8 100644 --- a/crates/fspy_preload_windows/src/lib.rs +++ b/crates/fspy_preload_windows/src/lib.rs @@ -1,5 +1,4 @@ #![cfg(windows)] #![feature(sync_unsafe_cell)] -mod stack_once; pub mod windows; diff --git a/crates/fspy_preload_windows/src/stack_once.rs b/crates/fspy_preload_windows/src/stack_once.rs deleted file mode 100644 index 9530ad16..00000000 --- a/crates/fspy_preload_windows/src/stack_once.rs +++ /dev/null @@ -1,40 +0,0 @@ -use std::{cell::Cell, thread::LocalKey}; - -pub struct StackOnceToken { - active: &'static LocalKey>, -} - -impl StackOnceToken { - #[doc(hidden)] - pub const fn new(active: &'static LocalKey>) -> StackOnceToken { - Self { active } - } - - pub fn try_enter(&self) -> Option { - if self.active.get() { - None - } else { - self.active.set(true); - Some(StackOnceGuard(self.active)) - } - } -} - -pub struct StackOnceGuard(&'static LocalKey>); - -impl Drop for StackOnceGuard { - fn drop(&mut self) { - self.0.set(false); - } -} - -macro_rules! stack_once_token { - ($name:ident) => { - static $name: $crate::stack_once::StackOnceToken = { - ::std::thread_local! { static ACTIVE: ::core::cell::Cell = ::core::cell::Cell::new(false) } - $crate::stack_once::StackOnceToken::new(&ACTIVE) - }; - }; -} - -pub(crate) use stack_once_token; diff --git a/crates/fspy_preload_windows/src/windows/client.rs b/crates/fspy_preload_windows/src/windows/client.rs index cdaa4e44..7d5df7ac 100644 --- a/crates/fspy_preload_windows/src/windows/client.rs +++ b/crates/fspy_preload_windows/src/windows/client.rs @@ -8,29 +8,11 @@ use fspy_shared::{ }; use winapi::{shared::minwindef::BOOL, um::winnt::HANDLE}; -use crate::stack_once::{StackOnceGuard, stack_once_token}; - pub struct Client<'a> { payload: Payload<'a>, ipc_sender: Option, } -stack_once_token!(PATH_ACCESS_ONCE); - -pub struct PathAccessSender<'a> { - ipc_sender: &'a Option, - _once_guard: StackOnceGuard, -} - -impl<'a> PathAccessSender<'a> { - pub fn send(&self, access: PathAccess<'_>) { - let Some(sender) = &self.ipc_sender else { - return; - }; - sender.write_encoded(&access, BINCODE_CONFIG).expect("failed to send path access"); - } -} - impl<'a> Client<'a> { pub fn from_payload_bytes(payload_bytes: &'a [u8]) -> Self { let (payload, decoded_len) = @@ -58,11 +40,6 @@ impl<'a> Client<'a> { sender.write_encoded(&access, BINCODE_CONFIG).expect("failed to send path access"); } - pub fn sender(&self) -> Option> { - let guard = PATH_ACCESS_ONCE.try_enter()?; - Some(PathAccessSender { ipc_sender: &self.ipc_sender, _once_guard: guard }) - } - pub unsafe fn prepare_child_process(&self, child_handle: HANDLE) -> BOOL { let payload_bytes = encode_to_vec(&self.payload, BINCODE_CONFIG).unwrap(); unsafe { diff --git a/crates/fspy_preload_windows/src/windows/detours/create_process.rs b/crates/fspy_preload_windows/src/windows/detours/create_process.rs index 445d38b5..4c8ee84b 100644 --- a/crates/fspy_preload_windows/src/windows/detours/create_process.rs +++ b/crates/fspy_preload_windows/src/windows/detours/create_process.rs @@ -1,8 +1,4 @@ -use std::ffi::CStr; - use fspy_detours_sys::{DetourCreateProcessWithDllExA, DetourCreateProcessWithDllExW}; -use fspy_shared::ipc::{AccessMode, NativeStr, PathAccess}; -use widestring::U16CStr; use winapi::{ shared::{ minwindef::{BOOL, DWORD, LPVOID}, @@ -24,6 +20,29 @@ use crate::windows::{ detour::{Detour, DetourAny}, }; +thread_local! { + static IS_HOOKING_CREATE_PROCESS: std::cell::Cell = std::cell::Cell::new(false); +} + +struct HookGuard; +impl HookGuard { + pub fn new() -> Option { + let already_hooking = IS_HOOKING_CREATE_PROCESS.with(|c| { + let v = c.get(); + c.set(true); + v + }); + if already_hooking { None } else { Some(Self) } + } +} +impl Drop for HookGuard { + fn drop(&mut self) { + IS_HOOKING_CREATE_PROCESS.with(|c| { + c.set(false); + }); + } +} + static DETOUR_CREATE_PROCESS_W: Detour< unsafe extern "system" fn( LPCWSTR, @@ -51,8 +70,7 @@ static DETOUR_CREATE_PROCESS_W: Detour< lp_startup_info: LPSTARTUPINFOW, lp_process_information: LPPROCESS_INFORMATION, ) -> BOOL { - let client = unsafe { global_client() }; - let Some(sender) = client.sender() else { + let Some(_hook_guard) = HookGuard::new() else { // Detect re-entrance and avoid double hooking return unsafe { (DETOUR_CREATE_PROCESS_W.real())( @@ -61,7 +79,7 @@ static DETOUR_CREATE_PROCESS_W: Detour< lp_process_attributes, lp_thread_attributes, b_inherit_handles, - dw_creation_flags | CREATE_SUSPENDED, + dw_creation_flags, lp_environment, lp_current_directory, lp_startup_info, @@ -70,16 +88,7 @@ static DETOUR_CREATE_PROCESS_W: Detour< }; }; - if !lp_application_name.is_null() { - unsafe { - sender.send(PathAccess { - mode: AccessMode::READ, - path: NativeStr::from_wide( - U16CStr::from_ptr_str(lp_application_name).as_slice(), - ), - }); - } - } + let client = unsafe { global_client() }; unsafe extern "system" fn create_process_with_payload_w( lp_application_name: LPCWSTR, @@ -175,8 +184,7 @@ static DETOUR_CREATE_PROCESS_A: Detour< lp_startup_info: LPSTARTUPINFOA, lp_process_information: LPPROCESS_INFORMATION, ) -> BOOL { - let client = unsafe { global_client() }; - let Some(sender) = client.sender() else { + let Some(_hook_guard) = HookGuard::new() else { // Detect re-entrance and avoid double hooking return unsafe { (DETOUR_CREATE_PROCESS_A.real())( @@ -185,7 +193,7 @@ static DETOUR_CREATE_PROCESS_A: Detour< lp_process_attributes, lp_thread_attributes, b_inherit_handles, - dw_creation_flags | CREATE_SUSPENDED, + dw_creation_flags, lp_environment, lp_current_directory, lp_startup_info, @@ -193,15 +201,7 @@ static DETOUR_CREATE_PROCESS_A: Detour< ) }; }; - - if !lp_application_name.is_null() { - unsafe { - sender.send(PathAccess { - mode: AccessMode::READ, - path: NativeStr::from_ansi(CStr::from_ptr(lp_application_name).to_bytes()), - }); - } - } + let client = unsafe { global_client() }; unsafe extern "system" fn create_process_with_payload_a( lp_application_name: LPCSTR, diff --git a/crates/fspy_shared/Cargo.toml b/crates/fspy_shared/Cargo.toml index 01cb6907..5e53d1b7 100644 --- a/crates/fspy_shared/Cargo.toml +++ b/crates/fspy_shared/Cargo.toml @@ -19,7 +19,6 @@ uuid = { workspace = true, features = ["v4"] } bytemuck = { workspace = true } os_str_bytes = { workspace = true } winapi = { workspace = true, features = ["std"] } -winsafe = { workspace = true } [dev-dependencies] assert2 = { workspace = true } diff --git a/crates/fspy_shared/src/ipc/native_str.rs b/crates/fspy_shared/src/ipc/native_str.rs index 37fe496a..58d527b9 100644 --- a/crates/fspy_shared/src/ipc/native_str.rs +++ b/crates/fspy_shared/src/ipc/native_str.rs @@ -13,11 +13,12 @@ use bincode::{BorrowDecode, Decode, Encode}; #[cfg(unix)] use bstr::BStr; -/// Similar to `OsStr`, but requires zero-copy to construct from either ansi or wide characters on Windows. +/// Similar to `OsStr`, but requires zero-copy to construct from either wide characters on Windows. #[derive(Encode, BorrowDecode, Clone, Copy, PartialEq, Eq)] pub struct NativeStr<'a> { - #[cfg(windows)] - is_wide: bool, + // On unix, this is the raw bytes of the OsStr. + // On windows, this is safely transmuted from `&[u16]` in `NativeStr::from_wide`. We don't declare it as `&[u16]` to allow `BorrowDecode`. + // Transmuting back to `&[u16]` would be unsafe because of different alignments between `u8` and `u16` (See `to_os_string`). data: &'a [u8], } @@ -46,11 +47,7 @@ impl<'a> NativeStr<'a> { let mut data = Vec::::with_capacity_in(self.data.len(), alloc); data.extend_from_slice(self.data); let data = data.leak::<'new_alloc>(); - NativeStr { - data, - #[cfg(windows)] - is_wide: self.is_wide, - } + NativeStr { data } } #[cfg(unix)] @@ -59,16 +56,10 @@ impl<'a> NativeStr<'a> { Self { data: bytes } } - #[cfg(windows)] - #[must_use] - pub const fn from_ansi(bytes: &'a [u8]) -> Self { - Self { is_wide: false, data: bytes } - } - #[cfg(windows)] pub fn from_wide(wide: &'a [u16]) -> Self { use bytemuck::must_cast_slice; - Self { is_wide: true, data: must_cast_slice(wide) } + Self { data: must_cast_slice(wide) } } #[cfg(unix)] @@ -90,20 +81,11 @@ impl<'a> NativeStr<'a> { use std::os::windows::ffi::OsStringExt; use bytemuck::{allocation::pod_collect_to_vec, try_cast_slice}; - use winsafe::{ - MultiByteToWideChar, - co::{CP, MBC}, - }; - - if self.is_wide { - if let Ok(wide) = try_cast_slice::(self.data) { - OsString::from_wide(wide) - } else { - let wide = pod_collect_to_vec::(self.data); - OsString::from_wide(&wide) - } + + if let Ok(wide) = try_cast_slice::(self.data) { + OsString::from_wide(wide) } else { - let wide = MultiByteToWideChar(CP::ACP, MBC::ERR_INVALID_CHARS, self.data).unwrap(); + let wide = pod_collect_to_vec::(self.data); OsString::from_wide(&wide) } } @@ -227,15 +209,6 @@ mod tests { #[cfg(windows)] use super::*; - #[cfg(windows)] - #[test] - fn test_from_ansi() { - let ansi_str = "hello"; - let native_str = NativeStr::from_ansi(ansi_str.as_bytes()); - let os_string = native_str.to_os_string(); - assert_eq!(os_string.to_str().unwrap(), ansi_str); - } - #[cfg(windows)] #[test] fn test_from_wide() {