add support for the tempfile crate

[Pointerbender]

• Support file creation modes where the owner must have read-write permissions and that none of the owner, group and other may have execute permissions.
• Add tempfile as a dependency in test_dependencies/Cargo.toml
• Add a test case for method tempfile::tempfile().

[Pointerbender]

I see the windows CI is failing, but there is a //@ignore-target-windows: no libc on Windows at the top of tests/pass-dep/shims/libc-fs.rs. How can I best proceed in this case? (Can I run the Windows tests from Ubuntu 18.04? I don't have access to a Windows machine, unfortunately).

[oli-obk]

Can I run the Windows tests from Ubuntu 18.04? I don't have access to a Windows machine, unfortunately).

Yes, miri supports cross interpretation. For testing you can use the MIRI_TEST_TARGET env var to choose your target

[Pointerbender]

I think I may still might be doing something the wrong way. If I look at the logs of the previous failed Windows run, I see two mentions of targets:

• i686-uwp-windows-msvc (at the very top of the Github job)
• x86_64-unknown-linux-gnu (in the arguments passed to the command that failed)

I tried running both targets on my Ubuntu machine like this:

$ MIRI_TEST_TARGET="i686-uwp-windows-msvc" ./miri test tests/pass-dep/shims/libc-fs.rs
$ MIRI_TEST_TARGET="x86_64-unknown-linux-gnu" ./miri test tests/pass-dep/shims/libc-fs.rs

and for good measure also all the other targets that mention Windows:

$ rustc --print target-list | grep windows
aarch64-pc-windows-gnullvm
aarch64-pc-windows-msvc
aarch64-uwp-windows-msvc
i586-pc-windows-msvc
i686-pc-windows-gnu
i686-pc-windows-msvc
i686-uwp-windows-gnu
i686-uwp-windows-msvc
thumbv7a-pc-windows-msvc
thumbv7a-uwp-windows-msvc
x86_64-pc-windows-gnu
x86_64-pc-windows-gnullvm
x86_64-pc-windows-msvc
x86_64-uwp-windows-gnu
x86_64-uwp-windows-msvc

$ rustc --print target-list | grep windows | xargs -I {} env MIRI_TEST_TARGET="{}" ./miri test tests/pass-dep/shims/libc-fs.rs

But the test passes on all Windows targets on my machine, with the exception of targets thumbv7a-uwp-windows-msvc and thumbv7a-pc-windows-msvc for which it failed to compile the dependencies and could not even run the test. It might be something specific to running the miri test on a Windows machine for a x86_64-unknown-linux-gnu target, perhaps?

The next Windows CI also finished just now and has a slightly different error now:

The first time around, the error was:

actual output differed from expected
  --- tests/pass-dep\shims\libc-fs.stderr
  +++ <stderr output>
   hello dup fd
  +thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "entity not found" }', $DIR/libc-fs.rs:LL:CC
  +stack backtrace:
  +   0: std::panicking::begin_panic_handler
  + at RUSTLIB/std/src/panicking.rs:LL:CC
  +   1: std::rt::panic_fmt
  + at RUSTLIB/core/src/panicking.rs:LL:CC
  +   2: std::result::unwrap_failed
  + at RUSTLIB/core/src/result.rs:LL:CC
  +   3: std::result::Result::unwrap
  + at RUSTLIB/core/src/result.rs:LL:CC
  +   4: test_tempfile
  + at $DIR/libc-fs.rs:LL:CC
  +   5: main
  + at $DIR/libc-fs.rs:LL:CC
  +   6: <fn() as std::ops::FnOnce<()>>::call_once - shim(fn())
  + at RUSTLIB/core/src/ops/function.rs:LL:CC
  +note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

And the second time the error is:

