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

kylematsuda · 2023-03-10T01:00:07Z

What does this PR try to resolve?

Windows environment variables are case-insensitive, which causes problems when looking them up in the Config env snapshot.

This PR adds another member (case_insensitive_env) in Config that maps upper-cased keys to their original values in the env (for example, "PATH" => "Path"). If lookup in self.env fails, this PR converts the key to upper case and looks it up in self.case_insensitive_env to obtain the correct key name if it exists (on Windows only).

How should we test and review this PR?

Please see the new tests in testsuite/config.rs and testsuite/cargo_command.rs.

Additional information

Currently, this uses str::to_uppercase to uppercase the keys. This requires key to be valid UTF-8, and may disagree with how the OS uppercases things (see the link in this comment for details).

…s when var is not in self.env

…ded for tests)

…unicode uppercase instead of ascii

rustbot · 2023-03-10T01:00:13Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

ehuss · 2023-03-10T01:57:06Z

r? @ehuss

ehuss

Does get_env_str() and env_has_key() also need to be updated?

ehuss · 2023-03-10T01:51:13Z

src/cargo/util/config/mod.rs

+            // 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()))


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().

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.

src/cargo/util/config/mod.rs

kylematsuda · 2023-03-10T06:38:51Z

Does get_env_str() and env_has_key() also need to be updated?

I think they don't need to be updated -- here's my reasoning:

Those two methods were added for convenience when self.env changed from HashMap<String, String> to HashMap<OsString, OsString>, and they replaced calls to self.env.get() and self.env.contains_key(). This means that these call sites were accessing data in self.env both before and after #11727. Therefore, if case mismatch is an issue with these, it would have already been an issue prior to #11727. From the perspective of these methods, the only difference after #11727 is that self.env might also contain a few items that aren't valid UTF-8.

On the other hand, adding config.get_env and config.get_env_os changed a bunch of calls to std::env::var(_os) to go through self.env instead, so it makes sense that those ones are now causing case mismatch issues.

Hopefully this makes sense, please let me know if anything is unclear or if I'm thinking about this wrong.

weihanglo

Looks reasonable to me!

Does it make sense that we make three env hashmaps (env, case_insensitive_env, and normalized_env) into a single struct, say Envs? The struct only exposes a limited API for Config, and all access to env must get through it.

That will be clearer what is accessible and what is not. We can also put that struct under its own module (config/mod.rs has grown too big 😆).

kylematsuda · 2023-03-10T17:10:31Z

Does it make sense that we make three env hashmaps (env, case_insensitive_env, and normalized_env) into a single struct, say Envs? The struct only exposes a limited API for Config, and all access to env must get through it.

Sure! I agree that would be clearer, I'll give that a shot now.

…psulation

kylematsuda · 2023-03-13T15:19:20Z

Hi @weihanglo, I've implemented your suggestion in the most recent commit. Please take a look when you have a chance and let me know if you have any suggestions 😃

weihanglo

Looks great. Thank you for extracting them as a struct! For other issues let's wait for Eric's feedback :)

kylematsuda · 2023-03-13T18:12:14Z

Sounds great, thanks for taking a look!

weihanglo

I read through this again. Seems good to merge.

Regarding UTF-8 validity of env value #11824 (comment), we just go back in the day before #11727 so should be no problem at all I guess. We can lift the restriction if needed after.

@ehuss, do you have any other thought on this?

ehuss

Thanks! I like wrapping things in the Env struct.

ehuss · 2023-03-15T23:50:19Z

src/cargo/util/config/environment.rs

+        .collect();
+    let normalized_env = env
+        .iter()
+        // Only keep entries where both the key and value are valid UTF-8


Can this comment also mention why it is skipping over non-utf-8 values? Perhaps something like "Because the config env vars only support utf-8, this needs to be kept in sync with that, otherwise the normalized map warning could incorrectly warn about entries that can't be read by the config system." Or something shorter if you can word it better.

ehuss · 2023-03-16T00:29:34Z

src/cargo/util/config/environment.rs

+    /// This is intended for use in private methods of `Config`,
+    /// and does not check for env key case mismatch.


Could this include a comment explaining why it is case-sensitive?

I was looking over the history, and it looks like this was an unintended regression in 1.28 via #5552. I think I somehow forgot about windows case sensitivity at that time, and have been confused on a few occasions (like #9169) due to the fact that Command used to force keys to be uppercase on Windows (fixed in 1.55 via rust-lang/rust#85270). When launching cargo from within cargo's testsuite, it used Command which ended up mucking with the environment keys.

