macOS: Implement stat64 to get rid of the termcap hack

[RalfJung]

We added this hack when env var exposure was implemented to avoid breakage:

https://github.com/rust-lang/miri/blob/master/src/shims/env.rs#L22-L23

However, since then @christianpoveda implemented basic file system access. So do we even still need that hack?

[pvdrz]

I can try that locally. Two questions:

• According to cargo miri test -- -Zmiri-disable-isolation fails #933, we just need to set TERM to anything to see this failing. Is that correct? or do we need an specific value?
• Do we keep the blacklisting flag for environment variables?

After some testing, we can't remove it:

   --> /home/christian/.rustup/toolchains/miri/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
    |

The stat64 shim is not implemented yet.

[RalfJung]

Do we keep the blacklisting flag for environment variables?

Sure, why not.

According to #933, we just need to set TERM to anything to see this failing. Is that correct? or do we need an specific value?

IIRC I just had to ran it. TERM is (almost?) always set when you are in a shell on Linux.

The stat64 shim is not implemented yet.

Ah, that's right. The surface-level function here is std::fs::metadata.

[pvdrz]

I can try to implement that shim. I've seen that function used in other parts of fs too.

[RalfJung]

You really don't want me to ever be done reviewing all your stuff, do you? I am already not keeping up.^^

But yes, please do. Juts throttle your PR output a bit maybe? :D

[pvdrz]

Hahahahaha. I think we will be back to normal after the current round of PRs.

[RalfJung]

Also see rust-lang/rust#65094

[RalfJung]

rust-lang/rust#65094 landed, so the shim you'll want to implement is statx.

[pvdrz]

I suppose that statx is only used on linux. So probably I'll have to add a hook for stat in macos or something, I'll figure it out later.

I was also thinking that a lot of information returned by both functions won't be available (and AFAIK it is not visible from std::fs::metadata) from a generic host, like the inode number, device id, etc. I believe the safest thing to do here is to leave that information uninitialized so we get an error when it is read. What do you think?

[RalfJung]

No that doesn't sound right... we would tell the user their program has UB when really we just didn't emulate the shim properly.

We could default it all to 0...

[pvdrz]

Shouldn't that produce unexpected behavior silently?

[RalfJung]

Yeah, maybe. OTOH I am not sure how much is really guaranteed for inodes. And when the host is Unix we can do the proper thing.

People have done API emulations before so maybe there's a way to get something "inode-like" on Windows.

[pvdrz]

I'll do an initial version with just the information that is exposed from std::fs::metadata and then I'll add incrementally the other fields when we figure out which is the proper way to emulate each one of them.

[pvdrz]

There is something else beyond implementing stat and statx there is a syscall done by std::fs::metadata, which currently fails on miri (given that we only support the getrandom syscall):

error[E0080]: Miri evaluation error: miri does not support syscall ID 332
  --> /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/sys/unix/weak.rs:94:13
   |
