Possible data race & use-after-free in std::env::var for unix implementation

[Nekrolm]

Investigating library sources for std::os::env::var/set_var, I have found that the copying of the env variable value is performed not under the lock:

pub fn getenv(k: &OsStr) -> Option<OsString> {
    // environment variables with a nul byte can't be set, so their value is
    // always None as well
    let s = run_with_cstr(k.as_bytes(), |k| {
        let _guard = env_read_lock();
        Ok(unsafe { libc::getenv(k.as_ptr()) } as *const libc::c_char)
       // lock is dropped here
    })
    .ok()?;
    if s.is_null() {
        None
    } else {
        // but access is here
        Some(OsStringExt::from_vec(unsafe { CStr::from_ptr(s) }.to_bytes().to_vec()))
    }
}

So there is a potential data race with setenv.

I tried this code with miri

fn main() {
    let t1 = std::thread::spawn(|| {
        let mut cnt = 0;
        for _ in 0..100000 {
            let var = std::env::var_os("HELLO");
            cnt += var.map(|v| v.len()).unwrap_or_default();
        }
        cnt
    });
    let t2 = std::thread::spawn(|| {
        for i in 0..100000 {
            let value = format!("helloooooooooooooo{i}");
            std::env::set_var("HELLO", &value);
        }
    });

    let cnt = t1.join().expect("ok");
    println!("{cnt}");
    t2.join();
}

And got

error: Undefined Behavior: Data race detected between (1) Read on thread `<unnamed>` and (2) Deallocate on thread `<unnamed>` at alloc3346+0x6. (2) just happened here
   --> /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/os.rs:613:26
    |
613 |             cvt(unsafe { libc::setenv(k.as_ptr(), v.as_ptr(), 1) }).map(drop)
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Read on thread `<unnamed>` and (2) Deallocate on thread `<unnamed>` at alloc3346+0x6. (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/main.rs:30:23
    |
30  |             let var = std::env::var_os("HELLO");
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE (of the first span):
    = note: inside closure at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/os.rs:613:26: 613:65
    = note: inside `std::sys::common::small_c_string::run_with_cstr::<(), [closure@std::sys::unix::os::setenv::{closure#0}::{closure#0}]>` at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/common/small_c_string.rs:43:18: 43:22
    = note: inside closure at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/os.rs:611:9: 614:11
    = note: inside `std::sys::common::small_c_string::run_with_cstr::<(), [closure@std::sys::unix::os::setenv::{closure#0}]>` at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/common/small_c_string.rs:43:18: 43:22
    = note: inside `std::sys::unix::os::setenv` at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/os.rs:610:5: 615:7
    = note: inside `std::env::_set_var` at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/env.rs:347:5: 347:31
    = note: inside `std::env::set_var::<&str, &std::string::String>` at /home/dmis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/env.rs:343:5: 343:43
note: inside closure
   --> src/main.rs:38:13

Have tried to move Some(OsStringExt::from_vec(unsafe { CStr::from_ptr(s) }.to_bytes().to_vec())) under the lock, but still got error with miri.

I was not able to trigger the real crash with it yet.

Meta

rustc --version --verbose:

rustc 1.71.1 (eb26296b5 2023-08-03)
binary: rustc
commit-hash: eb26296b556cef10fb713a38f3d16b9886080f26
commit-date: 2023-08-03
host: x86_64-unknown-linux-gnu
release: 1.71.1
LLVM version: 16.0.5

Bug was introduced by this commit: 86974b8 (PR: #93668)
Before that changes, read was under the lock.

[ShE3py]

@rustbot claim

[the8472]

😬

I think this happened because

• implicit scoping of the lock guard, which makes it easy to shorten its lifetime
• the rwlock isn't coupled to the resource via a lifetime, so it's easy for the extracted pointer to be used longer than the lock is valid

The first one could be made more visible by linting to add an explicit drop(guard) to make its lifetime more explicit in the code when a critical section ends. Especially when guard is otherwise unused which indicates that the lock doesn't directly own the resource it protects.
The second one could be solved by using a lifetime-tagged raw pointer and coupling that lifetime to the guard.

[ChrisDenton]

Side note: even when fixed this assumes all other read/writes acquire Rust's lock and is potentially unsound if they do not. This has often been discussed with a view to making set_var unsafe. Arguably if that happens then the lock would be redundant, although it could also be argued it's still useful to help avoid some footguns even if you can't make it totally safe.

[RalfJung]

Side note: even when fixed this assumes all other read/writes acquire Rust's lock and is potentially unsound if they do not. This has often been discussed with a view to making set_var unsafe. Arguably if that happens then the lock would be redundant, although it could also be argued it's still useful to help avoid some footguns even if you can't make it totally safe.

That's #90308.

[RalfJung]

That PR did not add a test, did it? Would be good to add one in std/tests/env.rs (those are run in Miri regularly). We probably don't want 100000 iterations though, that will take forever. 100 should easily be enough.

[ShE3py]

Something like

#[test] // miri shouldn't detect any data race in this fn
#[cfg_attr(all(target_family = "wasm", not(target_os = "wasi")), ignore)] // monothreaded platforms
fn test_env_get_set_multithreaded() {
    let t1 = std::thread::spawn(|| {
        let mut n = 0;
        
        for _ in 0..100 {
            let var = std::env::var_os("foo");
            n += var.map(|v| v.len()).unwrap_or_default();
        }
        
        n
    });
    
    let t2 = std::thread::spawn(|| {
        for i in 0..100 {
            let value = format!("bar{i:03}");
            std::env::set_var("foo", &value);
        }
    });

    let n = t1.join().unwrap();
    assert!(n % 2 == 0, "the sum of even values should be even");
    t2.join().unwrap();
}

?

[RalfJung]

Yeah something like that. For Miri it's not really necessary to do this "sum of the lengths" computation, not sure why that would be particularly susceptible to a data race.

The one existing concurrency test uses #[cfg_attr(target_os = "emscripten", ignore)], so probably it would make sense to use the same attribute here.