Change environment variable getters to error recoverably

[inquisitivecrystal]

This PR changes the standard library environment variable getter functions to error recoverably (i.e. not panic) when given an invalid value.

On some platforms, it is invalid for environment variable names to contain '\0' or '=', or for their values to contain '\0'. Currently, the standard library panics when manipulating environment variables with names or values that violate these invariants. However, this behavior doesn't make a lot of sense, at least in the case of getters. If the environment variable is missing, the standard library just returns an error value, rather than panicking. It doesn't make sense to treat the case where the variable is invalid any differently from that. See the internals thread for discussion. Thus, this PR changes the functions to error recoverably in this case as well.

If desired, I could change the functions that manipulate environment variables in other ways as well. I didn't do that here because it wasn't entirely clear what to change them to. Should they error silently or do something else? If someone tells me how to change them, I'm happy to implement the changes.

This fixes #86082, an ICE that arises from the current behavior. It also adds a regression test to make sure the ICE does not occur again in the future.

@rustbot label +T-libs
r? @joshtriplett

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[joshtriplett]

I can see the argument for failing more gracefully. On the other hand, this seems more like an internal assertion, and an entirely avoidable one: it'll only happen if you pass an invalid environment variable name. Silently turning that into a None as though the environment variable simply doesn't exist seems unfortunate, and could miss flagging an actual logic error in someone's code.

On the other other hand, it is in a sense accurate that no variable named varname=morevarname\0yetmorename exists. 🙂

[inquisitivecrystal]

On the other other hand, it is in a sense accurate that no variable named varname=morevarname\0yetmorename exists. 🙂

This would be my view. I wouldn't even say "in a sense". If you look for the variable in the environment, is it there? It wouldn't be. Thus, it's arguably somewhat irrelevant that it couldn't exist.

That said, if you would prefer to keep the library unchanged, that's an entirely fair call. How would you like to proceed on this?

Edit: I just noticed that you nominated it for the libs meeting. I look forward to hearing the result!

[ChrisDenton]

I can see the argument for informing the programmer that a constant key name will always be invalid (i.e. never be set). However a panic at runtime seems very heavy handed considering there's no good way for the application to recover (except for catch_unwind if not aborting on panic).

[inquisitivecrystal]

I can see the argument for informing the programmer that a constant key name will always be invalid (i.e. never be set). However a panic at runtime seems very heavy handed considering there's no good way to recover (except for catch_unwind if not aborting on panic).

Also, what if the end user is the one who provides the environment variable name, like in #86082 (in that case, the end user of rustc)? Then it's not the programmer's fault, and we're making them check specifically for a weird special case that only happens on certain platforms. Worse, there are platforms where this would be valid, so then they have to implement platform specific logic to handle information we already have available.

I don't really think the status quo is tenable. However, one could consider introducing a new API is a better course.

[m-ou-se]

We discussed this yesterday in the libs team meetings.

From an API perspective, we think it's perfectly reasonable to treat things like = or \0 as any other environment variable that doesn't exist, since we are sure that no such environment variable can exist.

For the implementation: Instead of having os_imp::getenv return an io::Error and ignoring it in _var_os, it'd be better to make getenv just return an Option and not a Result. Then it's clear at the implementation of getenv that errors are not going to be handled.

[m-ou-se]

Are there any platforms where this is possible?

[ChrisDenton]

On Windows nulls aren't allowed but it is technically possible for a key to begin with an = (e.g. =key=value). However this is essentially a backwards compatibility hack used only by cmd.exe so as to have hidden variables for internal use. It is not documented and is not meant to be set or used by other code. That said, nothing actually prevents a program from doing so.

[inquisitivecrystal]

There are also some targets where the env is literally a hashmap. Sgx, for example. I'm not sure in what sense it's an environment at that point, but perhaps I'm missing something.

[inquisitivecrystal]

Is there anything I should do about the documentation? I was just rereading it, and it doesn't feel like "errors" or "may error" is the clearest way to put things. I don't really know how to fix improve on that though..

[bors]

