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

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

Closed
saethlin opened this issue Apr 27, 2024 · 5 comments · Fixed by #3523
Closed

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

saethlin opened this issue Apr 27, 2024 · 5 comments · Fixed by #3523
Assignees
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug.

Comments

@saethlin
Copy link
Member

saethlin commented Apr 27, 2024

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:

miri/src/shims/time.rs

Lines 139 to 140 in f26bd28

// Convert that to local time, then return the broken-down time value.
let dt: DateTime<Local> = DateTime::from(dt_utc);

In other words, this test code is ineffective:

// Set timezone to GMT.
let key = "TZ";
env::set_var(key, "GMT");

I'm going to look into this.

cc @tiif @eduardosm

@saethlin saethlin self-assigned this Apr 27, 2024
@saethlin saethlin added C-bug Category: This is a bug. A-shims Area: This affects the external function shims labels Apr 27, 2024
@tiif
Copy link
Contributor

tiif commented Apr 28, 2024

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
Copy link
Member

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
Copy link
Contributor

tiif commented Apr 28, 2024

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
Copy link
Member

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
Copy link
Contributor

tiif commented Apr 28, 2024

This makes sense, thanks!

@bors bors closed this as completed in fc0db10 Apr 29, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this issue May 4, 2024
Use the interpreted program's TZ variable in localtime_r

This requires a bit of wiring and a new dependency, but the tests should correctly pass now regardless of what the host's time zone is.

Fixes rust-lang/miri#3522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants