From 0f237a766a6670bfccb0110838a3e80b2db8be7b Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:52:42 +0800 Subject: [PATCH] fix(update): verify the guard's writer by start time, and guard manual Setup runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round three, both findings and all three minors: - The guard no longer expires a live, verified holder: a pid is believed to be the writer only if the process behind it started before the guard was written (winproc::creation_time via GetProcessTimes), which is what tells a genuine holder from a recycled pid. The TTL now bounds only the unverifiable case, so an install slowed past ten minutes by an antivirus sweep keeps its protection. - Manual Setup runs get the guard too: the --stop-daemon --update-install-dir helper holds it in its parent's name — the Setup or uninstaller that keeps replacing files after the helper returns — and it goes stale when that parent exits. ensure_running gained five seconds of patience so the post-install "Launch tty7" click, racing Setup's own exit, gets its daemon instead of an error. - processes_running_from also matches images against the canonicalized install-dir spelling (junction, subst, 8.3 given form). - reconcile_portable_backups reports every interrupted backup, not the first. - The unix signal-and-wait loop now reuses wait_for_recorded_exit. --- crates/tty7-core/src/daemon/spawn.rs | 35 ++--- crates/tty7-core/src/daemon/update_guard.rs | 148 +++++++++++++++----- crates/tty7-core/src/daemon/winproc.rs | 62 +++++++- src/core/update.rs | 15 +- src/main.rs | 6 + 5 files changed, 209 insertions(+), 57 deletions(-) diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index 1bafb9d5..13b46388 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -198,16 +198,26 @@ pub fn ensure_running() -> anyhow::Result<()> { } } - // While an updater is replacing the installation, spawning a daemon would - // relock the very images the installer is clearing — the update would fail + // While an installer is replacing the installation, spawning a daemon + // would relock the very images it is clearing — the update would fail // with "files in use" caused by us. Connecting to a live daemon above is - // fine; only creating a new one waits. + // fine; only creating a new one waits. The short patience first is for + // the guard's holder being a Setup that is exiting right now — the + // post-install "Launch tty7" click — where the launch deserves its + // daemon, not an error. #[cfg(windows)] - if crate::daemon::update_guard::held() { - anyhow::bail!( - "a tty7 update is being installed right now; the daemon will return \ - when the updater relaunches the app" - ); + { + const UPDATE_GUARD_PATIENCE: Duration = Duration::from_secs(5); + let deadline = Instant::now() + UPDATE_GUARD_PATIENCE; + while crate::daemon::update_guard::held() { + if Instant::now() >= deadline { + anyhow::bail!( + "a tty7 update is being installed right now; the daemon will return \ + when the installer relaunches the app" + ); + } + std::thread::sleep(POLL_INTERVAL); + } } spawn_detached()?; @@ -400,14 +410,7 @@ fn reap_process(pid: libc::pid_t) { #[cfg(any(target_os = "macos", target_os = "linux"))] fn signal_and_await_exit(pid: libc::pid_t, sig: libc::c_int, timeout: Duration) -> bool { unsafe { libc::kill(pid, sig) }; - let deadline = Instant::now() + timeout; - while process_alive(pid) { - if Instant::now() >= deadline { - return false; - } - std::thread::sleep(POLL_INTERVAL); - } - true + wait_for_recorded_exit(pid as u32, timeout) } #[cfg(any(target_os = "macos", target_os = "linux"))] diff --git a/crates/tty7-core/src/daemon/update_guard.rs b/crates/tty7-core/src/daemon/update_guard.rs index 99f665a5..14f01b17 100644 --- a/crates/tty7-core/src/daemon/update_guard.rs +++ b/crates/tty7-core/src/daemon/update_guard.rs @@ -1,39 +1,78 @@ -//! A file that says "an updater is replacing this installation right now". +//! A file that says "an installer is replacing this installation right now". //! //! Between `spawn::stop_for_update` clearing the installed images and the //! installer finishing, nothing used to stop a `tty7` CLI call — or a //! manually launched GUI — from spawning a fresh daemon that relocks the very -//! files being replaced. The guard closes that window: the updater holds it -//! for the whole installation and releases it just before relaunching the -//! app, and `spawn::ensure_running` refuses to spawn a daemon while it is -//! held. Only spawning is deferred; connecting to a daemon that is already -//! running stays untouched. +//! files being replaced. The guard closes that window: whoever drives the +//! installation holds it, and `spawn::ensure_running` refuses to spawn a +//! daemon while it is held. Only spawning is deferred; connecting to a daemon +//! that is already running stays untouched. //! -//! The guard names its holder by pid so it can never outlive a crashed -//! updater: a guard whose writer is gone is removed on sight, and a TTL -//! backstops the one coincidence pid-liveness cannot see — the dead writer's -//! pid recycled by an unrelated long-lived process. +//! The guard names its holder by pid, and a pid is only believed to be the +//! holder while the process behind it *could* be: it must be alive, and it +//! must have started before the guard was written — a process born later +//! merely inherited the number. That check is what lets a holder keep the +//! guard for as long as its installation genuinely runs (an install slowed +//! past any fixed budget by an antivirus sweep stays protected), while a +//! crashed holder's guard goes stale the moment its pid dies or is recycled. +//! Only when the start time cannot be read at all does a TTL bound the doubt. +//! +//! Two kinds of holder: +//! - the auto-updater (`tty7-updater.exe install`/`install-portable`) holds +//! for itself, from stopping the daemon until it relaunches the app; +//! - the `--stop-daemon --update-install-dir` helper that Inno's +//! `PrepareToInstall`/`[UninstallRun]` runs holds for its *parent* — the +//! Setup or uninstaller that keeps replacing files long after the helper +//! returns. Nobody clears that one; it goes stale when Setup exits. -use std::path::{Path, PathBuf}; -use std::time::Duration; +use std::path::PathBuf; +use std::time::{Duration, SystemTime}; use crate::core::config; +use crate::daemon::winproc; -/// Far above any real installation's duration (Setup runs in seconds), and -/// the most a recycled writer pid can cost. +/// Bounds how long a writer whose start time cannot be read may hold spawns +/// back. Never reached by a verifiable writer — see [`writer_holds`]. const GUARD_TTL: Duration = Duration::from_secs(10 * 60); +/// Clock-versus-filesystem slack when comparing a process's start against the +/// guard's mtime. Generous: the two are the same machine's clock, but FAT +/// timestamps are coarse. +const START_SLACK: Duration = Duration::from_secs(10); + fn path() -> Option { config::config_path("update.lock") } /// Claims the guard for the calling process. pub fn hold() { + hold_for(std::process::id()); +} + +/// Claims the guard for the calling process's parent. For the helper Inno +/// runs: the helper exits as soon as the daemon stop returns, but its parent +/// — Setup — lives exactly as long as the files are being replaced, which is +/// the lifetime the guard has to match. Falls back to the caller itself when +/// the parent cannot be named; that guard goes stale at the caller's exit, +/// which is no worse than not holding one. +pub fn hold_for_parent() { + let own = std::process::id(); + let parent = winproc::snapshot() + .iter() + .find(|process| process.pid == own) + .map(|process| process.parent); + match parent { + Some(pid) if pid > 4 => hold_for(pid), + _ => hold(), + } +} + +fn hold_for(pid: u32) { let Some(path) = path() else { return }; if let Some(parent) = path.parent() { let _ = std::fs::create_dir_all(parent); } - if let Err(error) = std::fs::write(&path, std::process::id().to_string()) { + if let Err(error) = std::fs::write(&path, pid.to_string()) { log::warn!( "could not write the update guard {}: {error}", path.display() @@ -48,37 +87,51 @@ pub fn clear() { } } -/// Whether an updater is installing right now. A guard whose writer has -/// exited or that outlived the TTL is stale — the updater clears it on every -/// path that relaunches the app, so a leftover means it died — and is removed -/// here so one crash never costs more than one look. +/// Whether an installer is replacing the installation right now. A stale +/// guard — dead or recycled writer, unreadable garbage — is removed on sight, +/// so one crashed holder never costs more than one look. pub(crate) fn held() -> bool { let Some(path) = path() else { return false }; let Ok(contents) = std::fs::read_to_string(&path) else { return false; }; - let live = contents - .trim() - .parse::() - .is_ok_and(|pid| process_alive(pid)); - if !live || expired(&path) { + let written = std::fs::metadata(&path).and_then(|meta| meta.modified()).ok(); + let holds = contents.trim().parse::().ok().is_some_and(|pid| { + writer_holds(process_alive(pid), winproc::creation_time(pid), written) + }); + if !holds { log::info!("removing a stale update guard at {}", path.display()); let _ = std::fs::remove_file(&path); - return false; } - true + holds } -fn expired(path: &Path) -> bool { - std::fs::metadata(path) - .and_then(|meta| meta.modified()) - .ok() - .and_then(|modified| modified.elapsed().ok()) - .is_some_and(|age| age > GUARD_TTL) +/// The staleness policy, pure so every case is testable: `started` is when +/// the process wearing the recorded pid began, `written` the guard's mtime. +fn writer_holds( + alive: bool, + started: Option, + written: Option, +) -> bool { + if !alive { + return false; + } + match (started, written) { + // The writer wrote the guard after it started; a "writer" born later + // is a recycled pid wearing its number. A verified writer holds for + // as long as it lives — an install slowed past any fixed budget is + // still an install. + (Some(started), Some(written)) => started <= written + START_SLACK, + // Alive but unverifiable: the TTL bounds how long a pid that cannot + // be told from a recycled one may hold spawns back. + (None, Some(written)) => written.elapsed().is_ok_and(|age| age <= GUARD_TTL), + // No readable mtime to reason from at all. + _ => false, + } } fn process_alive(pid: u32) -> bool { - !crate::daemon::winproc::wait_for_exit(pid, Duration::ZERO) + !winproc::wait_for_exit(pid, Duration::ZERO) } #[cfg(test)] @@ -101,8 +154,31 @@ mod tests { pid } - // One test, like the pidfile's: the guard file is process-global state, - // and two tests sharing it would race each other. + #[test] + fn staleness_policy_trusts_only_a_live_writer_born_before_the_guard() { + let now = SystemTime::now(); + let before = now - Duration::from_secs(60); + let later = now + Duration::from_secs(60); + + assert!(writer_holds(true, Some(before), Some(now))); + assert!( + writer_holds(true, Some(now - GUARD_TTL * 3), Some(now - GUARD_TTL * 2)), + "a verified writer is never expired by the TTL, however long it runs" + ); + assert!( + !writer_holds(true, Some(later), Some(now)), + "a process born after the guard is a recycled pid" + ); + assert!(!writer_holds(false, Some(before), Some(now))); + + // Liveness without identity gets exactly the TTL. + assert!(writer_holds(true, None, Some(now))); + assert!(!writer_holds(true, None, Some(now - GUARD_TTL * 2))); + assert!(!writer_holds(true, None, None)); + } + + // One test for the file lifecycle, like the pidfile's: the guard file is + // process-global state, and two tests sharing it would race each other. #[test] fn guard_lifecycle_holds_for_a_live_writer_and_sheds_stale_files() { pin_config_dir(); @@ -110,7 +186,7 @@ mod tests { assert!(!held(), "no guard file, no guard"); hold(); - assert!(held(), "this process is alive, so its guard holds"); + assert!(held(), "this process is alive and older than its guard"); clear(); assert!(!held()); diff --git a/crates/tty7-core/src/daemon/winproc.rs b/crates/tty7-core/src/daemon/winproc.rs index fa28fb2c..364a3281 100644 --- a/crates/tty7-core/src/daemon/winproc.rs +++ b/crates/tty7-core/src/daemon/winproc.rs @@ -113,6 +113,41 @@ pub(crate) fn image_path(pid: u32) -> Option { } } +/// When the process started, or `None` for one this user cannot query. What +/// makes a pid an identity: a process claiming to be the writer of a file +/// must have started *before* that file was written, or it merely inherited +/// the writer's number. +pub(crate) fn creation_time(pid: u32) -> Option { + use windows_sys::Win32::Foundation::{CloseHandle, FILETIME}; + use windows_sys::Win32::System::Threading::{ + GetProcessTimes, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, + }; + + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + return None; + } + let mut creation: FILETIME = std::mem::zeroed(); + let mut exit: FILETIME = std::mem::zeroed(); + let mut kernel: FILETIME = std::mem::zeroed(); + let mut user: FILETIME = std::mem::zeroed(); + let ok = GetProcessTimes(handle, &mut creation, &mut exit, &mut kernel, &mut user); + CloseHandle(handle); + if ok == 0 { + return None; + } + let ticks = (u64::from(creation.dwHighDateTime) << 32) | u64::from(creation.dwLowDateTime); + // FILETIME counts 100 ns ticks from 1601-01-01; Unix time starts + // 11 644 473 600 seconds later. + let unix_ticks = ticks.checked_sub(11_644_473_600 * 10_000_000)?; + Some( + std::time::UNIX_EPOCH + + std::time::Duration::new(unix_ticks / 10_000_000, (unix_ticks % 10_000_000) as u32 * 100), + ) + } +} + /// Waits until the process is gone, up to `timeout`. Returns whether it exited /// in time. A pid that cannot be opened is reported as exited: the handle is /// what names the process, and no handle means there is nothing left to wait @@ -166,10 +201,29 @@ pub(crate) fn reap_descendants_of(root: u32, timeout: std::time::Duration) { /// to shut down. pub(crate) fn processes_running_from(dir: &std::path::Path) -> Vec { let own = std::process::id(); + // Image paths come back in long, resolved form, so a `dir` spelled + // through a junction, a subst drive, or an 8.3 short name would never + // prefix-match them. Trying the canonical spelling as well closes that — + // one syscall for the directory, not one per process. (A miss here still + // cannot let an installer proceed over a lock: `wait_until_images_unlocked` + // probes the files themselves and its callers abort on timeout.) + let canonical = std::fs::canonicalize(dir).ok().and_then(|real| { + let text = real.to_str()?; + Some(std::path::PathBuf::from( + text.strip_prefix(r"\\?\").unwrap_or(text), + )) + }); snapshot() .iter() .filter(|p| p.pid != own && p.pid > 4) - .filter(|p| image_path(p.pid).is_some_and(|image| path_is_under(&image, dir))) + .filter(|p| { + image_path(p.pid).is_some_and(|image| { + path_is_under(&image, dir) + || canonical + .as_deref() + .is_some_and(|canonical| path_is_under(&image, canonical)) + }) + }) .map(|p| p.pid) .collect() } @@ -290,6 +344,12 @@ mod tests { assert!(!path_is_under(Path::new(r"C:\anything"), Path::new(""))); } + #[test] + fn creation_time_of_this_process_is_in_the_past() { + let started = creation_time(std::process::id()).expect("own process is queryable"); + assert!(started <= std::time::SystemTime::now()); + } + #[test] fn exe_name_reads_up_to_the_nul() { let mut raw = [0u16; 260]; diff --git a/src/core/update.rs b/src/core/update.rs index d728cfeb..60ab8f5a 100644 --- a/src/core/update.rs +++ b/src/core/update.rs @@ -1015,13 +1015,20 @@ fn reconcile_portable_backups() -> Vec { return Vec::new(); }; let (interrupted, finished) = scan_portable_backups(&dir); - if let Some(backup) = interrupted.first() { + if !interrupted.is_empty() { + // All of them, not the first: two interrupted attempts in a row leave + // two backups, and one the user never hears about is one they delete + // blind or keep forever. + let preserved = interrupted + .iter() + .map(|backup| backup.display().to_string()) + .collect::>() + .join(", "); let detail = format!( "a previous update was interrupted while replacing the installed files, so {} may \ - mix two versions; the files from before that update are preserved at {} — restore \ + mix two versions; the files from before are preserved at {preserved} — restore \ them or reinstall, then delete the backup", - dir.display(), - backup.display() + dir.display() ); log::warn!("{detail}"); let mut state = UpdateState::load(); diff --git a/src/main.rs b/src/main.rs index e65aa1b6..b298ddce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -365,6 +365,12 @@ fn main() { // not). Invoked by the Inno PrepareToInstall step and the updater. #[cfg(windows)] if let Some(dir) = update_install_dir_from(args.iter().cloned()) { + // Held in the *parent's* name: this helper returns in seconds, + // but the Setup (or uninstaller) that invoked it keeps replacing + // files in `dir` until it exits — and a daemon spawned in that + // window would relock them. The guard needs no clearing; it goes + // stale the moment that parent is gone. + tty7_core::daemon::update_guard::hold_for_parent(); if let Err(error) = crate::daemon::spawn::stop_for_update(&dir) { log::error!( "preparing {} for replacement failed: {error}",