94 | /             syscall(
95 | |                 concat_idents!(SYS_, $name),
96 | |                 $($arg_name as c_long),*
97 | |             ) as $ret
   | |_____________^ Miri evaluation error: miri does not support syscall ID 332
   |
   = note: inside call to `std::sys::unix::fs::try_statx::statx` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/sys/unix/fs.rs:128:23
   = note: inside call to `std::sys::unix::fs::try_statx` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/sys/unix/fs.rs:984:37
   = note: inside call to `std::sys::unix::fs::stat` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/fs.rs:1520:5
   = note: inside call to `std::fs::metadata::<&std::path::PathBuf>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libterm/terminfo/searcher.rs:50:12

I looked up for the syscall and the name is compat_pwritev64 but I couldn't find anything about its behavior.

[bjorn3]

Syscall 332 is statx on x86_64 Linux, not compat_pwritev64:

https://github.com/torvalds/linux/blob/39a38bcba4ab6e5285b07675b0e42c96eec35e67/arch/x86/entry/syscalls/syscall_64.tbl#L343

That makes sense, as libstd calls statx using a syscall directly:

https://github.com/rust-lang/rust/blob/23f890f10202a71168c6424da0cdf94135d3c40c/src/libstd/sys/unix/fs.rs#L117

[pvdrz]

oh lol, thank you!

[RalfJung]

@christianpoveda any update on this? Would be great to get rid of that hack.

[pvdrz]

uhh yeah I forgot... sorry. I'll try to do a PR in a few days

[RalfJung]

Reopening as the issue remains for macOS.

[pvdrz]

We've done an advance by implementing stat in #1129. Now, miri is able to retrieve file metadata too but fails closing a file:

error: Miri evaluation error: The Operation not supported by device (os error 19) error cannot be transformed into a raw os error

   --> /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/fd.rs:289:26
    |
289 |         let _ = unsafe { libc::close(self.fd) };
    |                          ^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: The Operation not supported by device (os error 19) error cannot be transformed into a raw os error
    |

I'm almost certain this is happening because we are using File::sync_all in the close shim to retrieve possible errors. But this method calls fcntl(_, F_FULLFSYNC), which seems to be unavailable in the macos platform we're testing.

The "error cannot be transformed into a raw os error" is because there is no platform agnostic way of taking that error (which probably has kind ErrorKind::Other) and transforming it into a raw os error.

Also this particular error is happening in File::drop which ignores the OS error anyway. Maybe we could let close fail silently, std seems to ignore all the errors for it anyway. Another option would be to recapture this error and return another error that can be transformed into a raw os error.

[RalfJung]

But this method calls fcntl(_, F_FULLFSYNC), which seems to be unavailable in the macos platform we're testing.

To me, the actual bug seems to be that we fail to propagate the error to the client? Like, what we should do is set errno and return -1, right? The code in question ignores failures in close, so the program will continue.

But, why does this fcntl fail here but not fail elsewhere (like in the fs.rs test)? Something is odd here. Do you have a full backtrace?

[pvdrz]

I think it has something to do with the termcap file specifically:

error: Miri evaluation error: The Operation not supported by device (os error 19) error cannot be transformed into a raw os error

   --> /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/fd.rs:289:26

    |

289 |         let _ = unsafe { libc::close(self.fd) };

    |                          ^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: The Operation not supported by device (os error 19) error cannot be transformed into a raw os error

    |

    = note: inside call to `<std::sys::unix::fd::FileDesc as std::ops::Drop>::drop` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/ptr/mod.rs:184:1

    = note: inside call to `std::ptr::real_drop_in_place::<std::fs::File> - shim(Some(std::fs::File))` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/rand.rs:92:5

    = note: inside call to `std::sys::unix::rand::imp::fill_bytes` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/rand.rs:8:9

    = note: inside call to `std::sys::unix::rand::hashmap_random_keys` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:2460:23

    = note: inside call to `std::collections::hash_map::RandomState::new::KEYS::__init` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5

    = note: inside call to `<fn() -> std::cell::Cell<(u64, u64)> {std::collections::hash_map::RandomState::new::KEYS::__init} as std::ops::FnOnce<()>>::call_once - shim(fn() -> std::cell::Cell<(u64, u64)> {std::collections::hash_map::RandomState::new::KEYS::__init})` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:288:25

    = note: inside call to `std::thread::local::lazy::LazyKeyInner::<std::cell::Cell<(u64, u64)>>::initialize::<fn() -> std::cell::Cell<(u64, u64)> {std::collections::hash_map::RandomState::new::KEYS::__init}>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:424:22

    = note: inside call to `std::thread::__FastLocalKeyInner::<std::cell::Cell<(u64, u64)>>::try_initialize::<fn() -> std::cell::Cell<(u64, u64)> {std::collections::hash_map::RandomState::new::KEYS::__init}>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:409:25

    = note: inside call to `std::thread::__FastLocalKeyInner::<std::cell::Cell<(u64, u64)>>::get::<fn() -> std::cell::Cell<(u64, u64)> {std::collections::hash_map::RandomState::new::KEYS::__init}>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:175:17

    = note: inside call to `std::collections::hash_map::RandomState::new::KEYS::__getit` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:261:32

    = note: inside call to `std::thread::LocalKey::<std::cell::Cell<(u64, u64)>>::try_with::<[closure@DefId(1:1097 ~ std[a494]::collections[0]::hash[0]::map[0]::{{impl}}[66]::new[0]::{{closure}}[0])], std::collections::hash_map::RandomState>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/thread/local.rs:239:9

    = note: inside call to `std::thread::LocalKey::<std::cell::Cell<(u64, u64)>>::with::<[closure@DefId(1:1097 ~ std[a494]::collections[0]::hash[0]::map[0]::{{impl}}[66]::new[0]::{{closure}}[0])], std::collections::hash_map::RandomState>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:2463:9

    = note: inside call to `std::collections::hash_map::RandomState::new` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:2536:9

    = note: inside call to `<std::collections::hash_map::RandomState as std::default::Default>::default` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:2374:44

    = note: inside call to `<std::collections::HashMap<std::string::String, bool> as std::iter::FromIterator<(std::string::String, bool)>>::from_iter::<std::iter::adapters::ResultShunt<std::iter::FilterMap<std::ops::Range<usize>, [closure@DefId(28:173 ~ term[1b20]::terminfo[0]::parser[0]::compiled[0]::parse[0]::{{closure}}[1]) 0:&mut &mut dyn std::io::Read, 1:&&[&str]]>, std::io::Error>>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/iter/traits/iterator.rs:1494:9

    = note: inside call to `<std::iter::adapters::ResultShunt<std::iter::FilterMap<std::ops::Range<usize>, [closure@DefId(28:173 ~ term[1b20]::terminfo[0]::parser[0]::compiled[0]::parse[0]::{{closure}}[1]) 0:&mut &mut dyn std::io::Read, 1:&&[&str]]>, std::io::Error> as std::iter::Iterator>::collect::<std::collections::HashMap<std::string::String, bool>>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/result.rs:1470:53

    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/iter/adapters/mod.rs:2399:17

    = note: inside call to `std::iter::adapters::process_results::<std::iter::FilterMap<std::ops::Range<usize>, [closure@DefId(28:173 ~ term[1b20]::terminfo[0]::parser[0]::compiled[0]::parse[0]::{{closure}}[1]) 0:&mut &mut dyn std::io::Read, 1:&&[&str]]>, (std::string::String, bool), std::io::Error, [closure@DefId(2:5558 ~ core[488b]::result[0]::{{impl}}[35]::from_iter[0]::{{closure}}[0])], std::collections::HashMap<std::string::String, bool>>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/result.rs:1470:9

    = note: inside call to `<std::result::Result<std::collections::HashMap<std::string::String, bool>, std::io::Error> as std::iter::FromIterator<std::result::Result<(std::string::String, bool), std::io::Error>>>::from_iter::<std::iter::FilterMap<std::ops::Range<usize>, [closure@DefId(28:173 ~ term[1b20]::terminfo[0]::parser[0]::compiled[0]::parse[0]::{{closure}}[1]) 0:&mut &mut dyn std::io::Read, 1:&&[&str]]>>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/iter/traits/iterator.rs:1494:9

    = note: inside call to `<std::iter::FilterMap<std::ops::Range<usize>, [closure@DefId(28:173 ~ term[1b20]::terminfo[0]::parser[0]::compiled[0]::parse[0]::{{closure}}[1]) 0:&mut &mut dyn std::io::Read, 1:&&[&str]]> as std::iter::Iterator>::collect::<std::result::Result<std::collections::HashMap<std::string::String, bool>, std::io::Error>>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/parser/compiled.rs:250:9

    = note: inside call to `term::terminfo::parser::compiled::parse` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:102:9

    = note: inside call to `term::terminfo::TermInfo::_from_path` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:96:9

    = note: inside call to `term::terminfo::TermInfo::from_path::<&std::path::Path>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:91:27

    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libcore/result.rs:720:22

    = note: inside call to `std::result::Result::<std::path::PathBuf, term::terminfo::Error>::and_then::<term::terminfo::TermInfo, [closure@DefId(28:43 ~ term[1b20]::terminfo[0]::{{impl}}[2]::from_name[0]::{{closure}}[1])]>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:87:9

    = note: inside call to `term::terminfo::TermInfo::from_name` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:73:25

    = note: inside call to `term::terminfo::TermInfo::from_env` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/terminfo/mod.rs:224:9

    = note: inside call to `term::terminfo::TerminfoTerminal::<std::io::Stdout>::new` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libterm/lib.rs:62:5

    = note: inside call to `term::stdout` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libtest/console.rs:254:24

    = note: inside call to `test::test::run_tests_console` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libtest/lib.rs:122:15

    = note: inside call to `test::test::test_main` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libtest/lib.rs:141:5

    = note: inside call to `test::test::test_main_static`

    = note: inside call to `main` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/rt.rs:67:34

    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/rt.rs:52:73

    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:129:5

    = note: inside call to `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6082 ~ std[a494]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/rt.rs:52:13

    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/panicking.rs:296:40

    = note: inside call to `std::panicking::r#try::do_call::<[closure@DefId(1:6081 ~ std[a494]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/panicking.rs:272:13

    = note: inside call to `std::panicking::r#try::<i32, [closure@DefId(1:6081 ~ std[a494]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/panic.rs:394:14

    = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:6081 ~ std[a494]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/rt.rs:51:25

    = note: inside call to `std::rt::lang_start_internal` at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/rt.rs:67:5

    = note: inside call to `std::rt::lang_start::<()>`

