refactor(windows/uninstall-gc): refactor Windows uninstall GC APIs

Cargo.toml

@@ -110,7 +110,6 @@ snapbox = { version = "1", features = ["term-svg"], optional = true }
 walkdir = { version = "2", optional = true }
 
 [target."cfg(windows)".dependencies]
-scopeguard = "1"
 windows-registry = "0.100"
 windows-result = "0.100"
 

src/cli/self_update/windows.rs

@@ -3,20 +3,46 @@ use std::{
     env::{consts::EXE_SUFFIX, split_paths},
     ffi::{OsStr, OsString},
     fmt,
-    io::Write,
-    os::windows::ffi::OsStrExt,
-    path::Path,
-    process::Command,
+    fs::OpenOptions,
+    io::{self, Write},
+    mem,
+    os::windows::{
+        fs::OpenOptionsExt,
+        io::{AsRawHandle, FromRawHandle, OwnedHandle},
+    },
+    path::{Path, PathBuf},
+    process::{Command, Stdio},
+    ptr, thread,
+    time::Duration,
 };
 
 use anyhow::{Context, anyhow};
+use cc::windows_registry::{find_tool, find_vs_version};
 use itertools::Itertools;
 use tracing::{info, warn};
 #[cfg(any(test, feature = "test"))]
 use windows_registry::Value;
 use windows_registry::{CURRENT_USER, HSTRING, Key};
 use windows_result::WIN32_ERROR;
