mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 00:02:23 +00:00
fix(update): verify the guard's writer by start time, and guard manual Setup runs
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.
This commit is contained in:
@@ -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"))]
|
||||
|
||||
@@ -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<PathBuf> {
|
||||
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::<u32>()
|
||||
.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::<u32>().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<SystemTime>,
|
||||
written: Option<SystemTime>,
|
||||
) -> 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());
|
||||
|
||||
|
||||
@@ -113,6 +113,41 @@ pub(crate) fn image_path(pid: u32) -> Option<std::path::PathBuf> {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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<std::time::SystemTime> {
|
||||
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<u32> {
|
||||
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];
|
||||
|
||||
+11
-4
@@ -1015,13 +1015,20 @@ fn reconcile_portable_backups() -> Vec<PathBuf> {
|
||||
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::<Vec<_>>()
|
||||
.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();
|
||||
|
||||
@@ -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}",
|
||||
|
||||
Reference in New Issue
Block a user