When using process::Command on Windows, environment variable names must be case-preserving but case-insensitive

[ChrisDenton]

When using Command to set the environment variables, the key should be compared as uppercase Unicode but when set it should preserve the original case.

Fixes #85242

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[ChrisDenton]

I've moved the test to the right place. Sorry about that.

[CDirkx]

I checked and to_uppercase is not always correct: 'ß'.to_uppercase() == "SS", but the following fails

set_var("ß", "something");
var("SS").unwrap() // panic: Err<NotPresent>

I can't find anything about in official documentation, but from tests I did I suspect Windows uses simple case folding (for context see Unicode Technical Report #21 Case Mappings Section 1.3, simple case folding is the unicode case folding algorithm, but restricted to code points that don't expand under upper/lower casing like ß). I also checked if any normalization or locale-dependent behavior was happening, but that doesn't appear to be the case: é and é (e + acute accent combining character) are different keys and nothing changed around i/I on the Turkish locale (https://en.wikipedia.org/wiki/Dotted_and_dotless_I).

Not sure what the best way to use case folding in std would be (if that is even what Windows is indeed using). There are crates like unicode-rs/rust-caseless implementing case folding, but I couldn't find any that also implements the simple variant.

Edit: Windows does not use Unicode case folding, but instead has a different uppercase implementation: see the comment below.
Edit2: It does appear to be based on reverse unicode folding, but with some unique particularities, see https://github.com/ChrisDenton/Windows-case

[ChrisDenton]

Hm yeah, that is a problem. I think the only way to resolve this properly is to use the OS's own function for this. So in the next commit I've replaced my implementation with a Windows API call and expanded the testing slightly.

[CDirkx]

I looked through the docs some more:

File paths and environment variables should be compared using StringComparison.OrdinalIgnoreCase [1]. Comparisons made using OrdinalIgnoreCase are behaviorally the composition of two calls: calling ToUpperInvariant on both string arguments, and doing a comparison using Ordinal [2]. A comparison using Ordinal is just a comparison based on the numerical value of each utf16 code unit [3].

So the original approach was conceptually correct, but Windows apparently handles converting to uppercase differently than rust (I haven't yet found anywhere how ToUpperInvariant is actually implemented 🙁). I agree with the approach to instead just use the Windows API.

(As an aside, with the relevant Windows API added a future possibility for std would be to expose case insensitive functionality with a std::os extension trait, for example for correctly comparing paths case insensitively.)

[1] https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call
[2] https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#stringtoupper-and-stringtolower
[3] https://docs.microsoft.com/en-us/dotnet/api/system.stringcomparison?view=net-5.0#System_StringComparison_Ordinal

[m-ou-se]

Can you add this conclusion, why CompareStringOrdinal is used, in a comment in the code?

[ChrisDenton]

@m-ou-se I've added a comment (based on CDirkx's) to briefly explain how comparing environment keys works and thus why CompareStringOrdinal is used.

[Mark-Simulacrum]

@rustbot ping windows

It sounds like there's some differences between Rust's case-conversion and the one used by Windows - see this comment #85270 (comment) -- I'm wondering if some windows folks can weigh in on the right thing for us to do here for comparison.

I think it sounds like @ChrisDenton this PR is actually solving two different problems right now, right? We're currently exposing the case-converted (i.e., to_ascii_uppercase) environment variable names through std's APIs, which is loosely the decoupling intended by the first commit here. Then, the second question is whether to_ascii_uppercase is actually the right function to call (sounds like no), and what that function should be.

[rustbot]

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

[ChrisDenton]

To sum up where we are at now, there were two problems with the original code, before this PR:

1. It ASCII uppercased environment variable keys. It should have left them as-is.
2. The uppercasing was (possibly) done so that looking up environment variables by key would be case-insensitive. The problem with this is that Windows uses a variant of Unicode casing rules to do key comparisons, not just ASCII uppercasing.

My first attempt to fix this was to remove the original uppercasing and use Rust's Unicode functions to compare (and only to compare) the strings. As CDirkx has shown, this was wrong because Windows uses what it calls "ordinal" comparisons in this context. This ordinal comparison is documented as testing for "binary equality, not linguistic equality" though I'm a bit fuzzy on what that means for casing.

So now I'm using the Windows API function CompareStringOrdinal to do the comparison, which appears to do what the OS expects.

[ChrisDenton]

Ok so I investigated this further and I'm now even more confident that using the system APIs is the right one. Elsewhere I have figured out the case-folding algorithm for my system, but it's odd and can vary a bit depending on the OS version. Therefore asking the OS to compare strings is, I think, the best approach.

