Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set_var/remove_var are unsound in combination with C code that reads the environment #27970

Closed
bluss opened this issue Aug 23, 2015 · 133 comments · Fixed by #124636
Closed

set_var/remove_var are unsound in combination with C code that reads the environment #27970

bluss opened this issue Aug 23, 2015 · 133 comments · Fixed by #124636
Labels
A-concurrency Area: Concurrency C-bug Category: This is a bug. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-unix Operating system: Unix-like P-medium Medium priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@bluss
Copy link
Member

bluss commented Aug 23, 2015

Like the documentation for our set_var (setenv) says, care must be taken with mutating environment variables in a multithreaded program. See this glibc bug #13271 that says getaddrinfo may call getenv.

It looks like we have an unsynchronized call to getaddrinfo and this may cause trouble with glibc/Linux.

Seeing glibc's attitude to setenv in multithreaded programs, set_var seems like a big hazard in general(?).

Discovered as an issue tangential to #27966

cc @alexcrichton

@bluss
Copy link
Member Author

bluss commented Aug 23, 2015

cc @luqmana

@alexcrichton
Copy link
Member

If we did take this route I'd want to convert the environment lock to a rwlock to ensure that we could at least have parallel dns queries. Also cc #27705.

@bluss
Copy link
Member Author

bluss commented Aug 24, 2015

Yeah, either way the situation is tricky. We get tangled up in glibc internals.

@alexcrichton
Copy link
Member

I found the wording on this page also particularly interesting:

       env    Functions marked with env as an MT-Safety issue access the
              environment with getenv(3) or similar, without any guards to
              ensure safety in the presence of concurrent modifications.

              We do not mark these functions as MT-Unsafe, however, because
              functions that modify the environment are all marked with
              const:env and regarded as unsafe.  Being unsafe, the latter
              are not to be called when multiple threads are running or
              asynchronous signals are enabled, and so the environment can
              be considered effectively constant in these contexts, which
              makes the former safe.

The getaddrinfo function is indeed marked with this tag

@fweimer
Copy link

fweimer commented Dec 3, 2016

std::env::set_var is just not safe to use in a multi-threaded program, as I commented on issue #24741:

Even without low-level races, std::env::set_var is not quite safe because you can change a variable (such as TZ), do something, and change it back to the original value, without impacting something else in the process which runs at the same time.

This problem would not go away if we provided thread-safe environment access at the libc layer. Therefore, I'm surprised the function isn't marked unsafe.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: as far as I know, this hasn't changed at all. The docs do describe this possible footgun.

@smmalis37
Copy link
Contributor

This issue can also apply to any external crate that calls into something that calls get_env, for example time-rs/time#293 and chronotope/chrono#499. Maybe some way for these crates to observe the lock is called for?

@RalfJung
Copy link
Member

Seems to me like this is a soundness issue, and should be marked I-unsound?

@Mark-Simulacrum Mark-Simulacrum added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Nov 12, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 12, 2020
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 12, 2020
@spastorino
Copy link
Member

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@spastorino spastorino added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 18, 2020
@jhpratt
Copy link
Member

jhpratt commented Nov 19, 2020

Just repeating what I've said on Zulip: To resolve the soundness bug in time & chrono, one option would be to provide a std::env::lock method, which holds a lock on the environment until dropped. It's my understanding this is basically what is happening internally on some functions already in stdlib. Exposing this would allow much safer usage of FFI.

@RalfJung
Copy link
Member

The env lock has to be exposed somehow, I agree.

An alternative API would be a function that takes a closure that will be called with the lock held. This is less powerful (which might be a good thing: provide the least powerful API that is good enough).

@RalfJung
Copy link
Member

However, talking of the env lock -- also see #64718 for how the current use of the env lock is wrong, so exposing it in that state might be a problem.

@jhpratt
Copy link
Member

jhpratt commented Nov 19, 2020

Sure, that would work as well. End result is the same, if done properly.

@DemiMarie
Copy link
Contributor

Honestly, I don’t believe std::env::set_var can be made safe.

In a pure-Rust program, it is already safe, but in C, setenv is considered thread-unsafe. Therefore, C libraries assume that they can call getenv at any time. Furthermore, adding a call to getenv is probably not considered worthy of a major version bump in the C library, so what was previously a safe call into that library can become unsafe without warning!

Even without the thread safety problems, there are other reasons that std::env::set_var is unsafe. Libraries such as gtk will load plugins from a path specified in an environment variable, and loading a shared library is inherently unsafe. If the shared library is in the system shared library directory, it can be assumed safe, since the OS is considered trusted. But it isn’t safe to (say) load a library from the Downloads folder (which could contain anything).

What might be safe is a function that fails if multiple threads are running, and which only allows setting a limited number of env vars, such as the timezone.

tbu- added a commit to tbu-/rust that referenced this issue May 2, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
tbu- added a commit to tbu-/rust that referenced this issue May 3, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
bors added a commit to rust-lang-ci/rust that referenced this issue May 4, 2024
Make `std::env::{set_var, remove_var}` unsafe in edition 2024

Allow calling these functions without `unsafe` blocks in editions up until 2021, but don't trigger the `unused_unsafe` lint for `unsafe` blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
tbu- added a commit to tbu-/rust that referenced this issue May 13, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
tbu- added a commit to tbu-/rust that referenced this issue May 13, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
tbu- added a commit to tbu-/rust that referenced this issue May 24, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
tbu- added a commit to tbu-/rust that referenced this issue May 24, 2024
Allow calling these functions without `unsafe` blocks in editions up
until 2021, but don't trigger the `unused_unsafe` lint for `unsafe`
blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
fmease added a commit to fmease/rust that referenced this issue May 30, 2024
Make `std::env::{set_var, remove_var}` unsafe in edition 2024

