Use saturating_cast for env key length

[ChrisDenton]

Fixes #160893

I considered looping over CompareStringOrdinal but on balance I don't think the extra complexity is worth doing for something that isn't going to happen in practice. I cannot fathom a reason for wanting a >2 GiB environment key but even if that was wanted, it isn't supported in practical testing. While the maximum supported size of an environment variable key is not explicitly documented, the size of a variable is and it's not unreasonable to assume:

The maximum size of a user-defined environment variable is 32,767 characters

Which is i16::MAX. Although I would note "supported" and "works" are not necessarily equivalent.

I have added some debug_asserts and an explicit error during spawn, which is the only place we can error given the Command::env API.

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

[hanna-kruppe]

The general direction makes sense to me, but I have some questions about details.

r? me @rustbot author

View changes since this review

[hanna-kruppe]

I think this fast path is not quite correct any more. If self and other have different lengths > 2 GiB but the first 2 GiB are equal, then PartialOrd<str> would say they're Ordering::Equal but PartialEq<str> would say they're not equal, right?

edit: i32::MAX u16s, not 2 GiB

I'm not sure if this actually matters. What is this impl used for?

[hanna-kruppe]

Now that I think about it, the shortcut seems non-obvious even for strings of reasonable length. It seems to assume that the case folding done by the actual comparison will not change the UTF-8 encoded lengths. As a made-up example, if U+00DF ß (two bytes in UTF-8) was case-folded to U+0053 S (one byte), then the shortcut would be wrong. It seems plausible to me that this isn't an issue with the tables Windows actually uses, but it seems notable that we make this assumption when we otherwise try to be agnostic of the mapping other than it being purely u16 -> u16.

[ChrisDenton]

Hm, looking into it there are a few cases where UTF-8 lengths differ before/after uppercasing.

From	len	To	len
ɐ	2	Ɐ	3
ɑ	2	Ɑ	3
ɫ	2	Ɫ	3
ɱ	2	Ɱ	3
ɽ	2	Ɽ	3
ⱥ	3	Ⱥ	2
ⱦ	3	Ⱦ	2

The UTF-16 lengths never change though since the upper casing used only works on the basic plane for historic reasons (i.e. UCS-2).

To give some context here, Windows environment variables are case insensitive but case preserving. The variables are encoded into an environment block which must be sorted by the lexical order of keys after uppercasing. But the keys themselves preserve the original case when set or gotten.

Rust currently stores a BTreeMap of key/values so that users can retrieve those they've set. We could instead store exactly what the user gave us and do the re-encoding and sorting only at spawn time. The only user-visible effect will be for get_envs. But maybe that's ok because it's giving people back what they set.

[hanna-kruppe]

Thanks for the context, but I'm not considering any sweeping change to how we store the environment variables, just wondering about this correctness of the Partial{Eq,Ord}<str> impls. From what you said, I think they're plainly breaking the normal contract of those traits. PartialEq and PartialOrd must agree on whether values are equal (a == b if and only if a.partial_cmp(b) == Some(Equal)), but according to that table:

let a = EnvKey::new("ɐ");
let b = "Ɐ";
dbg!(a == b); // => false because of OsStr length comparison
dbg!(a.partial_cmp(b))); // => Some(Equal) because of case folding

I don't know if this is reachable from stable APIs. I don't think so because both inserting envs and querying via get_envs switch to OsStr immediately before hitting std::sys. But it seems dubious even for internal usage. What even is the internal usage? Would anything get slower if we just removed this fast path? Or is it only used with e.g. hard-coded ASCII &strs where case folding doesn't cause any problems?

[ChrisDenton]

The only place I can see it being compared to a str is here:

rust/library/std/src/sys/process/env.rs

