Merge #897

897: Remove special casing of stdin, stdout, and stderr in WASI FS r=MarkMcCaskey a=MarkMcCaskey


# Description
Properly fixes closing stdin, stdout, stderr.  Cleans up this part of the code while it's at it.

This PR introduces breaking changes to the pubic WASI API

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <mark@wasmer.io>

CHANGELOG.md

@@ -6,6 +6,7 @@ Blocks of changes will separated by version increments.
 
 ## **[Unreleased]**
 
+- [#897](https://github.com/wasmerio/wasmer/pull/897) Removes special casing of stdin, stdout, and stderr in WASI.  Closing these files now works.  Removes `stdin`, `stdout`, and `stderr` from `WasiFS`, replaced by the methods `stdout`, `stdout_mut`, and so on.
 - [#863](https://github.com/wasmerio/wasmer/pull/863) Fix min and max for cases involving NaN and negative zero when using the LLVM backend.
 
 ## 0.8.0 - 2019-10-02

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

@@ -0,0 +1,18 @@
+// !!! THIS IS A GENERATED FILE !!!
+// ANY MANUAL EDITS MAY BE OVERWRITTEN AT ANY TIME
+// Files autogenerated with cargo build (build/wasitests.rs).
+
+#[test]
+fn test_fd_close() {
+    assert_wasi_output!(
+        "../../wasitests/fd_close.wasm",
+        "fd_close",
+        vec![],
+        vec![(
+            ".".to_string(),
+            ::std::path::PathBuf::from("wasitests/test_fs/hamlet")
+        ),],
+        vec![],
+        "../../wasitests/fd_close.out"
+    );
+}

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

@@ -9,6 +9,7 @@ mod close_preopen_fd;
 mod create_dir;
 mod envvar;
 mod fd_allocate;
+mod fd_close;
 mod fd_pread;
 mod fd_read;
 mod fd_sync;

lib/wasi-tests/wasitests/fd_close.out

@@ -0,0 +1,3 @@
+Successfully closed file!
+Successfully closed stderr!
+Successfully closed stdin!

lib/wasi-tests/wasitests/fd_close.rs

@@ -0,0 +1,63 @@
+// Args:
+// mapdir: .:wasitests/test_fs/hamlet
+
+use std::fs;
+#[cfg(target_os = "wasi")]
+use std::os::wasi::prelude::AsRawFd;
+use std::path::PathBuf;
+
+#[cfg(target_os = "wasi")]
+#[link(wasm_import_module = "wasi_unstable")]
+extern "C" {
+    fn fd_close(fd: u32) -> u16;
+}
+
+fn main() {
+    #[cfg(not(target_os = "wasi"))]
+    let mut base = PathBuf::from("wasitests/test_fs/hamlet");
+    #[cfg(target_os = "wasi")]
+    let mut base = PathBuf::from(".");
+
+    base.push("act3/scene3.txt");
+    let file = fs::File::open(&base).expect("could not open file");
+
+    #[cfg(target_os = "wasi")]
+    {
+        let file_fd = file.as_raw_fd();
+        let stdout_fd = std::io::stdout().as_raw_fd();
+        let stderr_fd = std::io::stderr().as_raw_fd();
+        let stdin_fd = std::io::stdin().as_raw_fd();
+
+        let result = unsafe { fd_close(file_fd) };
+        if result == 0 {
+            println!("Successfully closed file!")
+        } else {
+            println!("Could not close file");
+        }
+
+        let result = unsafe { fd_close(stderr_fd) };
+        if result == 0 {
+            println!("Successfully closed stderr!")
+        } else {
+            println!("Could not close stderr");
+        }
+        let result = unsafe { fd_close(stdin_fd) };
+        if result == 0 {
+            println!("Successfully closed stdin!")
+        } else {
+            println!("Could not close stdin");
+        }
+        let result = unsafe { fd_close(stdout_fd) };
+        if result == 0 {
+            println!("Successfully closed stdout!")
+        } else {
+            println!("Could not close stdout");
+        }
+    }
+    #[cfg(not(target_os = "wasi"))]
+    {
+        println!("Successfully closed file!");
+        println!("Successfully closed stderr!");
+        println!("Successfully closed stdin!");
+    }
+}

lib/wasi/src/state/mod.rs

@@ -37,6 +37,19 @@ use wasmer_runtime_core::{debug, vm::Ctx};
 pub const VIRTUAL_ROOT_FD: __wasi_fd_t = 3;
 /// all the rights enabled
 pub const ALL_RIGHTS: __wasi_rights_t = 0x1FFFFFFF;
+const STDIN_DEFAULT_RIGHTS: __wasi_rights_t = __WASI_RIGHT_FD_DATASYNC
+    | __WASI_RIGHT_FD_READ
+    | __WASI_RIGHT_FD_SYNC
+    | __WASI_RIGHT_FD_ADVISE
+    | __WASI_RIGHT_FD_FILESTAT_GET
+    | __WASI_RIGHT_POLL_FD_READWRITE;
+const STDOUT_DEFAULT_RIGHTS: __wasi_rights_t = __WASI_RIGHT_FD_DATASYNC
+    | __WASI_RIGHT_FD_WRITE
+    | __WASI_RIGHT_FD_SYNC
+    | __WASI_RIGHT_FD_ADVISE
+    | __WASI_RIGHT_FD_FILESTAT_GET
+    | __WASI_RIGHT_POLL_FD_READWRITE;
+const STDERR_DEFAULT_RIGHTS: __wasi_rights_t = STDOUT_DEFAULT_RIGHTS;
 
 /// Get WasiState from a Ctx
 /// This function is unsafe because it must be called on a WASI Ctx
@@ -109,6 +122,7 @@ pub struct Fd {
     pub rights_inheriting: __wasi_rights_t,
     pub flags: __wasi_fdflags_t,
     pub offset: u64,
+    /// Used when reopening the file on the host system
     pub open_flags: u16,
     pub inode: Inode,
 }
@@ -134,10 +148,6 @@ pub struct WasiFs {
     inode_counter: Cell<u64>,
     /// for fds still open after the file has been deleted
     pub orphan_fds: HashMap<Inode, InodeVal>,
-
-    pub stdout: Box<dyn WasiFile>,
-    pub stderr: Box<dyn WasiFile>,
-    pub stdin: Box<dyn WasiFile>,
 }
 
 impl WasiFs {
@@ -155,11 +165,11 @@ impl WasiFs {
             next_fd: Cell::new(3),
             inode_counter: Cell::new(1024),
             orphan_fds: HashMap::new(),
-
-            stdin: Box::new(Stdin),
-            stdout: Box::new(Stdout),
-            stderr: Box::new(Stderr),
         };
+        wasi_fs.create_stdin();
+        wasi_fs.create_stdout();
+        wasi_fs.create_stderr();
+
         // create virtual root
         let root_inode = {
             let all_rights = 0x1FFFFFFF;
@@ -291,6 +301,67 @@ impl WasiFs {
         Ok(wasi_fs)
     }
 
+    /// Get the `WasiFile` object at stdout
+    pub fn stdout(&self) -> Result<&Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get(__WASI_STDOUT_FILENO)
+    }
+    /// Get the `WasiFile` object at stdout mutably
+    pub fn stdout_mut(&mut self) -> Result<&mut Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get_mut(__WASI_STDOUT_FILENO)
+    }
+
+    /// Get the `WasiFile` object at stderr
+    pub fn stderr(&self) -> Result<&Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get(__WASI_STDERR_FILENO)
+    }
+    /// Get the `WasiFile` object at stderr mutably
+    pub fn stderr_mut(&mut self) -> Result<&mut Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get_mut(__WASI_STDERR_FILENO)
+    }
+
+    /// Get the `WasiFile` object at stdin
+    pub fn stdin(&self) -> Result<&Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get(__WASI_STDIN_FILENO)
+    }
+    /// Get the `WasiFile` object at stdin mutably
+    pub fn stdin_mut(&mut self) -> Result<&mut Option<Box<dyn WasiFile>>, WasiFsError> {
+        self.std_dev_get_mut(__WASI_STDIN_FILENO)
+    }
+
+    /// Internal helper function to get a standard device handle.
+    /// Expects one of `__WASI_STDIN_FILENO`, `__WASI_STDOUT_FILENO`, `__WASI_STDERR_FILENO`.
+    fn std_dev_get(&self, fd: __wasi_fd_t) -> Result<&Option<Box<dyn WasiFile>>, WasiFsError> {
+        if let Some(fd) = self.fd_map.get(&fd) {
+            if let Kind::File { ref handle, .. } = self.inodes[fd.inode].kind {
+                Ok(handle)
+            } else {
+                // Our public API should ensure that this is not possible
+                unreachable!("Non-file found in standard device location")
+            }
+        } else {
+            // this should only trigger if we made a mistake in this crate
+            Err(WasiFsError::NoDevice)
+        }
+    }
+    /// Internal helper function to mutably get a standard device handle.
+    /// Expects one of `__WASI_STDIN_FILENO`, `__WASI_STDOUT_FILENO`, `__WASI_STDERR_FILENO`.
+    fn std_dev_get_mut(
+        &mut self,
+        fd: __wasi_fd_t,
+    ) -> Result<&mut Option<Box<dyn WasiFile>>, WasiFsError> {
+        if let Some(fd) = self.fd_map.get_mut(&fd) {
+            if let Kind::File { ref mut handle, .. } = self.inodes[fd.inode].kind {
+                Ok(handle)
+            } else {
+                // Our public API should ensure that this is not possible
+                unreachable!("Non-file found in standard device location")
+            }
+        } else {
+            // this should only trigger if we made a mistake in this crate
+            Err(WasiFsError::NoDevice)
+        }
+    }
+
     fn get_next_inode_index(&mut self) -> u64 {
         let next = self.inode_counter.get();
         self.inode_counter.set(next + 1);
@@ -358,36 +429,31 @@ impl WasiFs {
         fd: __wasi_fd_t,
         file: Box<dyn WasiFile>,
     ) -> Result<Option<Box<dyn WasiFile>>, WasiFsError> {
+        let mut ret = Some(file);
         match fd {
             __WASI_STDIN_FILENO => {
-                let mut ret = file;
-                std::mem::swap(&mut self.stdin, &mut ret);
-                Ok(Some(ret))
+                std::mem::swap(self.stdin_mut()?, &mut ret);
             }
             __WASI_STDOUT_FILENO => {
-                let mut ret = file;
-                std::mem::swap(&mut self.stdout, &mut ret);
-                Ok(Some(ret))
+                std::mem::swap(self.stdout_mut()?, &mut ret);
             }
             __WASI_STDERR_FILENO => {
-                let mut ret = file;
-                std::mem::swap(&mut self.stderr, &mut ret);
-                Ok(Some(ret))
+                std::mem::swap(self.stderr_mut()?, &mut ret);
             }
             _ => {
                 let base_fd = self.get_fd(fd).map_err(WasiFsError::from_wasi_err)?;
                 let base_inode = base_fd.inode;
 
                 match &mut self.inodes[base_inode].kind {
                     Kind::File { ref mut handle, .. } => {
-                        let mut ret = Some(file);
                         std::mem::swap(handle, &mut ret);
-                        Ok(ret)
                     }
                     _ => return Err(WasiFsError::NotAFile),
                 }
             }
         }
+
+        Ok(ret)
     }
 
     /// refresh size from filesystem
@@ -733,6 +799,17 @@ impl WasiFs {
     }
 
     pub fn fdstat(&self, fd: __wasi_fd_t) -> Result<__wasi_fdstat_t, __wasi_errno_t> {
+        match fd {
+            __WASI_STDOUT_FILENO => {
+                return Ok(__wasi_fdstat_t {
+                    fs_filetype: __WASI_FILETYPE_CHARACTER_DEVICE,
+                    fs_flags: 0,
+                    fs_rights_base: ALL_RIGHTS,
+                    fs_rights_inheriting: ALL_RIGHTS,
+                })
+            }
+            _ => (),
+        }
         let fd = self.get_fd(fd)?;
 
         debug!("fdstat: {:?}", fd);
@@ -773,8 +850,18 @@ impl WasiFs {
     pub fn flush(&mut self, fd: __wasi_fd_t) -> Result<(), __wasi_errno_t> {
         match fd {
             __WASI_STDIN_FILENO => (),
-            __WASI_STDOUT_FILENO => self.stdout.flush().map_err(|_| __WASI_EIO)?,
-            __WASI_STDERR_FILENO => self.stderr.flush().map_err(|_| __WASI_EIO)?,
+            __WASI_STDOUT_FILENO => self
+                .stdout_mut()
+                .map_err(WasiFsError::into_wasi_err)?
+                .as_mut()
+                .and_then(|f| f.flush().ok())
+                .ok_or(__WASI_EIO)?,
+            __WASI_STDERR_FILENO => self
+                .stderr_mut()
+                .map_err(WasiFsError::into_wasi_err)?
+                .as_mut()
+                .and_then(|f| f.flush().ok())
+                .ok_or(__WASI_EIO)?,
             _ => {
                 let fd = self.fd_map.get(&fd).ok_or(__WASI_EBADF)?;
                 if fd.rights & __WASI_RIGHT_FD_DATASYNC == 0 {
@@ -881,13 +968,78 @@ impl WasiFs {
         };
 
         self.inodes.insert(InodeVal {
-            stat: stat,
+            stat,
             is_preopened: true,
             name: "/".to_string(),
             kind: root_kind,
         })
     }
 
+    fn create_stdout(&mut self) {
+        self.create_std_dev_inner(
+            Box::new(Stdout),
+            "stdout",
+            __WASI_STDOUT_FILENO,
+            STDOUT_DEFAULT_RIGHTS,
+            __WASI_FDFLAG_APPEND,
+        );
+    }
+    fn create_stdin(&mut self) {
+        self.create_std_dev_inner(
+            Box::new(Stdin),
+            "stdin",
+            __WASI_STDIN_FILENO,
+            STDIN_DEFAULT_RIGHTS,
+            0,
+        );
+    }
+    fn create_stderr(&mut self) {
+        self.create_std_dev_inner(
+            Box::new(Stderr),
+            "stderr",
+            __WASI_STDERR_FILENO,
+            STDERR_DEFAULT_RIGHTS,
+            __WASI_FDFLAG_APPEND,
+        );
+    }
+
+    fn create_std_dev_inner(
+        &mut self,
+        handle: Box<dyn WasiFile>,
+        name: &'static str,
+        raw_fd: __wasi_fd_t,
+        rights: __wasi_rights_t,
+        fd_flags: __wasi_fdflags_t,
+    ) {
+        let stat = __wasi_filestat_t {
+            st_filetype: __WASI_FILETYPE_CHARACTER_DEVICE,
+            st_ino: self.get_next_inode_index(),
+            ..__wasi_filestat_t::default()
+        };
+        let kind = Kind::File {
+            handle: Some(handle),
+            path: "".into(),
+        };
+        let inode = self.inodes.insert(InodeVal {
+            stat,
+            is_preopened: true,
+            name: name.to_string(),
+            kind,
+        });
+        self.fd_map.insert(
+            raw_fd,
+            Fd {
+                rights,
+                rights_inheriting: 0,
+                flags: fd_flags,
+                // since we're not calling open on this, we don't need open flags
+                open_flags: 0,
+                offset: 0,
+                inode,
+            },
+        );
+    }
+
     pub fn get_stat_for_kind(&self, kind: &Kind) -> Option<__wasi_filestat_t> {
         let md = match kind {
             Kind::File { handle, path } => match handle {