refactor(host): three more Windows unsafe fns that had no caller contract
ci / web (push) Successful in 1m3s
ci / docs-site (push) Successful in 1m16s
android / android (push) Successful in 12m8s
windows-drivers / probe-and-proto (push) Successful in 29s
ci / rust (push) Failing after 10m31s
ci / rust-arm64 (push) Failing after 11m1s
ci / bench (push) Successful in 5m40s
decky / build-publish (push) Successful in 22s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 10s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 8s
windows-drivers / driver-build (push) Successful in 1m44s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 9s
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 9s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 8s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 9s
arch / build-publish (push) Successful in 16m4s
windows-host / package (push) Failing after 5m12s
windows-host / winget-source (push) Skipped
deb / build-publish (push) Successful in 8m59s
windows-msix / package (arm64, C:\Users\Public\ffmpeg-arm64, --no-default-features, aarch64-pc-windows-msvc, C:\t-a64) (push) Successful in 2m12s
flatpak / build-publish (push) Successful in 7m0s
deb / build-publish-client-arm64 (push) Successful in 9m59s
apple / swift (push) Successful in 5m24s
windows-msix / package (x64, C:\Users\Public\ffmpeg, , x86_64-pc-windows-msvc, C:\t) (push) Successful in 2m28s
windows / build (aarch64-pc-windows-msvc) (push) Failing after 1m9s
windows / build (x86_64-pc-windows-msvc) (push) Failing after 1m32s
deb / build-publish-host (push) Successful in 14m53s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Successful in 18m45s
docker / build-push-arm64cross (push) Successful in 10s
docker / deploy-docs (push) Successful in 25s
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Successful in 30m14s
release / apple (push) Successful in 39m36s
apple / screenshots (push) Successful in 23m3s

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.
This commit is contained in:
2026-07-28 21:31:42 +02:00
parent 307ca88ba7
commit c262bf43df
3 changed files with 71 additions and 48 deletions
@@ -245,13 +245,7 @@ fn capture_once(
.is_some_and(|(n, _)| wiring_plan::silent_sink(&n.to_lowercase())); .is_some_and(|(n, _)| wiring_plan::silent_sink(&n.to_lowercase()));
static INSTALL_TRIED: AtomicBool = AtomicBool::new(false); static INSTALL_TRIED: AtomicBool = AtomicBool::new(false);
if !have_silent && !INSTALL_TRIED.swap(true, Ordering::SeqCst) { if !have_silent && !INSTALL_TRIED.swap(true, Ordering::SeqCst) {
// SAFETY: `install_steam_audio_pair` is `unsafe` only because it `LoadLibraryExW`s if super::wasapi_mic::install_steam_audio_pair() {
// `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() } {
wiring = audio_control::wire_now(true); wiring = audio_control::wire_now(true);
} }
if !wiring if !wiring
@@ -153,14 +153,7 @@ fn resolve_target() -> Result<(wasapi::Device, String)> {
let mut wiring = audio_control::wire_now(false); let mut wiring = audio_control::wire_now(false);
if wiring.mic_render.is_none() { if wiring.mic_render.is_none() {
tracing::info!("no usable virtual mic device present — attempting auto-install"); tracing::info!("no usable virtual mic device present — attempting auto-install");
// SAFETY: `install_steam_audio_pair` is `unsafe` only because it `LoadLibraryExW`s if install_steam_audio_pair() {
// `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() } {
wiring = audio_control::wire_now(false); 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 /// 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 /// 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. /// (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). // Microphone first (the mic's actual target); speakers second (the distinct desktop-audio sink).
let mic = try_install_steam_audio("SteamStreamingMicrophone.inf"); let mic = try_install_steam_audio("SteamStreamingMicrophone.inf");
let spk = try_install_steam_audio("SteamStreamingSpeakers.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 /// `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 /// [`install_steam_audio_pair`] for the contract; `inf_name` is a bare filename under Steam's
/// per-arch `drivers\Windows10\{arch}\` directory. /// 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::core::{s, w, PCWSTR};
use windows::Win32::Foundation::HWND; use windows::Win32::Foundation::HWND;
use windows::Win32::System::Environment::ExpandEnvironmentStringsW; 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)) .chain(std::iter::once(0))
.collect(); .collect();
let mut path = vec![0u16; 1024]; 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() { if n == 0 || n as usize > path.len() {
return false; 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"); tracing::warn!("could not load newdev.dll — Steam-audio auto-install unavailable");
return false; 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; return false;
}; };
// BOOL DiInstallDriverW(HWND hwndParent, PCWSTR InfPath, DWORD Flags, PBOOL NeedReboot) // BOOL DiInstallDriverW(HWND hwndParent, PCWSTR InfPath, DWORD Flags, PBOOL NeedReboot)
type DiInstall = unsafe extern "system" fn(HWND, PCWSTR, u32, *mut i32) -> i32; type DiInstall = unsafe extern "system" fn(HWND, PCWSTR, u32, *mut i32) -> i32;
let f: DiInstall = std::mem::transmute(addr); // SAFETY: `addr` is the non-null export just resolved and `DiInstall` mirrors its documented
let ok = f( // signature (commented above).
HWND(std::ptr::null_mut()), let f: DiInstall = unsafe { std::mem::transmute(addr) };
PCWSTR(path.as_ptr()), // SAFETY: `path` is the expanded, NUL-terminated buffer above and outlives the call; a null
0, // parent HWND and a null `NeedReboot` are both documented as accepted.
std::ptr::null_mut(), let ok = unsafe {
) != 0; f(
HWND(std::ptr::null_mut()),
PCWSTR(path.as_ptr()),
0,
std::ptr::null_mut(),
)
} != 0;
if ok { if ok {
tracing::info!( tracing::info!(
inf = inf_name, 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 std::thread::sleep(Duration::from_secs(5)); // let the audio subsystem register the endpoint
} else { } 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!( tracing::info!(
inf = inf_name, inf = inf_name,
?err, ?err,
+37 -20
View File
@@ -357,7 +357,7 @@ fn supervise(stop: HANDLE, session_ev: HANDLE) -> Result<()> {
// straggler still inside it — no manual CloseHandle(job). // 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 + // 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. // 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; let mut restarts: u32 = 0;
loop { loop {
@@ -494,19 +494,29 @@ fn wait_any(handles: &[HANDLE], ms: u32) -> Option<usize> {
} }
/// A kill-on-close + breakaway-ok job object, returned as an `OwnedHandle` (auto-`CloseHandle` on drop). /// A kill-on-close + breakaway-ok job object, returned as an `OwnedHandle` (auto-`CloseHandle` on drop).
unsafe fn make_job() -> Result<OwnedHandle> { /// Safe: takes no arguments, and the handle it creates is wrapped in an `OwnedHandle` before any
let job_raw = CreateJobObjectW(None, PCWSTR::null()).context("CreateJobObjectW")?; /// fallible step — so there is no precondition for a caller to uphold and no way to leak it here.
fn make_job() -> Result<OwnedHandle> {
// 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. // 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(); let mut info = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default();
info.BasicLimitInformation.LimitFlags = info.BasicLimitInformation.LimitFlags =
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_BREAKAWAY_OK; JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_BREAKAWAY_OK;
SetInformationJobObject( // SAFETY: `job` is the live job object above; `info` is a local of exactly the type the
HANDLE(job.as_raw_handle()), // `JobObjectExtendedLimitInformation` class expects, and the size argument is its `size_of`.
JobObjectExtendedLimitInformation, unsafe {
&info as *const _ as *const c_void, SetInformationJobObject(
std::mem::size_of::<JOBOBJECT_EXTENDED_LIMIT_INFORMATION>() as u32, HANDLE(job.as_raw_handle()),
) JobObjectExtendedLimitInformation,
&info as *const _ as *const c_void,
std::mem::size_of::<JOBOBJECT_EXTENDED_LIMIT_INFORMATION>() as u32,
)
}
.context("SetInformationJobObject")?; .context("SetInformationJobObject")?;
Ok(job) 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). /// 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<HANDLE> { /// 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<HANDLE> {
let wpath: Vec<u16> = path let wpath: Vec<u16> = path
.as_os_str() .as_os_str()
.to_string_lossy() .to_string_lossy()
@@ -642,15 +655,19 @@ unsafe fn open_log_handle(path: &std::path::Path) -> Result<HANDLE> {
// access mask (FILE_GENERIC_WRITE minus WRITE_DATA, plus APPEND_DATA + SYNCHRONIZE/READ_CONTROL); // 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. // 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 access = (FILE_GENERIC_WRITE.0 & !FILE_WRITE_DATA.0) | FILE_APPEND_DATA.0;
let h = CreateFileW( // SAFETY: `wpath` is the locally built NUL-terminated UTF-16 path and `sa` a correctly sized
PCWSTR(wpath.as_ptr()), // local `SECURITY_ATTRIBUTES`; both outlive the call, and the result is checked by `?`.
access, let h = unsafe {
FILE_SHARE_READ | FILE_SHARE_WRITE, CreateFileW(
Some(&sa), PCWSTR(wpath.as_ptr()),
OPEN_ALWAYS, access,
windows::Win32::Storage::FileSystem::FILE_FLAGS_AND_ATTRIBUTES(0), FILE_SHARE_READ | FILE_SHARE_WRITE,
None, Some(&sa),
) OPEN_ALWAYS,
windows::Win32::Storage::FileSystem::FILE_FLAGS_AND_ATTRIBUTES(0),
None,
)
}
.context("CreateFileW(host.log)")?; .context("CreateFileW(host.log)")?;
Ok(h) Ok(h)
} }