Lines 79 to 80 in 6bb812b

	fn maybe_saw_path(&mut self, key: &EnvKey) {
	if !self.saw_path && key == "PATH" {

So, yeah, it doesn't particularly matter for non-ascii. That code is shared between platforms but maybe we could debug_assert that the key is ascii and just do an ascii case-insensitive compare here.

[hanna-kruppe]

A quick ./x check --target x86_64-pc-windows-msvc after removing both PartialEq<str> and PartialOrd<str> suggests this is only used in one place, for detecting when PATH is set. That seems harmless w.r.t. case folding, and the fast path is indeed correct, but it also doesn't seem like great justification for having such a subtle, technically incorrect impl. Probably out of scope for this PR, but an OS-dependent is_path predicate seems better.

[ChrisDenton]

Oh I think our messages raced. In either case I'm happy to leave addressing this to a follow up PR.

[hanna-kruppe]

Hm, I think there's something non-obvious going on here. A key of exactly i32::MAX u16s can be equal to a longer key, so if the CommandEnv ends up with an exactly-i32::MAX key (that replaced an even longer key), the error in make_envp won't fire, right? But perhaps it doesn't need to, as i32::MAX length here is 4 GiB of data, so an OS error is guaranteed anyway? But then why do we need the new error return in make_envp?

[ChrisDenton]

We don't really need to return an error I guess. I just thought it would be worth making it explicit in the code that we don't support such long keys. If we do defer the whole logic to spawn time then I think this becomes a lot simpler.

[hanna-kruppe]

Not sure what exactly you mean by "defer the whole logic to spawn time". I think mostly I was confused by two things here:

• Whether there is an actual hard limit on the size of env variables enforced by CreateProcessW or whether it's up to us to enforce this at spawn time.
• What exactly that limit is, in particular due to unit confusion between "2 GiB" and utf16.len() == i32::MAX (4 GiB)

[ChrisDenton]

I've update the PR to debug_assert on key creation which is hopefully more accurate to the intent. I don't know if that's more clear. I cannot get a process to spawn using such large keys but it's possible I'm hitting physical memory limitations. In any case, whether it works or not, I feel confident it's not a supported scenario given https://learn.microsoft.com/en-us/windows/win32/procthread/environment-variables states that:

The maximum size of a user-defined environment variable is 32,767 characters.

[hanna-kruppe]

I'm fine with assuming such long environment variables will be rejected by CreateProcessW (even if I'd love to have official confirmation). But I'm not sure how I feel about the debug_asserts. If the intent is that we want to treat excessively long env keys as breaking a (possibly implicit) precondition, i.e., it's a bug in the caller, then assert is fine. If asserting that is too expensive and the odds of someone doing this is too low to bother, then debug assertion may be fine. But it's not clear to me whether we should treat it as a bug or as invalid input that should get an Err return one way or another.

[rust-log-analyzer]

The job pr-check-1 failed! Check out the build log: (web) (plain enhanced) (plain)

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

#15 2.997 Building wheels for collected packages: reuse
#15 2.998   Building wheel for reuse (pyproject.toml): started
#15 3.221   Building wheel for reuse (pyproject.toml): finished with status 'done'
#15 3.222   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132857 sha256=6a78a69541e3df9c7c106367722bab90241125ee6b7cdb81c5e40637268f85a9
#15 3.222   Stored in directory: /tmp/pip-ephem-wheel-cache-jb0ebsxr/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#15 3.225 Successfully built reuse
#15 3.225 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#15 3.629 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#15 3.629 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#15 DONE 3.7s
---
[RUSTC-TIMING] object test:false 8.421
error[E0424]: cannot find value `self` in this scope
   --> library/std/src/sys/process/windows.rs:132:23
    |
128 |     fn from(k: OsString) -> Self {
    |        ---- this function doesn't have a `self` parameter
...
132 |         debug_assert!(self.utf16.len() <= i32::MAX as usize);
    |                       ^^^^ `self` value is a keyword only available in methods with a `self` parameter
    |
    = note: a module named `self` exists in another namespace
help: add a `self` receiver parameter to make the associated `fn` a method
    |
128 |     fn from(&self, k: OsString) -> Self {