[pvdrz]

According to apple's manual:

     F_FULLFSYNC        Does the same thing as fsync(2) then asks the drive to
                        flush all buffered data to the permanent storage
                        device (arg is ignored).  This is currently implemented implemented
                        mented on HFS, MS-DOS (FAT), and Universal Disk Format
                        (UDF) file systems.  The operation may take quite a
                        while to complete.  Certain FireWire drives have also
                        been known to ignore the request to flush their
                        buffered data.

maybe the termcap file is in a FS where this is not implemented? I'm unfamiliar with this.

[RalfJung]

The failure is inside hashmap_random_keys. This has nothing to do with termcap; the program is opening /dev/random and it looks like that cannot be synced.

Do we do the syncing also on files that are opened read-only? Would it maybe make sense to not do it there?

[pvdrz]

yes we always sync. I could store the access mode to the file in the FileHandle and then not sync if the file was opened as read only.

Edit: I checked the possible errors for close

       EBADF  fd isn't a valid open file descriptor.

       EINTR  The close() call was interrupted by a signal; see signal(7).

       EIO    An I/O error occurred.

       ENOSPC, EDQUOT
              On NFS, these errors are not normally reported against the
              first write which exceeds the available storage space, but
              instead against a subsequent write(2), fsync(2), or close().

• EBADF is already taken care of by our file handler.
• ENOSPC and ENDQUOT only happens if someone wrote (so this doesn't apply to read-only files) to the file before closing it.
• I'm not sure about the meaning of EIO happening on closing a read-only file.
• EINTR might be relevant because that would mean that the file should stay open and we would delete its handle from the file handler.

[RalfJung]

I could store the access mode to the file in the FileHandle and then not sync if the file was opened as read only.

Sounds like a good plan to me.