☔ The latest upstream changes (presumably #79965) made this pull request unmergeable. Please resolve the merge conflicts.

[inquisitivecrystal]

I fixed up the docs and rebased.

@m-ou-se As far as I know, this is ready to merge.

[m-ou-se]

Thanks!

@bors r+

[bors]

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

[bors]

⌛ Testing commit d9752c7 with merge effea9a...

[lopopolo]

With this change, how does a user of env::var_os distinguish between an env variable name not existing and the underlying platform returning EINVAL?

I'm interested in being able to distinguish this behavior since I'm implementing Ruby's ENV which raises on invalid names:

• https://ruby-doc.org/core-3.0.2/ENV.html#method-c-5B-5D
• https://ruby-doc.org/core-3.0.2/ENV.html#class-ENV-label-Invalid+Names+and+Values

I think in my ideal world, there'd a fallible version of env::var_os that doesn't squash to None or panic. Similarly, a fallible version of env::set_var (which still panics with invalid var names after this PR) would help here too.

With the panicking behavior prior to this PR being merged, I had to pull the EINVAL checking into my own code above the call to std::env::var_os: https://github.com/artichoke/artichoke/blob/daf01d7fa995ab42e21444f3834c49f4563d7301/spinoso-env/src/env/system.rs#L97-L127.

Additionally, I think this PR should be tagged with relnotes.

[lopopolo]

Is it not a breaking change to turn documented panics into something else?

I reported the undocumented panics here #63456 and they were added to the docs as part of the API contract here #63461.

[RalfJung]

Cc @rust-lang/libs-api for the questions raised above -- though this was approved by a team member.

[lopopolo]

Python has a similar behavior to Ruby.

root@e8db2cec4f95:/# python3 -c "import os; os.environ['a'] = 'abc\0xyz'"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python3.8/os.py", line 681, in __setitem__
    self.putenv(key, value)
ValueError: embedded null byte
root@e8db2cec4f95:/# python3 -c "import os; os.environ['a\0b'] = 'e'"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python3.8/os.py", line 681, in __setitem__
    self.putenv(key, value)
ValueError: embedded null byte
root@e8db2cec4f95:/# python3 -c "import os; os.environ['a=b'] = 'e'"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python3.8/os.py", line 681, in __setitem__
    self.putenv(key, value)
ValueError: illegal environment variable name
root@e8db2cec4f95:/# python3 -c "import os; print(os.getenv('a=b'))"
None
root@e8db2cec4f95:/# python3 -c "import os; print(os.getenv('a\0b'))"
None
root@e8db2cec4f95:/# python3 -c "import os; print(os.getenv('a=b'))"
None
root@e8db2cec4f95:/# python3 -c "import os; print(os.environ.get('a\0b'))"
None
root@e8db2cec4f95:/# python3 -c "import os; print(os.environ.get('a=b'))"
None

root@e8db2cec4f95:/# ruby -e 'puts ENV["x\0y"]'
Traceback (most recent call last):
	1: from -e:1:in `<main>'
-e:1:in `[]': bad environment variable name: contains null byte (ArgumentError)
root@e8db2cec4f95:/# ruby -e 'puts ENV["x=y"]'

root@e8db2cec4f95:/# ruby -e 'ENV["x\0y"] = "a"'
Traceback (most recent call last):
	1: from -e:1:in `<main>'
-e:1:in `[]=': bad environment variable name: contains null byte (ArgumentError)
root@e8db2cec4f95:/# ruby -e 'ENV["x=y"] = "a"'
Traceback (most recent call last):
	1: from -e:1:in `<main>'
-e:1:in `[]=': Invalid argument - setenv(x=y) (Errno::EINVAL)
root@e8db2cec4f95:/# ruby -e 'ENV["xy"] = "a\0z"'
Traceback (most recent call last):
	1: from -e:1:in `<main>'
-e:1:in `[]=': bad environment variable value: contains null byte (ArgumentError)

How would one implement this API with the existing APIs in std? I know it's not a goal for std to replicate the APIs of other programming languages, but I expect Rust as a systems language to expose the details and limitations of the target platform when I need it to.

For APIs in std that call into libc functions that set errno, I would hope to have the option to use a fallible version. For APIs in std that call into libc functions that have different preconditions on their inputs than encoded in OsStr, I would hope to have the option to use a fallible version.

The previous behavior with panics was not ideal, but it did allow callers to observe failures and recover from them with catch_unwind.

[inquisitivecrystal]

Is it not a breaking change to turn documented panics into something else?

I'm definitely not an expert, but I would think that making something that didn't work start working would be allowed. Of course, if it broke a lot of people's use cases, it's somewhat irrelevant...

[lopopolo]

This change alters the semantics of the env::var_os API to mask a previously documented and observable panic due to a limitation of the underlying platform. This may be the right change to make for env::var_os, but without a replacement API, maybe env::try_var_os or env::var_os_for_key, less information is available to callers than before.

[joshtriplett]

We reviewed this in today's @rust-lang/libs-api meeting.

We appreciate the specific use case of needing to provide more nuanced validation for the purposes of implementing another API. However, we feel like the changes previously made here will be what the majority of users will expect; in particular, it looks like from the implementation of Artichoke that you're actually using the same semantic for keys containing = (returning the same result as if they didn't exist). Given that, we generally felt like doing this kind of validation separately in advance (or afterward iff the result is None) seems reasonable.

That said, we'd also be sympathetic to improving the convenience of working with OsStr (cc @dtolnay). And if there's an additional semantic here that's OS-specific (rather than something reasonably easy to check for all platforms such as containing = or \0), and a try-ish method would actually provide meaningful value, we'd be willing to consider such a method.

[lopopolo]

Thanks @joshtriplett and Rust Libs API team for taking a look. I do agree that the changes made here better serve most users and what I'm hoping to do is a bit niche.

I'd be super excited to see improvements for the ergonomics of OsStr. I think I've seen an idea for getting something like the pattern API implemented for OsStr on internals a couple of times. Being able to do contains checks (like for this env var name and value case) and starts_with checks (like for checking for - and -- flags and switches in parsing command line arguments) would go a long way to avoiding round tripping through &str or &[u8].