Revert fstatat on *nix and test symlinks in path_filestat calls (#1725)

* Revert fstatat on *nix and test symlinks in path_filestat calls

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect `path_filestat_{get, set_times}` behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.

In order to shield ourselves from similar errors in the future, I've
augmented the `path_filestat` test cases with symlink checks as well.

* Pass appropriate flags to fstatat and utimensat

* Fix formatting

* Fix Windows build

* Expand final symlinks if follow is set on Windows

* Fix formatting

* Do not follow symlinks unless specified on Windows

* Update comments and restart CI

* Skip testing volatile atim field

crates/test-programs/wasi-tests/src/bin/path_filestat.rs

@@ -55,31 +55,20 @@ unsafe fn test_path_filestat(dir_fd: wasi::Fd) {
     assert_eq!(stat.size, 0, "file size should be 0");
 
     // Check path_filestat_set_times
-    let old_atim = stat.atim;
     let new_mtim = stat.mtim - 100;
-    wasi::path_filestat_set_times(
+    wasi::path_filestat_set_times(dir_fd, 0, "file", 0, new_mtim, wasi::FSTFLAGS_MTIM)
-        dir_fd,
-        0,
-        "file",
-        // on purpose: the syscall should not touch atim, because
-        // neither of the ATIM flags is set
-        new_mtim,
-        new_mtim,
-        wasi::FSTFLAGS_MTIM,
-    )
         .expect("path_filestat_set_times should succeed");
 
     stat = wasi::path_filestat_get(dir_fd, 0, "file")
         .expect("reading file stats after path_filestat_set_times");
     assert_eq!(stat.mtim, new_mtim, "mtim should change");
-    assert_eq!(stat.atim, old_atim, "atim should not change");
 
     assert_eq!(
         wasi::path_filestat_set_times(
             dir_fd,
             0,
             "file",
-            new_mtim,
+            0,
             new_mtim,
             wasi::FSTFLAGS_MTIM | wasi::FSTFLAGS_MTIM_NOW,
         )
@@ -93,16 +82,14 @@ unsafe fn test_path_filestat(dir_fd: wasi::Fd) {
     stat = wasi::path_filestat_get(dir_fd, 0, "file")
         .expect("reading file stats after ERRNO_INVAL fd_filestat_set_times");
     assert_eq!(stat.mtim, new_mtim, "mtim should not change");
-    assert_eq!(stat.atim, old_atim, "atim should not change");
 
-    let new_atim = old_atim - 100;
     assert_eq!(
         wasi::path_filestat_set_times(
             dir_fd,
             0,
             "file",
-            new_atim,
+            0,
-            new_atim,
+            0,
             wasi::FSTFLAGS_ATIM | wasi::FSTFLAGS_ATIM_NOW,
         )
         .expect_err("ATIM & ATIM_NOW can't both be set")
@@ -111,14 +98,46 @@ unsafe fn test_path_filestat(dir_fd: wasi::Fd) {
         "errno should be ERRNO_INVAL"
     );
 
-    // check if the times were untouched
+    // Create a symlink
+    wasi::path_symlink("file", dir_fd, "symlink").expect("creating symlink to a file");
+
+    // Check path_filestat_set_times on the symlink itself
+    let mut sym_stat = wasi::path_filestat_get(dir_fd, 0, "file").expect("reading file stats");
+
+    let sym_new_mtim = sym_stat.mtim - 200;
+    wasi::path_filestat_set_times(dir_fd, 0, "symlink", 0, sym_new_mtim, wasi::FSTFLAGS_MTIM)
+        .expect("path_filestat_set_times should succeed on symlink");
+
+    sym_stat = wasi::path_filestat_get(dir_fd, 0, "symlink")
+        .expect("reading file stats after path_filestat_set_times");
+    assert_eq!(sym_stat.mtim, sym_new_mtim, "mtim should change");
+
+    // Now, dereference the symlink
+    sym_stat = wasi::path_filestat_get(dir_fd, wasi::LOOKUPFLAGS_SYMLINK_FOLLOW, "symlink")
+        .expect("reading file stats on the dereferenced symlink");
+    assert_eq!(
+        sym_stat.mtim, stat.mtim,
+        "symlink mtim should be equal to pointee's when dereferenced"
+    );
+
+    // Finally, change stat of the original file by dereferencing the symlink
+    wasi::path_filestat_set_times(
+        dir_fd,
+        wasi::LOOKUPFLAGS_SYMLINK_FOLLOW,
+        "symlink",
+        0,
+        sym_stat.mtim,
+        wasi::FSTFLAGS_MTIM,
+    )
+    .expect("path_filestat_set_times should succeed on setting stat on original file");
+
     stat = wasi::path_filestat_get(dir_fd, 0, "file")
-        .expect("reading file stats after ERRNO_INVAL path_filestat_set_times");
+        .expect("reading file stats after path_filestat_set_times");
-    assert_eq!(stat.mtim, new_mtim, "mtim should not change");
+    assert_eq!(stat.mtim, sym_stat.mtim, "mtim should change");
-    assert_eq!(stat.atim, old_atim, "atim should not change");
 
     wasi::fd_close(file_fd).expect("closing a file");
     wasi::path_unlink_file(dir_fd, "file").expect("removing a file");
+    wasi::path_unlink_file(dir_fd, "symlink").expect("removing a symlink");
 }
 fn main() {
     let mut args = env::args();

crates/wasi-common/src/handle.rs

@@ -135,6 +135,19 @@ pub(crate) trait Handle {
     fn create_directory(&self, _path: &str) -> Result<()> {
         Err(Errno::Acces)
     }
+    fn filestat_get_at(&self, _path: &str, _follow: bool) -> Result<types::Filestat> {
+        Err(Errno::Acces)
+    }
+    fn filestat_set_times_at(
+        &self,
+        _path: &str,
+        _atim: types::Timestamp,
+        _mtim: types::Timestamp,
+        _fst_flags: types::Fstflags,
+        _follow: bool,
+    ) -> Result<()> {
+        Err(Errno::Acces)
+    }
     fn openat(
         &self,
         _path: &str,

crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs

@@ -469,15 +469,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
         let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_GET);
         let entry = self.get_entry(dirfd)?;
         let (dirfd, path) = path::get(&entry, &required_rights, flags, path, false)?;
-        let host_filestat = dirfd
+        let host_filestat =
-            .openat(
+            dirfd.filestat_get_at(&path, flags.contains(&types::Lookupflags::SYMLINK_FOLLOW))?;
-                &path,
-                false,
-                false,
-                types::Oflags::empty(),
-                types::Fdflags::empty(),
-            )?
-            .filestat_get()?;
         Ok(host_filestat)
     }
 
@@ -493,15 +486,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
         let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_SET_TIMES);
         let entry = self.get_entry(dirfd)?;
         let (dirfd, path) = path::get(&entry, &required_rights, flags, path, false)?;
-        dirfd
+        dirfd.filestat_set_times_at(
-            .openat(
             &path,
-                false,
+            atim,
-                false,
+            mtim,
-                types::Oflags::empty(),
+            fst_flags,
-                types::Fdflags::empty(),
+            flags.contains(&types::Lookupflags::SYMLINK_FOLLOW),
-            )?
+        )?;
-            .filestat_set_times(atim, mtim, fst_flags)?;
         Ok(())
     }
 

crates/wasi-common/src/sys/osdir.rs

@@ -69,6 +69,19 @@ impl Handle for OsDir {
     fn create_directory(&self, path: &str) -> Result<()> {
         path::create_directory(self, path)
     }
+    fn filestat_get_at(&self, path: &str, follow: bool) -> Result<types::Filestat> {
+        path::filestat_get_at(self, path, follow)
+    }
+    fn filestat_set_times_at(
+        &self,
+        path: &str,
+        atim: types::Timestamp,
+        mtim: types::Timestamp,
+        fst_flags: types::Fstflags,
+        follow: bool,
+    ) -> Result<()> {
+        path::filestat_set_times_at(self, path, atim, mtim, fst_flags, follow)
+    }
     fn openat(
         &self,
         path: &str,

crates/wasi-common/src/sys/unix/path.rs

@@ -1,7 +1,8 @@
 use crate::handle::{Handle, HandleRights};
 use crate::sys::osdir::OsDir;
+use crate::sys::AsFile;
 use crate::wasi::{types, Errno, Result};
-use std::convert::TryFrom;
+use std::convert::{TryFrom, TryInto};
 use std::ffi::OsStr;
 use std::fs::File;
 use std::os::unix::prelude::{AsRawFd, FromRawFd, OsStrExt};
@@ -204,3 +205,57 @@ pub(crate) fn remove_directory(dirfd: &OsDir, path: &str) -> Result<()> {
     unsafe { unlinkat(dirfd.as_raw_fd(), path, AtFlag::REMOVEDIR)? };
     Ok(())
 }
+
+pub(crate) fn filestat_get_at(dirfd: &OsDir, path: &str, follow: bool) -> Result<types::Filestat> {
+    use yanix::file::{fstatat, AtFlag};
+    let flags = if follow {
+        AtFlag::empty()
+    } else {
+        AtFlag::SYMLINK_NOFOLLOW
+    };
+    let stat = unsafe { fstatat(dirfd.as_raw_fd(), path, flags)? };
+    let stat = stat.try_into()?;
+    Ok(stat)
+}
+
+pub(crate) fn filestat_set_times_at(
+    dirfd: &OsDir,
+    path: &str,
+    atim: types::Timestamp,
+    mtim: types::Timestamp,
+    fst_flags: types::Fstflags,
+    follow: bool,
+) -> Result<()> {
+    use std::time::{Duration, UNIX_EPOCH};
+    use yanix::filetime::*;
+
+    let set_atim = fst_flags.contains(&types::Fstflags::ATIM);
+    let set_atim_now = fst_flags.contains(&types::Fstflags::ATIM_NOW);
+    let set_mtim = fst_flags.contains(&types::Fstflags::MTIM);
+    let set_mtim_now = fst_flags.contains(&types::Fstflags::MTIM_NOW);
+
+    if (set_atim && set_atim_now) || (set_mtim && set_mtim_now) {
+        return Err(Errno::Inval);
+    }
+
+    let atim = if set_atim {
+        let time = UNIX_EPOCH + Duration::from_nanos(atim);
+        FileTime::FileTime(filetime::FileTime::from_system_time(time))
+    } else if set_atim_now {
+        FileTime::Now
+    } else {
+        FileTime::Omit
+    };
+    let mtim = if set_mtim {
+        let time = UNIX_EPOCH + Duration::from_nanos(mtim);
+        FileTime::FileTime(filetime::FileTime::from_system_time(time))
+    } else if set_mtim_now {
+        FileTime::Now
+    } else {
+        FileTime::Omit
+    };
+
+    utimensat(&*dirfd.as_file()?, path, atim, mtim, !follow)?;
+
+    Ok(())
+}

crates/wasi-common/src/sys/windows/path.rs

@@ -1,6 +1,6 @@
 use crate::handle::{Handle, HandleRights};
 use crate::sys::osdir::OsDir;
-use crate::sys::AsFile;
+use crate::sys::{fd, AsFile};
 use crate::wasi::{types, Errno, Result};
 use std::convert::TryFrom;
 use std::ffi::{OsStr, OsString};
@@ -494,3 +494,42 @@ pub(crate) fn remove_directory(dirfd: &OsDir, path: &str) -> Result<()> {
     let path = concatenate(dirfd, path)?;
     std::fs::remove_dir(&path).map_err(Into::into)
 }
+
+pub(crate) fn filestat_get_at(dirfd: &OsDir, path: &str, follow: bool) -> Result<types::Filestat> {
+    use winx::file::Flags;
+    let path = concatenate(dirfd, path)?;
+    let mut opts = OpenOptions::new();
+
+    if !follow {
+        // By specifying FILE_FLAG_OPEN_REPARSE_POINT, we force Windows to *not* dereference symlinks.
+        opts.custom_flags(Flags::FILE_FLAG_OPEN_REPARSE_POINT.bits());
+    }
+
+    let file = opts.read(true).open(path)?;
+    let stat = fd::filestat_get(&file)?;
+    Ok(stat)
+}
+
+pub(crate) fn filestat_set_times_at(
+    dirfd: &OsDir,
+    path: &str,
+    atim: types::Timestamp,
+    mtim: types::Timestamp,
+    fst_flags: types::Fstflags,
+    follow: bool,
+) -> Result<()> {
+    use winx::file::{AccessMode, Flags};
+    let path = concatenate(dirfd, path)?;
+    let mut opts = OpenOptions::new();
+
+    if !follow {
+        // By specifying FILE_FLAG_OPEN_REPARSE_POINT, we force Windows to *not* dereference symlinks.
+        opts.custom_flags(Flags::FILE_FLAG_OPEN_REPARSE_POINT.bits());
+    }
+
+    let file = opts
+        .access_mode(AccessMode::FILE_WRITE_ATTRIBUTES.bits())
+        .open(path)?;
+    fd::filestat_set_times(&file, atim, mtim, fst_flags)?;
+    Ok(())
+}

crates/wasi-common/src/virtfs.rs

@@ -600,6 +600,36 @@ impl Handle for VirtualDir {
             }
         }
     }
+    fn filestat_get_at(&self, path: &str, _follow: bool) -> Result<types::Filestat> {
+        let stat = self
+            .openat(
+                path,
+                false,
+                false,
+                types::Oflags::empty(),
+                types::Fdflags::empty(),
+            )?
+            .filestat_get()?;
+        Ok(stat)
+    }
+    fn filestat_set_times_at(
+        &self,
+        path: &str,
+        atim: types::Timestamp,
+        mtim: types::Timestamp,
+        fst_flags: types::Fstflags,
+        _follow: bool,
+    ) -> Result<()> {
+        self.openat(
+            path,
+            false,
+            false,
+            types::Oflags::empty(),
+            types::Fdflags::empty(),
+        )?
+        .filestat_set_times(atim, mtim, fst_flags)?;
+        Ok(())
+    }
     fn openat(
         &self,
         path: &str,