-use windows_sys::Win32::Foundation::{ERROR_FILE_NOT_FOUND, ERROR_INVALID_DATA};
+use windows_sys::Win32::{
+    Foundation::{
+        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,
+    },
+    System::{
+        Diagnostics::ToolHelp::{
+            CreateToolhelp32Snapshot, PROCESSENTRY32, Process32First, Process32Next,
+            TH32CS_SNAPPROCESS,
+        },
+        Threading::{GetCurrentProcessId, INFINITE, OpenProcess, WaitForSingleObject},
+    },
+    UI::WindowsAndMessaging::{
+        HWND_BROADCAST, SMTO_ABORTIFHUNG, SendMessageTimeoutA, WM_SETTINGCHANGE,
+    },
+};
 
 use crate::{
     cli::{
@@ -41,7 +67,7 @@ pub(crate) fn ensure_prompt(process: &Process) -> anyhow::Result<()> {
 fn choice(max: u8, process: &Process) -> anyhow::Result<Option<u8>> {
     write!(process.stdout().lock(), ">")?;
 
-    let _ = std::io::stdout().flush();
+    let _ = io::stdout().flush();
     let input = common::read_line(process)?;
 
     let r = match str::parse(&input) {
@@ -191,20 +217,19 @@ pub(crate) fn do_msvc_check(opts: &InstallOpts<'_>, process: &Process) -> Option
         return None;
     }
 
-    use cc::windows_registry;
     let host_tuple = if let Some(tuple) = opts.default_host_tuple.as_ref() {
         tuple.to_owned()
     } else {
         TargetTuple::from_host_or_build(process).to_string()
     };
     let installing_msvc = host_tuple.contains("msvc");
-    let have_msvc = windows_registry::find_tool(&host_tuple, "cl.exe").is_some();
+    let have_msvc = find_tool(&host_tuple, "cl.exe").is_some();
     if installing_msvc && !have_msvc {
         // Visual Studio build tools are required.
         // If the user does not have Visual Studio installed and their host
         // machine is i686 or x86_64 then it's OK to try an auto install.
         // Otherwise a manual install will be required.
-        let has_any_vs = windows_registry::find_vs_version().is_ok();
+        let has_any_vs = find_vs_version().is_ok();
         let is_x86 = host_tuple.contains("i686") || host_tuple.contains("x86_64");
         if is_x86 && !has_any_vs {
             Some(VsInstallPlan::Automatic)
@@ -358,46 +383,32 @@ 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> {
-    use std::process::Stdio;
+    let uninstall = wait_for_parent().and_then(|()| {
+        let no_modify_path = process.var_os(GC_MODIFY_PATH).as_deref() != Some(OsStr::new("1"));
 
-    wait_for_parent()?;
-
-    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.
-    let cargo_home = process.cargo_home()?;
-    super::clean_cargo_home(no_modify_path, process, &cargo_home)?;
+        // Now that the parent has exited there are hopefully no more files open in CARGO_HOME.
+        let cargo_home = process.cargo_home()?;
+        super::clean_cargo_home(no_modify_path, process, &cargo_home)
+    });
 
     // 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 so the standard library passes GC's delete-on-close
+    // handle to the cleanup child without raw handle APIs.
+    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))
 }
 
 pub(crate) fn wait_for_parent() -> anyhow::Result<()> {
-    use std::{io, mem};
-
-    use windows_sys::Win32::{
-        Foundation::{CloseHandle, INVALID_HANDLE_VALUE, WAIT_OBJECT_0},
-        Storage::FileSystem::SYNCHRONIZE,
-        System::{
-            Diagnostics::ToolHelp::{
-                CreateToolhelp32Snapshot, PROCESSENTRY32, Process32First, Process32Next,
-                TH32CS_SNAPPROCESS,
-            },
-            Threading::{GetCurrentProcessId, INFINITE, OpenProcess, WaitForSingleObject},
-        },
-    };
-
     unsafe {
         // Take a snapshot of system processes, one of which is ours
         // and contains our parent's pid
@@ -407,23 +418,21 @@ pub(crate) fn wait_for_parent() -> anyhow::Result<()> {
             return Err(err).context(CliError::WindowsUninstallMadness);
         }
 
-        let snapshot = scopeguard::guard(snapshot, |h| {
-            let _ = CloseHandle(h);
-        });
+        let snapshot = OwnedHandle::from_raw_handle(snapshot);
 
         let mut entry: PROCESSENTRY32 = mem::zeroed();
         entry.dwSize = size_of::<PROCESSENTRY32>() as u32;
 
         // Iterate over system processes looking for ours
-        let success = Process32First(*snapshot, &mut entry);
+        let success = Process32First(snapshot.as_raw_handle(), &mut entry);
         if success == 0 {
             let err = io::Error::last_os_error();
             return Err(err).context(CliError::WindowsUninstallMadness);
         }
 
         let this_pid = GetCurrentProcessId();
         while entry.th32ProcessID != this_pid {
-            let success = Process32Next(*snapshot, &mut entry);
+            let success = Process32Next(snapshot.as_raw_handle(), &mut entry);
             if success == 0 {
                 let err = io::Error::last_os_error();
                 return Err(err).context(CliError::WindowsUninstallMadness);
@@ -442,12 +451,10 @@ pub(crate) fn wait_for_parent() -> anyhow::Result<()> {
             return Ok(());
         }
 
-        let parent = scopeguard::guard(parent, |h| {
-            let _ = CloseHandle(h);
-        });
+        let parent = OwnedHandle::from_raw_handle(parent);
 
         // Wait for our parent to exit
-        let res = WaitForSingleObject(*parent, INFINITE);
+        let res = WaitForSingleObject(parent.as_raw_handle(), INFINITE);
 
         if res != WAIT_OBJECT_0 {
             let err = io::Error::last_os_error();
@@ -464,15 +471,6 @@ pub(crate) fn do_add_to_path(process: &Process) -> anyhow::Result<()> {
 }
 
 fn _apply_new_path(new_path: Option<HSTRING>, process: &Process) -> anyhow::Result<()> {
-    use std::ptr;
-
-    use windows_sys::Win32::{
-        Foundation::{LPARAM, WPARAM},
-        UI::WindowsAndMessaging::{
-            HWND_BROADCAST, SMTO_ABORTIFHUNG, SendMessageTimeoutA, WM_SETTINGCHANGE,
-        },
-    };
-
     let Some(new_path) = new_path else {
         return Ok(()); // No need to set the path
     };
@@ -622,8 +620,6 @@ pub(crate) fn update_uninstall_registry_display_version(
 }
 
 pub(crate) fn add_uninstall_registry_entry(process: &Process) -> anyhow::Result<()> {
-    use std::path::PathBuf;
-
     let key = rustup_uninstall_registry_key(process)?;
 
     // Don't overwrite registry if Rustup is already installed
@@ -692,14 +688,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.
 //
@@ -712,17 +707,6 @@ 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 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,
-        },
-    };
-
     // CARGO_HOME, hopefully empty except for bin/rustup.exe
     let cargo_home = process.cargo_home()?;
     // The rustup.exe bin
@@ -739,39 +723,20 @@ 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);
-        })
-    };
+    // OpenOptions preserves the read, sharing and delete-on-close flags while
+    // letting File own the 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)
+    // 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)?;

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(format!("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;