Merge #449

449: avoid opening files when not needed in WASI, check for write permissions r=MarkMcCaskey a=MarkMcCaskey resolves #438 Follow up to #448. Turns out we don't have to complect things in that way, we can just be more conservative about opening files and granting write permissions Co-authored-by: Mark McCaskey <[email protected]>
wasmerio · May 15, 2019 · 88124d9 · 88124d9
2 parents 245cc32 + 83deae8
commit 88124d9
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@ Blocks of changes will separated by version increments.
 
 ## **[Unreleased]**
 
+- [#449](https://github.com/wasmerio/wasmer/pull/449) Fix bugs: opening host files in filestat and opening with write permissions unconditionally in path_open
 - [#442](https://github.com/wasmerio/wasmer/pull/442) Misc. WASI FS fixes and implement readdir
 - [#440](https://github.com/wasmerio/wasmer/pull/440) Fix type mismatch between `wasmer_instance_call` and `wasmer_export_func_*_arity` functions in the runtime C API.
 - [#269](https://github.com/wasmerio/wasmer/pull/269) Add better runtime docs

diff --git a/lib/wasi/src/state.rs b/lib/wasi/src/state.rs
@@ -451,3 +451,16 @@ pub struct WasiState<'a> {
     pub args: &'a [Vec<u8>],
     pub envs: &'a [Vec<u8>],
 }
+
+pub fn host_file_type_to_wasi_file_type(file_type: fs::FileType) -> __wasi_filetype_t {
+    // TODO: handle other file types
+    if file_type.is_dir() {
+        __WASI_FILETYPE_DIRECTORY
+    } else if file_type.is_file() {
+        __WASI_FILETYPE_REGULAR_FILE
+    } else if file_type.is_symlink() {
+        __WASI_FILETYPE_SYMBOLIC_LINK
+    } else {
+        __WASI_FILETYPE_UNKNOWN
+    }
+}
diff --git a/lib/wasi/src/syscalls/mod.rs b/lib/wasi/src/syscalls/mod.rs
@@ -8,7 +8,9 @@ pub mod windows;
 use self::types::*;
 use crate::{
     ptr::{Array, WasmPtr},
-    state::{Fd, InodeVal, Kind, WasiFile, WasiState, MAX_SYMLINKS},
+    state::{
+        host_file_type_to_wasi_file_type, Fd, InodeVal, Kind, WasiFile, WasiState, MAX_SYMLINKS,
+    },
     ExitCode,
 };
 use rand::{thread_rng, Rng};
@@ -796,19 +798,9 @@ pub fn fd_readdir(
                 d_next: cur_cookie,
                 d_ino: 0, // TODO: inode
                 d_namlen: namlen as u32,
-                d_type: {
-                    let file_type = wasi_try!(entry.file_type().map_err(|_| __WASI_EIO));
-                    // TODO: handle other file types
-                    if file_type.is_dir() {
-                        __WASI_FILETYPE_DIRECTORY
-                    } else if file_type.is_file() {
-                        __WASI_FILETYPE_REGULAR_FILE
-                    } else if file_type.is_symlink() {
-                        __WASI_FILETYPE_SYMBOLIC_LINK
-                    } else {
-                        __WASI_FILETYPE_UNKNOWN
-                    }
-                },
+                d_type: host_file_type_to_wasi_file_type(wasi_try!(entry
+                    .file_type()
+                    .map_err(|_| __WASI_EIO))),
             };
             let dirent_bytes = dirent_to_le_bytes(&dirent);
             let upper_limit = std::cmp::min(
@@ -1219,64 +1211,31 @@ pub fn path_filestat_get(
         }
     }
 
-    let final_inode = match &state.fs.inodes[inode].kind {
+    let stat = match &state.fs.inodes[inode].kind {
         Kind::Dir { path, entries, .. } => {
-            // TODO: fail earlier if size 0
+            // read it from internal data structures if we can
             let last_segment = path_vec.last().unwrap();
             cumulative_path.push(last_segment);
 
             if entries.contains_key(last_segment) {
-                entries[last_segment]
+                state.fs.inodes[entries[last_segment]].stat
             } else {
-                // lazily load it if we can
+                // otherwise read it from the host FS
                 if !cumulative_path.exists() {
                     return __WASI_ENOENT;
                 }
                 let final_path_metadata =
                     wasi_try!(cumulative_path.metadata().map_err(|_| __WASI_EIO));
-                let new_inode = if final_path_metadata.is_dir() {
-                    debug!("Opening host directory {:#?}", &cumulative_path);
-                    state.fs.inodes.insert(InodeVal {
-                        stat: __wasi_filestat_t::default(),
-                        is_preopened: false, // is this correct?
-                        name: last_segment.clone(),
-                        kind: Kind::Dir {
-                            parent: Some(inode),
-                            path: std::path::PathBuf::from(&last_segment),
-                            entries: Default::default(),
-                        },
-                    })
-                } else {
-                    debug!("Opening host file {:#?}", &cumulative_path);
-                    let real_open_file = wasi_try!(std::fs::OpenOptions::new()
-                        .read(true)
-                        .write(true)
-                        .open(&cumulative_path)
-                        .map_err(|_| __WASI_ENOENT));
-
-                    state.fs.inodes.insert(InodeVal {
-                        stat: __wasi_filestat_t::default(),
-                        is_preopened: false, // is this correct?
-                        name: last_segment.clone(),
-                        kind: Kind::File {
-                            handle: WasiFile::HostFile(real_open_file),
-                        },
-                    })
-                };
-                // reborrow to insert entry
-                if let Kind::Dir { entries, .. } = &mut state.fs.inodes[inode].kind {
-                    entries.insert(last_segment.clone(), new_inode);
+                __wasi_filestat_t {
+                    st_filetype: host_file_type_to_wasi_file_type(final_path_metadata.file_type()),
+                    ..Default::default()
                 }
-                new_inode
             }
         }
         _ => {
             return __WASI_ENOTDIR;
         }
     };
-
-    let stat = state.fs.inodes[final_inode].stat;
-
     buf_cell.set(stat);
 
     __WASI_ESUCCESS
@@ -1540,7 +1499,12 @@ pub fn path_open(
                 // file is not a dir
                 let real_opened_file = {
                     let mut open_options = std::fs::OpenOptions::new();
-                    let open_options = open_options.read(true).write(true);
+                    let open_options = open_options.read(true);
+                    let open_options = if fs_rights_base & __WASI_RIGHT_FD_WRITE != 0 {
+                        open_options.write(true)
+                    } else {
+                        open_options
+                    };
                     let open_options = if o_flags & __WASI_O_CREAT != 0 {
                         debug!(
                             "File {:?} may be created when opened if it does not exist",