I don't think this needs to be fixed here in this PR. I don't think anyone has complained about this since then, so maybe it's best just to leave it.

So, a comment could explain something like:

This is case-sensitive on Windows (even though Windows is usually case-insensitive) due to an unintended regression in 1.28 (via #5552). This should only affect keys used for cargo's config-system env variables (CARGO_ prefixed ones) which are currently all uppercase. We may want to consider rectifying it if users report issues. One thing that adds a wrinkle here is the unstable advanced-env option which requires case-sensitive keys.

Do not use this for any other purposes. Use [Env::get_env_os] or [Env::get_env] instead, which properly handle case insensitivity on Windows.

kylematsuda · 2023-03-16T21:32:34Z

Thanks for the helpful feedback and context! I added those comments you suggested in the latest commit.

I also added a bit more documentation on Env itself to try to provide more context. I think something that has been confusing me is that environment variables can be accessed through two different APIs on Config: Config::get_env[_os] or Config::get (for CARGO_ prefixed keys). The Env::case_insensitive_env is only used in the former; Env::normalized_env is only used in the latter. So I tried to point this out explicitly in the docstrings for Env and its fields. Please let me know if you think it's confusing/too much/etc., I'm happy to remove it if it's not helpful.

weihanglo · 2023-03-16T22:01:42Z

Thank you for the write-up! We have really wanted to document every design rationale to help ourselves and contributors.

BTW, CI is currently blocked. We are waiting for the next nightly release.

weihanglo · 2023-03-17T01:06:30Z

Talked to ehuss and we both agree it is pretty good to merge!

@bors r+

bors · 2023-03-17T01:06:32Z

📌 Commit 7af55d8 has been approved by weihanglo

It is now in the queue for this repository.

ehuss · 2023-03-17T01:06:42Z

Thanks! It is much appreciated.

@bors r+

bors · 2023-03-17T01:06:44Z

💡 This pull request was already approved, no need to approve it again.

bors · 2023-03-17T01:06:45Z

📌 Commit 7af55d8 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-03-17T01:06:45Z

⌛ Testing commit 7af55d8 with merge 6270d15...

Handle case mismatches when looking up env vars in the Config snapshot ### What does this PR try to resolve? Fixes #11814. Windows environment variables are case-insensitive, which causes problems when looking them up in the `Config` env snapshot. This PR adds another member (`case_insensitive_env`) in `Config` that maps upper-cased keys to their original values in the env (for example, `"PATH" => "Path"`). If lookup in `self.env` fails, this PR converts the key to upper case and looks it up in `self.case_insensitive_env` to obtain the correct key name if it exists (on Windows only). ### How should we test and review this PR? Please see the new tests in `testsuite/config.rs` and `testsuite/cargo_command.rs`. ### Additional information Currently, this uses `str::to_uppercase` to uppercase the keys. This requires key to be valid UTF-8, and may disagree with how the OS uppercases things (see the link in [this comment](#11814 (comment)) for details).

bors · 2023-03-17T01:06:51Z

⌛ Testing commit 7af55d8 with merge c9faf70...

bors · 2023-03-17T02:07:36Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing c9faf70 to master...

…=ehuss Handle case mismatches when looking up env vars in the Config snapshot ### What does this PR try to resolve? Fixes rust-lang#11814. Windows environment variables are case-insensitive, which causes problems when looking them up in the `Config` env snapshot. This PR adds another member (`case_insensitive_env`) in `Config` that maps upper-cased keys to their original values in the env (for example, `"PATH" => "Path"`). If lookup in `self.env` fails, this PR converts the key to upper case and looks it up in `self.case_insensitive_env` to obtain the correct key name if it exists (on Windows only). ### How should we test and review this PR? Please see the new tests in `testsuite/config.rs` and `testsuite/cargo_command.rs`. ### Additional information Currently, this uses `str::to_uppercase` to uppercase the keys. This requires key to be valid UTF-8, and may disagree with how the OS uppercases things (see the link in [this comment](rust-lang#11814 (comment)) for details).

[beta-1.69] backport #11824 Beta backports: - #11824 In order to make CI pass, the following PRs are also cherry-picked: - —

…nglo [beta-1.69] cargo beta backports 3 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7b18c85808a6b45ec8364bf730617b6f13e0f9f8 2023-02-28 19:39:39 +0000 to 2023-03-17 12:29:33 +0000 - [beta-1.69] backport rust-lang/cargo#11824 (rust-lang/cargo#11863) - [beta-1.69] backport rust-lang/cargo#11820 (rust-lang/cargo#11823) - chore: Backport rust-lang/cargo#11630 to `1.69.0` (rust-lang/cargo#11806) r? `@ghost`

Update cargo 11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3 2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000 - docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870) - docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869) - Update curl-sys (rust-lang/cargo#11871) - Poll loop fixes (rust-lang/cargo#11624) - clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828) - Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859) - Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824) - align semantics of generated vcs ignore files (rust-lang/cargo#11855) - Add more information to wait-for-publish (rust-lang/cargo#11713) - docs: Address warnings (rust-lang/cargo#11856) - docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)

kylematsuda added 4 commits March 9, 2023 16:54

add unit test for config and integration test with cargo --list

8353396

store uppercased env var names; look for uppercased version on Window…

0255c30

…s when var is not in self.env

change set_env to update case_insensitive_env and normalized_env (nee…

3c8633a

…ded for tests)

use String instead of OsString in case_insensitive_env so we can use …

8ae0773

…unicode uppercase instead of ascii

rustbot assigned weihanglo Mar 10, 2023

rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2023

weihanglo added the beta-nominated Nominated to backport to the beta branch. label Mar 10, 2023

rustbot assigned ehuss and unassigned weihanglo Mar 10, 2023

ehuss reviewed Mar 10, 2023

View reviewed changes

add free function to generate auxiliary envs

35b53b8

weihanglo reviewed Mar 10, 2023

View reviewed changes

factor out three hashmaps in Config into a new struct for better enca…

f810234

…psulation

weihanglo reviewed Mar 13, 2023

View reviewed changes

weihanglo approved these changes Mar 15, 2023

View reviewed changes

ehuss reviewed Mar 16, 2023

View reviewed changes

improve docstrings for config::environment

7af55d8

kylematsuda force-pushed the windows-config-env-fix branch from ae52cb2 to 7af55d8 Compare March 16, 2023 21:15

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 17, 2023

bors merged commit c9faf70 into rust-lang:master Mar 17, 2023
6 of 15 checks passed

bors mentioned this pull request Mar 17, 2023

clippy: warn disallowed_methods for std::env::var and friends #11828

Merged

weihanglo mentioned this pull request Mar 17, 2023

[beta-1.69] backport #11824 #11863

Merged

bors added a commit that referenced this pull request Mar 17, 2023

Auto merge of #11863 - weihanglo:rust-1.69.0, r=ehuss

7b18c85

[beta-1.69] backport #11824 Beta backports: - #11824 In order to make CI pass, the following PRs are also cherry-picked: - —

weihanglo mentioned this pull request Mar 17, 2023

[beta-1.69] cargo beta backports rust-lang/rust#109265

Merged

weihanglo mentioned this pull request Mar 21, 2023

Update cargo rust-lang/rust#109463

Merged

ehuss added this to the 1.70.0 milestone Mar 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kylematsuda commented Mar 10, 2023

rustbot commented Mar 10, 2023

ehuss commented Mar 10, 2023

ehuss left a comment

ehuss Mar 10, 2023

kylematsuda Mar 10, 2023

kylematsuda commented Mar 10, 2023

weihanglo left a comment

kylematsuda commented Mar 10, 2023

kylematsuda commented Mar 13, 2023

weihanglo left a comment

kylematsuda commented Mar 13, 2023

weihanglo left a comment

ehuss left a comment

ehuss Mar 15, 2023

ehuss Mar 16, 2023

kylematsuda commented Mar 16, 2023

weihanglo commented Mar 16, 2023

weihanglo commented Mar 17, 2023

bors commented Mar 17, 2023

ehuss commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

		/// This is intended for use in private methods of `Config`,
		/// and does not check for env key case mismatch.

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

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

Conversation

kylematsuda commented Mar 10, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Mar 10, 2023

ehuss commented Mar 10, 2023

ehuss left a comment

Choose a reason for hiding this comment

ehuss Mar 10, 2023

Choose a reason for hiding this comment

kylematsuda Mar 10, 2023

Choose a reason for hiding this comment

kylematsuda commented Mar 10, 2023

weihanglo left a comment

Choose a reason for hiding this comment

kylematsuda commented Mar 10, 2023

kylematsuda commented Mar 13, 2023

weihanglo left a comment

Choose a reason for hiding this comment

kylematsuda commented Mar 13, 2023

weihanglo left a comment

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

ehuss Mar 15, 2023

Choose a reason for hiding this comment

ehuss Mar 16, 2023

Choose a reason for hiding this comment

kylematsuda commented Mar 16, 2023

weihanglo commented Mar 16, 2023

weihanglo commented Mar 17, 2023

bors commented Mar 17, 2023

ehuss commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023

bors commented Mar 17, 2023