From 61c02e695efcddf9d249a08602cff369d6d19a96 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 26 Jun 2026 07:22:46 +0000 Subject: [PATCH] refactor(windows-host): OwnedHandle for the SCM STOP/SESSION events (Goal-3, last unsafe reduction) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The service's STOP/SESSION manual-reset events were smuggled across the C SCM control-handler boundary as raw `isize` in `AtomicIsize` statics (the handler is a capture-free `'static` closure, so it can't hold a non-`Send` `HANDLE` — it has to reach the events through statics), reconstructed via `load_event`, and explicitly `CloseHandle`d at `run_service` end. Replace the raw-`isize` statics with `OnceLock`: - `run_service` creates each event, wraps it in an `OwnedHandle`, derives a borrowed `HANDLE` for `supervise` (unchanged signature), and `set`s the OnceLock (once per process) — all BEFORE the handler is registered, so the handler always sees `Some`. - The handler reads `event_handle(&STOP_EVENT)` (a borrow) and `SetEvent`s it, with a defensive `None` guard (matches the old `SetEvent(HANDLE(0))` no-op if it ever fired pre-init). - The events are owned by the OnceLocks for the process lifetime (the service process exits right after `run_service` returns, so the OS reaps them at exit). Dropping the explicit `CloseHandle` also removes the latent close-then-signal window the old statics had (the raw isize lingered after the close). Deletes the `AtomicIsize`/`Ordering` import + `load_event` + the raw-isize smuggle — the last host-side raw-handle reduction. Behaviour-preserving (same events, same signal/wait/reset, same once-per-process init order). Linux check + fmt clean; the file is #[cfg(windows)] → to be box-validated (compile + a service stop/restart). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/punktfunk-host/src/windows/service.rs | 54 ++++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 5ca29bf..ea3dcb8 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -25,7 +25,7 @@ use anyhow::{bail, Context, Result}; use std::ffi::{c_void, OsString}; use std::os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle}; use std::path::PathBuf; -use std::sync::atomic::{AtomicIsize, Ordering}; +use std::sync::OnceLock; use std::time::Duration; use windows::core::{PCWSTR, PWSTR}; @@ -65,18 +65,19 @@ const SERVICE_DESCRIPTION: &str = /// legacy GCM nonce reuse — security-review #5/#9; native clients only). const DEFAULT_HOST_CMD: &str = "serve --gamestream"; -/// Event handles shared between the SCM control handler (which signals them) and the supervision loop -/// (which waits on them). Stored as raw `isize` so the `'static + Send` handler can reach them without -/// a non-`Send` `HANDLE` capture. Set once in `run_service`. -/// -/// Intentionally left as raw-`isize` statics + their explicit `CloseHandle` in `run_service` (not -/// `OwnedHandle`): they're smuggled across the C SCM control-handler boundary, so converting them is a -/// separate, riskier redesign out of scope for the process/job-handle ownership change here. -static STOP_EVENT: AtomicIsize = AtomicIsize::new(0); -static SESSION_EVENT: AtomicIsize = AtomicIsize::new(0); +/// The STOP and SESSION manual-reset events, shared between the SCM control handler (a capture-free +/// `'static` closure that SIGNALS them) and the supervision loop (which WAITS on them). They live in +/// `OnceLock`s — a static the handler can reach without capturing a non-`Send` `HANDLE` — and each owns +/// its handle (`OwnedHandle`) for the process lifetime: the service process exits right after +/// `run_service` returns, so the OS reaps them at exit, and owning them past the handler's last possible +/// call avoids the close-then-signal window the old raw-`isize` statics had. Set once, in `run_service`. +static STOP_EVENT: OnceLock = OnceLock::new(); +static SESSION_EVENT: OnceLock = OnceLock::new(); -fn load_event(a: &AtomicIsize) -> HANDLE { - HANDLE(a.load(Ordering::Relaxed) as *mut c_void) +/// Borrow an event's handle for the control handler's `SetEvent`. `None` until `run_service` creates the +/// events — but the handler is registered only AFTER they're set, so in practice this is always `Some`. +fn event_handle(ev: &OnceLock) -> Option { + ev.get().map(|h| HANDLE(h.as_raw_handle())) } /// Dispatch `service `. @@ -204,12 +205,19 @@ fn run_service() -> Result<()> { // Two manual-reset events: STOP (set once, never reset) and SESSION (set on a console // connect/disconnect, reset by the supervisor after it reacts). - let stop = + let stop_raw = unsafe { CreateEventW(None, true, false, PCWSTR::null()) }.context("CreateEvent stop")?; - let session = unsafe { CreateEventW(None, true, false, PCWSTR::null()) } + let session_raw = unsafe { CreateEventW(None, true, false, PCWSTR::null()) } .context("CreateEvent session")?; - STOP_EVENT.store(stop.0 as isize, Ordering::Relaxed); - SESSION_EVENT.store(session.0 as isize, Ordering::Relaxed); + // Own each event handle (the OS reaps them at process exit); the handler reaches them through the + // OnceLocks, while `supervise` waits on the borrowed `HANDLE`s. SAFETY: each is a fresh CreateEventW + // handle we own — take ownership exactly once. + let stop_owned = unsafe { OwnedHandle::from_raw_handle(stop_raw.0) }; + let session_owned = unsafe { OwnedHandle::from_raw_handle(session_raw.0) }; + let stop = HANDLE(stop_owned.as_raw_handle()); + let session = HANDLE(session_owned.as_raw_handle()); + let _ = STOP_EVENT.set(stop_owned); // set once per process + let _ = SESSION_EVENT.set(session_owned); // The control handler captures nothing — it reaches the events through the statics, so it stays // `Fn + Send + 'static`. Session lock/unlock are handled inside the host (DesktopWatcher), so we @@ -217,7 +225,9 @@ fn run_service() -> Result<()> { let handler = move |control| -> ServiceControlHandlerResult { match control { ServiceControl::Stop | ServiceControl::Preshutdown | ServiceControl::Shutdown => { - unsafe { SetEvent(load_event(&STOP_EVENT)) }.ok(); + if let Some(h) = event_handle(&STOP_EVENT) { + unsafe { SetEvent(h) }.ok(); + } ServiceControlHandlerResult::NoError } ServiceControl::SessionChange(param) => { @@ -226,7 +236,9 @@ fn run_service() -> Result<()> { param.reason, ConsoleConnect | ConsoleDisconnect | SessionLogon ) { - unsafe { SetEvent(load_event(&SESSION_EVENT)) }.ok(); + if let Some(h) = event_handle(&SESSION_EVENT) { + unsafe { SetEvent(h) }.ok(); + } } ServiceControlHandlerResult::NoError } @@ -263,10 +275,8 @@ fn run_service() -> Result<()> { controls_accepted: ServiceControlAccept::empty(), ..running }); - unsafe { - let _ = CloseHandle(stop); - let _ = CloseHandle(session); - } + // The STOP/SESSION events stay owned by the OnceLocks for the process lifetime (the OS reaps them at + // exit); NOT closing them while the SCM handler could still fire avoids a use-after-close. result }