Add discussion that concurrent access to the environment is unsafe

[tbu-]

The bug report #27970 has existed for 8 years, the actual bug dates back to Rust pre-1.0. I documented it since it's in the interest of the user to be aware of it. The note can be removed once #27970 is fixed.

[rustbot]

r? @Mark-Simulacrum

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

[Noratrieb]

this makes it sound like rust will fix the concurrent access problem, but it cannot. it should say that this function will probably become unsafe in the future.

[Mark-Simulacrum]

It also seems to me that get_var has the same problems, right? It's reading data that might be changed by concurrent C code.

[tbu-]

Since getenv is usually considered safe in a multithreaded C environment, but setenv/putenv is not, I do not think that it requires an extra note.

[thomcc]

Yeah, C code designed for multithreaded use will almost never write to the env.

[thomcc]

This is requesting something impossible in most cases. I'm not sure that's reasonable.

[tbu-]

Do you have a suggestion what to write there?

Yes, it's unreasonable, but that's the current state of the function AFAICT.

[tbu-]

@rustbot ready

[tbu-]

@Mark-Simulacrum I don't see any actionable feedback, why is it "waiting-on-author"?

[Mark-Simulacrum]

fixed in a future version of Rust

I think at minimum we need to remove this implication, this isn't directly fixable in a future version of Rust.

I'm going to also nominate for T-libs-api -- it's not clear to me if the framing here meets our current thinking. In particular,

Note that while concurrent access to environment variables ought to be safe in Rust

My suspicion is that in hindsight these functions should have either not existed or not been safe. i.e., "ought to be safe" isn't accurate here -- usually we do not try to paper over platform unsoundness like this, especially if it's impossible (as is the case here).

[Mark-Simulacrum]

(It also seems like this is just extending existing documentation -- i.e., the documentation already calls out this is an underlying unsafe API)

[Amanieu]

We discussed this in the libs-api meeting. What we would like to see happen is that this function gets turned into an unsafe function for the 2024 edition, but on older editions only emits a warning if not inside an unsafe block (possibly using deprecated_safe #94978).

As such the documentation should be written as if this was an unsafe function, clearly explaining the pre-conditions that must be upheld:

• This function is safe to call in single-threaded programs.
• In multi-threaded programs, you must ensure that there are no other threads concurrently reading or writing the environment variables while calling this function through APIs other than the standard library (e.g. C code). You are responsible for figuring out how to achieve this, but we strongly suggest not calling set_var in multi-threaded programs at all.

[ChrisDenton]

In multi-threaded programs, you must ensure that there are no other threads concurrently reading or writing the environment variables while calling this function through APIs other than the standard library (e.g. C code).

It was also brought up that this isn't quite true currently as the rust standard library calls libc functions that may themselves read from the environment. So either those std functions need to be changed to take the environment lock or else we also need to warn against using std functions that may call out to libc.

[tbu-]

As such the documentation should be written as if this was an unsafe function, clearly explaining the pre-conditions that must be upheld: […]

Done.

[Amanieu]

@bors r+ rollup

[bors]

📌 Commit 2093d0c has been approved by Amanieu

It is now in the queue for this repository.

[hkBst]

The suggested wording seems to imply that it is fine to call the functions in this module in parallel if you don't use any other ways to access the environment, presumably because these functions use some way to synchronize their accesses. It would be great if the text was more explicit about whether this is the case or not.

[tbu-]

The suggested wording seems to imply that it is fine to call the functions in this module in parallel if you don't use any other ways to access the environment, presumably because these functions use some way to synchronize their accesses.

You must also ensure that you don't call any Rust standard library function that reads from the environment (e.g. ToSocketAddrs does). It is not documented which Rust standard library function reads from the environment and can change from version to version.

You must also not call into any C libraries that might read from the environment. Most C libraries do not document whether or in which functions they access the environment, and it can usually also change from version to version.

In that way, you could say that your statement is technically correct. Practically speaking, since you cannot even guarantee what the Rust standard library does, you must only modify the environment variables when you're single-threaded.

It would be great if the text was more explicit about whether this is the case or not.

I agree, and this is why I suggested removing the lock guarantees from the documentation: #125937. IMO unfortunately, this seems to be rejected. You could add a comment there if you think it's an improvement.