Allow calling these functions without `unsafe` blocks in editions up until 2021, but don't trigger the `unused_unsafe` lint for `unsafe` blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
bors added a commit to rust-lang-ci/rust that referenced this issue May 30, 2024
Make `std::env::{set_var, remove_var}` unsafe in edition 2024

Allow calling these functions without `unsafe` blocks in editions up until 2021, but don't trigger the `unused_unsafe` lint for `unsafe` blocks containing these functions.

Fixes rust-lang#27970.
Fixes rust-lang#90308.
CC rust-lang#124866.
@bors bors closed this as completed in 5d8f9b4 May 30, 2024
@fweimer-rh
Copy link

I've posted a patch that should fix this for glibc:

Reviews would be appreciated. It does not solve the conceptual problem that the environment is a process-global resource. Other threads still observe temporary environment changes intended for the current thread only. Just the crashes and missed variable lookups are gone.

@briansmith
Copy link
Contributor

I've posted a patch that should fix this for glibc:

Just to clarify, your patch doesn't fix it "enough" for Rust to consider them to set_var/remove_var to be safe, because C code is still allowed to read directly from environ and do other things that aren't protected by your improvements. That doesn't mean your improvement isn't welcome though.

@richfelker
Copy link

I don't see how this provides any significant thread-safety. The core unsafety of modifying the environment is that the lifetime of a string returned by getenv can end asynchronously if another thread changes the environment. The only way to avoid this is to leak everything ever added.

@tbu-
Copy link
Contributor

tbu- commented Jun 29, 2024

Another way is for getenv to put the result into a thread-local variable. This is compatible with POSIX's guarantees, but probably not in line with glibc's backward compatibility guarantees.

@RalfJung
Copy link
Member

From the patch:

I think this achieves full thread safety and async-signal-safety for
getenv if the environment is only updated using setenv, unsetenv,
clearenv. If putenv is used, it depends on how the passed string is
updated afterwards. Likewise for direct environ writes. This is fairly
easy to accomplish because the current implementation never frees any
variables set using setenv.

So, seems to me like setenv is already leaking, and this patch builds on that.

Very nice to see progress on this!

@VorpalBlade
Copy link

Even if that patch is accepted in glibc, POSIX doesn't guarantee thread safety, so this cannot change the decision for Rust (unless you get a change into musl, all the *BSDs, OpenSolaris, Darwin/Mac OS X etc, as well as the POSIX standard itself). And I don't foresee that happening due to backward compatibility around environ as well as the issue mentioned with putenv.

Still, it is a step in the right direction.

@tmccombs
Copy link
Contributor

Well, if it weren't for direct access to environ and the putenv issue, there could possibly be a safe function that is only available on platforms that use a new enough version of glibc.

@tbu-
Copy link
Contributor

tbu- commented Jul 1, 2024

Even if that patch is accepted in glibc, POSIX doesn't guarantee thread safety, so this cannot change the decision for Rust (unless you get a change into musl, all the *BSDs, OpenSolaris, Darwin/Mac OS X etc, as well as the POSIX standard itself).

Note that Rust doesn't strictly adhere to POSIX, Rust is fine with stuff that all the platforms implement. E.g. pthread_t is assumed to be an integer even though POSIX explicitly says that it might not be.

I haven't looked into whether musl, *BSDs or Darwin implement synchronization for environment variables. I just wanted to say that POSIX itself might not be a requirement.

@tmccombs
Copy link
Contributor

tmccombs commented Jul 1, 2024

What if another thread reads environ directly? Would that still be undefined behavior unless setenv uses an atomic operation to swap the pointer, and the reader uses an atomic load to read environ?

@fweimer-rh
Copy link

What if another thread reads environ directly? Would that still be undefined behavior unless setenv uses an atomic operation to swap the pointer, and the reader uses an atomic load to read environ?

@tmccombs Is this a question about the proposed new glibc implementation? It's not possible to ensure a proper snapshot with just atomic access to environ. Even if individual accesses never fault, it's impossible to know whether you have actually observed all currently active environment variables. Concurrent unsetenv calls may have shuffled around the array while you were reading it. I assume that this question is related to the implementation of std::env:vars(). Realistically, this will a new interface. For glibc's internal use in execve, posix_spawn etc., we can add something that treats environ differently so that we can obtain a snapshot similar to getenv, but exposing that externally as-is would constraint the implementation too much.

@tmccombs
Copy link
Contributor

tmccombs commented Jul 1, 2024

My point is that even if a c library reads from environ directly, then no amount of synchronization around setenv in rust would be safe in a multi-threaded program.

ruben-arts pushed a commit to prefix-dev/pixi that referenced this issue Jul 8, 2024
Fix for flaky task test, not using `remove_var` because of:
https://doc.rust-lang.org/std/env/fn.remove_var.html


> Even though this function is currently not marked as unsafe, it needs
to be because invoking it can cause undefined behaviour. The function
will be marked unsafe in a future version of Rust. This is tracked in
[rust#27970](rust-lang/rust#27970).

> This function is safe to call in a single-threaded program.

> In multi-threaded programs, you must ensure that are no other threads
concurrently writing or reading(!) from the environment through
functions other than the ones in this module. You are responsible for
figuring out how to achieve this, but we strongly suggest not using
set_var or remove_var in multi-threaded programs at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency C-bug Category: This is a bug. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-unix Operating system: Unix-like P-medium Medium priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.