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

Cannot list environment variables #756

Closed
shepmaster opened this issue May 31, 2019 · 14 comments · Fixed by #1208
Closed

Cannot list environment variables #756

shepmaster opened this issue May 31, 2019 · 14 comments · Fixed by #1208
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@shepmaster
Copy link
Member

Cargo.toml

[package]
name = "repro"
version = "0.1.0"
authors = ["An Devloper <[email protected]>"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

src/main.rs

fn main() {
    std::env::vars_os();
}

Execution

$ cargo +nightly-2019-05-27 run
   Compiling repro v0.1.0 (/private/tmp/repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.25s
     Running `target/debug/repro`

$ cargo +nightly-2019-05-27 miri
   Compiling repro v0.1.0 (/private/tmp/repro)
error[E0080]: Miri evaluation error: can't call foreign function: _NSGetEnviron
   --> /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/sys/unix/os.rs:405:5
    |
405 |     _NSGetEnviron()
    |     ^^^^^^^^^^^^^^^ Miri evaluation error: can't call foreign function: _NSGetEnviron
    |
    = note: inside call to `std::sys::unix::os::environ` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/sys/unix/os.rs:426:28
    = note: inside call to `std::sys::unix::os::env` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/env.rs:143:21
note: inside call to `std::env::vars_os` at src/main.rs:2:5
   --> src/main.rs:2:5
    |
2   |     std::env::vars_os();
    |     ^^^^^^^^^^^^^^^^^^^
    = note: inside call to `main` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/rt.rs:64:34
    = note: inside call to closure at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/rt.rs:52:53
    = note: inside call to closure at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/panicking.rs:293:40
    = note: inside call to `std::panicking::try::do_call::<[closure@DefId(1:5938 ~ std[29d7]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/panicking.rs:289:5
    = note: inside call to `std::panicking::try::<i32, [closure@DefId(1:5938 ~ std[29d7]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/panic.rs:388:9
    = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:5938 ~ std[29d7]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/rt.rs:52:25
    = note: inside call to `std::rt::lang_start_internal` at /Users/shep/.rustup/toolchains/nightly-2019-05-27-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/rt.rs:64:5
    = note: inside call to `std::rt::lang_start::<()>`
@RalfJung RalfJung added A-mac Area: Affects only macOS targets A-shims Area: This affects the external function shims C-bug Category: This is a bug. labels May 31, 2019
@RalfJung
Copy link
Member

This actually fails on Linux as well.

error[E0080]: Miri evaluation error: can't access foreign static: environ
   --> /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys/unix/os.rs:411:5
    |
411 |     &mut environ
    |     ^^^^^^^^^^^^ Miri evaluation error: can't access foreign static: environ
    |
    = note: inside call to `std::sys::unix::os::environ` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys/unix/os.rs:426:28
    = note: inside call to `std::sys::unix::os::env` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/env.rs:143:21
note: inside call to `std::env::vars_os` at src/main.rs:2:5
   --> src/main.rs:2:5
    |
2   |     std::env::vars_os();
    |     ^^^^^^^^^^^^^^^^^^^
    = note: inside call to `main` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:64:34
    = note: inside call to closure at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:53
    = note: inside call to closure at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:293:40
    = note: inside call to `std::panicking::try::do_call::<[closure@DefId(1:5870 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:289:5
    = note: inside call to `std::panicking::try::<i32, [closure@DefId(1:5870 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:388:9
    = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:5870 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:25
    = note: inside call to `std::rt::lang_start_internal` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:64:5
    = note: inside call to `std::rt::lang_start::<()>`

@RalfJung RalfJung removed the A-mac Area: Affects only macOS targets label May 31, 2019
@RalfJung RalfJung changed the title Cannot access environment variables on macOS: can't call foreign function: _NSGetEnviron Cannot list environment variables May 31, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

I remember not implementing these. We essentially need to manually create an allocation for our env vars instead of having the current datastructure. This means we have to start emulating all of these platforms' env datastructure

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement and removed C-bug Category: This is a bug. labels Jun 28, 2019
@pvdrz
Copy link
Contributor

pvdrz commented Aug 7, 2019

Based on the work already done in #894 I could implement this for linux at least and also make it work with communication enabled.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2019

This is much harder though if I recall. With this, the interpreted code iterates over some data structure we provide to it, so we have to provide them in the right (platform-specific) format.

I haven't looked into what this takes, but it will very likely be different for macOS, Windows and Linux. If you want to give it a shot, feel free to! Having it for Linux only would be better than not having it at all, I guess, though platform differences are also annoying.

@pvdrz
Copy link
Contributor

pvdrz commented Aug 7, 2019

I'm going to start working based on the linux's man entry for environ.

Edit: in a fresh new branch with a different PR :D

@pvdrz
Copy link
Contributor

pvdrz commented Aug 7, 2019

Apparently rust just calls _NSGetEnviron in OSX and environ in other unix platforms but both are handled in the same way. I actually have a mac to test it so I'm going to try to produce the correct *mut *const *const c_char so everything works accordingly.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2019

Please really keep it a separate PR this time though. :)

Review complexity scales superlinearly.

@pvdrz
Copy link
Contributor

pvdrz commented Aug 7, 2019

Hahaha yeah, lesson learned :)

@pvdrz
Copy link
Contributor

pvdrz commented Aug 8, 2019

After thinking this during the day I think is better to wait for #894 to land first. The utility function defined in shims::env might be used heavily.

I have a question about how environ is actually accessed. Following the trace, I can see that Memory::get_static_alloc is called because in Memory::get the AllocId for environ does not exist inside Memory.alloc_map. Is this supposed to be the case? or when does the AllocId for environ is decided? It looks like miri is looking for an specific AllocId.

Also, I think we need to change the way environment variables are stored inside Evaluator:

Currently we have a HashMap<Vec<u8>, Pointer<Tag>>. Where each Vec<u8> is the name of a variable and Pointer<Tag> points to an allocation with its value.

However, to reproduce environ and _NSGetEnviron we need to build a pointer to strings storing the variables as name=value. I believe this could be easily done by allocating the whole name=value string instead of just the value. Also we would need to have a memory region for the *const *const c_char itself.

This means that we would need to change setenv to allocate the whole name=value string, and keep updated the *const *const c_char (I'm not sure how messy this might be, because we might need to rellocate the whole thing and update all the pointers inside the hashmap from time to time). And change getenv to read just the value part of the allocation.

Another option would be to allocate the whole environment every time (set|unset)env happens and add the *const *const c_char as a field in Evaluator. However, this would mean duplicating all the evironment content.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 8, 2019

another option is to run libc through the c to rust compiler (and hope it works) and then interpret the *env functions that are now rust code

@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2019

another option is to run libc through the c to rust compiler (and hope it works) and then interpret the *env functions that are now rust code

There's no kill like overkill? ;)

bors added a commit that referenced this issue Aug 27, 2019
Write name and value for each env var

In order to res0lve #756 is necessary to have the whole `"NAME=VALUE"` sequence of bytes written into memory instead of just the value.

This change does not affect the interface of the `shim::envs::EnvVars` type in any way.

r? @RalfJung @oli-obk
@pvdrz
Copy link
Contributor

pvdrz commented Oct 24, 2019

This has been a little problematic. I was not able to create the required static without having stacked borrows issues.

@RalfJung
Copy link
Member

@christianpoveda do you have your failed attempt still sitting around somewhere? I'm sure it's just a little thing where you have to properly maintain the tags somewhere, or avoid deriving ptrs with different tags, or so.

@pvdrz
Copy link
Contributor

pvdrz commented Nov 30, 2019

Hmmm I checked my branches and it seems it is gone :( I can try it again soon with new eyes :)

@pvdrz pvdrz mentioned this issue Feb 22, 2020
@pvdrz pvdrz mentioned this issue Mar 8, 2020
@bors bors closed this as completed in #1208 Mar 8, 2020
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-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants