From e69fb234d91e097b40c46d7fbc0f6f46d182c71c Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 19 Nov 2025 15:03:06 +0100 Subject: [PATCH 1/3] Lock download file to avoid races See #TBD for details. This is one of two possible implementations, the easier one is using a temporary directory or a unique filename to ensure processes don't collide. Using locks has less platform support, but OTOH means that we don't create stray temp files that don't get cleaned up (because we never know if one of those files is actually used by another process). This does not fix #TBD completely. It only fixed the downloaded, parallel `rustup-init` calls still fail at the transaction stage. This PR introduces a `LockedFile` primitive that's modeled after the uv type of the same name. It's currently very simple and locks forever. We can extend it with a timeout that makes rustup exit after a certain duration and print an error message, instead of an inscrutable waiting without any message. --- src/dist/download.rs | 23 +++++++---- src/download/mod.rs | 98 ++++++++++++++++++++++++++++++++++++++++++-- src/errors.rs | 4 ++ 3 files changed, 112 insertions(+), 13 deletions(-) diff --git a/src/dist/download.rs b/src/dist/download.rs index 6342d4cdd3..70050cbc1d 100644 --- a/src/dist/download.rs +++ b/src/dist/download.rs @@ -15,7 +15,7 @@ use url::Url; use crate::config::Cfg; use crate::dist::manifest::Manifest; use crate::dist::{Channel, DEFAULT_DIST_SERVER, ToolchainDesc, temp}; -use crate::download::{download_file, download_file_with_resume}; +use crate::download::{LockedFile, download_file, download_file_with_resume}; use crate::errors::RustupError; use crate::process::Process; use crate::utils; @@ -55,6 +55,18 @@ impl<'a> DownloadCfg<'a> { utils::ensure_dir_exists("Download Directory", self.download_dir)?; let target_file = self.download_dir.join(Path::new(hash)); + let file_name = target_file + .file_name() + .map(|s| s.to_str().unwrap_or("_")) + .unwrap_or("_") + .to_owned(); + let locked_file = target_file.with_file_name(file_name.clone() + ".lock"); + + let _locked_file = LockedFile::create(&locked_file) + .with_context(|| RustupError::CreateLockedFile(locked_file.clone()))? + .lock() + .with_context(|| RustupError::LockedFile(locked_file.clone()))?; + if target_file.exists() { let cached_result = file_hash(&target_file)?; if hash == cached_result { @@ -67,14 +79,7 @@ impl<'a> DownloadCfg<'a> { } } - let partial_file_path = target_file.with_file_name( - target_file - .file_name() - .map(|s| s.to_str().unwrap_or("_")) - .unwrap_or("_") - .to_owned() - + ".partial", - ); + let partial_file_path = target_file.with_file_name(file_name + ".partial"); let partial_file_existed = partial_file_path.exists(); diff --git a/src/download/mod.rs b/src/download/mod.rs index b32524a596..62c3969423 100644 --- a/src/download/mod.rs +++ b/src/download/mod.rs @@ -1,8 +1,9 @@ //! Easy file downloading -use std::fs::remove_file; +use std::fs::{File, OpenOptions, remove_file}; +use std::io; use std::num::NonZero; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str::FromStr; use std::time::Duration; @@ -26,6 +27,95 @@ use crate::{dist::download::DownloadStatus, errors::RustupError, process::Proces #[cfg(test)] mod tests; +/// An OS lock on a file that unlocks when dropping the file. +pub(crate) struct LockedFile { + path: PathBuf, + file: File, +} + +impl LockedFile { + /// Creates the file if it does not exit, does not lock. + #[cfg(unix)] + pub(crate) fn create(path: impl Into) -> Result { + use std::os::unix::fs::PermissionsExt; + use tempfile::NamedTempFile; + + let path = path.into(); + + // If path already exists, return it. + if let Ok(file) = OpenOptions::new().read(true).write(true).open(&path) { + return Ok(Self { path, file }); + } + + // Otherwise, create a temporary file with 777 permissions, to handle races between + // processes running under different UIDs (e.g., in Docker containers). We must set + // permissions _after_ creating the file, to override the `umask`. + let file = if let Some(parent) = path.parent() { + NamedTempFile::new_in(parent)? + } else { + NamedTempFile::new()? + }; + if let Err(err) = file + .as_file() + .set_permissions(std::fs::Permissions::from_mode(0o777)) + { + warn!("failed to set permissions on temporary file: {err}"); + } + + // Try to move the file to path, but if path exists now, just open path + match file.persist_noclobber(&path) { + Ok(file) => Ok(Self { path, file }), + Err(err) => { + if err.error.kind() == io::ErrorKind::AlreadyExists { + let file = OpenOptions::new().read(true).write(true).open(&path)?; + Ok(Self { path, file }) + } else { + Err(err.error) + } + } + } + } + + /// Creates the file if it does not exit, does not lock. + #[cfg(not(unix))] + pub(crate) fn create(path: impl Into) -> Result { + let path = path.into(); + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(&path)?; + Ok(Self { path, file }) + } + + /// Acquire an exclusive lock on a file, blocking until the file is ready. + pub(crate) fn lock(self) -> Result { + match self.file.lock() { + Err(err) if err.kind() == io::ErrorKind::Unsupported => { + warn!("locking files is not supported, running rustup in parallel may error"); + Ok(self) + } + Err(err) => Err(err), + Ok(()) => Ok(self), + } + } +} + +impl Drop for LockedFile { + /// Unlock the file. + fn drop(&mut self) { + if let Err(err) = self.file.unlock() { + warn!( + "failed to unlock {}; program may be stuck: {}", + self.path.display(), + err + ); + } else { + debug!("released lock at `{}`", self.path.display()); + } + } +} + pub(crate) async fn download_file( url: &Url, path: &Path, @@ -48,7 +138,7 @@ pub(crate) async fn download_file_with_resume( match download_file_(url, path, hasher, resume_from_partial, status, process).await { Ok(_) => Ok(()), Err(e) => { - if e.downcast_ref::().is_some() { + if e.downcast_ref::().is_some() { return Err(e); } let is_client_error = match e.downcast_ref::() { @@ -722,7 +812,7 @@ enum DownloadError { #[error("{0}")] Message(String), #[error(transparent)] - IoError(#[from] std::io::Error), + IoError(#[from] io::Error), #[cfg(any(feature = "reqwest-rustls-tls", feature = "reqwest-native-tls"))] #[error(transparent)] Reqwest(#[from] ::reqwest::Error), diff --git a/src/errors.rs b/src/errors.rs index 8fb12b6913..61bfccd850 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -27,6 +27,10 @@ pub struct OperationError(pub anyhow::Error); pub enum RustupError { #[error("partially downloaded file may have been damaged and was removed, please try again")] BrokenPartialFile, + #[error("failed to create '{0}' as lockfile")] + CreateLockedFile(PathBuf), + #[error("failed to lock '{0}'")] + LockedFile(PathBuf), #[error("component download failed for {0}")] ComponentDownloadFailed(String), #[error("failure removing component '{name}', directory does not exist: '{}'", .path.display())] From 76e69294d753f8fea2b84bc0f98a1520feabfe64 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 19 Nov 2025 20:44:35 +0100 Subject: [PATCH 2/3] Windows and clippy --- src/download/mod.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/download/mod.rs b/src/download/mod.rs index 62c3969423..12dfc11578 100644 --- a/src/download/mod.rs +++ b/src/download/mod.rs @@ -78,12 +78,13 @@ impl LockedFile { /// Creates the file if it does not exit, does not lock. #[cfg(not(unix))] - pub(crate) fn create(path: impl Into) -> Result { + pub(crate) fn create(path: impl Into) -> Result { let path = path.into(); let file = OpenOptions::new() .read(true) .write(true) .create(true) + .truncate(true) .open(&path)?; Ok(Self { path, file }) } @@ -318,9 +319,9 @@ const CURL_USER_AGENT: &str = concat!("rustup/", env!("CARGO_PKG_VERSION"), " (c #[cfg(feature = "reqwest-native-tls")] const REQWEST_DEFAULT_TLS_USER_AGENT: &str = concat!( - "rustup/", - env!("CARGO_PKG_VERSION"), - " (reqwest; default-tls)" +"rustup/", +env!("CARGO_PKG_VERSION"), +" (reqwest; default-tls)" ); #[cfg(feature = "reqwest-rustls-tls")] @@ -438,7 +439,7 @@ impl Backend { None => Ok(()), } }) - .await?; + .await?; file.borrow_mut() .sync_data() From a5b0a8205dad45b56fc73a2dc01ee47e7a1887a7 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 19 Nov 2025 20:45:25 +0100 Subject: [PATCH 3/3] Rustfmt Uninstalling rust also makes `cargo fmt` go away --- src/download/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/download/mod.rs b/src/download/mod.rs index 12dfc11578..602b01ae4f 100644 --- a/src/download/mod.rs +++ b/src/download/mod.rs @@ -319,9 +319,9 @@ const CURL_USER_AGENT: &str = concat!("rustup/", env!("CARGO_PKG_VERSION"), " (c #[cfg(feature = "reqwest-native-tls")] const REQWEST_DEFAULT_TLS_USER_AGENT: &str = concat!( -"rustup/", -env!("CARGO_PKG_VERSION"), -" (reqwest; default-tls)" + "rustup/", + env!("CARGO_PKG_VERSION"), + " (reqwest; default-tls)" ); #[cfg(feature = "reqwest-rustls-tls")] @@ -439,7 +439,7 @@ impl Backend { None => Ok(()), } }) - .await?; + .await?; file.borrow_mut() .sync_data()