uucore: centralize unsafe env::set_var/remove_var in a wrapper module

[sylvestre]

No description provided.

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/rm/many-dir-entries-vs-OOM is now being skipped but was previously passing.
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.
Congrats! The gnu test tests/tail/pipe-f is now passing!

[sylvestre]

@oech3 wdyt ? :)

[oech3]

This does not make set_var an actual thread safe function...

[xtqqczze]

This violates Rust's safety model. Safe Rust code must never be able to trigger UB by calling a safe function according to its documented signature. This wrapper makes the function safe to call from anywhere, but does not enforce the required invariants.

[sylvestre]

sure but i don't think it is a big deal. i don't think we have a program dealing with env variable in a parallel context

[oech3]

dd, sort

[oech3]

and external crates

[sylvestre]

i think you are overreacting. afaik, it doesn't introduce safety issues

[sylvestre]

This does not make set_var an actual thread safe function...

of course but it centralized its management

[oech3]

How about adding unsafe { uucore::env::declare_single_thread() }?

[xtqqczze]

How about adding unsafe { uucore::env::declare_single_thread() }?

How would that help? std::env::set_var was made unsafe precisely because there’s no way to guarantee that external C libraries access the environment in a thread-safe manner.

[xtqqczze]

I think we should remove use of these functions instead: #12238

[oech3]

Can we wrap Mutex here?

[xtqqczze]

A mutex doesn't address the soundness issue, see rust-lang/rust#124636 (comment).

cc: @ChrisDenton

[ChrisDenton]

In an application if you can be 100% certain there is only one thread then it is safe to mutate the environment because another function literally can't be called while you're using set_var or remove_var.

If there are, or could be, multiple threads then there would need to be some way to guarantee that no thread (directly or indirectly) accesses the environment. This is practically impossible to uphold since many things can read the environment (e.g. almost any libc function reserves the right to do so). A mutex doesn't really help.

[xtqqczze]

@sylvestre The proposed uucore::env::set_var and uucore::env::remove_var functions are public APIs, so we can’t make guarantees about threading. Once exposed, they need to remain sound under arbitrary usage, including multi-threaded contexts.

[tbu-]

It's a bit weird to "centralize" unsafe functions into a single function not marked as unsafe. It moves the unsafe block away from the place where you need to argue that the usage is actually safe.