Fix Snapshot0 fd_filestat_get and path_filestat_get

lib/wasi/src/syscalls/legacy/snapshot0.rs

@@ -15,12 +15,7 @@ use crate::{
     Memory32, MemorySize, WasiEnv, WasiError,
 };
 
-/// Wrapper around `syscalls::fd_filestat_get` with extra logic to handle the size
-/// difference of `wasi_filestat_t`
-///
-/// WARNING: this function involves saving, clobbering, and restoring unrelated
-/// Wasm memory.  If the memory clobbered by the current syscall is also used by
-/// that syscall, then it may break.
+/// Wrapper around `syscalls::fd_filestat_get` for old Snapshot0
 #[instrument(level = "debug", skip_all, ret)]
 pub fn fd_filestat_get(
     mut ctx: FunctionEnvMut<WasiEnv>,
@@ -29,51 +24,12 @@ pub fn fd_filestat_get(
 ) -> Errno {
     let env = ctx.data();
     let memory = env.memory_view(&ctx);
-    // TODO: understand what's happening inside this function, then do the correct thing
-
-    // transmute the WasmPtr<T1> into a WasmPtr<T2> where T2 > T1, this will read extra memory.
-    // The edge case of this cenv.mausing an OOB is not handled, if the new field is OOB, then the entire
-    // memory access will fail.
-    let new_buf: WasmPtr<Filestat, Memory32> = buf.cast();
-
-    // Copy the data including the extra data
-    let new_filestat_setup: Filestat = wasi_try_mem!(new_buf.read(&memory));
-
-    // Set up complete, make the call with the pointer that will write to the
-    // struct and some unrelated memory after the struct.
-    let result = syscalls::fd_filestat_get::<Memory32>(ctx.as_mut(), fd, new_buf);
-
-    // reborrow memory
-    let env = ctx.data();
-    let memory = env.memory_view(&ctx);
-
-    // get the values written to memory
-    let new_filestat = wasi_try_mem!(new_buf.deref(&memory).read());
-    // translate the new struct into the old struct in host memory
-    let old_stat = Snapshot0Filestat {
-        st_dev: new_filestat.st_dev,
-        st_ino: new_filestat.st_ino,
-        st_filetype: new_filestat.st_filetype,
-        st_nlink: new_filestat.st_nlink as u32,
-        st_size: new_filestat.st_size,
-        st_atim: new_filestat.st_atim,
-        st_mtim: new_filestat.st_mtim,
-        st_ctim: new_filestat.st_ctim,
-    };
-
-    // write back the original values at the pointer's memory locations
-    // (including the memory unrelated to the pointer)
-    wasi_try_mem!(new_buf.deref(&memory).write(new_filestat_setup));
-
-    // Now that this memory is back as it was, write the translated filestat
-    // into memory leaving it as it should be
-    wasi_try_mem!(buf.deref(&memory).write(old_stat));
+    let result = syscalls::fd_filestat_get_old::<Memory32>(ctx.as_mut(), fd, buf);
 
     result
 }
 
-/// Wrapper around `syscalls::path_filestat_get` with extra logic to handle the size
-/// difference of `wasi_filestat_t`
+/// Wrapper around `syscalls::path_filestat_get` for old Snapshot0
 #[instrument(level = "debug", skip_all, ret)]
 pub fn path_filestat_get(
     mut ctx: FunctionEnvMut<WasiEnv>,
@@ -83,35 +39,11 @@ pub fn path_filestat_get(
     path_len: u32,
     buf: WasmPtr<Snapshot0Filestat, Memory32>,
 ) -> Errno {
-    // TODO: understand what's happening inside this function, then do the correct thing
-
-    // see `fd_filestat_get` in this file for an explanation of this strange behavior
     let env = ctx.data();
     let memory = env.memory_view(&ctx);
 
-    let new_buf: WasmPtr<Filestat, Memory32> = buf.cast();
-    let new_filestat_setup: Filestat = wasi_try_mem!(new_buf.read(&memory));
-
     let result =
-        syscalls::path_filestat_get::<Memory32>(ctx.as_mut(), fd, flags, path, path_len, new_buf);
-
-    // need to re-borrow
-    let env = ctx.data();
-    let memory = env.memory_view(&ctx);
-    let new_filestat = wasi_try_mem!(new_buf.deref(&memory).read());
-    let old_stat = Snapshot0Filestat {
-        st_dev: new_filestat.st_dev,
-        st_ino: new_filestat.st_ino,
-        st_filetype: new_filestat.st_filetype,
-        st_nlink: new_filestat.st_nlink as u32,
-        st_size: new_filestat.st_size,
-        st_atim: new_filestat.st_atim,
-        st_mtim: new_filestat.st_mtim,
-        st_ctim: new_filestat.st_ctim,
-    };
-
-    wasi_try_mem!(new_buf.deref(&memory).write(new_filestat_setup));
-    wasi_try_mem!(buf.deref(&memory).write(old_stat));
+        syscalls::path_filestat_get_old::<Memory32>(ctx.as_mut(), fd, flags, path, path_len, buf);
 
     result
 }

lib/wasi/src/syscalls/wasi/fd_filestat_get.rs

@@ -1,5 +1,6 @@
 use super::*;
 use crate::syscalls::*;
+use crate::types::wasi::Snapshot0Filestat;
 
 /// ### `fd_filestat_get()`
 /// Get the metadata of an open file
@@ -15,7 +16,14 @@ pub fn fd_filestat_get<M: MemorySize>(
     fd: WasiFd,
     buf: WasmPtr<Filestat, M>,
 ) -> Errno {
-    fd_filestat_get_internal(&mut ctx, fd, buf)
+    let stat = wasi_try!(fd_filestat_get_internal(&mut ctx, fd));
+
+    let env = ctx.data();
+    let (memory, _) = env.get_memory_and_wasi_state(&ctx, 0);
+    let buf = buf.deref(&memory);
+    wasi_try_mem!(buf.write(stat));
+
+    Errno::Success
 }
 
 /// ### `fd_filestat_get()`
@@ -26,22 +34,51 @@ pub fn fd_filestat_get<M: MemorySize>(
 /// Output:
 /// - `__wasi_filestat_t *buf`
 ///     Where the metadata from `fd` will be written
-pub(crate) fn fd_filestat_get_internal<M: MemorySize>(
+pub(crate) fn fd_filestat_get_internal(
     ctx: &mut FunctionEnvMut<'_, WasiEnv>,
     fd: WasiFd,
-    buf: WasmPtr<Filestat, M>,
-) -> Errno {
+) -> Result<Filestat, Errno> {
     let env = ctx.data();
-    let (memory, mut state, inodes) = env.get_memory_and_wasi_state_and_inodes(&ctx, 0);
-    let fd_entry = wasi_try!(state.fs.get_fd(fd));
+    let (_, mut state, inodes) = env.get_memory_and_wasi_state_and_inodes(&ctx, 0);
+    let fd_entry = state.fs.get_fd(fd)?;
     if !fd_entry.rights.contains(Rights::FD_FILESTAT_GET) {
-        return Errno::Access;
+        return Err(Errno::Access);
     }
 
-    let stat = wasi_try!(state.fs.filestat_fd(fd));
+    state.fs.filestat_fd(fd)
+}
+
+/// ### `fd_filestat_get_old()`
+/// Get the metadata of an open file
+/// Input:
+/// - `Fd fd`
+///     The open file descriptor whose metadata will be read
+/// Output:
+/// - `Snapshot0Filestat *buf`
+///     Where the metadata from `fd` will be written
+#[instrument(level = "debug", skip_all, fields(%fd), ret)]
+pub fn fd_filestat_get_old<M: MemorySize>(
+    mut ctx: FunctionEnvMut<'_, WasiEnv>,
+    fd: WasiFd,
+    buf: WasmPtr<Snapshot0Filestat, M>,
+) -> Errno {
+    let stat = wasi_try!(fd_filestat_get_internal(&mut ctx, fd));
+
+    let env = ctx.data();
+    let (memory, _) = env.get_memory_and_wasi_state(&ctx, 0);
+    let old_stat = Snapshot0Filestat {
+        st_dev: stat.st_dev,
+        st_ino: stat.st_ino,
+        st_filetype: stat.st_filetype,
+        st_nlink: stat.st_nlink as u32,
+        st_size: stat.st_size,
+        st_atim: stat.st_atim,
+        st_mtim: stat.st_mtim,
+        st_ctim: stat.st_ctim,
+    };
 
     let buf = buf.deref(&memory);
-    wasi_try_mem!(buf.write(stat));
+    wasi_try_mem!(buf.write(old_stat));
 
     Errno::Success
 }

lib/wasi/src/syscalls/wasi/path_filestat_get.rs

@@ -1,5 +1,6 @@
 use super::*;
 use crate::syscalls::*;
+use crate::types::wasi::Snapshot0Filestat;
 
 /// ### `path_filestat_get()`
 /// Access metadata about a file or directory
@@ -49,20 +50,8 @@ pub fn path_filestat_get<M: MemorySize>(
     Errno::Success
 }
 
-/// ### `path_filestat_get()`
-/// Access metadata about a file or directory
-/// Inputs:
-/// - `Fd fd`
-///     The directory that `path` is relative to
-/// - `LookupFlags flags`
-///     Flags to control how `path` is understood
-/// - `const char *path`
-///     String containing the file path
-/// - `u32 path_len`
-///     The length of the `path` string
-/// Output:
-/// - `__wasi_file_stat_t *buf`
-///     The location where the metadata will be stored
+/// ### `path_filestat_get_internal()`
+/// return a Filstat or Errno
 pub(crate) fn path_filestat_get_internal(
     memory: &MemoryView,
     state: &WasiState,
@@ -92,3 +81,61 @@ pub(crate) fn path_filestat_get_internal(
     stat.st_ino = st_ino;
     Ok(stat)
 }
+
+/// ### `path_filestat_get_old()`
+/// Access metadata about a file or directory
+/// Inputs:
+/// - `Fd fd`
+///     The directory that `path` is relative to
+/// - `LookupFlags flags`
+///     Flags to control how `path` is understood
+/// - `const char *path`
+///     String containing the file path
+/// - `u32 path_len`
+///     The length of the `path` string
+/// Output:
+/// - `__wasi_file_stat_t *buf`
+///     The location where the metadata will be stored
+#[instrument(level = "trace", skip_all, fields(%fd, path = field::Empty), ret)]
+pub fn path_filestat_get_old<M: MemorySize>(
+    ctx: FunctionEnvMut<'_, WasiEnv>,
+    fd: WasiFd,
+    flags: LookupFlags,
+    path: WasmPtr<u8, M>,
+    path_len: M::Offset,
+    buf: WasmPtr<Snapshot0Filestat, M>,
+) -> Errno {
+    let env = ctx.data();
+    let (memory, mut state, inodes) = env.get_memory_and_wasi_state_and_inodes(&ctx, 0);
+
+    let mut path_string = unsafe { get_input_str!(&memory, path, path_len) };
+
+    // Convert relative paths into absolute paths
+    if path_string.starts_with("./") {
+        path_string = ctx.data().state.fs.relative_path_to_absolute(path_string);
+    }
+    tracing::trace!(path = path_string.as_str());
+
+    let stat = wasi_try!(path_filestat_get_internal(
+        &memory,
+        state,
+        inodes,
+        fd,
+        flags,
+        &path_string
+    ));
+
+    let old_stat = Snapshot0Filestat {
+        st_dev: stat.st_dev,
+        st_ino: stat.st_ino,
+        st_filetype: stat.st_filetype,
+        st_nlink: stat.st_nlink as u32,
+        st_size: stat.st_size,
+        st_atim: stat.st_atim,
+        st_mtim: stat.st_mtim,
+        st_ctim: stat.st_ctim,
+    };
+    wasi_try_mem!(buf.deref(&memory).write(old_stat));
+
+    Errno::Success
+}