Merge #643

643: Improve error reporting of IO errors, implement path_symlink r=MarkMcCaskey a=MarkMcCaskey



Co-authored-by: Mark McCaskey <[email protected]>

CHANGELOG.md

@@ -8,6 +8,7 @@ Blocks of changes will separated by version increments.
 
 Special thanks to @YaronWittenstein @penberg for their contributions.
 
+- [#643](https://github.com/wasmerio/wasmer/issues/643) Implement `wasi::path_symlink` and improve WASI FS public api IO error reporting
 - [#608](https://github.com/wasmerio/wasmer/issues/608) Implement wasi syscalls `fd_allocate`, `fd_sync`, `fd_pread`, `path_link`, `path_filestat_set_times`; update WASI fs API in a WIP way; reduce coupling of WASI code to host filesystem; make debug messages from WASI more readable; improve rights-checking when calling syscalls; implement reference counting on inodes; misc bug fixes and improvements
 - [#616](https://github.com/wasmerio/wasmer/issues/616) Create the import object separately from instance instantiation in `runtime-c-api`
 - [#620](https://github.com/wasmerio/wasmer/issues/620) Replace one `throw()` with `noexcept` in llvm backend

lib/wasi-tests/tests/wasitests/mod.rs

@@ -16,6 +16,7 @@ mod fseek;
 mod hello;
 mod mapdir;
 mod path_link;
+mod path_symlink;
 mod quine;
 mod readlink;
 mod wasi_sees_virtual_root;

lib/wasi-tests/tests/wasitests/path_symlink.rs

@@ -0,0 +1,20 @@
+#[test]
+fn test_path_symlink() {
+    assert_wasi_output!(
+        "../../wasitests/path_symlink.wasm",
+        "path_symlink",
+        vec![],
+        vec![
+            (
+                "temp".to_string(),
+                ::std::path::PathBuf::from("wasitests/test_fs/temp")
+            ),
+            (
+                "hamlet".to_string(),
+                ::std::path::PathBuf::from("wasitests/test_fs/hamlet")
+            ),
+        ],
+        vec![],
+        "../../wasitests/path_symlink.out"
+    );
+}

lib/wasi-tests/wasitests/path_symlink.out

@@ -0,0 +1,4 @@
+ACT III
+SCENE I. A room in the castle.
+
+    Enter KING CLAUDIUS,

lib/wasi-tests/wasitests/path_symlink.rs

@@ -0,0 +1,30 @@
+// Args:
+// mapdir: temp:wasitests/test_fs/temp
+// mapdir: hamlet:wasitests/test_fs/hamlet
+
+use std::fs;
+use std::io::{Read, Seek, SeekFrom};
+use std::path::PathBuf;
+
+fn main() {
+    #[cfg(not(target_os = "wasi"))]
+    let mut base = PathBuf::from("wasitests/test_fs");
+    #[cfg(target_os = "wasi")]
+    let mut base = PathBuf::from("/");
+
+    let symlink_loc = base.join("temp/act3");
+    let symlink_target = "../hamlet/act3";
+    let scene1 = symlink_loc.join("scene1.txt");
+
+    std::fs::soft_link(&symlink_target, &symlink_loc);
+
+    let mut file = fs::File::open(&scene1).expect("Could not open file");
+
+    let mut buffer = [0u8; 64];
+
+    assert_eq!(file.read(&mut buffer).unwrap(), 64);
+    let str_val = std::str::from_utf8(&buffer[..]).unwrap();
+    println!("{}", str_val);
+
+    std::fs::remove_file(symlink_loc).unwrap();
+}

lib/wasi/src/state/mod.rs

@@ -393,8 +393,11 @@ impl WasiFs {
         let path: &Path = Path::new(path);
 
         let mut cur_inode = base_dir.inode;
+        let n_components = path.components().count();
         // TODO: rights checks
-        'path_iter: for component in path.components() {
+        'path_iter: for (i, component) in path.components().enumerate() {
+            // used to terminate symlink resolution properly
+            let last_component = i + 1 == n_components;
             // for each component traverse file structure
             // loading inodes as necessary
             'symlink_resolution: while symlink_count < MAX_SYMLINKS {
@@ -430,7 +433,6 @@ impl WasiFs {
                                 cd.push(component);
                                 cd
                             };
-                            // TODO: verify this returns successfully when given a non-symlink
                             let metadata = file.symlink_metadata().ok().ok_or(__WASI_EINVAL)?;
                             let file_type = metadata.file_type();
                             // we want to insert newly opened dirs and files, but not transient symlinks
@@ -488,6 +490,7 @@ impl WasiFs {
                             cur_inode = new_inode;
 
                             if loop_for_symlink && follow_symlinks {
+                                debug!("Following symlink to {:?}", cur_inode);
                                 continue 'symlink_resolution;
                             }
                         }
@@ -530,14 +533,23 @@ impl WasiFs {
                             base.push(relative_path);
                             base.to_string_lossy().to_string()
                         };
+                        debug!("Following symlink recursively");
                         let symlink_inode = self.get_inode_at_path_inner(
                             new_base_dir,
                             &new_path,
                             symlink_count + 1,
                             follow_symlinks,
                         )?;
                         cur_inode = symlink_inode;
-                        //continue 'symlink_resolution;
+                        // if we're at the very end and we found a file, then we're done
+                        // TODO: figure out if this should also happen for directories?
+                        if let Kind::File { .. } = &self.inodes[cur_inode].kind {
+                            // check if on last step
+                            if last_component {
+                                break 'symlink_resolution;
+                            }
+                        }
+                        continue 'symlink_resolution;
                     }
                 }
                 break 'symlink_resolution;
@@ -571,6 +583,64 @@ impl WasiFs {
         Err(__WASI_EINVAL) // this may not make sense
     }
 
+    // if this is still dead code and the year is 2020 or later, please delete this function
+    #[allow(dead_code)]
+    pub(crate) fn path_relative_to_fd(
+        &self,
+        fd: __wasi_fd_t,
+        inode: Inode,
+    ) -> Result<PathBuf, __wasi_errno_t> {
+        let mut stack = vec![];
+        let base_fd = self.get_fd(fd)?;
+        let base_inode = base_fd.inode;
+        let mut cur_inode = inode;
+
+        while cur_inode != base_inode {
+            stack.push(self.inodes[cur_inode].name.clone());
+            match &self.inodes[cur_inode].kind {
+                Kind::Dir { parent, .. } => {
+                    if let Some(p) = parent {
+                        cur_inode = *p;
+                    }
+                }
+                _ => return Err(__WASI_EINVAL),
+            }
+        }
+
+        let mut out = PathBuf::new();
+        for p in stack.iter().rev() {
+            out.push(p);
+        }
+        Ok(out)
+    }
+
+    /// finds the number of directories between the fd and the inode if they're connected
+    /// expects inode to point to a directory
+    pub(crate) fn path_depth_from_fd(
+        &self,
+        fd: __wasi_fd_t,
+        inode: Inode,
+    ) -> Result<usize, __wasi_errno_t> {
+        let mut counter = 0;
+        let base_fd = self.get_fd(fd)?;
+        let base_inode = base_fd.inode;
+        let mut cur_inode = inode;
+
+        while cur_inode != base_inode {
+            counter += 1;
+            match &self.inodes[cur_inode].kind {
+                Kind::Dir { parent, .. } => {
+                    if let Some(p) = parent {
+                        cur_inode = *p;
+                    }
+                }
+                _ => return Err(__WASI_EINVAL),
+            }
+        }
+
+        Ok(counter)
+    }
+
     /// gets a host file from a base directory and a path
     /// this function ensures the fs remains sandboxed
     // NOTE: follow symlinks is super weird right now
@@ -704,6 +774,24 @@ impl WasiFs {
         }))
     }
 
