diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 28fcdcfdac..a37b49a8cb 100644 --- a/src/cli/self_update/windows.rs +++ b/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,27 @@ fn has_windows_sdk_libs(process: &Process) -> bool { pub fn complete_windows_uninstall(process: &Process) -> anyhow::Result { 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 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)) } @@ -691,14 +692,13 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result // - 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 +711,10 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result // .. 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 +733,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::() 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)?; diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 83ca66b671..827fa19ff8 100644 --- a/tests/suite/cli_self_upd.rs +++ b/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::>(); + 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::>(); - if garbage.is_empty() { - Ok(()) - } else { - Err(GcErr(garbage)) - } -} - -#[derive(thiserror::Error, Debug)] -#[error("garbage remaining: {:?}", .0)] -#[cfg(windows)] -struct GcErr(Vec); - #[tokio::test] async fn update_exact() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await;