Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle case mismatches when looking up env vars in the Config snapshot #11824

Merged
merged 7 commits into from
Mar 17, 2023
65 changes: 53 additions & 12 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,13 @@ pub struct Config {
target_dir: Option<Filesystem>,
/// Environment variables, separated to assist testing.
env: HashMap<OsString, OsString>,
/// Environment variables, converted to uppercase to check for case mismatch
upper_case_env: HashMap<String, String>,
/// Environment variables converted to uppercase to check for case mismatch
/// (relevant on Windows, where environment variables are case-insensitive).
case_insensitive_env: HashMap<String, String>,
/// Environment variables converted to uppercase and with "-" replaced by "_"
/// (the format expected by Cargo). This only contains entries where the key and variable are
/// both valid UTF-8.
normalized_env: HashMap<String, String>,
/// Tracks which sources have been updated to avoid multiple updates.
updated_sources: LazyCell<RefCell<HashSet<SourceId>>>,
/// Cache of credentials from configuration or credential providers.
Expand Down Expand Up @@ -262,10 +267,17 @@ impl Config {

let env: HashMap<_, _> = env::vars_os().collect();

let upper_case_env = env
let case_insensitive_env: HashMap<_, _> = env
.keys()
.filter_map(|k| k.to_str())
.map(|k| (k.to_uppercase(), k.to_owned()))
.collect();

let normalized_env = env
.iter()
.filter_map(|(k, _)| k.to_str()) // Only keep valid UTF-8
.map(|k| (k.to_uppercase().replace("-", "_"), k.to_owned()))
// Only keep entries where both the key and value are valid UTF-8
.filter_map(|(k, v)| Some((k.to_str()?, v.to_str()?)))
.map(|(k, _)| (k.to_uppercase().replace("-", "_"), k.to_owned()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say why these lines changed? I would not expect any changes to normalized_env.

Though if you want to clean this up, it seems like it would be nicer to use .keys() instead of .iter().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, I meant to add a comment about this but forgot. I realized when I was looking back at #11727 that we might have accidentally subtly changed the behavior of normalized_env there.

Before #11727, the normalized_env (called upper_case_env there) was generated from self.env, which held pairs where both the key and value were required to be valid UTF-8. In #11727, upper_case_env started using all the keys that were valid UTF-8 (regardless of whether the value was also UTF-8), so in principle more keys might potentially be included.

Since I noticed it here, I put back the check that both the env key and env var are valid UTF-8 before using the key in normalized_env. I don't really have a sense of whether this small change is important. If you think that it's probably not, then I'd be glad to use .keys() instead, which I'd agree is more readable.

.collect();

let cache_key: &OsStr = "CARGO_CACHE_RUSTC_INFO".as_ref();
Expand Down Expand Up @@ -303,7 +315,8 @@ impl Config {
creation_time: Instant::now(),
target_dir: None,
env,
upper_case_env,
case_insensitive_env,
normalized_env,
updated_sources: LazyCell::new(),
credential_cache: LazyCell::new(),
package_cache_lock: RefCell::new(None),
Expand Down Expand Up @@ -730,6 +743,14 @@ impl Config {
/// Helper primarily for testing.
pub fn set_env(&mut self, env: HashMap<String, String>) {
self.env = env.into_iter().map(|(k, v)| (k.into(), v.into())).collect();
self.case_insensitive_env = self
.env_keys()
.map(|k| (k.to_uppercase(), k.to_owned()))
.collect();
self.normalized_env = self
.env()
.map(|(k, _)| (k.to_uppercase().replace("-", "_"), k.to_owned()))
.collect();
kylematsuda marked this conversation as resolved.
Show resolved Hide resolved
}

/// Returns all environment variables as an iterator, filtering out entries
Expand Down Expand Up @@ -772,10 +793,10 @@ impl Config {
/// This can be used similarly to `std::env::var`.
pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
let key = key.as_ref();
let s = match self.env.get(key) {
Some(s) => s,
None => bail!("{key:?} could not be found in the environment snapshot",),
};
let s = self
.get_env_os(key)
.ok_or_else(|| anyhow!("{key:?} could not be found in the environment snapshot"))?;

match s.to_str() {
Some(s) => Ok(s.to_owned()),
None => bail!("environment variable value is not valid unicode: {s:?}"),
Expand All @@ -786,7 +807,27 @@ impl Config {
///
/// This can be used similarly to `std::env::var_os`.
pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
self.env.get(key.as_ref()).cloned()
match self.env.get(key.as_ref()) {
Some(s) => Some(s.clone()),
None => {
if cfg!(windows) {
self.get_env_case_insensitive(key).cloned()
} else {
None
}
}
}
}

/// Wrapper for `self.env.get` when `key` should be case-insensitive.
/// This is relevant on Windows, where environment variables are case-insensitive.
/// Note that this only works on keys that are valid UTF-8.
fn get_env_case_insensitive(&self, key: impl AsRef<OsStr>) -> Option<&OsString> {
let upper_case_key = key.as_ref().to_str()?.to_uppercase();
// `self.case_insensitive_env` holds pairs like `("PATH", "Path")`
// or `("MY-VAR", "my-var")`.
let env_key: &OsStr = self.case_insensitive_env.get(&upper_case_key)?.as_ref();
self.env.get(env_key)
}

/// Get the value of environment variable `key`.
Expand Down Expand Up @@ -821,7 +862,7 @@ impl Config {
}

fn check_environment_key_case_mismatch(&self, key: &ConfigKey) {
if let Some(env_key) = self.upper_case_env.get(key.as_env_key()) {
if let Some(env_key) = self.normalized_env.get(key.as_env_key()) {
let _ = self.shell().warn(format!(
"Environment variables are expected to use uppercase letters and underscores, \
the variable `{}` will be ignored and have no effect",
Expand Down
26 changes: 26 additions & 0 deletions tests/testsuite/cargo_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,32 @@ fn list_command_looks_at_path() {
);
}

#[cfg(windows)]
#[cargo_test]
fn list_command_looks_at_path_case_mismatch() {
let proj = project()
.executable(Path::new("path-test").join("cargo-1"), "")
.build();

let mut path = path();
path.push(proj.root().join("path-test"));
let path = env::join_paths(path.iter()).unwrap();

// See issue #11814: Environment variable names are case-insensitive on Windows.
// We need to check that having "Path" instead of "PATH" is okay.
let output = cargo_process("-v --list")
.env("Path", &path)
.env_remove("PATH")
.exec_with_output()
.unwrap();
let output = str::from_utf8(&output.stdout).unwrap();
assert!(
output.contains("\n 1 "),
"missing 1: {}",
output
);
}

#[cargo_test]
fn list_command_handles_known_external_commands() {
let p = project()
Expand Down
25 changes: 25 additions & 0 deletions tests/testsuite/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,31 @@ f1 = 123
assert_eq!(s, S { f1: Some(456) });
}

#[cfg(windows)]
#[cargo_test]
fn environment_variable_casing() {
// Issue #11814: Environment variable names are case-insensitive on Windows.
let config = ConfigBuilder::new()
.env("Path", "abc")
.env("Two-Words", "abc")
.env("two_words", "def")
.build();

let var = config.get_env("PATH").unwrap();
assert_eq!(var, String::from("abc"));

let var = config.get_env("path").unwrap();
assert_eq!(var, String::from("abc"));

let var = config.get_env("TWO-WORDS").unwrap();
assert_eq!(var, String::from("abc"));

// Make sure that we can still distinguish between dashes and underscores
// in variable names.
let var = config.get_env("Two_Words").unwrap();
assert_eq!(var, String::from("def"));
}

#[cargo_test]
fn config_works_with_extension() {
write_config_toml(
Expand Down