diff --git a/Cargo.lock b/Cargo.lock index e936caa9d4..cb3c1b7017 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2107,6 +2107,7 @@ dependencies = [ "tracing-log", "tracing-opentelemetry", "tracing-subscriber", + "unicode-security", "url", "wait-timeout", "walkdir", @@ -2546,6 +2547,21 @@ dependencies = [ "zerovec", ] +[[package]] +name = "tinyvec" +version = "1.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb4ebadaa0af04fab11ae01eb5f9fdb5f9c5b875506e210e71c07873528baa7f" +dependencies = [ + "tinyvec_macros", +] + +[[package]] +name = "tinyvec_macros" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" + [[package]] name = "tokio" version = "1.53.1" @@ -2876,6 +2892,31 @@ version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +[[package]] +name = "unicode-normalization" +version = "0.1.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fd4f6878c9cb28d874b009da9e8d183b5abc80117c40bbd187a1fde336be6e8" +dependencies = [ + "tinyvec", +] + +[[package]] +name = "unicode-script" +version = "0.5.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "383ad40bb927465ec0ce7720e033cb4ca06912855fc35db31b5755d0de75b1ee" + +[[package]] +name = "unicode-security" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2e4ddba1535dd35ed8b61c52166b7155d7f4e4b8847cec6f48e71dc66d8b5e50" +dependencies = [ + "unicode-normalization", + "unicode-script", +] + [[package]] name = "unicode-width" version = "0.2.2" diff --git a/Cargo.toml b/Cargo.toml index eeb5769af8..0ff745fb1e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,6 +101,7 @@ tracing = "0.1" tracing-log = "0.2" tracing-opentelemetry = { version = "0.33", optional = true } tracing-subscriber = { version = "0.3.23", features = ["env-filter"] } +unicode-security = "0.1" url = "2.4" wait-timeout = "0.2" xz2 = "0.1.3" diff --git a/doc/user-guide/src/concepts/toolchains.md b/doc/user-guide/src/concepts/toolchains.md index 336d6a5ae8..b80cb8aa2a 100644 --- a/doc/user-guide/src/concepts/toolchains.md +++ b/doc/user-guide/src/concepts/toolchains.md @@ -57,6 +57,14 @@ local builds of the Rust toolchain. To teach `rustup` about your build, run: $ rustup toolchain link my-toolchain path/to/my/toolchain/sysroot ``` +Custom toolchain names may contain any character that [UTS #39] allows in an +identifier, except for `:` and `'`. In practice that means letters and digits +in any script, along with `.`, `_`, and `-`; whitespace, most punctuation, +emoji, and invisible or direction-altering characters are rejected. The names +`.` and `..` are not accepted either. + +[UTS #39]: https://www.unicode.org/reports/tr39/#Identifier_Status_and_Type + For example, on Ubuntu you might clone `rust-lang/rust` into `~/rust`, build it, and then run: diff --git a/src/toolchain/names.rs b/src/toolchain/names.rs index 71c6415832..4f7709b3d8 100644 --- a/src/toolchain/names.rs +++ b/src/toolchain/names.rs @@ -49,6 +49,7 @@ use std::{ }; use thiserror::Error; +use unicode_security::GeneralSecurityProfile; use crate::dist::{PartialToolchainDesc, TargetTuple, ToolchainDesc}; @@ -71,19 +72,6 @@ pub enum InvalidName { PlusPrefix(String), } -/// Common validate rules for all sorts of toolchain names -fn validate(candidate: &str) -> Result<&str, InvalidName> { - if let Some(without_plus) = candidate.strip_prefix('+') { - return Err(InvalidName::PlusPrefix(without_plus.to_string())); - } - let normalized_name = candidate.trim_end_matches('/'); - if normalized_name.is_empty() { - Err(InvalidName::ToolchainName(candidate.into())) - } else { - Ok(normalized_name) - } -} - /// A toolchain name from user input. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub(crate) enum ResolvableToolchainName { @@ -99,11 +87,15 @@ impl ResolvableToolchainName { Self::Official(desc) => ToolchainName::Official(desc.resolve(host)?), }) } +} + +impl FromStr for ResolvableToolchainName { + type Err = InvalidName; - // If candidate could be resolved, return a ready to resolve version of it. + // If value could be resolved, return a ready to resolve version of it. // Otherwise error. - fn validate(candidate: &str) -> Result { - let candidate = validate(candidate)?; + fn from_str(value: &str) -> Result { + let candidate = validate_named_toolchain(value)?; if let Ok(desc) = PartialToolchainDesc::from_str(candidate) { return Ok(Self::Official(desc)); } @@ -115,14 +107,6 @@ impl ResolvableToolchainName { } } -impl FromStr for ResolvableToolchainName { - type Err = InvalidName; - - fn from_str(value: &str) -> Result { - Self::validate(value) - } -} - impl From<&PartialToolchainDesc> for ResolvableToolchainName { fn from(value: &PartialToolchainDesc) -> Self { Self::Official(value.to_owned()) @@ -147,22 +131,16 @@ pub(crate) enum MaybeResolvableToolchainName { None, } -impl MaybeResolvableToolchainName { - // If candidate could be resolved, return a ready to resolve version of it. - // Otherwise error. - fn validate(candidate: &str) -> Result { - Ok(match validate(candidate)? { - "none" => Self::None, - candidate => Self::Some(ResolvableToolchainName::validate(candidate)?), - }) - } -} - impl FromStr for MaybeResolvableToolchainName { type Err = InvalidName; + // If value could be resolved, return a ready to resolve version of it. + // Otherwise error. fn from_str(value: &str) -> Result { - Self::validate(value) + Ok(match normalize_name(value)? { + "none" => Self::None, + candidate => Self::Some(ResolvableToolchainName::from_str(candidate)?), + }) } } @@ -183,9 +161,11 @@ pub(crate) enum MaybeOfficialToolchainName { Some(PartialToolchainDesc), } -impl MaybeOfficialToolchainName { - fn validate(candidate: &str) -> Result { - Ok(match validate(candidate)? { +impl FromStr for MaybeOfficialToolchainName { + type Err = InvalidName; + + fn from_str(value: &str) -> Result { + Ok(match validate_named_toolchain(value)? { "none" => Self::None, candidate => Self::Some( PartialToolchainDesc::from_str(candidate) @@ -195,14 +175,6 @@ impl MaybeOfficialToolchainName { } } -impl FromStr for MaybeOfficialToolchainName { - type Err = InvalidName; - - fn from_str(value: &str) -> Result { - Self::validate(value) - } -} - impl Display for MaybeOfficialToolchainName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -221,21 +193,6 @@ pub enum ToolchainName { Custom(CustomToolchainName), } -impl ToolchainName { - /// If the string is already resolved, allow direct conversion - fn validate(candidate: &str) -> Result { - let candidate = validate(candidate)?; - if let Ok(desc) = ToolchainDesc::from_str(candidate) { - return Ok(Self::Official(desc)); - } - - match CustomToolchainName::from_str(candidate) { - Ok(custom) => Ok(Self::Custom(custom)), - Err(_) => Err(InvalidName::ToolchainName(candidate.into())), - } - } -} - impl From for ToolchainName { fn from(value: ToolchainDesc) -> Self { Self::Official(value) @@ -251,8 +208,17 @@ impl From for ToolchainName { impl FromStr for ToolchainName { type Err = InvalidName; + /// If the string is already resolved, allow direct conversion fn from_str(value: &str) -> Result { - Self::validate(value) + let candidate = validate_named_toolchain(value)?; + if let Ok(desc) = ToolchainDesc::from_str(candidate) { + return Ok(Self::Official(desc)); + } + + match CustomToolchainName::from_str(candidate) { + Ok(custom) => Ok(Self::Custom(custom)), + Err(_) => Err(InvalidName::ToolchainName(candidate.into())), + } } } @@ -282,25 +248,25 @@ impl ResolvableLocalToolchainName { Self::Path(t) => Ok(LocalToolchainName::Path(t)), } } - - /// Validates if the string is a resolvable toolchain, or a path based toolchain. - fn validate(candidate: &str) -> Result { - let candidate = validate(candidate)?; - if let Ok(name) = ResolvableToolchainName::from_str(candidate) { - return Ok(Self::Named(name)); - } - - Ok(Self::Path(PathBasedToolchainName::try_from( - &PathBuf::from(candidate) as &Path, - )?)) - } } impl FromStr for ResolvableLocalToolchainName { type Err = InvalidName; + /// Parses a resolvable toolchain, or a path based toolchain. fn from_str(value: &str) -> Result { - Self::validate(value) + let candidate = normalize_name(value)?; + if let Ok(name) = ResolvableToolchainName::from_str(candidate) { + return Ok(Self::Named(name)); + } + + if candidate.contains('/') || candidate.contains('\\') { + let path = PathBuf::from(candidate); + let path = PathBasedToolchainName::try_from(&path as &Path)?; + return Ok(Self::Path(path)); + } + + Err(InvalidName::ToolchainName(candidate.into())) } } @@ -371,21 +337,6 @@ impl Display for LocalToolchainName { #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] pub struct CustomToolchainName(String); -impl CustomToolchainName { - fn validate(candidate: &str) -> Result { - let candidate = validate(candidate)?; - if candidate.parse::().is_ok() - || candidate == "none" - || candidate.contains('/') - || candidate.contains('\\') - { - Err(InvalidName::CustomName(candidate.into())) - } else { - Ok(Self(candidate.into())) - } - } -} - impl Deref for CustomToolchainName { type Target = str; @@ -398,7 +349,13 @@ impl FromStr for CustomToolchainName { type Err = InvalidName; fn from_str(value: &str) -> Result { - Self::validate(value) + let candidate = + validate_named_toolchain(value).map_err(|_| InvalidName::CustomName(value.into()))?; + if candidate.parse::().is_ok() || candidate == "none" { + Err(InvalidName::CustomName(candidate.into())) + } else { + Ok(Self(candidate.into())) + } } } @@ -463,6 +420,52 @@ impl Deref for PathBasedToolchainName { } } +/// Common validate rules for toolchain names that aren't paths. +/// +/// Beyond the shared [`normalize_name`] rules, every character must be allowed in an +/// identifier by the Unicode general security profile (UTS #39). That admits +/// ASCII letters, digits, `.`, `_` and `-` -- so every official toolchain name +/// still parses -- along with unicode identifiers such as `合法的`, while +/// rejecting whitespace, most punctuation, emoji, and invisible or +/// direction-altering characters. +fn validate_named_toolchain(candidate: &str) -> Result<&str, InvalidName> { + let candidate = normalize_name(candidate)?; + // `.` and `..` pass the profile but would escape or alias the toolchains + // directory, so they need rejecting on their own. + if !matches!(candidate, "." | "..") + && candidate + .chars() + .all(|c| c.identifier_allowed() && !EXCLUDED.contains(&c)) + { + Ok(candidate) + } else { + Err(InvalidName::ToolchainName(candidate.to_owned())) + } +} + +/// Normalization shared by all sorts of toolchain names. +/// +/// Strips the trailing slashes a shell may have completed onto a toolchain +/// directory, and rejects the `+toolchain` argument form along with names that +/// are empty once normalized. +fn normalize_name(candidate: &str) -> Result<&str, InvalidName> { + if let Some(without_plus) = candidate.strip_prefix('+') { + return Err(InvalidName::PlusPrefix(without_plus.to_string())); + } + let normalized_name = candidate.trim_end_matches('/'); + if normalized_name.is_empty() { + Err(InvalidName::ToolchainName(candidate.into())) + } else { + Ok(normalized_name) + } +} + +/// Characters that UTS #39 permits in identifiers but that we reject anyway, +/// because a named toolchain also has to work as a directory name under +/// `.rustup/toolchains`: `:` would open an NTFS alternate data stream, and `'` +/// needs quoting in enough shells to be a nuisance. +const EXCLUDED: &[char] = &[':', '\'']; + #[cfg(test)] mod tests { use std::str::FromStr; @@ -480,13 +483,21 @@ mod tests { fn partial_toolchain_desc_regex() -> String { let tuple_regex = format!( r"(-({}))?(?:-({}))?(?:-({}))?", - LIST_ARCHS.join("|"), - LIST_OSES.join("|"), - LIST_ENVS.join("|") + regex_alternates(LIST_ARCHS), + regex_alternates(LIST_OSES), + regex_alternates(LIST_ENVS) ); r"(nightly|beta|stable|[0-9]{1}(\.(0|[1-9][0-9]{0,2}))(\.(0|[1-9][0-9]{0,1}))?(-beta(\.(0|[1-9][1-9]{0,1}))?)?)(-([0-9]{4}-[0-9]{2}-[0-9]{2}))?".to_owned() + &tuple_regex } + fn regex_alternates(values: &[&str]) -> String { + values + .iter() + .map(|value| regex::escape(value)) + .collect::>() + .join("|") + } + prop_compose! { fn arb_partial_toolchain_desc() (s in string_regex(&partial_toolchain_desc_regex()).unwrap()) -> String { @@ -496,9 +507,8 @@ mod tests { prop_compose! { fn arb_custom_name() - (s in r"[^\\/+][^\\/]*") -> String { + (s in r"[A-Za-z0-9._-]+") -> String { // perhaps need to filter 'none' and partial toolchains - but they won't typically be generated anyway. - // Also filter '+' prefix as that's reserved for +toolchain syntax. s } } @@ -531,11 +541,18 @@ mod tests { #[test] fn test_parse_custom(name in arb_custom_name()) { + prop_assume!(name != "none"); + prop_assume!(name != "."); + prop_assume!(name != ".."); + prop_assume!(PartialToolchainDesc::from_str(&name).is_err()); CustomToolchainName::from_str(&name).unwrap(); } #[test] fn test_parse_resolvable_name(name in arb_resolvable_name()) { + prop_assume!(name != "none"); + prop_assume!(name != "."); + prop_assume!(name != ".."); ResolvableToolchainName::from_str(&name).unwrap(); } @@ -567,10 +584,10 @@ mod tests { "1.8.0-x86_64-apple-darwin", "1.8.0-x86_64-unknown-linux-gnu", "1.10.0-x86_64-unknown-linux-gnu", - "bar(baz)", - "foo#bar", - "the cake is a lie", - "this.is.not-a+semver", + "bar.baz", + "foo_bar", + "stage1-local", + "this.is.not-a_semver", ] .into_iter() .map(|s| ToolchainName::from_str(s).unwrap()) @@ -591,11 +608,11 @@ mod tests { "1.8.0-beta-x86_64-apple-darwin", "1.8.0-beta.2-x86_64-apple-darwin", // https://github.com/rust-lang/rustup/issues/3517 - "foo#bar", - "bar(baz)", - "this.is.not-a+semver", + "foo_bar", + "bar.baz", + "this.is.not-a_semver", // https://github.com/rust-lang/rustup/issues/3168 - "the cake is a lie", + "stage1-local", ] .into_iter() .map(|s| ToolchainName::from_str(s).unwrap()) @@ -605,4 +622,51 @@ mod tests { assert_eq!(expected, v); } + + #[test] + fn custom_names_reject_special_characters() { + for name in [ + "bar(baz)", + "foo#bar", + "the cake is a lie", + "this.is.not-a+semver", + ".", + "..", + // permitted by UTS #39, excluded by us + "quote'toolchain", + "stream:name", + // rejected by UTS #39 + "µ", // MICRO SIGN, restricted in favour of U+03BC + "🦀", // emoji + "tilde~home", // not identifier punctuation + "abc\u{202E}def", // RIGHT-TO-LEFT OVERRIDE + "zero\u{200D}width", // ZERO WIDTH JOINER + "no\u{00A0}break", // NO-BREAK SPACE + ] { + CustomToolchainName::from_str(name).unwrap_err(); + ResolvableToolchainName::from_str(name).unwrap_err(); + ToolchainName::from_str(name).unwrap_err(); + } + } + + #[test] + fn custom_names_accept_unicode_identifiers() { + for name in ["合法的", "μ", "café", "Ελληνικά", "тулчейн"] { + CustomToolchainName::from_str(name).unwrap(); + ResolvableToolchainName::from_str(name).unwrap(); + ToolchainName::from_str(name).unwrap(); + } + } + + #[test] + fn official_names_survive_the_identifier_profile() { + for name in [ + "stable-x86_64-unknown-linux-gnu", + "nightly-2015-01-01-aarch64-apple-darwin", + "1.86.0-x86_64-pc-windows-msvc", + "beta-i686-pc-windows-gnullvm", + ] { + ToolchainName::from_str(name).unwrap(); + } + } } diff --git a/tests/suite/cli_misc.rs b/tests/suite/cli_misc.rs index 1f52643e88..09674cd7c4 100644 --- a/tests/suite/cli_misc.rs +++ b/tests/suite/cli_misc.rs @@ -98,6 +98,24 @@ error:[..] invalid custom toolchain name 'beta' ... error:[..] invalid custom toolchain name 'stable' ... +"#]]) + .is_err(); + cx.config + .expect(["rustup", "toolchain", "link", "bad name", "foo"]) + .await + .with_stderr(snapbox::str![[r#" +... +error:[..] invalid custom toolchain name 'bad name' +... +"#]]) + .is_err(); + cx.config + .expect(["rustup", "toolchain", "link", "foo#bar", "foo"]) + .await + .with_stderr(snapbox::str![[r#" +... +error:[..] invalid custom toolchain name 'foo#bar' +... "#]]) .is_err(); } diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index ee69fdb81d..1f412e7b14 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -3790,7 +3790,7 @@ async fn non_utf8_toolchain() { .await .with_stderr(snapbox::str![[r#" ... -error: toolchain '�(' is not installed +error: invalid toolchain name '�(' ... "#]]); } @@ -3817,7 +3817,7 @@ async fn non_utf8_toolchain() { .await .with_stderr(snapbox::str![[r#" ... -error: toolchain '��' is not installed +error: invalid toolchain name '��' ... "#]]); } diff --git a/tests/suite/cli_v2.rs b/tests/suite/cli_v2.rs index c4a0efd495..ebbb0b89ca 100644 --- a/tests/suite/cli_v2.rs +++ b/tests/suite/cli_v2.rs @@ -1662,7 +1662,7 @@ async fn cannot_add_empty_named_custom_toolchain() { .await .with_stderr(snapbox::str![[r#" ... -error: invalid value '' for '': invalid toolchain name '' +error: invalid value '' for '': invalid custom toolchain name '' ... "#]]) .is_err();