diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 2f60fad5b4..6e5c0572c9 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -13,13 +13,12 @@ //! * update the PATH in a system-specific way //! * run the equivalent of `rustup default stable` //! -//! During upgrade (`rustup self upgrade`): +//! During upgrade (`rustup self update`): //! -//! * download rustup-init to $CARGO_HOME/bin/rustup-init -//! * run rustup-init with appropriate flags to indicate -//! this is a self-upgrade -//! * rustup-init copies bins and hardlinks into place. On windows -//! this happens *after* the upgrade command exits successfully. +//! * download rustup-init to a managed path under `$RUSTUP_HOME` +//! * run the downloaded binary in replacement mode +//! * atomically replace rustup and update its proxy links. On Windows +//! this happens after the update command exits. //! //! During uninstall (`rustup self uninstall`): //! @@ -77,29 +76,32 @@ use crate::{ #[macro_use] mod msg; +mod stage; +use stage::{PreparedUpdate, SelfUpdateLock}; + #[cfg(unix)] mod shell; #[cfg(unix)] mod unix; #[cfg(unix)] -use unix::{do_add_to_path, do_remove_from_path}; +pub(crate) use unix::self_replace; #[cfg(unix)] -pub(crate) use unix::{run_update, self_replace}; +use unix::{do_add_to_path, do_remove_from_path, run_update}; #[cfg(windows)] mod windows; #[cfg(windows)] pub use windows::complete_windows_uninstall; +#[cfg(windows)] +pub(crate) use windows::self_replace; #[cfg(all(windows, feature = "test"))] pub use windows::{RUSTUP_REGISTRY_TEST_ID, RegistryValueId, USER_PATH, get_path}; #[cfg(windows)] use windows::{ add_uninstall_registry_entry, do_add_to_path, do_remove_from_path, - remove_uninstall_registry_entry, + remove_uninstall_registry_entry, run_update, }; -#[cfg(windows)] -pub(crate) use windows::{run_update, self_replace}; pub(crate) struct InstallOpts<'a> { pub default_host_tuple: Option, @@ -533,10 +535,10 @@ impl SelfUpdateMode { SelfUpdatePermission::Permit => {} } - let setup_path = prepare_update(dl_cfg).await?; + let prepared_update = prepare_update(dl_cfg).await?; - if let Some(setup_path) = &setup_path { - return run_update(setup_path, dl_cfg.process); + if let Some(prepared_update) = prepared_update { + return run_update(prepared_update, dl_cfg.process); } else { // Try again in case we emitted "tool `{}` is already installed" last time. install_proxies(dl_cfg.process)?; @@ -583,8 +585,6 @@ impl fmt::Display for SelfUpdateMode { } } -static DEFAULT_UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup"; - fn update_root(process: &Process) -> String { process .var("RUSTUP_UPDATE_ROOT") @@ -773,19 +773,7 @@ fn warn_if_default_linker_missing(process: &Process) { } fn install_bins(process: &Process) -> anyhow::Result<()> { - let bin_path = process.cargo_home()?.join("bin"); - let this_exe_path = utils::current_exe()?; - let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); - - utils::ensure_dir_exists("bin", &bin_path)?; - // NB: Even on Linux we can't just copy the new binary over the (running) - // old binary; we must unlink it first. - if rustup_path.exists() { - utils::remove_file("rustup-bin", &rustup_path)?; - } - utils::copy_file_symlink_to_source(&this_exe_path, &rustup_path)?; - utils::make_executable(&rustup_path)?; - install_proxies(process) + SelfUpdateLock::acquire(process)?.install_bins(process) } pub(crate) fn install_proxies(process: &Process) -> anyhow::Result<()> { @@ -1122,21 +1110,10 @@ pub(crate) fn self_update_permitted(explicit: bool) -> anyhow::Result) -> anyhow::Result { common::warn_if_host_is_emulated(cfg.process); @@ -1162,8 +1139,9 @@ pub(crate) async fn update(cfg: &Cfg<'_>) -> anyhow::Result { } match prepare_update(&DownloadCfg::new(cfg)).await? { - Some(setup_path) => { - let Some(version) = get_and_parse_new_rustup_version(&setup_path) else { + Some(prepared_update) => { + let Some(version) = get_and_parse_new_rustup_version(prepared_update.updater_path()) + else { error!("failed to get rustup version"); return Ok(ExitCode::FAILURE); }; @@ -1173,7 +1151,7 @@ pub(crate) async fn update(cfg: &Cfg<'_>) -> anyhow::Result { PackageUpdate::Rustup, Ok(UpdateStatus::Updated(version)), ); - return run_update(&setup_path, cfg.process); + return run_update(prepared_update, cfg.process); } None => { let _ = common::show_channel_update( @@ -1214,18 +1192,14 @@ fn parse_new_rustup_version(version: String) -> String { String::from(matched_version) } -pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result> { +async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result> { let cargo_home = dl_cfg.process.cargo_home()?; let rustup_path = cargo_home.join(format!("bin{MAIN_SEPARATOR}rustup{EXE_SUFFIX}")); - let setup_path = cargo_home.join(format!("bin{MAIN_SEPARATOR}rustup-init{EXE_SUFFIX}")); if !rustup_path.exists() { return Err(CliError::NotSelfInstalled { p: cargo_home }.into()); } - - if setup_path.exists() { - utils::remove_file("setup", &setup_path)?; - } + let self_update_lock = SelfUpdateLock::acquire(dl_cfg.process)?; // Get build tuple let tuple = TargetTuple::from_build(); @@ -1265,18 +1239,22 @@ pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result) -> anyhow::Result { @@ -1371,16 +1349,15 @@ pub(crate) async fn check_rustup_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Res #[tracing::instrument(level = "trace")] pub(crate) fn cleanup_self_updater(process: &Process) -> anyhow::Result<()> { - let cargo_home = process.cargo_home()?; - let setup = cargo_home.join(format!("bin/rustup-init{EXE_SUFFIX}")); - - if setup.exists() { - utils::remove_file("setup", &setup)?; - } - - Ok(()) + stage::cleanup(process) } +static DEFAULT_UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup"; +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_UPDATE_PREPARED: &str = "self-update-prepared"; +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_REPLACE_READY: &str = "self-replace-ready"; + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -1436,8 +1413,10 @@ info: default host tuple is {0} fn install_bins_creates_cargo_home() { let root_dir = test_dir().unwrap(); let cargo_home = root_dir.path().join("cargo"); + let rustup_home = root_dir.path().join("rustup"); let mut vars = HashMap::new(); vars.env("CARGO_HOME", cargo_home.to_string_lossy().to_string()); + vars.env("RUSTUP_HOME", rustup_home); let tp = TestProcess::with_vars(vars); super::install_bins(&tp.process).unwrap(); assert!(cargo_home.exists()); diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs new file mode 100644 index 0000000000..6f170d8807 --- /dev/null +++ b/src/cli/self_update/stage.rs @@ -0,0 +1,543 @@ +use std::{ + env::consts::EXE_SUFFIX, + fs::{self, File, OpenOptions}, + io, + path::{Path, PathBuf}, + process::Command, + time::{Duration, SystemTime}, +}; + +use anyhow::Context; +use tempfile::TempPath; +use tracing::{debug, warn}; + +use super::install_proxies; +use crate::{process::Process, utils}; + +/// Exclusive right to download the updater or replace the installed rustup. +pub(super) struct SelfUpdateLock { + directory: PathBuf, + _file: File, +} + +impl SelfUpdateLock { + pub(super) fn acquire(process: &Process) -> anyhow::Result { + let lock = Self::open(process)?; + lock._file.lock().context("failed to lock self-update")?; + Ok(lock) + } + + fn try_acquire(process: &Process) -> anyhow::Result> { + let lock = Self::open(process)?; + match lock._file.try_lock() { + Ok(()) => Ok(Some(lock)), + Err(fs::TryLockError::WouldBlock) => Ok(None), + Err(fs::TryLockError::Error(error)) => Err(error).context("failed to lock self-update"), + } + } + + fn open(process: &Process) -> anyhow::Result { + let directory = stage_root(process)?; + utils::ensure_dir_exists("self-update", &directory)?; + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(directory.join(SELF_UPDATE_LOCK_FILE)) + .context("failed to open self-update lock")?; + + Ok(Self { + directory, + _file: file, + }) + } + + /// Clears the previous update's leftovers and reserves the managed updater path. + pub(super) fn prepare_updater(self) -> anyhow::Result { + let updater_path = self.updater_path(); + utils::ensure_file_removed("self-updater", &updater_path)?; + for marker in [Marker::Complete, Marker::Failed] { + utils::ensure_file_removed( + "self-update status marker", + &self.directory.join(marker.as_str()), + )?; + } + Ok(PreparedUpdate { + updater_path, + _lock: self, + }) + } + + /// Installs the running executable as `$CARGO_HOME/bin/rustup` and refreshes its proxies. + pub(super) fn install_bins(&self, process: &Process) -> anyhow::Result<()> { + self.install_bins_from(&utils::current_exe()?, process) + } + + fn install_bins_from(&self, this_exe_path: &Path, process: &Process) -> anyhow::Result<()> { + let bin_path = process.cargo_home()?.join("bin"); + let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); + + utils::ensure_dir_exists("bin", &bin_path)?; + let pending = stage_pending_binary(this_exe_path, &bin_path)?; + replace_rustup_binary(&pending, &rustup_path)?; + install_proxies(process) + } + + fn updater_path(&self) -> PathBuf { + self.directory.join(format!("rustup-init{EXE_SUFFIX}")) + } +} + +/// The managed updater path, held together with the lock that protects it. +pub(super) struct PreparedUpdate { + updater_path: PathBuf, + _lock: SelfUpdateLock, +} + +impl PreparedUpdate { + pub(super) fn replacer_command(&self) -> anyhow::Result { + let stage = self + .updater_path + .parent() + .context("self-updater path has no parent directory")?; + let mut command = Command::new(&self.updater_path); + command.env(STAGE_ENV, stage); + Ok(command) + } + + pub(super) fn updater_path(&self) -> &Path { + &self.updater_path + } +} + +pub(super) fn mark_result(process: &Process, succeeded: bool) { + let Some(stage) = process.var_os(STAGE_ENV).map(PathBuf::from) else { + return; + }; + let marker = if succeeded { + Marker::Complete + } else { + Marker::Failed + }; + if let Err(error) = mark_stage(process, &stage, marker) { + warn!("could not record self-update result: {error}"); + } +} + +pub(super) fn cleanup(process: &Process) -> anyhow::Result<()> { + cleanup_at(process, SystemTime::now()) +} + +fn stage_pending_binary(this_exe_path: &Path, bin_path: &Path) -> anyhow::Result { + let pending = tempfile::Builder::new() + .prefix(PENDING_BINARY_PREFIX) + .tempfile_in(bin_path) + .context("failed to reserve a pending rustup binary")? + .into_temp_path(); + + // TempPath reserves a unique name, but preserving a source symlink requires + // an absent destination rather than an existing empty file. + fs::remove_file(&pending).context("failed to prepare the pending rustup path")?; + utils::copy_file_symlink_to_source(this_exe_path, &pending)?; + utils::make_executable(&pending)?; + if !fs::symlink_metadata(&pending)?.file_type().is_symlink() { + OpenOptions::new() + .write(true) + .open(&pending) + .and_then(|file| file.sync_all()) + .context("failed to sync the pending rustup binary")?; + } + + Ok(pending) +} + +fn replace_rustup_binary(replacement: &Path, rustup: &Path) -> anyhow::Result<()> { + // `rename` replaces an existing destination in one step on every platform, + // so a failure here leaves the installed rustup untouched. + fs::rename(replacement, rustup).with_context(|| { + format!( + "failed to replace rustup binary '{}' with '{}'", + rustup.display(), + replacement.display() + ) + })?; + // Make the rename durable. Windows has no directory handle to sync. + #[cfg(unix)] + File::open( + rustup + .parent() + .context("installed rustup binary has no parent directory")?, + ) + .and_then(|directory| directory.sync_all()) + .context("failed to sync rustup binary directory")?; + + Ok(()) +} + +fn mark_stage(process: &Process, stage: &Path, marker: Marker) -> anyhow::Result<()> { + if stage != stage_root(process)? { + warn!( + "ignoring self-update stage outside the managed directory: {}", + stage.display() + ); + return Ok(()); + } + + utils::write_file( + "self-update status marker", + &stage.join(marker.as_str()), + "", + ) +} + +fn cleanup_at(process: &Process, now: SystemTime) -> anyhow::Result<()> { + if let Some(lock) = SelfUpdateLock::try_acquire(process)? { + let updater = lock.updater_path(); + if (is_finished(&lock.directory) || is_stale(&updater, now)) + && remove_file_best_effort("self-updater", &updater) + { + for marker in [Marker::Complete, Marker::Failed] { + remove_file_best_effort( + "self-update status marker", + &lock.directory.join(marker.as_str()), + ); + } + } + } + + let bin = process.cargo_home()?.join("bin"); + match fs::read_dir(&bin) { + Ok(entries) => { + for entry in entries.flatten() { + let path = entry.path(); + if entry + .file_name() + .to_string_lossy() + .starts_with(PENDING_BINARY_PREFIX) + && is_stale(&path, now) + { + remove_file_best_effort("pending rustup binary", &path); + } + } + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + warn!( + "could not inspect pending rustup binaries in {}: {error}", + bin.display() + ); + } + } + + let updater = bin.join(format!("rustup-init{EXE_SUFFIX}")); + // Legacy updaters have no result marker, and an older rustup process may + // still own the shared path. + if is_stale(&updater, now) { + remove_file_best_effort("legacy self-updater", &updater); + } + + Ok(()) +} + +fn remove_file_best_effort(name: &str, path: &Path) -> bool { + match fs::remove_file(path) { + Ok(()) => { + debug!(path = %path.display(), "removed {name}"); + true + } + Err(error) if error.kind() == io::ErrorKind::NotFound => true, + Err(error) + if matches!( + error.kind(), + io::ErrorKind::PermissionDenied | io::ErrorKind::ResourceBusy + ) => + { + debug!(path = %path.display(), "leaving busy {name}"); + false + } + Err(error) => { + warn!("could not remove {name} {}: {error}", path.display()); + false + } + } +} + +fn is_finished(stage: &Path) -> bool { + [Marker::Complete, Marker::Failed] + .iter() + .any(|marker| stage.join(marker.as_str()).is_file()) +} + +fn is_stale(path: &Path, now: SystemTime) -> bool { + fs::metadata(path) + .and_then(|metadata| metadata.modified()) + .ok() + .and_then(|modified| now.duration_since(modified).ok()) + .is_some_and(|age| age >= ABANDONED_UPDATE_AGE) +} + +fn stage_root(process: &Process) -> anyhow::Result { + Ok(process.rustup_home()?.join(SELF_UPDATE_DIRECTORY)) +} + +/// Outcome recorded next to the managed updater once replacement has finished. +#[derive(Clone, Copy)] +enum Marker { + Complete, + Failed, +} + +impl Marker { + fn as_str(&self) -> &'static str { + match self { + Self::Complete => "complete", + Self::Failed => "failed", + } + } +} + +const SELF_UPDATE_DIRECTORY: &str = "self-update"; +const SELF_UPDATE_LOCK_FILE: &str = "self-update.lock"; +const STAGE_ENV: &str = "RUSTUP_SELF_UPDATE_STAGE"; +const PENDING_BINARY_PREFIX: &str = ".rustup-pending-"; +const ABANDONED_UPDATE_AGE: Duration = Duration::from_secs(24 * 60 * 60); + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use super::*; + use crate::{ + process::TestProcess, + test::{Env, test_dir}, + }; + + #[tokio::test] + async fn updater_path_is_stable() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let first = SelfUpdateLock::acquire(&process.process).unwrap(); + let first_path = first.updater_path(); + let stage = first.directory.clone(); + fs::write(&first_path, "").unwrap(); + fs::write(stage.join(Marker::Complete.as_str()), "").unwrap(); + drop(first); + let second = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + + assert_eq!(&first_path, second.updater_path()); + assert!(!stage.join(Marker::Complete.as_str()).exists()); + } + + #[tokio::test] + async fn self_update_lock_is_global() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let lock = SelfUpdateLock::acquire(&process.process).unwrap(); + let contender = OpenOptions::new() + .read(true) + .write(true) + .open( + stage_root(&process.process) + .unwrap() + .join(SELF_UPDATE_LOCK_FILE), + ) + .unwrap(); + + assert!(matches!( + contender.try_lock(), + Err(fs::TryLockError::WouldBlock) + )); + drop(lock); + contender.try_lock().unwrap(); + } + + #[tokio::test] + async fn install_bins_preserves_existing_rustup_if_source_disappears() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let rustup = root.path().join(format!("cargo/bin/rustup{EXE_SUFFIX}")); + fs::create_dir_all(rustup.parent().unwrap()).unwrap(); + fs::write(&rustup, "old rustup").unwrap(); + + SelfUpdateLock::acquire(&process.process) + .unwrap() + .install_bins_from(&root.path().join("missing-updater"), &process.process) + .unwrap_err(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "old rustup"); + } + + #[test] + fn failed_replace_preserves_existing_rustup() { + let root = test_dir().unwrap(); + let rustup = root.path().join(format!("rustup{EXE_SUFFIX}")); + fs::write(&rustup, "old rustup").unwrap(); + + replace_rustup_binary(&root.path().join("missing"), &rustup).unwrap_err(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "old rustup"); + } + + #[test] + fn replace_publishes_pending_rustup() { + let root = test_dir().unwrap(); + let rustup = root.path().join(format!("rustup{EXE_SUFFIX}")); + let pending = root.path().join("pending"); + fs::write(&rustup, "old rustup").unwrap(); + fs::write(&pending, "new rustup").unwrap(); + + replace_rustup_binary(&pending, &rustup).unwrap(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "new rustup"); + assert!(!pending.exists()); + } + + #[tokio::test] + async fn cleanup_keeps_locked_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let lock = SelfUpdateLock::acquire(&process.process).unwrap(); + let updater = lock.updater_path(); + fs::write(&updater, "").unwrap(); + fs::write(lock.directory.join(Marker::Complete.as_str()), "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(updater.exists()); + drop(lock); + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(!updater.exists()); + } + + #[tokio::test] + async fn replacer_command_rejects_parentless_path() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_update = PreparedUpdate { + updater_path: PathBuf::new(), + _lock: SelfUpdateLock::acquire(&process.process).unwrap(), + }; + let error = prepared_update.replacer_command().err().unwrap(); + + assert_eq!( + error.to_string(), + "self-updater path has no parent directory" + ); + } + + #[tokio::test] + async fn cleanup_keeps_fresh_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(updater.exists()); + } + + #[tokio::test] + async fn cleanup_removes_finished_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let stage = stage_root(&process.process).unwrap(); + + for marker in [Marker::Complete, Marker::Failed] { + let prepared_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + mark_stage(&process.process, &stage, marker).unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(!updater.exists()); + assert!(!stage.join(marker.as_str()).exists()); + } + } + + #[tokio::test] + async fn cleanup_removes_abandoned_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + + assert!(!updater.exists()); + } + + #[tokio::test] + async fn cleanup_delays_removing_legacy_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let updater = root + .path() + .join(format!("cargo/bin/rustup-init{EXE_SUFFIX}")); + fs::create_dir_all(updater.parent().unwrap()).unwrap(); + fs::write(&updater, "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(updater.exists()); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + assert!(!updater.exists()); + } + + #[tokio::test] + async fn cleanup_removes_abandoned_pending_binary() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let pending = root + .path() + .join("cargo/bin") + .join(format!("{PENDING_BINARY_PREFIX}orphan")); + fs::create_dir_all(pending.parent().unwrap()).unwrap(); + fs::write(&pending, "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(pending.exists()); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + assert!(!pending.exists()); + } + + fn test_process(root: &Path) -> TestProcess { + let mut vars = HashMap::new(); + vars.env("HOME", root); + vars.env("CARGO_HOME", root.join("cargo")); + vars.env("RUSTUP_HOME", root.join("rustup")); + TestProcess::with_vars(vars) + } +} diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 5da3d78ba0..7ec980765e 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -1,14 +1,11 @@ -use std::{ - path::{Path, PathBuf}, - process::Command, -}; +use std::path::PathBuf; use anyhow::{Context, bail}; use tracing::{error, warn}; use super::{ - install_bins, shell::{self, Posix, UnixShell}, + stage::{self, PreparedUpdate, SelfUpdateLock}, }; use crate::{process::Process, utils}; @@ -121,13 +118,21 @@ pub(crate) fn do_write_env_files(process: &Process) -> anyhow::Result<()> { Ok(()) } -/// Tell the upgrader to replace the rustup bins, then delete -/// itself. -pub(crate) fn run_update(setup_path: &Path, _process: &Process) -> anyhow::Result { - let status = Command::new(setup_path) +pub(super) fn run_update( + prepared_update: PreparedUpdate, + _process: &Process, +) -> anyhow::Result { + let setup_path = prepared_update.updater_path().to_owned(); + let mut updater = prepared_update + .replacer_command()? .arg("--self-replace") - .status() + .spawn() .context(format!("unable to run updater ({})", setup_path.display()))?; + drop(prepared_update); + let status = updater.wait().context(format!( + "unable to wait for updater ({})", + setup_path.display() + ))?; if !status.success() { bail!("self-updated failed to replace rustup executable"); @@ -140,7 +145,12 @@ pub(crate) fn run_update(setup_path: &Path, _process: &Process) -> anyhow::Resul /// `$CARGO_HOME/bin/rustup` with the running exe, and updates the /// links to it. pub(crate) fn self_replace(process: &Process) -> anyhow::Result { - install_bins(process)?; + let self_update_lock = SelfUpdateLock::acquire(process)?; + #[cfg(feature = "test")] + process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); + let result = self_update_lock.install_bins(process); + stage::mark_result(process, result.is_ok()); + result?; Ok(utils::ExitCode(0)) } diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 28fcdcfdac..0311760a31 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -3,9 +3,8 @@ use std::{ env::{consts::EXE_SUFFIX, split_paths}, ffi::{OsStr, OsString}, fmt, - io::Write, + io::{self, Write}, os::windows::ffi::OsStrExt, - path::Path, process::Command, }; @@ -18,13 +17,12 @@ 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 super::{ + InstallOpts, report_error, + stage::{self, PreparedUpdate, SelfUpdateLock}, +}; use crate::{ - cli::{ - common, - errors::CliError, - markdown::md, - self_update::{InstallOpts, install_bins, report_error}, - }, + cli::{common, errors::CliError, markdown::md}, dist::TargetTuple, download::DownloadOptions, process::{ColorableTerminal, Process}, @@ -41,7 +39,7 @@ pub(crate) fn ensure_prompt(process: &Process) -> anyhow::Result<()> { fn choice(max: u8, process: &Process) -> anyhow::Result> { 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) { @@ -658,24 +656,28 @@ pub(crate) fn remove_uninstall_registry_entry(process: &Process) -> anyhow::Resu } } -pub(crate) fn run_update(setup_path: &Path, process: &Process) -> anyhow::Result { - Command::new(setup_path) +pub(super) fn run_update( + prepared_update: PreparedUpdate, + _process: &Process, +) -> anyhow::Result { + prepared_update + .replacer_command()? .arg("--self-replace") .spawn() .context("unable to run updater")?; - - let Some(version) = super::get_and_parse_new_rustup_version(setup_path) else { - warn!("failed to get the new rustup version in order to update `DisplayVersion`"); - return Ok(utils::ExitCode(1)); - }; - update_uninstall_registry_display_version(&version, process)?; + drop(prepared_update); Ok(utils::ExitCode(0)) } pub(crate) fn self_replace(process: &Process) -> anyhow::Result { wait_for_parent()?; - install_bins(process)?; + let self_update_lock = SelfUpdateLock::acquire(process)?; + let result = self_update_lock.install_bins(process).and_then(|()| { + update_uninstall_registry_display_version(env!("CARGO_PKG_VERSION"), process) + }); + stage::mark_result(process, result.is_ok()); + result?; Ok(utils::ExitCode(0)) } diff --git a/src/process.rs b/src/process.rs index 177b8a790b..53508789a5 100644 --- a/src/process.rs +++ b/src/process.rs @@ -262,7 +262,7 @@ impl Process { /// Registers a testing checkpoint with the given name and parks the current thread. /// - /// Usually, the current process will be killed by the test driver. + /// The test driver can either remove the marker to resume or kill the process. #[cfg(feature = "test")] pub(crate) fn checkpoint(&self, name: &str) { if self.var(CHECKPOINT_ENV).as_deref() != Ok(name) { @@ -275,13 +275,16 @@ impl Process { let test_root = rustup_home .parent() .expect("test RUSTUP_HOME must be inside the test root"); - fs::write(checkpoint_path(test_root, name), name) - .expect("failed to write test checkpoint marker"); + let marker = checkpoint_path(test_root, name); + fs::write(&marker, name).expect("failed to write test checkpoint marker"); let start_time = Instant::now(); let max_wait = Duration::from_mins(5); while start_time.elapsed() < max_wait { - thread::sleep(Duration::from_secs(10)); + if !marker.exists() { + return; + } + thread::sleep(Duration::from_millis(10)); } panic!( "test checkpoint '{name}' timed out after {max_wait:?} without being killed by the test driver", diff --git a/src/test/clitools.rs b/src/test/clitools.rs index d2dad716b7..190b4b7a1d 100644 --- a/src/test/clitools.rs +++ b/src/test/clitools.rs @@ -1031,6 +1031,7 @@ impl CliTestContext { cmd.spawn() .expect("failed to start command for checkpoint test") }), + marker: marker.clone(), } }; @@ -1138,6 +1139,7 @@ impl Drop for WorkDirGuard<'_> { #[must_use] pub struct ParkedChild { child: Option, + marker: PathBuf, } impl ParkedChild { @@ -1147,9 +1149,21 @@ impl ParkedChild { child .kill() .expect("failed to terminate command at checkpoint"); - child + let status = child + .wait() + .expect("failed to reap command after checkpoint"); + remove_checkpoint_marker(&self.marker); + status + } + + /// Resume the parked command and wait for it to finish. + pub fn resume(mut self) -> ExitStatus { + remove_checkpoint_marker(&self.marker); + self.child + .take() + .unwrap() .wait() - .expect("failed to reap command after checkpoint") + .expect("failed to reap resumed checkpoint command") } } @@ -1160,6 +1174,15 @@ impl Drop for ParkedChild { }; let _ = child.kill(); let _ = child.wait(); + remove_checkpoint_marker(&self.marker); + } +} + +fn remove_checkpoint_marker(marker: &Path) { + if let Err(error) = fs::remove_file(marker) + && error.kind() != io::ErrorKind::NotFound + { + panic!("failed to remove checkpoint marker: {error}"); } } diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 83ca66b671..89eae6f39a 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -1,17 +1,24 @@ //! Testing self install, uninstall and update -use std::{env, env::consts::EXE_SUFFIX, fs, path::Path, process::Command}; +use std::{ + env::consts::EXE_SUFFIX, + fs, + path::{Path, PathBuf}, + process::Command, +}; use remove_dir_all::remove_dir_all; -#[cfg(windows)] 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, test::{ CROSS_ARCH1, CliTestContext, Scenario, SelfUpdateTestContext, calc_hash, output_release_file, this_host_tuple, @@ -21,8 +28,6 @@ use rustup::{ #[cfg(windows)] use windows_registry::{CURRENT_USER, Value}; -const TEST_VERSION: &str = "1.1.1"; - /// Empty dist server, rustup installed with no toolchain async fn setup_empty_installed() -> CliTestContext { let cx = CliTestContext::new(Scenario::Empty).await; @@ -500,6 +505,7 @@ async fn update_overwrites_programs_display_version() { ) .unwrap(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); assert_eq!( USER_RUSTUP_VERSION .get(test_id, CURRENT_USER) @@ -509,12 +515,6 @@ async fn update_overwrites_programs_display_version() { ); } -#[cfg(windows)] -const USER_RUSTUP_VERSION: RegistryValueId = RegistryValueId { - sub_key: r"Software\Microsoft\Windows\CurrentVersion\Uninstall\Rustup", - value_name: "DisplayVersion", -}; - #[tokio::test] async fn update_but_not_installed() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; @@ -531,9 +531,8 @@ error: rustup is not installed at '[CARGO_DIR]' } #[tokio::test] -async fn update_but_delete_existing_updater_first() { +async fn update_does_not_reuse_legacy_updater_path() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; - // The updater is stored in a known location let setup = cx .config .cargodir @@ -544,8 +543,6 @@ async fn update_but_delete_existing_updater_first() { .await .is_ok(); - // If it happens to already exist for some reason it - // should just be deleted. raw::write_file(&setup, "").unwrap(); cx.config .expect(&["rustup", "self", "update"]) @@ -554,6 +551,58 @@ async fn update_but_delete_existing_updater_first() { let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); assert!(rustup.exists()); + assert!(setup.exists()); + assert!(managed_updater(&cx.config.rustupdir.rustupdir).exists()); +} + +#[tokio::test] +async fn managed_updater_survives_concurrent_proxy_cleanup() { + let mut cx = CliTestContext::new(Scenario::SimpleV2).await; + let _update_server = cx.with_update_server(TEST_VERSION); + cx.config + .expect(["rustup-init", "-y", "--no-modify-path"]) + .await + .is_ok(); + + let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); + let before_hash = calc_hash(&rustup); + let parked = cx.spawn_at( + CHECKPOINT_SELF_UPDATE_PREPARED, + ["rustup", "self", "update"], + ); + let updater = managed_updater(&cx.config.rustupdir.rustupdir); + + cx.config.expect(["rustc", "--version"]).await.is_ok(); + + assert!(updater.exists()); + assert!(parked.resume().success()); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); + assert_ne!(before_hash, calc_hash(&rustup)); +} + +#[cfg(unix)] +#[tokio::test] +async fn self_update_replacement_survives_proxy_cleanup() { + let mut cx = CliTestContext::new(Scenario::SimpleV2).await; + let _update_server = cx.with_update_server(TEST_VERSION); + cx.config + .expect(["rustup-init", "-y", "--no-modify-path"]) + .await + .is_ok(); + + let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); + let before_hash = calc_hash(&rustup); + let parked = cx.spawn_at(CHECKPOINT_SELF_REPLACE_READY, ["rustup", "self", "update"]); + + cx.config.expect(["rustc", "--version"]).await.is_ok(); + + let status = parked.resume(); + assert!( + rustup.exists(), + "concurrent proxy removed the installed rustup during self-update ({status})" + ); + assert!(status.success(), "self-update failed: {status}"); + assert_ne!(before_hash, calc_hash(&rustup)); } #[tokio::test] @@ -790,11 +839,7 @@ async fn updater_leaves_itself_for_later_deletion() { .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(setup.exists()); + assert!(managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -809,17 +854,14 @@ async fn updater_is_deleted_after_running_rustup() { .await .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); cx.config .expect(["rustup", "update", "nightly"]) .await .is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(!setup.exists()); + assert!(!managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -834,14 +876,11 @@ async fn updater_is_deleted_after_running_rustc() { .await .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); cx.config.expect(["rustc", "--version"]).await.is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(!setup.exists()); + assert!(!managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -1225,3 +1264,30 @@ async fn install_minimal_profile() { cx.config.expect_component_executable("rustc").await; cx.config.expect_component_not_executable("cargo").await; } + +fn wait_for_completed_update(rustup_home: &Path) { + let stage = rustup_home.join("self-update"); + retry(Fibonacci::from_millis(1).map(jitter).take(23), || { + if stage.join("complete").is_file() { + Ok(()) + } else if stage.join("failed").is_file() { + Err("self-update failed") + } else { + Err("self-update has not completed") + } + }) + .unwrap(); +} + +fn managed_updater(rustup_home: &Path) -> PathBuf { + rustup_home + .join("self-update") + .join(format!("rustup-init{EXE_SUFFIX}")) +} + +const TEST_VERSION: &str = "1.1.1"; +#[cfg(windows)] +const USER_RUSTUP_VERSION: RegistryValueId = RegistryValueId { + sub_key: r"Software\Microsoft\Windows\CurrentVersion\Uninstall\Rustup", + value_name: "DisplayVersion", +};