diff --git a/src/config.rs b/src/config.rs index bcfa565e88..e92f43084f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -641,11 +641,15 @@ impl Cfg { }; if let Ok(contents) = contents { - let add_file_context = || format!("in {}", toolchain_file.to_string_lossy()); // XXX Should not return the unvalidated contents; but a new // internal only safe struct - let override_file = Cfg::parse_override_file(contents, parse_mode) - .with_context(add_file_context)?; + let override_file = + Cfg::parse_override_file(contents, parse_mode).with_context(|| { + RustupError::ParsingFile { + name: "override", + path: toolchain_file.clone(), + } + })?; if let Some(toolchain_name_str) = &override_file.toolchain.channel { let toolchain_name = ResolvableToolchainName::try_from(toolchain_name_str)?; let default_host_triple = get_default_host_triple(settings); diff --git a/src/dist/config.rs b/src/dist/config.rs index 2febfb48c5..492426493c 100644 --- a/src/dist/config.rs +++ b/src/dist/config.rs @@ -43,7 +43,7 @@ impl Config { } pub(crate) fn parse(data: &str) -> Result { - let value = toml::from_str(data).context("error parsing manifest")?; + let value = toml::from_str(data).context("error parsing config")?; Self::from_toml(value, "") } diff --git a/src/dist/dist.rs b/src/dist/dist.rs index 3980ebb9b0..ce5fb874e7 100644 --- a/src/dist/dist.rs +++ b/src/dist/dist.rs @@ -1096,7 +1096,11 @@ pub(crate) async fn dl_v2_manifest( return Ok(None); }; let manifest_str = utils::read_file("manifest", &manifest_file)?; - let manifest = ManifestV2::parse(&manifest_str)?; + let manifest = + ManifestV2::parse(&manifest_str).with_context(|| RustupError::ParsingFile { + name: "manifest", + path: manifest_file.to_path_buf(), + })?; Ok(Some((manifest, manifest_hash))) } diff --git a/src/dist/manifestation.rs b/src/dist/manifestation.rs index 7568e34d38..02e5b0f872 100644 --- a/src/dist/manifestation.rs +++ b/src/dist/manifestation.rs @@ -313,8 +313,12 @@ impl Manifestation { // Read configuration and delete it let rel_config_path = prefix.rel_manifest_file(CONFIG_FILE); - let config_str = utils::read_file("dist config", &prefix.path().join(&rel_config_path))?; - let config = Config::parse(&config_str)?; + let abs_config_path = prefix.path().join(&rel_config_path); + let config_str = utils::read_file("dist config", &abs_config_path)?; + let config = Config::parse(&config_str).with_context(|| RustupError::ParsingFile { + name: "config", + path: abs_config_path, + })?; tx.remove_file("dist config", rel_config_path)?; for component in config.components { @@ -359,7 +363,12 @@ impl Manifestation { let config_path = prefix.path().join(rel_config_path); if utils::path_exists(&config_path) { let config_str = utils::read_file("dist config", &config_path)?; - Ok(Some(Config::parse(&config_str)?)) + Ok(Some(Config::parse(&config_str).with_context(|| { + RustupError::ParsingFile { + name: "Config", + path: config_path, + } + })?)) } else { Ok(None) } @@ -371,7 +380,12 @@ impl Manifestation { let old_manifest_path = prefix.manifest_file(DIST_MANIFEST); if utils::path_exists(&old_manifest_path) { let manifest_str = utils::read_file("installed manifest", &old_manifest_path)?; - Ok(Some(Manifest::parse(&manifest_str)?)) + Ok(Some(Manifest::parse(&manifest_str).with_context(|| { + RustupError::ParsingFile { + name: "manifest", + path: old_manifest_path, + } + })?)) } else { Ok(None) } diff --git a/src/errors.rs b/src/errors.rs index a320ee4d5f..dafc05b857 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -71,6 +71,8 @@ pub(crate) enum RustupError { ReadingDirectory { name: &'static str, path: PathBuf }, #[error("could not read {name} file: '{}'", .path.display())] ReadingFile { name: &'static str, path: PathBuf }, + #[error("could not parse {name} file: '{}'", .path.display())] + ParsingFile { name: &'static str, path: PathBuf }, #[error("could not remove '{}' directory: '{}'", .name, .path.display())] RemovingDirectory { name: &'static str, path: PathBuf }, #[error("could not remove '{name}' file: '{}'", .path.display())] diff --git a/src/settings.rs b/src/settings.rs index 6fa222c14b..43d5ec19dd 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -43,7 +43,10 @@ impl SettingsFile { drop(b); *self.cache.borrow_mut() = Some(if utils::is_file(&self.path) { let content = utils::read_file("settings", &self.path)?; - Settings::parse(&content)? + Settings::parse(&content).with_context(|| RustupError::ParsingFile { + name: "settings", + path: self.path.clone(), + })? } else { needs_save = true; Default::default() diff --git a/tests/suite/cli_v2.rs b/tests/suite/cli_v2.rs index 41b848c86a..77bc623a5a 100644 --- a/tests/suite/cli_v2.rs +++ b/tests/suite/cli_v2.rs @@ -299,6 +299,48 @@ fn bad_sha_on_manifest() { }); } +#[test] +fn bad_manifest() { + // issue #3851 + setup(&|config| { + // install some toolchain + config.expect_ok(&["rustup", "update", "nightly"]); + + #[cfg(not(target_os = "windows"))] + let path = format!( + "toolchains/nightly-{}/lib/rustlib/multirust-channel-manifest.toml", + this_host_triple(), + ); + + #[cfg(target_os = "windows")] + let path = format!( + r"toolchains\nightly-{}\lib/rustlib\multirust-channel-manifest.toml", + this_host_triple(), + ); + + assert!(config.rustupdir.has(&path)); + let path = config.rustupdir.join(&path); + + // corrupt the manifest file by inserting a NUL byte at some position + let old = fs::read_to_string(&path).unwrap(); + let pattern = "[[pkg.rust.targ"; + let (prefix, suffix) = old.split_once(pattern).unwrap(); + let new = format!("{prefix}{pattern}\u{0}{suffix}"); + fs::write(&path, new).unwrap(); + + // run some commands that try to load the manifest and + // check that the manifest parsing error includes the manifest file path + config.expect_err( + &["rustup", "check"], + &format!("error: could not parse manifest file: '{}'", path.display()), + ); + config.expect_err( + &["cargo", "--help"], + &format!("error: could not parse manifest file: '{}'", path.display()), + ); + }); +} + #[test] fn bad_sha_on_installer() { setup(&|config| {