+    /// creates an inode and inserts it given a Kind, does not assume the file exists to
+    pub fn create_inode_with_default_stat(
+        &mut self,
+        kind: Kind,
+        is_preopened: bool,
+        name: String,
+    ) -> Inode {
+        let mut stat = __wasi_filestat_t::default();
+        stat.st_ino = self.get_next_inode_index();
+
+        self.inodes.insert(InodeVal {
+            stat,
+            is_preopened,
+            name,
+            kind,
+        })
+    }
+
     pub fn create_fd(
         &mut self,
         rights: __wasi_rights_t,

lib/wasi/src/state/types.rs

@@ -8,7 +8,7 @@ use std::{
 };
 
 /// Error type for external users
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
 #[allow(dead_code)]
 // dead code beacuse this is for external use
 pub enum WasiFsError {
@@ -23,6 +23,38 @@ pub enum WasiFsError {
     /// Something failed when doing IO. These errors can generally not be handled.
     /// It may work if tried again.
     IOError,
+    /// The address was in use
+    AddressInUse,
+    /// The address could not be found
+    AddressNotAvailable,
+    /// A pipe was closed
+    BrokenPipe,
+    /// The connection was aborted
+    ConnectionAborted,
+    /// The connection request was refused
+    ConnectionRefused,
+    /// The connection was reset
+    ConnectionReset,
+    /// The operation was interrupted before it could finish
+    Interrupted,
+    /// Invalid internal data, if the argument data is invalid, use `InvalidInput`
+    InvalidData,
+    /// The provided data is invalid
+    InvalidInput,
+    /// Could not perform the operation because there was not an open connection
+    NotConnected,
+    /// The requested file or directory could not be found
+    EntityNotFound,
+    /// Caller was not allowed to perform this operation
+    PermissionDenied,
+    /// The operation did not complete within the given amount of time
+    TimedOut,
+    /// Found EOF when EOF was not expected
+    UnexpectedEof,
+    /// Operation would block, this error lets the caller know that they can try again
+    WouldBlock,
+    /// A call to write returned 0
+    WriteZero,
     /// A WASI error without an external name.  If you encounter this it means
     /// that there's probably a bug on our side (maybe as simple as forgetting to wrap
     /// this error, but perhaps something broke)
@@ -35,9 +67,51 @@ impl WasiFsError {
             __WASI_EBADF => WasiFsError::InvalidFd,
             __WASI_EEXIST => WasiFsError::AlreadyExists,
             __WASI_EIO => WasiFsError::IOError,
+            __WASI_EADDRINUSE => WasiFsError::AddressInUse,
+            __WASI_EADDRNOTAVAIL => WasiFsError::AddressNotAvailable,
+            __WASI_EPIPE => WasiFsError::BrokenPipe,
+            __WASI_ECONNABORTED => WasiFsError::ConnectionAborted,
+            __WASI_ECONNREFUSED => WasiFsError::ConnectionRefused,
+            __WASI_ECONNRESET => WasiFsError::ConnectionReset,
+            __WASI_EINTR => WasiFsError::Interrupted,
+            __WASI_EINVAL => WasiFsError::InvalidInput,
+            __WASI_ENOTCONN => WasiFsError::NotConnected,
+            __WASI_ENOENT => WasiFsError::EntityNotFound,
+            __WASI_EPERM => WasiFsError::PermissionDenied,
+            __WASI_ETIMEDOUT => WasiFsError::TimedOut,
+            __WASI_EPROTO => WasiFsError::UnexpectedEof,
+            __WASI_EAGAIN => WasiFsError::WouldBlock,
+            __WASI_ENOSPC => WasiFsError::WriteZero,
             _ => WasiFsError::UnknownError(err),
         }
     }
+
+    pub fn into_wasi_err(self) -> __wasi_errno_t {
+        match self {
+            WasiFsError::AlreadyExists => __WASI_EEXIST,
+            WasiFsError::AddressInUse => __WASI_EADDRINUSE,
+            WasiFsError::AddressNotAvailable => __WASI_EADDRNOTAVAIL,
+            WasiFsError::BaseNotDirectory => __WASI_ENOTDIR,
+            WasiFsError::BrokenPipe => __WASI_EPIPE,
+            WasiFsError::ConnectionAborted => __WASI_ECONNABORTED,
+            WasiFsError::ConnectionRefused => __WASI_ECONNREFUSED,
+            WasiFsError::ConnectionReset => __WASI_ECONNRESET,
+            WasiFsError::Interrupted => __WASI_EINTR,
+            WasiFsError::InvalidData => __WASI_EIO,
+            WasiFsError::InvalidFd => __WASI_EBADF,
+            WasiFsError::InvalidInput => __WASI_EINVAL,
+            WasiFsError::IOError => __WASI_EIO,
+            WasiFsError::NotAFile => __WASI_EINVAL,
+            WasiFsError::NotConnected => __WASI_ENOTCONN,
+            WasiFsError::EntityNotFound => __WASI_ENOENT,
+            WasiFsError::PermissionDenied => __WASI_EPERM,
+            WasiFsError::TimedOut => __WASI_ETIMEDOUT,
+            WasiFsError::UnexpectedEof => __WASI_EPROTO,
+            WasiFsError::WouldBlock => __WASI_EAGAIN,
+            WasiFsError::WriteZero => __WASI_ENOSPC,
+            WasiFsError::UnknownError(ec) => ec,
+        }
+    }
 }
 
 /// This trait relies on your file closing when it goes out of scope via `Drop`
@@ -68,20 +142,20 @@ pub trait WasiFile: std::fmt::Debug + Write + Read + Seek {
     /// Change the size of the file, if the `new_size` is greater than the current size
     /// the extra bytes will be allocated and zeroed
     // TODO: stablize this in 0.7.0 by removing default impl
-    fn set_len(&mut self, _new_size: __wasi_filesize_t) -> Option<()> {
+    fn set_len(&mut self, _new_size: __wasi_filesize_t) -> Result<(), WasiFsError> {
         panic!("Default implementation for compatibilty in the 0.6.X releases; this will be removed in 0.7.0.  Please implement WasiFile::allocate for your type before then");
     }
 
     /// Request deletion of the file
     // TODO: break this out into a WasiPath trait which is dynamically in Kind::File
     // this change can't be done until before release
-    fn unlink(&mut self) -> Option<()> {
+    fn unlink(&mut self) -> Result<(), WasiFsError> {
         panic!("Default implementation for compatibilty in the 0.6.X releases; this will be removed in 0.7.0.  Please implement WasiFile::unlink for your type before then");
     }
 
     /// Store file contents and metadata to disk
     // TODO: stablize this in 0.7.0 by removing default impl
-    fn sync_to_disk(&self) -> Option<()> {
+    fn sync_to_disk(&self) -> Result<(), WasiFsError> {
         panic!("Default implementation for compatibilty in the 0.6.X releases; this will be removed in 0.7.0.  Please implement WasiFile::sync_to_disk for your type before then");
     }
 }
@@ -187,15 +261,42 @@ impl WasiFile for HostFile {
         self.metadata().len()
     }
 
-    fn set_len(&mut self, new_size: __wasi_filesize_t) -> Option<()> {
-        fs::File::set_len(&self.inner, new_size).ok()
+    fn set_len(&mut self, new_size: __wasi_filesize_t) -> Result<(), WasiFsError> {
+        fs::File::set_len(&self.inner, new_size).map_err(Into::into)
     }
 
-    fn unlink(&mut self) -> Option<()> {
-        std::fs::remove_file(&self.host_path).ok()
+    fn unlink(&mut self) -> Result<(), WasiFsError> {
+        std::fs::remove_file(&self.host_path).map_err(Into::into)
+    }
+    fn sync_to_disk(&self) -> Result<(), WasiFsError> {
+        self.inner.sync_all().map_err(Into::into)
     }
-    fn sync_to_disk(&self) -> Option<()> {
-        self.inner.sync_all().ok()
+}
+
+impl From<io::Error> for WasiFsError {
+    fn from(io_error: io::Error) -> Self {
+        match io_error.kind() {
+            io::ErrorKind::AddrInUse => WasiFsError::AddressInUse,
+            io::ErrorKind::AddrNotAvailable => WasiFsError::AddressNotAvailable,
+            io::ErrorKind::AlreadyExists => WasiFsError::AlreadyExists,
+            io::ErrorKind::BrokenPipe => WasiFsError::BrokenPipe,
+            io::ErrorKind::ConnectionAborted => WasiFsError::ConnectionAborted,
+            io::ErrorKind::ConnectionRefused => WasiFsError::ConnectionRefused,
+            io::ErrorKind::ConnectionReset => WasiFsError::ConnectionReset,
+            io::ErrorKind::Interrupted => WasiFsError::Interrupted,
+            io::ErrorKind::InvalidData => WasiFsError::InvalidData,
+            io::ErrorKind::InvalidInput => WasiFsError::InvalidInput,
+            io::ErrorKind::NotConnected => WasiFsError::NotConnected,
+            io::ErrorKind::NotFound => WasiFsError::EntityNotFound,
+            io::ErrorKind::PermissionDenied => WasiFsError::PermissionDenied,
+            io::ErrorKind::TimedOut => WasiFsError::TimedOut,
+            io::ErrorKind::UnexpectedEof => WasiFsError::UnexpectedEof,
+            io::ErrorKind::WouldBlock => WasiFsError::WouldBlock,
+            io::ErrorKind::WriteZero => WasiFsError::WriteZero,
+            io::ErrorKind::Other => WasiFsError::IOError,
+            // if the following triggers, a new error type was added to this non-exhaustive enum
+            _ => WasiFsError::UnknownError(__WASI_EIO),
+        }
     }
 }