Add shims for env var emulation in Windows #1225

JOE1994 · 2020-03-13T17:57:06Z

This PR attempts to implement the final step of the instructions laid out in #707 (comment) , and is yet a work in progress.

STATUS

Add general target methods for read_str/alloc_str that dispatch to either c_str or wide_str variants
(helpers.rs)
Implement shims fn getenvironmentvariablew/fn setenvironmentvariablew
(std::env::var(), std::env::set_var())
Implement shim GetEnvironmentStringsW (std::env::vars())
Implement shim FreeEnvironmentStringsW

ISSUES (updated on 03/21/2020)

MIRI errors while running std::env::remove_var() in Windows.
MIRI complaining about raw pointer usage in
Rust standard library src/libstd/sys/windows/os.rs.

TODO (probably on a separate PR)

Move string helpers into a new file to avoid bloating src/helpers.rs too much. (shims/os_str.rs)

src/helpers.rs

RalfJung · 2020-03-13T18:10:17Z

Move string helpers into a new file to avoid bloating src/helpers.rs too much.
(shims/str.rs ?)

That sounds like a good idea, do you want to do that in a separate PR?
I propose shims/os_str.rs.

src/helpers.rs

JOE1994 · 2020-03-13T20:39:48Z

Move string helpers into a new file to avoid bloating src/helpers.rs too much.
(shims/str.rs ?)

That sounds like a good idea, do you want to do that in a separate PR?
I propose shims/os_str.rs.

It's actually the idea that you suggested last year 😃 I will open a separate PR to take care of it after this PR is merged.

src/helpers.rs

JOE1994 · 2020-03-21T20:28:37Z

Testing MIRI on tests/run-pass/env.rs gives an evaluation error.
error: Miri evaluation error: invalid use of NULL pointer

error is about the source code of standard library, and not from the added changes in this PR

bjorn3 · 2020-03-22T10:43:28Z

value_ptr seems to be set to a nullptr when unsetting an env var on windows. The setenv function doesn't check for this.

bors · 2020-03-22T22:52:40Z

☔ The latest upstream changes (presumably #1250) made this pull request unmergeable. Please resolve the merge conflicts.

src/shims/env.rs

Co-Authored-By: Ralf Jung <[email protected]>

RalfJung · 2020-03-27T14:17:36Z

Okay, I think the code is fine now except for that WinHeap problem. I will look into that later today or tomorrow.

RalfJung · 2020-03-27T15:31:16Z

Ah I forgot, there are some more tests that can have their "ignore-windows" removed:

tests/run-pass/env-exclude.rs
tests/run-pass/env-without-isolation.rs

Also, in tests/compile-fail/environ-gets-deallocated.rs, the comment should be updated to something like

//ignore-windows: Windows does not have a global environ list that the program can access directly

src/shims/env.rs

RalfJung · 2020-03-27T19:38:38Z

All right, thanks for your persistence and well done. :)
@bors r+

bors · 2020-03-27T19:38:39Z

📌 Commit 579b3c4 has been approved by RalfJung

bors · 2020-03-27T19:38:47Z

⌛ Testing commit 579b3c4 with merge 01bc08a...

bors · 2020-03-27T20:02:30Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 01bc08a to master...

@JOE1994

avoid creating unnecessary reference in Windows Env iterator Discovered in rust-lang/miri#1225: the Windows `Env` iterator violates Stacked Borrows by creating an `&u16`, turning it into a raw pointer, and then accessing memory outside the range of that type. There is no need to create a reference here in the first place, so the fix is trivial. Cc @JOE1994 Cc rust-lang/unsafe-code-guidelines#134

A small fix was made to libstd in rust-lang/rust#70479 (back in March). (Miri reported UB due to Stacked Borrows violation - [link to Miri error log](rust-lang#1225 (comment))) Thank you for reviewing 👍

Add a case to list of 'StackedBorrows violations' A small fix was made to libstd in rust-lang/rust#70479 (back in March). (Miri reported UB due to Stacked Borrows violation - [link to Miri error log](#1225 (comment))) Thank you for reviewing 👍

RalfJung reviewed Mar 13, 2020

View reviewed changes