fix(windows/uninstall-gc): cleanup gc program even if uninstall failed

src/cli/self_update/windows.rs

@@ -4,7 +4,6 @@ use std::{
     ffi::{OsStr, OsString},
     fmt,
     io::Write,
-    os::windows::ffi::OsStrExt,
     path::Path,
     process::Command,
 };
@@ -360,25 +359,26 @@ fn has_windows_sdk_libs(process: &Process) -> bool {
 pub fn complete_windows_uninstall(process: &Process) -> anyhow::Result<utils::ExitCode> {
     use std::process::Stdio;
 
-    wait_for_parent()?;
-
-    let no_modify_path = process.var_os(GC_MODIFY_PATH).as_deref() != Some(OsStr::new("1"));
+    let uninstall = wait_for_parent().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.
-    super::clean_cargo_home(no_modify_path, process)?;
+        // Now that the parent has exited there are hopefully no more files open in CARGO_HOME.
+        super::clean_cargo_home(no_modify_path, process)
+    });
 
     // Now, run a *system* binary to inherit the DELETE_ON_CLOSE
     // handle to *this* process, then exit. The OS will delete the gc
-    // exe when it exits.
-    let rm_gc_exe = OsStr::new("net");
-
-    Command::new(rm_gc_exe)
-        .stdin(Stdio::null())
+    // exe when it exits. Do this even if uninstalling failed.
+    // Leave stdin inherited: it carries GC's delete-on-close handle.
+    let cleanup = Command::new("net")
         .stdout(Stdio::null())
         .stderr(Stdio::null())
         .spawn()
-        .context(CliError::WindowsUninstallMadness)?;
+        .context(CliError::WindowsUninstallMadness);
 
+    // Preserve the original uninstall error if starting cleanup also failed.
+    uninstall?;
+    cleanup?;
     Ok(utils::ExitCode(0))
 }
 
@@ -691,14 +691,13 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result<utils::ExitCode>
 // - Open the gc exe with the FILE_FLAG_DELETE_ON_CLOSE and
 //   FILE_SHARE_DELETE flags. This is going to be the last
 //   file to remove, and the OS is going to do it for us.
-//   This file is opened as inheritable so that subsequent
-//   processes created with the option to inherit handles
-//   will also keep them open.
+//   Pass this handle as stdin so the standard library manages inheritance.
+//   GC does not read stdin; it uses it only to carry the deletion handle.
 // - Run the gc exe, which waits for the original rustup.exe
 //   process to close, then deletes CARGO_HOME. This process
 //   has inherited a FILE_FLAG_DELETE_ON_CLOSE handle to itself.
-// - Finally, spawn yet another system binary with the inherit handles
-//   flag, so *it* inherits the FILE_FLAG_DELETE_ON_CLOSE handle to
+// - Finally, spawn yet another system binary inheriting stdin,
+//   so *it* inherits the FILE_FLAG_DELETE_ON_CLOSE handle to
 //   the gc exe. If the gc exe exits before the system exe then at
 //   last it will be deleted when the handle closes.
 //
@@ -711,15 +710,10 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result<utils::ExitCode>
 // .. augmented with this SO answer
 // https://stackoverflow.com/questions/10319526/understanding-a-self-deleting-program-in-c
 pub(crate) fn spawn_uninstall_gc(no_modify_path: bool, process: &Process) -> anyhow::Result<()> {
-    use std::{io, ptr, thread, time::Duration};
+    use std::{fs::OpenOptions, os::windows::fs::OpenOptionsExt, thread, time::Duration};
 
-    use windows_sys::Win32::{
-        Foundation::{CloseHandle, GENERIC_READ, INVALID_HANDLE_VALUE},
-        Security::SECURITY_ATTRIBUTES,
-        Storage::FileSystem::{
-            CreateFileW, FILE_FLAG_DELETE_ON_CLOSE, FILE_SHARE_DELETE, FILE_SHARE_READ,
-            OPEN_EXISTING,
-        },
+    use windows_sys::Win32::Storage::FileSystem::{
+        FILE_FLAG_DELETE_ON_CLOSE, FILE_SHARE_DELETE, FILE_SHARE_READ,
     };
 
     // CARGO_HOME, hopefully empty except for bin/rustup.exe
@@ -738,39 +732,18 @@ pub(crate) fn spawn_uninstall_gc(no_modify_path: bool, process: &Process) -> any
     let gc_exe = work_path.join(format!("rustup-gc-{numbah:x}.exe"));
     // Copy rustup (probably this process's exe) to the gc exe
     utils::copy_file_symlink_to_source(&rustup_path, &gc_exe)?;
-    let gc_exe_win: Vec<_> = gc_exe.as_os_str().encode_wide().chain(Some(0)).collect();
-
-    // Make the sub-process opened by gc exe inherit its attribute.
-    let sa = SECURITY_ATTRIBUTES {
-        nLength: size_of::<SECURITY_ATTRIBUTES>() as u32,
-        lpSecurityDescriptor: ptr::null_mut(),
-        bInheritHandle: 1,
-    };
-
-    let _g = unsafe {
-        // Open an inheritable handle to the gc exe marked
-        // FILE_FLAG_DELETE_ON_CLOSE.
-        let gc_handle = CreateFileW(
-            gc_exe_win.as_ptr(),
-            GENERIC_READ,
-            FILE_SHARE_READ | FILE_SHARE_DELETE,
-            &sa,
-            OPEN_EXISTING,
-            FILE_FLAG_DELETE_ON_CLOSE,
-            ptr::null_mut(),
-        );
-
-        if gc_handle == INVALID_HANDLE_VALUE {
-            let err = io::Error::last_os_error();
-            return Err(err).context(CliError::WindowsUninstallMadness);
-        }
-
-        scopeguard::guard(gc_handle, |h| {
-            let _ = CloseHandle(h);
-        })
-    };
+    // File owns the delete-on-close handle until it is passed to Command below.
+    let gc_handle = OpenOptions::new()
+        .read(true)
+        .share_mode(FILE_SHARE_READ | FILE_SHARE_DELETE)
+        .custom_flags(FILE_FLAG_DELETE_ON_CLOSE)
+        .open(&gc_exe)
+        .context(CliError::WindowsUninstallMadness)?;
 
-    Command::new(gc_exe)
+    // Keep Command alive through the sleep so it retains our deletion handle.
+    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)?;

tests/suite/cli_self_upd.rs

@@ -395,39 +395,31 @@ async fn uninstall_doesnt_leave_gc_file() {
     // 100ms, but during the contention of test suites can be substantially
     // longer while still succeeding.
 
-    let check = || ensure_empty(parent);
+    let check = || {
+        let garbage = fs::read_dir(parent)
+            .unwrap()
+            .filter_map(|entry| {
+                let path = entry.unwrap().path();
+                let name = path.file_name()?.to_str()?;
+                // On Windows, this binary is cleaned up on exit
+                if !(name.starts_with("rustup-gc-") && name.ends_with(EXE_SUFFIX)) {
+                    return None;
+                }
+                Some(path.to_string_lossy().to_string())
+            })
+            .collect::<Vec<_>>();
+        if garbage.is_empty() {
+            Ok(())
+        } else {
+            Err(anyhow::anyhow!("garbage remaining: {garbage:?}"))
+        }
+    };
     match retry(Fibonacci::from_millis(1).map(jitter).take(23), check) {
         Ok(_) => (),
         Err(e) => panic!("{e}"),
     }
 }
 
-#[cfg(windows)]
-fn ensure_empty(dir: &Path) -> Result<(), GcErr> {
-    let garbage = fs::read_dir(dir)
-        .unwrap()
-        .filter_map(|entry| {
-            let path = entry.unwrap().path();
-            let name = path.file_name()?.to_str()?;
-            // On Windows, this binary is cleaned up on exit
-            if !(name.starts_with("rustup-gc-") && name.ends_with(EXE_SUFFIX)) {
-                return None;
-            }
-            Some(path.to_string_lossy().to_string())
-        })
-        .collect::<Vec<_>>();
-    if garbage.is_empty() {
-        Ok(())
-    } else {
-        Err(GcErr(garbage))
-    }
-}
-
-#[derive(thiserror::Error, Debug)]
-#[error("garbage remaining: {:?}", .0)]
-#[cfg(windows)]
-struct GcErr(Vec<String>);
-
 #[tokio::test]
 async fn update_exact() {
     let cx = SelfUpdateTestContext::new(TEST_VERSION).await;