From c262bf43dffcafb5c19cb3bde79d9bebc56be6df Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 21:18:04 +0200 Subject: [PATCH] refactor(host): three more Windows unsafe fns that had no caller contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Continues the crate-by-crate `unsafe_op_in_unsafe_fn` work. Same finding as pf-frame: the useful move is often deleting the `unsafe fn` marker, not decorating the body. `install_steam_audio_pair` (no arguments) and `try_install_steam_audio(&str)` are now safe fns. The call sites had already worked this out — both carried a SAFETY comment opening "`install_steam_audio_pair` is `unsafe` only because it `LoadLibraryExW`s `newdev.dll`", i.e. the obligation was never the caller's. Making them safe deletes 14 lines of that prose from two call sites and moves the real reasoning next to the `LoadLibraryExW`/`transmute`/call chain it actually describes. wasapi_mic.rs: 8 -> 0. `make_job` (no arguments, and it wraps its handle in an `OwnedHandle` before the first fallible step) and `open_log_handle(&Path)` are safe fns too. The latter returns a raw `HANDLE`, but that is an ownership obligation, not a safety one — nothing a caller does with it can cause UB, and it is inherited by the child and closed there. service.rs: 19 -> 14. `spawn_host` keeps `unsafe fn`: it takes a raw `HANDLE`, and an invalid one is UB. That is a contract worth stating, so it keeps the marker. Windows-only note: `LoadLibraryExW` here already passes `LOAD_LIBRARY_SEARCH_SYSTEM32`, so the newdev.dll load cannot pick up a planted DLL from the working directory — now recorded in the SAFETY note rather than left implicit. Verified on Windows .47: punktfunk-host clean at EXITCODE=0 — zero errors, zero unused-unsafe. Crate's own files now 29 E0133 (interactive.rs 15, service.rs 14), down from 42. --- .../src/audio/windows/wasapi_cap.rs | 8 +-- .../src/audio/windows/wasapi_mic.rs | 54 +++++++++++------- crates/punktfunk-host/src/windows/service.rs | 57 ++++++++++++------- 3 files changed, 71 insertions(+), 48 deletions(-) diff --git a/crates/punktfunk-host/src/audio/windows/wasapi_cap.rs b/crates/punktfunk-host/src/audio/windows/wasapi_cap.rs index 443a37df..2dfcbfe2 100644 --- a/crates/punktfunk-host/src/audio/windows/wasapi_cap.rs +++ b/crates/punktfunk-host/src/audio/windows/wasapi_cap.rs @@ -245,13 +245,7 @@ fn capture_once( .is_some_and(|(n, _)| wiring_plan::silent_sink(&n.to_lowercase())); static INSTALL_TRIED: AtomicBool = AtomicBool::new(false); if !have_silent && !INSTALL_TRIED.swap(true, Ordering::SeqCst) { - // SAFETY: `install_steam_audio_pair` is `unsafe` only because it `LoadLibraryExW`s - // `newdev.dll` and calls `DiInstallDriverW` through a `transmute`d function pointer; - // calling it imposes no extra precondition here (it takes no args and aliases - // nothing). Its internal contract holds: the `DiInstall` type matches the documented - // `BOOL DiInstallDriverW(HWND, PCWSTR, DWORD, PBOOL)` ABI, and it passes a - // NUL-terminated UTF-16 INF path with null/zero optional args. - if unsafe { super::wasapi_mic::install_steam_audio_pair() } { + if super::wasapi_mic::install_steam_audio_pair() { wiring = audio_control::wire_now(true); } if !wiring diff --git a/crates/punktfunk-host/src/audio/windows/wasapi_mic.rs b/crates/punktfunk-host/src/audio/windows/wasapi_mic.rs index f317b374..cac735a8 100644 --- a/crates/punktfunk-host/src/audio/windows/wasapi_mic.rs +++ b/crates/punktfunk-host/src/audio/windows/wasapi_mic.rs @@ -153,14 +153,7 @@ fn resolve_target() -> Result<(wasapi::Device, String)> { let mut wiring = audio_control::wire_now(false); if wiring.mic_render.is_none() { tracing::info!("no usable virtual mic device present — attempting auto-install"); - // SAFETY: `install_steam_audio_pair` is `unsafe` only because it `LoadLibraryExW`s - // `newdev.dll` and calls `DiInstallDriverW` through a `transmute`d function pointer; - // calling it imposes no extra precondition here (it takes no args and aliases nothing). - // Its internal contract holds: the `DiInstall` type matches the documented - // `BOOL DiInstallDriverW(HWND, PCWSTR, DWORD, PBOOL)` ABI, and it passes a - // NUL-terminated UTF-16 INF path with null/zero optional args. Invoked once on the - // dedicated mic thread. - if unsafe { install_steam_audio_pair() } { + if install_steam_audio_pair() { wiring = audio_control::wire_now(false); } } @@ -185,7 +178,7 @@ fn resolve_target() -> Result<(wasapi::Device, String)> { /// capture ([`super::wasapi_cap`]) also installs the pair when no silent sink exists. Returns true /// if either installed. No-op when Steam isn't installed (INFs absent), the install is denied /// (needs admin — the host runs as SYSTEM), or `PUNKTFUNK_NO_MIC_INSTALL` is set. -pub(crate) unsafe fn install_steam_audio_pair() -> bool { +pub(crate) fn install_steam_audio_pair() -> bool { // Microphone first (the mic's actual target); speakers second (the distinct desktop-audio sink). let mic = try_install_steam_audio("SteamStreamingMicrophone.inf"); let spk = try_install_steam_audio("SteamStreamingSpeakers.inf"); @@ -196,7 +189,11 @@ pub(crate) unsafe fn install_steam_audio_pair() -> bool { /// `newdev.dll`, like Apollo, to avoid an extra windows-crate feature). See /// [`install_steam_audio_pair`] for the contract; `inf_name` is a bare filename under Steam's /// per-arch `drivers\Windows10\{arch}\` directory. -unsafe fn try_install_steam_audio(inf_name: &str) -> bool { +/// +/// Safe: `inf_name` is a `&str` and every FFI argument is built locally from it, so there is no +/// precondition a caller could break — the `unsafe` is the `LoadLibraryExW`/`transmute`/call chain +/// inside, which is this function's own business. +fn try_install_steam_audio(inf_name: &str) -> bool { use windows::core::{s, w, PCWSTR}; use windows::Win32::Foundation::HWND; use windows::Win32::System::Environment::ExpandEnvironmentStringsW; @@ -220,27 +217,41 @@ unsafe fn try_install_steam_audio(inf_name: &str) -> bool { .chain(std::iter::once(0)) .collect(); let mut path = vec![0u16; 1024]; - let n = ExpandEnvironmentStringsW(PCWSTR(template.as_ptr()), Some(path.as_mut_slice())); + // SAFETY: `template` is a locally built NUL-terminated UTF-16 buffer that outlives the call, and + // the output slice is a live local whose length the callee is told via the slice itself. + let n = + unsafe { ExpandEnvironmentStringsW(PCWSTR(template.as_ptr()), Some(path.as_mut_slice())) }; if n == 0 || n as usize > path.len() { return false; } - let Ok(newdev) = LoadLibraryExW(w!("newdev.dll"), None, LOAD_LIBRARY_SEARCH_SYSTEM32) else { + // SAFETY: a static NUL-terminated literal, loaded from System32 only (the flag), so this cannot + // pick up a planted `newdev.dll` from the working directory. The handle is checked before use. + let Ok(newdev) = + (unsafe { LoadLibraryExW(w!("newdev.dll"), None, LOAD_LIBRARY_SEARCH_SYSTEM32) }) + else { tracing::warn!("could not load newdev.dll — Steam-audio auto-install unavailable"); return false; }; - let Some(addr) = GetProcAddress(newdev, s!("DiInstallDriverW")) else { + // SAFETY: `newdev` is the live module just loaded; the export name is a static literal. + let Some(addr) = (unsafe { GetProcAddress(newdev, s!("DiInstallDriverW")) }) else { return false; }; // BOOL DiInstallDriverW(HWND hwndParent, PCWSTR InfPath, DWORD Flags, PBOOL NeedReboot) type DiInstall = unsafe extern "system" fn(HWND, PCWSTR, u32, *mut i32) -> i32; - let f: DiInstall = std::mem::transmute(addr); - let ok = f( - HWND(std::ptr::null_mut()), - PCWSTR(path.as_ptr()), - 0, - std::ptr::null_mut(), - ) != 0; + // SAFETY: `addr` is the non-null export just resolved and `DiInstall` mirrors its documented + // signature (commented above). + let f: DiInstall = unsafe { std::mem::transmute(addr) }; + // SAFETY: `path` is the expanded, NUL-terminated buffer above and outlives the call; a null + // parent HWND and a null `NeedReboot` are both documented as accepted. + let ok = unsafe { + f( + HWND(std::ptr::null_mut()), + PCWSTR(path.as_ptr()), + 0, + std::ptr::null_mut(), + ) + } != 0; if ok { tracing::info!( inf = inf_name, @@ -248,7 +259,8 @@ unsafe fn try_install_steam_audio(inf_name: &str) -> bool { ); std::thread::sleep(Duration::from_secs(5)); // let the audio subsystem register the endpoint } else { - let err = windows::Win32::Foundation::GetLastError(); + // SAFETY: reads this thread's last-error value; takes no arguments and touches no memory. + let err = unsafe { windows::Win32::Foundation::GetLastError() }; tracing::info!( inf = inf_name, ?err, diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index e92ea123..328f03db 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -357,7 +357,7 @@ fn supervise(stop: HANDLE, session_ev: HANDLE) -> Result<()> { // straggler still inside it — no manual CloseHandle(job). // SAFETY: `make_job` is unsafe only for its Win32 FFI; it has no caller preconditions and creates + // immediately takes RAII ownership of the job object, so calling it here is sound. - let job = unsafe { make_job() }.context("create job object")?; + let job = make_job().context("create job object")?; let mut restarts: u32 = 0; loop { @@ -494,19 +494,29 @@ fn wait_any(handles: &[HANDLE], ms: u32) -> Option { } /// A kill-on-close + breakaway-ok job object, returned as an `OwnedHandle` (auto-`CloseHandle` on drop). -unsafe fn make_job() -> Result { - let job_raw = CreateJobObjectW(None, PCWSTR::null()).context("CreateJobObjectW")?; +/// Safe: takes no arguments, and the handle it creates is wrapped in an `OwnedHandle` before any +/// fallible step — so there is no precondition for a caller to uphold and no way to leak it here. +fn make_job() -> Result { + // SAFETY: a null security descriptor and a null name are both documented as "unnamed, default + // security"; the returned handle is checked by `?` and owned on the next line. + let job_raw = unsafe { CreateJobObjectW(None, PCWSTR::null()) }.context("CreateJobObjectW")?; // Own it immediately so any early return (e.g. a failed SetInformationJobObject) still closes it. - let job = OwnedHandle::from_raw_handle(job_raw.0); + // SAFETY: `job_raw` is the handle just created, is non-null, and is not owned anywhere else — + // ownership passes to the `OwnedHandle`, which closes it exactly once. + let job = unsafe { OwnedHandle::from_raw_handle(job_raw.0) }; let mut info = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default(); info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_BREAKAWAY_OK; - SetInformationJobObject( - HANDLE(job.as_raw_handle()), - JobObjectExtendedLimitInformation, - &info as *const _ as *const c_void, - std::mem::size_of::() as u32, - ) + // SAFETY: `job` is the live job object above; `info` is a local of exactly the type the + // `JobObjectExtendedLimitInformation` class expects, and the size argument is its `size_of`. + unsafe { + SetInformationJobObject( + HANDLE(job.as_raw_handle()), + JobObjectExtendedLimitInformation, + &info as *const _ as *const c_void, + std::mem::size_of::() as u32, + ) + } .context("SetInformationJobObject")?; Ok(job) } @@ -625,7 +635,10 @@ unsafe fn spawn_host( } /// Open `path` for appending, as an INHERITABLE handle (so the child can use it as stdout/stderr). -unsafe fn open_log_handle(path: &std::path::Path) -> Result { +/// Safe: `path` is a `&Path` and every FFI argument is built locally from it. The returned `HANDLE` +/// is owned by the caller (it is inherited by the child and closed there), which is an ownership +/// obligation, not a safety one — nothing a caller can do here causes UB. +fn open_log_handle(path: &std::path::Path) -> Result { let wpath: Vec = path .as_os_str() .to_string_lossy() @@ -642,15 +655,19 @@ unsafe fn open_log_handle(path: &std::path::Path) -> Result { // access mask (FILE_GENERIC_WRITE minus WRITE_DATA, plus APPEND_DATA + SYNCHRONIZE/READ_CONTROL); // bare FILE_APPEND_DATA alone produced a child handle that silently dropped writes. let access = (FILE_GENERIC_WRITE.0 & !FILE_WRITE_DATA.0) | FILE_APPEND_DATA.0; - let h = CreateFileW( - PCWSTR(wpath.as_ptr()), - access, - FILE_SHARE_READ | FILE_SHARE_WRITE, - Some(&sa), - OPEN_ALWAYS, - windows::Win32::Storage::FileSystem::FILE_FLAGS_AND_ATTRIBUTES(0), - None, - ) + // SAFETY: `wpath` is the locally built NUL-terminated UTF-16 path and `sa` a correctly sized + // local `SECURITY_ATTRIBUTES`; both outlive the call, and the result is checked by `?`. + let h = unsafe { + CreateFileW( + PCWSTR(wpath.as_ptr()), + access, + FILE_SHARE_READ | FILE_SHARE_WRITE, + Some(&sa), + OPEN_ALWAYS, + windows::Win32::Storage::FileSystem::FILE_FLAGS_AND_ATTRIBUTES(0), + None, + ) + } .context("CreateFileW(host.log)")?; Ok(h) }