actual output differed from expected
  --- tests/pass-dep\shims\libc-fs.stderr
  +++ <stderr output>
   hello dup fd
  +thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 36, kind: InvalidFilename, message: "invalid filename" }', $DIR/libc-fs.rs:LL:CC
  +stack backtrace:
  +   0: std::panicking::begin_panic_handler
  + at RUSTLIB/std/src/panicking.rs:LL:CC
  +   1: std::rt::panic_fmt
  + at RUSTLIB/core/src/panicking.rs:LL:CC
  +   2: std::result::unwrap_failed
  + at RUSTLIB/core/src/result.rs:LL:CC
  +   3: std::result::Result::unwrap
  + at RUSTLIB/core/src/result.rs:LL:CC
  +   4: test_tempfile
  + at $DIR/libc-fs.rs:LL:CC
  +   5: main
  + at $DIR/libc-fs.rs:LL:CC
  +   6: <fn() as std::ops::FnOnce<()>>::call_once - shim(fn())
  + at RUSTLIB/core/src/ops/function.rs:LL:CC
  +note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The difference is that in the first time, the test said (now no longer visible due to my earlier force push):

/// Test that the [`tempfile`] crate is compatible with miri.
fn test_tempfile() {
    tempfile::tempfile().unwrap();
}

Which I changed to:

/// Test that the [`tempfile`] crate is compatible with miri.
fn test_tempfile() {
    let dir_path = tmp();
    tempfile::tempfile_in(dir_path).unwrap();
}

One other idea I could try is to add #[cfg(not(target_family = "windows"))] to fn test_tempfile(), but I suspect that won't matter in this case, because Windows fails on the test target for x86_64-unknown-linux-gnu :'-( I think I hit a dead end again on how to debug this one, might I solicit here again for another thing I could try? :)

Also the Mac OS build had a network blip just now and failed early. I tried force pushing it again, but that doesn't seem to trigger a restart of the CI without changes (git refuses to force push because it claims "Everything up-to-date"). Is there a recommended way to restart the Mac OS job in case I run into that again? :)

Thanks!

[oli-obk]

ah, the problem is a windows host interpreting a linux target. Oof. Maybe this is due to backslashes in names or sth? Not sure how to debug this.

[LegNeato]

