feat(self-update): use DownloadCfg for self-updates

src/cli/self_update.rs

@@ -63,7 +63,7 @@ use crate::{
         ChannelToolchainName, DistOptions, PartialChannelToolchainName, Profile, TargetTuple,
         download::DownloadCfg,
     },
-    download::DownloadOptions,
+    download::{DownloadError, DownloadOptions},
     errors::RustupError,
     install::{InstallMethod, UpdateStatus},
     process::Process,
@@ -1258,17 +1258,44 @@ async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result<Option<Prepa
     // Get download path
     let download_url = utils::parse_url(&url)?;
     let prepared_updater = PreparedUpdater::try_from(self_update_lock)?;
-    let setup_path: &Path = &prepared_updater;
 
-    // Download new version
+    // Download the new rustup executable
     info!("downloading self-update (new version: {available_version})");
-    DownloadOptions::try_from(dl_cfg.process)?
-        .start(&download_url, setup_path)
-        .download()
-        .await?;
+    match dl_cfg.download_hash(download_url.as_str()).await {
+        Ok(hash) => {
+            let name = "rustup";
+            let status = dl_cfg.status_for(name, name.len());
+
+            let installer = dl_cfg
+                .download_with_retry(&download_url, &hash, &status, dl_cfg.max_retries())
+                .await?;
+
+            utils::rename(
+                name,
+                &installer,
+                &prepared_updater,
+                dl_cfg.permit_copy_rename,
+            )?;
+        }
+        // If the corresponding `.sha256` file is not found, skip hash check and use simple download
+        // as a fallback.
+        Err(e)
+            if matches!(
+                e.downcast_ref::<DownloadError>(),
+                Some(DownloadError::HttpStatus(404) | DownloadError::FileNotFound)
+            ) =>
+        {
+            warn!(url = %download_url, "`.sha256` checksum not found, skipping hash check");
+            DownloadOptions::try_from(dl_cfg.process)?
+                .start(&download_url, &prepared_updater)
+                .download()
+                .await?;
+        }
+        Err(e) => return Err(e),
+    };
 
     // Mark as executable
-    utils::make_executable(setup_path)?;
+    utils::make_executable(&prepared_updater)?;
 
     #[cfg(feature = "test")]
     dl_cfg.process.checkpoint(CHECKPOINT_SELF_UPDATE_PREPARED);

src/dist/download.rs

@@ -33,7 +33,7 @@ pub struct DownloadCfg<'a> {
     pub tmp_cx: Arc<temp::Context>,
     pub download_dir: &'a PathBuf,
     pub(super) tracker: DownloadTracker,
-    pub(super) permit_copy_rename: bool,
+    pub permit_copy_rename: bool,
     pub process: &'a Process,
 }
 
@@ -52,6 +52,45 @@ impl<'a> DownloadCfg<'a> {
         }
     }
 
+    /// Same as [`DownloadCfg::download()`], but also retries the download if it fails.
+    ///
+    /// Only certain errors are considered retriable, and the download will be retried up to
+    /// `retries` times. The error from the latest retry is returned if all retries are exhausted.
+    pub(crate) async fn download_with_retry(
+        &self,
+        url: &Url,
+        hash: &str,
+        status: &DownloadStatus,
+        mut retries: usize,
+    ) -> anyhow::Result<File> {
+        loop {
+            let downloaded = self.partial_len(hash);
+            let future = self.download(url, hash, status);
+            let err = match future.await {
+                Err(err) => err,
+                Ok(file) => break Ok(file),
+            };
+
+            // retry only known retriable cases
+            match err.downcast_ref::<RustupError>() {
+                Some(RustupError::BrokenPartialFile)
+                | Some(RustupError::IncompletePartialFile)
+                | Some(RustupError::DownloadingFile { .. }) => {}
+                _ => return Err(err),
+            }
+
+            let progress = self.partial_len(hash).saturating_sub(downloaded);
+            if progress < Self::MIN_PROGRESS {
+                if retries == 0 {
+                    return Err(err);
+                }
+                retries -= 1;
+            }
+
+            status.retrying();
+        }
+    }
+
     /// Downloads a file and validates its hash. Resumes interrupted downloads.
     /// Partial downloads are stored in `self.download_dir`, keyed by hash. If the
     /// target file already exists, then the hash is checked and it is returned
@@ -145,14 +184,28 @@ impl<'a> DownloadCfg<'a> {
         Ok(())
     }
 
