fix(self-update): wait on inherited parent handles on Windows

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()))
     }
 }
 

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<utils::ExitCode> {
-    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<utils::Ex
     Ok(utils::ExitCode(0))
 }
 
-pub(crate) fn wait_for_parent() -> 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<Child> {
+    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::<usize>()? 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<utils::ExitCode> {
-    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";

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<std::process::Child>);
+    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;