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

cargo miri test -- -Zmiri-disable-isolation fails #933

Closed
RalfJung opened this issue Aug 28, 2019 · 9 comments
Closed

cargo miri test -- -Zmiri-disable-isolation fails #933

RalfJung opened this issue Aug 28, 2019 · 9 comments
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

Ever since we got full env var forwarding (Cc @christianpoveda), running cargo miri test -- -Zmiri-disable-isolation fails with something like:

error[E0080]: Miri evaluation error: can't call foreign function: stat64
   --> /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys/unix/fs.rs:803:9
    |
803 |         stat64(p.as_ptr(), &mut stat)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: can't call foreign function: stat64
    |
    = note: inside call to `std::sys::unix::fs::stat` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/fs.rs:1516:5
    = note: inside call to `std::fs::metadata::<&std::path::PathBuf>` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libterm/terminfo/searcher.rs:50:12
    = note: inside call to `term::terminfo::searcher::get_dbpath_for_term` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:87:9
    = note: inside call to `term::terminfo::TermInfo::from_name` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:73:25
    = note: inside call to `term::terminfo::TermInfo::from_env` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:232:9
    = note: inside call to `term::terminfo::TerminfoTerminal::<std::io::Stdout>::new` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libterm/lib.rs:61:5
    = note: inside call to `term::stdout` at /home/r/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:921:24

Looks like the presence of some env vars makes terminfo do something that we don't support yet.

Thanks @elichai for the original report!

@RalfJung
Copy link
Member Author

Specifically, if TERM is set, it starts to traverse the file system for information about that kind of terminal. That code looks like it needs a significant fraction of #923 to work.

As a hack, we could blacklist TERM from the env var import that we do when isolation is disabled...

@RalfJung RalfJung added A-shims Area: This affects the external function shims C-bug Category: This is a bug. labels Aug 28, 2019
@pvdrz
Copy link
Contributor

pvdrz commented Aug 28, 2019

Say no more, time to implement #923 :P I can blacklist TERM and add a FIXME so we can remove it later. I could add an arguments with the blacklisted vars to EnvVars::init

Edit: We could add a -Zmiri-blacklisted-env-vars=<names> flag so we don't have to do a commit for every single variable.

@RalfJung
Copy link
Member Author

I could add an arguments with the blacklisted vars to EnvVars::init

I'd just hard-code the blacklist in env.rs.

We could add a -Zmiri-blacklisted-env-vars= flag so we don't have to do a commit for every single variable.

Well, we still want a default that "makes stuff work". So that doesn't really save us from maintaining a list.
But indeed having a user-controlled whitelist or blacklist for the "exposed environment" is a feature that got requested before.

@pvdrz
Copy link
Contributor

pvdrz commented Aug 28, 2019

Well if you're ok with it. Give me a less clumsy name and I'll do the rest :P

@RalfJung
Copy link
Member Author

-Zmiri-env-exclude? Not sure what it should use as separator though. Maybe just passing the flag multiple times.

bors added a commit that referenced this issue Aug 29, 2019
Exclude environment variables from host communication

related issue: #933

r? @RalfJung
@RalfJung
Copy link
Member Author

RalfJung commented Aug 29, 2019

@christianpoveda did the first step, thanks. :)

But I think we will want to add a test to test-cargo-miri/run-test.py:

def test_cargo_miri_test():
    test("cargo miri test",
        cargo_miri("test") + ["--", "-Zmiri-seed=feed"],
        "test.stdout.ref", "test.stderr.ref"
    )
    test("cargo miri test (with filter)",
        cargo_miri("test") + ["--", "--", "le1"],
        "test.stdout.ref2", "test.stderr.ref"
    )
    test("cargo miri test (without isolation)",
        cargo_miri("test") + ["--", "-Zmiri-disable-isolation", "--", "num_cpus"],
        "test.stdout.ref3", "test.stderr.ref"
    )

And this will still currently fail.
Basically, if the exclusion list is empty, TERM should be added to it. Or maybe it should be always added? Something like that.

bors added a commit that referenced this issue Aug 29, 2019
Exclude environment variables from host communication

related issue: #933

r? @RalfJung
bors added a commit that referenced this issue Aug 29, 2019
Fix cargo-miri with disabled isolation

Related issue: #933
I'm not sure if that's the better place to blacklist `TERM`, @RalfJung let me know if there is a less hacky place to put it.
@pvdrz
Copy link
Contributor

pvdrz commented Aug 29, 2019

Hopefully #937 should fix this :)

@RalfJung
Copy link
Member Author

And with rust-lang/rust#64001 that PR will be in a rustup component near you soon. :)

bors added a commit to rust-lang/rust that referenced this issue Sep 2, 2019
@RalfJung
Copy link
Member Author

Looks like we forgot to close this.

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.

2 participants