Skip to content

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

Open
sylvestre wants to merge 1 commit intouutils:mainfrom
sylvestre:set_var
Open

uucore: centralize unsafe env::set_var/remove_var in a wrapper module#12068
sylvestre wants to merge 1 commit intouutils:mainfrom
sylvestre:set_var

Conversation

@sylvestre
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre commented Apr 28, 2026

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 28, 2026 •
edited
Loading

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 sylvestre force-pushed the set_var branch 3 times, most recently from 2b3c43d to 14394df Compare April 28, 2026 21:06
@sylvestre sylvestre marked this pull request as ready for review April 28, 2026 21:20
@sylvestre
Copy link
Copy Markdown
Contributor Author

sylvestre commented Apr 28, 2026

@oech3 wdyt ? :)

@oech3
Copy link
Copy Markdown
Contributor

oech3 commented Apr 28, 2026

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

xtqqczze
xtqqczze suggested changes Apr 29, 2026
View reviewed changes
Comment thread src/uucore/src/lib/mods/env.rs
///
/// Wrapper around [`std::env::set_var`]. See the module documentation for
/// the safety considerations callers must uphold.
pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@sylvestre sylvestre Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@oech3 oech3 Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dd, sort

Copy link
Copy Markdown
Contributor

@oech3 oech3 Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and external crates

Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze Apr 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think violating the safety model is a big deal...

Since we seem to set/remove vars only in tests, could we lock the environment using env-lock crate?.

@sylvestre
Copy link
Copy Markdown
Contributor Author

sylvestre commented Apr 29, 2026

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

of course but it centralized its management

@sylvestre sylvestre requested a review from cakebaker April 30, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cakebaker cakebaker Awaiting requested review from cakebaker

2 more reviewers

@xtqqczze xtqqczze xtqqczze requested changes

@oech3 oech3 oech3 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sylvestre @oech3 @xtqqczze