Introduce strongly-typed system primitives

This commit does a lot of reshuffling and even some more. It introduces
strongly-typed system primitives which are: `OsFile`, `OsDir`, `Stdio`,
and `OsOther`. Those primitives are separate structs now, each implementing
a subset of `Handle` methods, rather than all being an enumeration of some
supertype such as `OsHandle`. To summarise the structs:

* `OsFile` represents a regular file, and implements fd-ops
  of `Handle` trait
* `OsDir` represents a directory, and primarily implements path-ops, plus
  `readdir` and some common fd-ops such as `fdstat`, etc.
* `Stdio` represents a stdio handle, and implements a subset of fd-ops
  such as `fdstat` _and_ `read_` and `write_vectored` calls
* `OsOther` currently represents anything else and implements a set similar
  to that implemented by `Stdio`

This commit is effectively an experiment and an excercise into better
understanding what's going on for each OS resource/type under-the-hood.
It's meant to give us some intuition in order to move on with the idea
of having strongly-typed handles in WASI both in the syscall impl as well
as at the libc level.

Some more minor changes include making `OsHandle` represent an OS-specific
wrapper for a raw OS handle (Unix fd or Windows handle). Also, since `OsDir`
is tricky across OSes, we also have a supertype of `OsHandle` called
`OsDirHandle` which may store a `DIR*` stream pointer (mainly BSD). Last but not
least, the `Filetype` and `Rights` are now computed when the resource is created,
rather than every time we call `Handle::get_file_type` and `Handle::get_rights`.
Finally, in order to facilitate the latter, I've converted `EntryRights` into
`HandleRights` and pushed them into each `Handle` implementor.

Author: Jakub Konka

crates/wasi-common/src/ctx.rs

@@ -1,13 +1,15 @@
 use crate::entry::{Entry, EntryHandle};
 use crate::fdpool::FdPool;
 use crate::handle::Handle;
-use crate::sys::oshandle::{OsHandle, OsHandleExt};
+use crate::sys::osfile::{OsFile, OsFileExt};
+use crate::sys::stdio::{Stdio, StdioExt};
 use crate::virtfs::{VirtualDir, VirtualDirEntry};
 use crate::wasi::types;
 use crate::wasi::{Errno, Result};
 use std::borrow::Borrow;
 use std::cell::RefCell;
 use std::collections::HashMap;
+use std::convert::TryFrom;
 use std::ffi::{self, CString, OsString};
 use std::fs::File;
 use std::path::{Path, PathBuf};