@CDirkx should I squish my commits to make my changes clearer?

[CDirkx]

I was going to comment that this behavior should be documented somewhere on the process::Command docs, but it already is:

Note that environment variable names are case-insensitive (but case-preserving) on Windows, and case-sensitive on all other platforms.

(https://doc.rust-lang.org/std/process/struct.Command.html#method.env)

[CDirkx]

PR looks good to me, @ChrisDenton could you squash the commits?

[CDirkx]

@Mark-Simulacrum this looks ready for review (although I'm not from the Windows group). This PR addresses two problems:

• Env keys on Windows are case-insensitive, but case-preserving. The original implementation stored the make_ascii_uppercased key, making the derived Ord implementation effectively case-insensitive (although not completely correct, see the second point), but not preserving case. This PR stores the key as given, instead adding a manual case-insensitive Ord implementation, thus preserving case.
• The way keys are compared is changed because just uppercasing and comparing is not sufficient, Windows has complex case-folding rules, based on an older version of Unicode case folding, but with additional custom mappings. Instead of re-implementing this functionality in std and possibly getting it wrong, the Windows API CompareStringOrdinal with bIgnoreCase = TRUE is used. This does have some costs however, OsString is a Vec<u8> and Windows API calls expect *const u16. To avoid having to convert (which requires an allocation) every time the BTreeMap might compare keys, both the original OsString and the converted UTF16-encoded Vec<u16> are stored in the key, resulting in a bit more memory use. I don't expect this to be an issue however.

So in conclusion, this PR makes sure that the code actually matches what is already in the documentation, implementing case-preserving and correct case-insensitivity:

Note that environment variable names are case-insensitive (but case-preserving) on Windows, and case-sensitive on all other platforms.

(https://doc.rust-lang.org/std/process/struct.Command.html#method.env)

[Mark-Simulacrum]

r=me on the implementation. This is technically going to break any user expecting the to_uppercase behavior on Windows, right? e.g., previously env("Cargo_incremental", "1") would set CARGO_INCREMENTAL, but now won't. Seems like an unlikely situation but we'll likely want to FCP and relnotes if we land this.

[CDirkx]

I don't think there would really be such an expectation. The docs already stated that on Windows env vars are case-preserving and in general that is what Windows users would expect. I would consider the current implementation a bug, as the to_uppercase doesn't preserve case and thus does not match the documentation.

There is however indeed the small change of breaking changes; the OS will handle "CARGO_INCREMENTAL" and "Cargo_incremental" the same but applications might not.

[Mark-Simulacrum]

I agree that the current implementation is a bug. That doesn't necessarily mean we can 'just' break things for users regardless, though. @rust-lang/libs, would someone care to weigh in here? I don't know if an FCP is in order, but as this is technically a breaking change it seems potentially appropriate.

[bors]

⌛ Testing commit a200c01 with merge 6d06f09a029d58eca4dd36a9381acd50512d2cbe...

[rust-log-analyzer]

The job x86_64-msvc-cargo failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- tool_paths::target_in_environment_contains_lower_case stdout ----
running `D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe build -v --target x86_64-pc-windows-msvc`
thread 'tool_paths::target_in_environment_contains_lower_case' panicked at '
test failed running `D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe build -v --target x86_64-pc-windows-msvc`
error: process exited with code 0 (expected 101)

--- stderr
--- stderr
   Compiling foo v0.0.1 (D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\tmp\cit\t2034\foo)
     Running `rustc --crate-name foo src\main.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 -C metadata=74785c1c2d8cc586 --out-dir D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\tmp\cit\t2034\foo\target\x86_64-pc-windows-msvc\debug\deps --target x86_64-pc-windows-msvc -L dependency=D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\tmp\cit\t2034\foo\target\x86_64-pc-windows-msvc\debug\deps -L dependency=D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\tmp\cit\t2034\foo\target\debug\deps`
', src\tools\cargo\tests\testsuite\tool_paths.rs:381:11


failures:

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

Ok, the failure is being addressed here: rust-lang/cargo#9646

If I'm understanding right the problem was that cargo:

1. It always spawns itself
2. In doing so it sets an environment variable, using env, which caused Rust to upper case all environment variables
3. It had Windows specific tests that assumed lower cased environment variables would always be converted to upper case (which was previously true for cargo itself because of 1 and 2).

If this PR goes ahead I think the release notes should emphasize that, on Windows, using env with Command would previously cause all environment variables to be ASCII-uppercased.

[ehuss]

@bors r=m-ou-se

[bors]

📌 Commit a200c01 has been approved by m-ou-se

[bors]

⌛ Testing commit a200c01 with merge 1540711...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 1540711 to master...