I would imagine it is because the call to tmp is returning a unix path (because the host is unix and miri's fs implementation is sort of a passthrough with some path mangling), which is then passed into tempfile_in(), which is "running" on windows, so a windows API gets a unix path. And vice-versa. With the old test, the path doesn't round trip through the host, which is why the error is different.

These sort of issues are going to keep coming up, as I mentioned in https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Filesystem.20abstraction. I'd love to get some time to tackle it but currently I do not :-(

[Pointerbender]

Thanks for that pointer @LegNeato . Going through the zulip topic that you linked, it sounds like it would be a proper effort add path translation from a unix target family to a windows Host. On the bright side, we now have an actual example of this being a problem on Windows running a linux target :P (cc #1537)

Considering that this can not be properly fixed without access to a Windows environment and a significant time investment, I believe that leaves me with two possible alternatives:

Alternative 1

Adjust the test case for tempfile that it doesn't fail when the tempfile methods return a InvalidFilename or NotFound IO error kind and see if that is enough to make the Windows CI pass. Perhaps there is even a way to detect from a test whether it's running a linux target on a windows host, perhaps through an existing environment variable, to narrow it down even more? Then we track this support gap for tempfile on #1537 or in a separate Github issue, in the hopes that someone with a Windows machine and enough time finds it and fixes it properly some day.

Alternative 2

We do not commit at this point in time to adding tempfile as a supported third party dependency by removing the tempfile test case(s) and the tempfile dependency; and replace those with test cases for the file creation modes only. This will make miri play nice with tempfile on host/target combinations that do not suffer from path translation issues, but without "official" support for the tempfile crate.

@oli-obk What do you think is the best course of action? Or maybe you know of a third viable alternative?

Thanks!

[RalfJung]

The PR seems to effectively just ignore most of the mode. Is that really a good idea?

[RalfJung]

Regarding the path problem: we already do path separator adjustment if host!=target, which so far was enough. It would be good to figure out what exactly happens here to understand why this is no longer enough. It is not too surprising that naive separator substitution is insufficient but it would still be good to learn why exactly it fails now. I don't have a Windows machine either, but you can still add a bunch of debug printing and then see what happens on CI.

[RalfJung]

My first guess would be that the !path.is_absolute() check is going wrong -- the path translation between Linux and Windows hosts maps C:\dir to C:/dir which is not considered absolute as a Unix path. I am trying to fix that in #2725, so once that lands it might be worth rebasing and retrying.

[RalfJung]

FWIW I think this would also not have been a problem if tempfile just called canonicalize to get an absolute path -- but instead it hand-rolls (an approximation of) that function:

    if !path.is_absolute() {
        let cur_dir = env::current_dir()?;
        tmp = cur_dir.join(path);
        path = &tmp;
    }

[Pointerbender]

Thanks for all that input @RalfJung !

My next steps for this PR will be:

• Wait for make unix path handling on Windows hosts (and vice versa) preserve absoluteness #2725 to be merged.
• Rebase master onto the branch of this PR.
• Move the tempfile tests to tests/pass-dep/tempfile.rs.
• Run the CI again to see what happens. Obtain some extra information by adding debug statements if it's still failing.
• In the mean time, explore if/how we could support the file modes cross-platform (due to that the host platform could differ from the target platform).

[RalfJung]

In the mean time, explore if/how we could support the file modes cross-platform (due to that the host platform could differ from the target platform).

Yeah that is the most pressing point. On a unix host we could directly use mode; on a Windows host access_mode seems like the right thing, but the flags will need translation.

[Pointerbender]

the flags will need translation

From what I've been able to research so far, it appears to be the case that the Windows file creation API has no concept of group permissions. Even if we find a work-around by calling the group permissions APIs ourselves after creating the file first, there is a discrepancy between the permission models of Windows and Unix: Unix distinguishes between the owner, a single additional group and everyone else; while Windows has a more complicated model that allows multiple users and multiple groups, and there is not a clear-cut way to map the only Linux owner/group to a Windows equivalent ("Everybody" is a group in Windows, though, so that part is at least somewhat straight-forward - assuming there is a Windows API that we can call to set the permissions for "Everyone", I was unable to find which API we need for that so far).

[Pointerbender]

The CI passes with the fixes from #2725 applied 🎉

The remaining open question is how to support Unix file modes on a Windows host, before this can be merged.

Some thoughts about the file mode flag translation:

• Miri already supports the 0o666 (= everybody read/write permissions) flag currently.
• To support the 0o600 file mode (= only owner has read-write permissions, tempfile wants this), it should be possible somehow to construct a security descriptor which contains an Access Control List that says "only the owner has read+write permissions and everybody else is denied access to this file", which can be passed to the CreateFileA function through the lpSecurityAttributes argument (thanks to @ChrisDenton for this tip!).
• I'm still trying to figure out what is the best way to set the security descriptor / Access Control List (either at file creation, which is not supported by the Rust std library, or after file creation through a third party crate API, if that exists).

[RalfJung]

Setting it later won't be entirely correct though, someone could access the file between it being created and the permissions being adjusted.

I guess the alternative would be to just not support this on a Windows host. So far we have kept our set of supported operations host-independent but as we move beyond what the Rust stdlib can provide, that might not always be feasible.

[Pointerbender]

That is a good point. In that case, does it makes sense to support all file modes on Unix platforms (including execute permissions) or still just a subset of the file modes which lets miri stay forward compatible for when the std lib does add support for this in the future? So far I was only able to deduce that modes 0o600 and 0o666 are both compatible with the Unix and Windows permission models (there might be more compatible file modes, I just didn't have a chance to explore all of them yet).

To clarify what you mean by "not support this on a Windows host", should we either: 1) halt the interpreter by showing an unsupported error or 2) silently default to the default permissions when executing a Unix target on a Windows host?

Thanks!

[RalfJung]

In this version we would

• support all modes on a Unix host, just forwarding the flag
• throw_unsup on a Windows host for non-default flags
• add //@ignore-host-windows to the test (which needs to be implemented in ui_test first)

[RalfJung]

@rustbot author

[bors]

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

[RalfJung]

@Pointerbender I am closing this PR due to inactivity. Feel free to reopen or open a new PR if you want to get back to this!

[Jefffrey]

Hey I was wondering on the status of this. Would it be possible for me to help push this along, as the PR has been inactive for a while now?

cc @Pointerbender