rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.17k stars 320 forks source link

localtime_r shim uses TZ variable from the host, not the interpreted program #3522

Closed saethlin closed 2 months ago

saethlin commented 2 months ago

Currently the test suite may fail to pass, unless you run it with TZ=UTC ./miri test. This is because the localtime_r shim reads the TZ variable from the host on this line: https://github.com/rust-lang/miri/blob/f26bd28c4d0e96a3b855b7f544030ee593b3090a/src/shims/time.rs#L139-L140

In other words, this test code is ineffective: https://github.com/rust-lang/miri/blob/f26bd28c4d0e96a3b855b7f544030ee593b3090a/tests/pass-dep/shims/libc-misc.rs#L220-L222

I'm going to look into this.

cc @tiif @eduardosm

tiif commented 2 months ago

Thanks for looking into this, and sorry for causing this. I ran into this issue while testing and discussed with @oli-obk before, but I thought there is something wrong with my machine because it passed the CI and env::set_var works when I isolated it out in a test.

This is how the test looks like: In tests/pass-dep/shims/libc-misc.rs

fn test_tz_var_setting() {
    use chrono::Local;
    use std::env;

    let key = "TZ";
    env::set_var(key, "GMT");
    assert_eq!(env::var(key), Ok("GMT".to_string()));
    let offset_in_second = Local::now().offset().local_minus_utc();
    // If the timezone is GMT, tm_gmtoff will be 0
    let tm_gmtoff = offset_in_second;
    assert_eq!(tm_gmtoff, 0);
    env::remove_var(key);
    assert!(env::var(key).is_err());
}

In test_dependencies/Cargo.toml, add chrono to dependencies:

[dependencies]
# all dependencies (and their transitive ones) listed here can be used in `tests/`.
libc = "0.2"
num_cpus = "1.10.1"
tempfile = "3"
chrono = "0.4.37"

This test still pass after I merged the latest update from master (commit f26bd28c).

RalfJung commented 2 months ago

This is how the test looks like:

That's using chrono::Local inside the program, so when it does env::get it sees the variables of the interpreted program.

However, the shim uses chrono::Local on the host, so that will use the host's env::get instead of the one of the interpreted program.

tiif commented 2 months ago

That's using chrono::Local inside the program, so when it does env::get it sees the variables of the interpreted program.

I see, so env::set_var in test in fact can't change the TZ variable in the host?

RalfJung commented 2 months ago

No, it's impossible to change the host environment from the interpreted program.

Mutating the environment is unsafe so it would be rather bad to let the interpreted program do that. ;)

tiif commented 2 months ago

This makes sense, thanks!