-    async fn download_hash(&self, url: &str) -> anyhow::Result<String> {
+    pub(crate) async fn download_hash(&self, url: &str) -> anyhow::Result<String> {
         let hash_url = utils::parse_url(&(url.to_owned() + ".sha256"))?;
         let hash_file = self.tmp_cx.new_file()?;
         DownloadOptions::try_from(self.process)?
             .start(&hash_url, &hash_file)
             .download()
             .await?;
-        utils::read_file("hash", &hash_file).map(|s| s[0..64].to_owned())
+
+        let mut contents = utils::read_file("hash", &hash_file)?;
+        // NOTE: According to <https://www.gnu.org/software/coreutils/manual/html_node/md5sum-invocation.html>,
+        // the first column of a `.sha256` file must be a SHA256 sum in hex characters.
+        let hash_len = 64;
+        if let Some(hash) = contents.split_whitespace().next()
+            && hash.len() == hash_len
+            && hash.chars().all(|c| c.is_ascii_hexdigit())
+        {
+            contents.truncate(hash_len);
+            return Ok(contents);
+        }
+        Err(anyhow!(
+            "illegal hash file found at `{hash_url}`, expected a checksum in {hash_len} hex characters"
+        ))
     }
 
     pub(crate) async fn dl_v2_manifest(
@@ -323,6 +376,15 @@ impl<'a> DownloadCfg<'a> {
         }
     }
 
+    pub(crate) fn max_retries(&self) -> usize {
+        const DEFAULT_MAX_RETRIES: usize = 3;
+
+        match self.process.var("RUSTUP_MAX_RETRIES") {
+            Ok(s) if let Ok(n) = s.parse() => n,
+            Ok(_) | Err(_) => DEFAULT_MAX_RETRIES,
+        }
+    }
+
     pub(crate) fn url(&self, url: &str) -> anyhow::Result<Url> {
         match &*self.tmp_cx.dist_server {
             server if server != DEFAULT_DIST_SERVER => utils::parse_url(
@@ -331,6 +393,8 @@ impl<'a> DownloadCfg<'a> {
             _ => utils::parse_url(url),
         }
     }
+
+    const MIN_PROGRESS: u64 = 1024 * 1024;
 }
 
 /// Tracks download progress and displays information about it to a terminal.

src/dist/manifestation.rs

@@ -196,14 +196,6 @@ impl Manifestation {
             .concurrent_downloads()
             .unwrap_or(DEFAULT_CONCURRENT_DOWNLOADS);
 
-        const DEFAULT_MAX_RETRIES: usize = 3;
-        let max_retries: usize = download_cfg
-            .process
-            .var("RUSTUP_MAX_RETRIES")
-            .ok()
-            .and_then(|s| s.parse().ok())
-            .unwrap_or(DEFAULT_MAX_RETRIES);
-
         // Begin transaction
         let mut tx = Transaction::new(
             prefix.clone(),
@@ -268,7 +260,9 @@ impl Manifestation {
                 // Must live outside of `InstallEvents` because we can't write the type of future
                 while stream.components.len() > 0 && stream.downloads.len() < concurrent_downloads {
                     if let Some(bin) = stream.components.next() {
-                        stream.downloads.push(bin.download(max_retries));
+                        stream
+                            .downloads
+                            .push(bin.download(download_cfg.max_retries()));
                     }
                 }
 
@@ -770,45 +764,15 @@ impl<'a> ComponentBinary<'a> {
         }))
     }
 
-    async fn download(self, mut retries: usize) -> anyhow::Result<(ComponentInstall, &'a str)> {
+    async fn download(self, retries: usize) -> anyhow::Result<(ComponentInstall, &'a str)> {
         let url = self.download_cfg.url(&self.binary.url)?;
-        let installer = loop {
-            let downloaded = self.download_cfg.partial_len(&self.binary.hash);
-            let future = self
-                .download_cfg
-                .download(&url, &self.binary.hash, &self.status);
-            let err = match future.await {
-                Ok(file) => break file,
-                Err(err) => err,
-            };
-
-            // retry only known retriable cases
-            match err.downcast_ref::<RustupError>() {
-                Some(RustupError::BrokenPartialFile)
-                | Some(RustupError::IncompletePartialFile)
-                | Some(RustupError::DownloadingFile { .. }) => {}
-                _ => {
-                    return Err(err).with_context(|| {
-                        RustupError::ComponentDownloadFailed(self.manifest.name(&self.component))
-                    });
-                }
-            }
-
-            let progress = self
-                .download_cfg
-                .partial_len(&self.binary.hash)
-                .saturating_sub(downloaded);
-            if progress < Self::MIN_PROGRESS {
-                if retries == 0 {
-                    return Err(err).with_context(|| {
-                        RustupError::ComponentDownloadFailed(self.manifest.name(&self.component))
-                    });
-                }
-                retries -= 1;
-            }
-
-            self.status.retrying();
-        };
+        let installer = self
+            .download_cfg
+            .download_with_retry(&url, &self.binary.hash, &self.status, retries)
+            .await
+            .with_context(|| {
+                RustupError::ComponentDownloadFailed(self.manifest.name(&self.component))
+            })?;
 
         let install = ComponentInstall {
             status: self.status,
@@ -825,8 +789,6 @@ impl<'a> ComponentBinary<'a> {
 
         Ok((install, &self.binary.hash))
     }
-
-    const MIN_PROGRESS: u64 = 1024 * 1024;
 }
 
 struct ComponentInstall {

src/download/mod.rs

@@ -439,7 +439,7 @@ fn native_tls_client(timeout: Duration) -> Result<&'static Client, DownloadError
 static CLIENT_NATIVE_TLS: OnceLock<Client> = OnceLock::new();
 
 #[derive(Debug, Error)]
-enum DownloadError {
+pub enum DownloadError {
     #[error("http request returned an unsuccessful status code: {0}")]
     HttpStatus(u32),
     #[error("file not found")]

src/test/clitools.rs

@@ -5,7 +5,7 @@ use std::{
     cell::RefCell,
     collections::HashMap,
     env::{self, consts::EXE_SUFFIX},
-    ffi::OsStr,
+    ffi::{OsStr, OsString},
     fmt::Debug,
     fs,
     io::{self, Write},
@@ -26,7 +26,7 @@ use tempfile::TempDir;
 use url::Url;
 
 use super::{
-    CHECKPOINT_ENV, CROSS_ARCH1, CROSS_ARCH2, MULTI_ARCH1, checkpoint_path,
+    CHECKPOINT_ENV, CROSS_ARCH1, CROSS_ARCH2, MULTI_ARCH1, calc_hash, checkpoint_path,
     dist::{MockDistServer, MockManifestVersion, Release, RlsStatus, change_channel_date},
     mock::MockFile,
 };
@@ -59,7 +59,7 @@ pub struct Config {
     /// ~
     pub homedir: PathBuf,
     /// Root for updates to rustup itself aka RUSTUP_UPDATE_ROOT
-    rustup_update_root: Option<String>,
+    pub rustup_update_root: Option<String>,
     /// This is cwd for the test
     pub workdir: RefCell<PathBuf>,
     /// This is the test root for keeping stuff together
@@ -952,9 +952,10 @@ impl SelfUpdateTestContext {
         let mut dest_file = fs::OpenOptions::new()
             .append(true)
             .create(true)
-            .open(dist_exe)
+            .open(&dist_exe)
             .unwrap();
         writeln!(dest_file).unwrap();
+        gen_hash_file(&dist_exe).unwrap();
 
         Self {
             config: cx.config,
@@ -1048,7 +1049,7 @@ impl CliTestContext {
             }
         };
 
-        let deadline = Instant::now() + StdDuration::from_secs(10);
+        let deadline = Instant::now() + StdDuration::from_secs(30);
         loop {
             if matches!(fs::read_to_string(&marker), Ok(contents) if contents == checkpoint) {
                 break parked;
@@ -1098,9 +1099,10 @@ impl CliTestContext {
         let mut dest_file = fs::OpenOptions::new()
             .append(true)
             .create(true)
-            .open(dist_exe)
+            .open(&dist_exe)
             .unwrap();
         writeln!(dest_file).unwrap();
+        gen_hash_file(&dist_exe).unwrap();
 
         self.config.rustup_update_root = Some(root_url);
         UpdateServerGuard {
@@ -1224,6 +1226,19 @@ impl Drop for DistDirGuard<'_> {
     }
 }
 
+fn gen_hash_file(src: &Path) -> io::Result<()> {
+    let hash = calc_hash(src);
+    let src_name = src
+        .file_name()
+        .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "input file has no name"))?
+        .display();
+    let contents = format!("{hash}  {src_name}\n");
+
+    let mut dst = OsString::from(src);
+    dst.push(".sha256");
+    fs::write(Path::new(&dst), contents)
+}
+
 fn create_local_update_server(self_dist: &Path, exedir: &Path, version: &str) -> String {
     let trip = this_host_tuple();
     let dist_dir = self_dist.join(format!("archive/{version}/{trip}"));