From 6afe8ea679dbd80bc9c0696fe78d7403e3526545 Mon Sep 17 00:00:00 2001 From: Cloud0310 <60375730+Cloud0310@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:15:54 +0800 Subject: [PATCH] fix(self-update): wait on inherited parent handles on Windows --- src/cli/self_update/stage.rs | 12 ++-- src/cli/self_update/windows.rs | 70 ++++++++++++++++---- tests/suite/cli_self_upd.rs | 115 ++++++++++++++++++++++++++++++++- 3 files changed, 176 insertions(+), 21 deletions(-) diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index 401a9dff72..948ddd606f 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -124,11 +124,13 @@ impl PreparedUpdater { .path .parent() .context("self-updater path has no parent directory")?; - Command::new(&self.path) - .env(STAGE_ENV, stage) - .arg("--self-replace") - .spawn() - .with_context(|| format!("unable to run updater ({})", self.path.display())) + let mut command = Command::new(&self.path); + command.env(STAGE_ENV, stage).arg("--self-replace"); + #[cfg(windows)] + let child = super::windows::spawn_with_parent_handle(&mut command); + #[cfg(not(windows))] + let child = command.spawn(); + child.with_context(|| format!("unable to run updater ({})", self.path.display())) } } diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index c4ca3d538a..deb87be797 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -8,10 +8,10 @@ use std::{ mem, os::windows::{ fs::OpenOptionsExt, - io::{AsRawHandle, FromRawHandle, OwnedHandle}, + io::{AsRawHandle, FromRawHandle, OwnedHandle, RawHandle}, }, path::{Path, PathBuf}, - process::{Command, Stdio}, + process::{Child, Command, Stdio}, ptr, thread, time::Duration, }; @@ -26,8 +26,8 @@ use windows_registry::{CURRENT_USER, HSTRING, Key}; use windows_result::WIN32_ERROR; use windows_sys::Win32::{ Foundation::{ - ERROR_FILE_NOT_FOUND, ERROR_INVALID_DATA, INVALID_HANDLE_VALUE, LPARAM, WAIT_OBJECT_0, - WPARAM, + DuplicateHandle, ERROR_FILE_NOT_FOUND, ERROR_INVALID_DATA, INVALID_HANDLE_VALUE, LPARAM, + WAIT_OBJECT_0, WPARAM, }, Storage::FileSystem::{ FILE_FLAG_DELETE_ON_CLOSE, FILE_SHARE_DELETE, FILE_SHARE_READ, SYNCHRONIZE, @@ -37,7 +37,9 @@ use windows_sys::Win32::{ CreateToolhelp32Snapshot, PROCESSENTRY32, Process32First, Process32Next, TH32CS_SNAPPROCESS, }, - Threading::{GetCurrentProcessId, INFINITE, OpenProcess, WaitForSingleObject}, + Threading::{ + GetCurrentProcess, GetCurrentProcessId, INFINITE, OpenProcess, WaitForSingleObject, + }, }, UI::WindowsAndMessaging::{ HWND_BROADCAST, SMTO_ABORTIFHUNG, SendMessageTimeoutA, WM_SETTINGCHANGE, @@ -382,7 +384,7 @@ fn has_windows_sdk_libs(process: &Process) -> bool { /// Run by rustup-gc-$num.exe to delete CARGO_HOME #[tracing::instrument(level = "trace")] pub fn complete_windows_uninstall(process: &Process) -> anyhow::Result { - let uninstall = wait_for_parent().and_then(|()| { + let uninstall = wait_for_parent(process).and_then(|()| { let no_modify_path = process.var_os(GC_MODIFY_PATH).as_deref() != Some(OsStr::new("1")); // Now that the parent has exited there are hopefully no more files open in CARGO_HOME. @@ -407,7 +409,45 @@ pub fn complete_windows_uninstall(process: &Process) -> anyhow::Result anyhow::Result<()> { +/// Starts a helper that calls `wait_for_parent` before replacing or deleting files. +pub(super) fn spawn_with_parent_handle(command: &mut Command) -> io::Result { + let mut parent = ptr::null_mut(); + // SAFETY: GetCurrentProcess is valid for this call, and parent is writable. + // Duplicate the pseudo-handle into a real, inheritable SYNCHRONIZE handle. + let duplicated = unsafe { + let current = GetCurrentProcess(); + DuplicateHandle(current, current, current, &mut parent, SYNCHRONIZE, 1, 0) + }; + if duplicated == 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: DuplicateHandle succeeded and ownership has not been transferred yet. + let parent = unsafe { OwnedHandle::from_raw_handle(parent) }; + + // Keep our handle open until spawn returns; the child then owns its inherited copy. + command + .env(PARENT_HANDLE, (parent.as_raw_handle() as usize).to_string()) + .spawn() +} + +fn wait_for_parent(process: &Process) -> anyhow::Result<()> { + let Some(parent) = process.var_opt(PARENT_HANDLE)? else { + return wait_for_parent_legacy(); + }; + let parent = parent.parse::()? as RawHandle; + // SAFETY: spawn_with_parent_handle transfers a dedicated inherited process handle. + // This is its only Rust owner in the helper, which calls this function once. + let parent = unsafe { OwnedHandle::from_raw_handle(parent) }; + // SAFETY: parent keeps the inherited SYNCHRONIZE handle open throughout the wait. + if unsafe { WaitForSingleObject(parent.as_raw_handle(), INFINITE) } != WAIT_OBJECT_0 { + return Err(io::Error::last_os_error()).context("failed to wait for parent process"); + } + Ok(()) +} + +// Compatibility with launchers which do not pass a parent process handle. +// TODO: Delete this when it's suitable. +fn wait_for_parent_legacy() -> anyhow::Result<()> { unsafe { // Take a snapshot of system processes, one of which is ours // and contains our parent's pid @@ -664,7 +704,9 @@ pub(super) fn run_update( } pub(crate) fn self_replace(process: &Process) -> anyhow::Result { - wait_for_parent()?; + #[cfg(feature = "test")] + process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); + wait_for_parent(process)?; let self_update_lock = SelfUpdateLock::lock(process)?; let result = process.cargo_home().and_then(|cargo_home| { self_update_lock.install_bins(&cargo_home.join("bin"), super::force_hard_links(process))?; @@ -729,15 +771,13 @@ pub(crate) fn spawn_uninstall_gc(no_modify_path: bool, cargo_home: &Path) -> any .open(&gc_exe) .context(CliError::WindowsUninstallMadness)?; - // Pass the file as GC stdin so the standard library manages inheritance. - // Command retains the parent handle after spawn; keep it alive through the sleep. let mut command = Command::new(gc_exe); command .stdin(gc_handle) - .env(GC_MODIFY_PATH, if no_modify_path { "0" } else { "1" }) - .spawn() - .context(CliError::WindowsUninstallMadness)?; + .env(GC_MODIFY_PATH, if no_modify_path { "0" } else { "1" }); + spawn_with_parent_handle(&mut command).context(CliError::WindowsUninstallMadness)?; + // Command retains the deletion handle after spawn; keep it alive through the sleep. // The catch 22 article says we must sleep here to give // Windows a chance to bump the processes file reference // count. acrichto though is in disbelief and *demanded* that @@ -754,6 +794,10 @@ pub(crate) fn spawn_uninstall_gc(no_modify_path: bool, cargo_home: &Path) -> any // so we use env var here, notifying it if we need to remove $CARGO_HOME/bin from $PATH const GC_MODIFY_PATH: &str = "RUSTUP_GC_MODIFY_PATH"; +// Decimal value of the process handle inherited by GC or the self-replacer. +// Older launchers omit it, so their helpers use PID lookup instead. +const PARENT_HANDLE: &str = "RUSTUP_PARENT_HANDLE"; + /// Environment variable carrying the per-test registry ID. #[cfg(any(test, feature = "test"))] pub const RUSTUP_REGISTRY_TEST_ID: &str = "RUSTUP_REGISTRY_TEST_ID"; diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index bba052364c..32320016b8 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -12,14 +12,13 @@ use retry::{ delay::{Fibonacci, jitter}, retry, }; -#[cfg(unix)] -use rustup::cli::self_update::CHECKPOINT_SELF_REPLACE_READY; #[cfg(windows)] use rustup::test::RegistryValueId; use rustup::{ DUP_TOOLS, TOOLS, cli::self_update::{ - CHECKPOINT_SELF_UPDATE_PREPARED, Marker, SELF_UPDATE_DIRECTORY, updater_path, + CHECKPOINT_SELF_REPLACE_READY, CHECKPOINT_SELF_UPDATE_PREPARED, Marker, + SELF_UPDATE_DIRECTORY, updater_path, }, test::{ CROSS_ARCH1, CliTestContext, Scenario, SelfUpdateTestContext, calc_hash, @@ -509,6 +508,116 @@ async fn update_overwrites_programs_display_version() { ); } +#[cfg(windows)] +#[tokio::test] +async fn self_replace_waits_for_legacy_parent() { + use std::io::Write; + + let cx = setup_empty_installed().await; + let rustup_home = &cx.config.rustupdir.rustupdir; + let stage = rustup_home.join(SELF_UPDATE_DIRECTORY); + fs::create_dir_all(&stage).unwrap(); + let updater = managed_updater(rustup_home); + fs::copy(cx.config.exedir.join("rustup-init.exe"), &updater).unwrap(); + // Make the replacement distinguishable without changing its behavior. + writeln!(fs::OpenOptions::new().append(true).open(&updater).unwrap()).unwrap(); + let rustup = cx.config.cargodir.join("bin/rustup.exe"); + let expected_hash = calc_hash(&updater); + assert_ne!(calc_hash(&rustup), expected_hash); + + // The test subprocess acts as an old launcher: it spawns the new updater + // directly, without providing the inherited-parent-handle protocol. + let mut launcher = Command::new(std::env::current_exe().unwrap()); + cx.config.env(&mut launcher); + let status = launcher + .args([ + "--exact", + "suite::cli_self_upd::legacy_self_update_launcher", + "--ignored", + "--nocapture", + ]) + .env("RUSTUP_TEST_LEGACY_UPDATER", &updater) + .env("RUSTUP_SELF_UPDATE_STAGE", &stage) + .env(rustup::test::CHECKPOINT_ENV, CHECKPOINT_SELF_REPLACE_READY) + .env_remove("RUSTUP_PARENT_HANDLE") + .status() + .unwrap(); + assert!(status.success(), "legacy launcher failed: {status}"); + + wait_for_completed_update(rustup_home); + assert_eq!(calc_hash(&rustup), expected_hash); + let mut updated = Command::new(&rustup); + cx.config.env(&mut updated); + assert!(updated.arg("--version").status().unwrap().success()); +} + +#[cfg(windows)] +#[test] +#[ignore = "subprocess helper for self_replace_waits_for_legacy_parent"] +fn legacy_self_update_launcher() { + use std::{ + thread, + time::{Duration, Instant}, + }; + + // Reap the updater on assertion failure, so a failed test cannot leave it + // running against a test directory that is being removed. + struct Updater(Option); + impl Drop for Updater { + fn drop(&mut self) { + if let Some(child) = &mut self.0 { + let _ = child.kill(); + let _ = child.wait(); + } + } + } + + let updater = std::env::var_os("RUSTUP_TEST_LEGACY_UPDATER").unwrap(); + let rustup_home = PathBuf::from(std::env::var_os("RUSTUP_HOME").unwrap()); + let stage = rustup_home.join(SELF_UPDATE_DIRECTORY); + let rustup = PathBuf::from(std::env::var_os("CARGO_HOME").unwrap()).join("bin/rustup.exe"); + let before_hash = calc_hash(&rustup); + let marker = + rustup::test::checkpoint_path(rustup_home.parent().unwrap(), CHECKPOINT_SELF_REPLACE_READY); + let mut child = Updater(Some( + Command::new(updater) + .arg("--self-replace") + .env_remove("RUSTUP_PARENT_HANDLE") + .spawn() + .unwrap(), + )); + let deadline = Instant::now() + Duration::from_secs(10); + while !marker.is_file() { + assert!( + child.0.as_mut().unwrap().try_wait().unwrap().is_none(), + "updater exited before checkpoint" + ); + assert!( + Instant::now() < deadline, + "updater did not reach checkpoint" + ); + thread::sleep(Duration::from_millis(10)); + } + fs::remove_file(marker).unwrap(); + + // Allow the updater to run beyond the checkpoint. It must still be waiting + // for this process, even though no parent handle was supplied. + let deadline = Instant::now() + Duration::from_millis(250); + while Instant::now() < deadline { + assert!( + child.0.as_mut().unwrap().try_wait().unwrap().is_none(), + "updater exited while its parent was alive" + ); + assert!(!Marker::Complete.path(&stage).exists()); + assert!(!Marker::Failed.path(&stage).exists()); + thread::sleep(Duration::from_millis(10)); + } + assert_eq!(calc_hash(&rustup), before_hash); + // Leave the updater running; returning from this subprocess releases its + // parent wait, and the outer test verifies the completed replacement. + child.0.take(); +} + #[tokio::test] async fn update_but_not_installed() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await;