@@ -43,7 +45,7 @@ pub enum WasiCtxBuilderError {
 type WasiCtxBuilderResult<T> = std::result::Result<T, WasiCtxBuilderError>;
 
 enum PendingEntry {
-    Thunk(fn() -> io::Result<OsHandle>),
+    Thunk(fn() -> io::Result<Box<dyn Handle>>),
     File(File),
 }
 
@@ -53,7 +55,7 @@ impl std::fmt::Debug for PendingEntry {
             Self::Thunk(f) => write!(
                 fmt,
                 "PendingEntry::Thunk({:p})",
-                f as *const fn() -> io::Result<OsHandle>
+                f as *const fn() -> io::Result<Box<dyn Handle>>
             ),
             Self::File(f) => write!(fmt, "PendingEntry::File({:?})", f),
         }
@@ -118,9 +120,9 @@ pub struct WasiCtxBuilder {
 impl WasiCtxBuilder {
     /// Builder for a new `WasiCtx`.
     pub fn new() -> Self {
-        let stdin = Some(PendingEntry::Thunk(OsHandle::from_null));
-        let stdout = Some(PendingEntry::Thunk(OsHandle::from_null));
-        let stderr = Some(PendingEntry::Thunk(OsHandle::from_null));
+        let stdin = Some(PendingEntry::Thunk(OsFile::from_null));
+        let stdout = Some(PendingEntry::Thunk(OsFile::from_null));
+        let stderr = Some(PendingEntry::Thunk(OsFile::from_null));
 
         Self {
             stdin,
@@ -167,27 +169,27 @@ impl WasiCtxBuilder {
 
     /// Inherit stdin from the host process.
     pub fn inherit_stdin(&mut self) -> &mut Self {
-        self.stdin = Some(PendingEntry::Thunk(|| Ok(OsHandle::stdin())));
+        self.stdin = Some(PendingEntry::Thunk(Stdio::stdin));
         self
     }
 
     /// Inherit stdout from the host process.
     pub fn inherit_stdout(&mut self) -> &mut Self {
-        self.stdout = Some(PendingEntry::Thunk(|| Ok(OsHandle::stdout())));
+        self.stdout = Some(PendingEntry::Thunk(Stdio::stdout));
         self
     }
 
     /// Inherit stdout from the host process.
     pub fn inherit_stderr(&mut self) -> &mut Self {
-        self.stderr = Some(PendingEntry::Thunk(|| Ok(OsHandle::stderr())));
+        self.stderr = Some(PendingEntry::Thunk(Stdio::stderr));
         self
     }
 
     /// Inherit the stdin, stdout, and stderr streams from the host process.
     pub fn inherit_stdio(&mut self) -> &mut Self {
-        self.stdin = Some(PendingEntry::Thunk(|| Ok(OsHandle::stdin())));
-        self.stdout = Some(PendingEntry::Thunk(|| Ok(OsHandle::stdout())));
-        self.stderr = Some(PendingEntry::Thunk(|| Ok(OsHandle::stderr())));
+        self.stdin = Some(PendingEntry::Thunk(Stdio::stdin));
+        self.stdout = Some(PendingEntry::Thunk(Stdio::stdout));
+        self.stderr = Some(PendingEntry::Thunk(Stdio::stderr));
         self
     }
 
@@ -251,7 +253,7 @@ impl WasiCtxBuilder {
     pub fn preopened_dir<P: AsRef<Path>>(&mut self, dir: File, guest_path: P) -> &mut Self {
         self.preopens.as_mut().unwrap().push((
             guest_path.as_ref().to_owned(),
-            Box::new(OsHandle::from(dir)),
+            <Box<dyn Handle>>::try_from(dir).expect("valid handle"),
         ));
         self
     }
@@ -336,15 +338,16 @@ impl WasiCtxBuilder {
             log::debug!("WasiCtx inserting entry {:?}", pending);
             let fd = match pending {
                 PendingEntry::Thunk(f) => {
-                    let handle = EntryHandle::new(f()?);
-                    let entry = Entry::from(handle)?;
+                    let handle = EntryHandle::from(f()?);
+                    let entry = Entry::new(handle);
                     entries
                         .insert(entry)
                         .ok_or(WasiCtxBuilderError::TooManyFilesOpen)?
                 }
                 PendingEntry::File(f) => {
-                    let handle = EntryHandle::new(OsHandle::from(f));
-                    let entry = Entry::from(handle)?;
+                    let handle = <Box<dyn Handle>>::try_from(f)?;
+                    let handle = EntryHandle::from(handle);
+                    let entry = Entry::new(handle);
                     entries
                         .insert(entry)
                         .ok_or(WasiCtxBuilderError::TooManyFilesOpen)?
@@ -359,7 +362,7 @@ impl WasiCtxBuilder {
             }
 
             let handle = EntryHandle::from(dir);
-            let mut entry = Entry::from(handle)?;
+            let mut entry = Entry::new(handle);
             entry.preopen_path = Some(guest_path);
             let fd = entries
                 .insert(entry)

crates/wasi-common/src/entry.rs

@@ -1,19 +1,13 @@
-use crate::handle::Handle;
+use crate::handle::{Handle, HandleRights};
 use crate::wasi::types::{Filetype, Rights};
 use crate::wasi::{Errno, Result};
-use std::cell::Cell;
 use std::ops::Deref;
 use std::path::PathBuf;
 use std::rc::Rc;
-use std::{fmt, io};
 
 pub(crate) struct EntryHandle(Rc<dyn Handle>);
 
 impl EntryHandle {
-    pub(crate) fn new<T: Handle + 'static>(handle: T) -> Self {
-        Self(Rc::new(handle))
-    }
-
     pub(crate) fn get(&self) -> Self {
         Self(Rc::clone(&self.0))
     }
@@ -33,118 +27,79 @@ impl Deref for EntryHandle {
     }
 }
 
-/// An abstraction struct serving as a wrapper for a host `Descriptor` object which requires
-/// certain rights `rights` in order to be accessed correctly.
+/// An abstraction struct serving as a wrapper for a `Handle` object.
 ///
-/// Here, the `descriptor` field stores the host `Descriptor` object (such as a file descriptor, or
-/// stdin handle), and accessing it can only be done via the provided `Entry::as_descriptor` method
+/// Here, the `handle` field stores an instance of `Handle` type (such as a file descriptor, or
+/// stdin handle), and accessing it can only be done via the provided `Entry::as_handle` method
 /// which require a set of base and inheriting rights to be specified, verifying whether the stored
-/// `Descriptor` object is valid for the rights specified.
+/// `Handle` object is valid for the rights specified.
 pub(crate) struct Entry {
-    pub(crate) file_type: Filetype,
     handle: EntryHandle,
-    pub(crate) rights: Cell<EntryRights>,
     pub(crate) preopen_path: Option<PathBuf>,
     // TODO: directories
 }
 
-/// Represents rights of an `Entry` entity, either already held or
-/// required.
-#[derive(Debug, Copy, Clone)]
-pub(crate) struct EntryRights {
-    pub(crate) base: Rights,
-    pub(crate) inheriting: Rights,
-}
-
-impl EntryRights {
-    pub(crate) fn new(base: Rights, inheriting: Rights) -> Self {
-        Self { base, inheriting }
-    }
-
-    /// Create new `EntryRights` instance from `base` rights only, keeping
-    /// `inheriting` set to none.
-    pub(crate) fn from_base(base: Rights) -> Self {
-        Self {
-            base,
-            inheriting: Rights::empty(),
-        }
-    }
-
-    /// Create new `EntryRights` instance with both `base` and `inheriting`
-    /// rights set to none.
-    pub(crate) fn empty() -> Self {
+impl Entry {
+    pub(crate) fn new(handle: EntryHandle) -> Self {
+        let preopen_path = None;
         Self {
-            base: Rights::empty(),
-            inheriting: Rights::empty(),
+            handle,
+            preopen_path,
         }
     }
 
-    /// Check if `other` is a subset of those rights.
-    pub(crate) fn contains(&self, other: &Self) -> bool {
-        self.base.contains(&other.base) && self.inheriting.contains(&other.inheriting)
+    pub(crate) fn get_file_type(&self) -> Filetype {
+        self.handle.get_file_type()
     }
-}
 
-impl fmt::Display for EntryRights {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
-            f,
-            "EntryRights {{ base: {}, inheriting: {} }}",
-            self.base, self.inheriting
-        )
+    pub(crate) fn get_rights(&self) -> HandleRights {
+        self.handle.get_rights()
     }
-}
 
-impl Entry {
-    pub(crate) fn from(handle: EntryHandle) -> io::Result<Self> {
-        let file_type = handle.get_file_type()?;
-        let rights = handle.get_rights()?;
-        Ok(Self {
-            file_type,
-            handle,
-            rights: Cell::new(rights),
-            preopen_path: None,
-        })
+    pub(crate) fn set_rights(&self, rights: HandleRights) {
+        self.handle.set_rights(rights)
     }
 
-    /// Convert this `Entry` into a host `Descriptor` object provided the specified
+    /// Convert this `Entry` into a `Handle` object provided the specified
     /// `rights` rights are set on this `Entry` object.
     ///
-    /// The `Entry` can only be converted into a valid `Descriptor` object if
+    /// The `Entry` can only be converted into a valid `Handle` object if
     /// the specified set of base rights, and inheriting rights encapsulated within `rights`
-    /// `EntryRights` structure is a subset of rights attached to this `Entry`. The check is
+    /// `HandleRights` structure is a subset of rights attached to this `Entry`. The check is
     /// performed using `Entry::validate_rights` method. If the check fails, `Errno::Notcapable`
     /// is returned.
-    pub(crate) fn as_handle(&self, rights: &EntryRights) -> Result<EntryHandle> {
+    pub(crate) fn as_handle(&self, rights: &HandleRights) -> Result<EntryHandle> {
         self.validate_rights(rights)?;
         Ok(self.handle.get())
     }
 
-    /// Check if this `Entry` object satisfies the specified `EntryRights`; i.e., if
+    /// Check if this `Entry` object satisfies the specified `HandleRights`; i.e., if
     /// rights attached to this `Entry` object are a superset.
     ///
     /// Upon unsuccessful check, `Errno::Notcapable` is returned.
-    pub(crate) fn validate_rights(&self, rights: &EntryRights) -> Result<()> {
-        if self.rights.get().contains(rights) {
+    pub(crate) fn validate_rights(&self, rights: &HandleRights) -> Result<()> {
+        let this_rights = self.handle.get_rights();
+        if this_rights.contains(rights) {
             Ok(())
         } else {
             log::trace!(
                 "     | validate_rights failed: required rights = {}; actual rights = {}",
                 rights,
-                self.rights.get(),
+                this_rights,
             );
             Err(Errno::Notcapable)
         }
     }
 
+    // TODO we should probably have a separate struct representing
+    // char device
     /// Test whether this descriptor is considered a tty within WASI.
     /// Note that since WASI itself lacks an `isatty` syscall and relies
     /// on a conservative approximation, we use the same approximation here.
     pub(crate) fn isatty(&self) -> bool {
-        self.file_type == Filetype::CharacterDevice
-            && self
-                .rights
-                .get()
-                .contains(&EntryRights::from_base(Rights::FD_SEEK | Rights::FD_TELL))
+        let file_type = self.handle.get_file_type();
+        let rights = self.handle.get_rights();
+        let required_rights = HandleRights::from_base(Rights::FD_SEEK | Rights::FD_TELL);
+        file_type == Filetype::CharacterDevice && rights.contains(&required_rights)
     }
 }

crates/wasi-common/src/handle.rs

@@ -1,20 +1,65 @@
-use crate::entry::EntryRights;
-use crate::wasi::{types, Errno, Result};
+use crate::wasi::types::{self, Rights};
+use crate::wasi::{Errno, Result};
 use std::any::Any;
+use std::fmt;
 use std::io::{self, SeekFrom};
 
+/// Represents rights of a `Handle`, either already held or required.
+#[derive(Debug, Copy, Clone)]
+pub(crate) struct HandleRights {
+    pub(crate) base: Rights,
+    pub(crate) inheriting: Rights,
+}
+
+impl HandleRights {
+    pub(crate) fn new(base: Rights, inheriting: Rights) -> Self {
+        Self { base, inheriting }
+    }
+
+    /// Create new `HandleRights` instance from `base` rights only, keeping
+    /// `inheriting` set to none.
+    pub(crate) fn from_base(base: Rights) -> Self {
+        Self {
+            base,
+            inheriting: Rights::empty(),
+        }
+    }
+
+    /// Create new `HandleRights` instance with both `base` and `inheriting`
+    /// rights set to none.
+    pub(crate) fn empty() -> Self {
+        Self {
+            base: Rights::empty(),
+            inheriting: Rights::empty(),
+        }
+    }
+
+    /// Check if `other` is a subset of those rights.
+    pub(crate) fn contains(&self, other: &Self) -> bool {
+        self.base.contains(&other.base) && self.inheriting.contains(&other.inheriting)
+    }
+}
+
+impl fmt::Display for HandleRights {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(
+            f,
+            "HandleRights {{ base: {}, inheriting: {} }}",
+            self.base, self.inheriting
+        )
+    }
+}
+
 pub(crate) trait Handle {
     fn as_any(&self) -> &dyn Any;
     fn try_clone(&self) -> io::Result<Box<dyn Handle>>;
-    fn get_file_type(&self) -> io::Result<types::Filetype>;
-    fn get_rights(&self) -> io::Result<EntryRights> {
-        Ok(EntryRights::empty())
+    fn get_file_type(&self) -> types::Filetype;
+    fn get_rights(&self) -> HandleRights {
+        HandleRights::empty()
     }
+    fn set_rights(&self, rights: HandleRights);
     fn is_directory(&self) -> bool {
-        if let Ok(ft) = self.get_file_type() {
-            return ft == types::Filetype::Directory;
-        }
-        false
+        self.get_file_type() == types::Filetype::Directory
     }
     // TODO perhaps should be a separate trait?
     // FdOps

crates/wasi-common/src/path.rs

@@ -1,5 +1,5 @@
-use crate::entry::{Entry, EntryRights};
-use crate::handle::Handle;
+use crate::entry::Entry;
+use crate::handle::{Handle, HandleRights};
 use crate::wasi::{types, Errno, Result};
 use std::path::{Component, Path};
 use std::str;
@@ -12,7 +12,7 @@ pub(crate) use crate::sys::path::{from_host, open_rights};
 /// This is a workaround for not having Capsicum support in the OS.
 pub(crate) fn get(
     entry: &Entry,
-    required_rights: &EntryRights,
+    required_rights: &HandleRights,
     dirflags: types::Lookupflags,
     path: &GuestPtr<'_, str>,
     needs_final_component: bool,
@@ -33,7 +33,7 @@ pub(crate) fn get(
         return Err(Errno::Ilseq);
     }
 
-    if entry.file_type != types::Filetype::Directory {
+    if entry.get_file_type() != types::Filetype::Directory {
         // if `dirfd` doesn't refer to a directory, return `Notdir`.
         return Err(Errno::Notdir);
     }