Allow different Handles to act as stdio (#1600)

* Allow any type which implements Handle to act as stdio

There have been requests to allow more than just raw OS handles to
act as stdio in `wasi-common`. This commit makes this possible by
loosening the requirement of the `WasiCtxBuilder` to accept any
type `T: Handle + 'static` to act as any of the stdio handles.

A couple words about correctness of this approach. Currently, since
we only have a single `Handle` super-trait to represent all possible
WASI handle types (files, dirs, stdio, pipes, virtual, etc.), it
is possible to pass in any type to act as stdio which can be wrong.
However, I envision this being a problem only in the near(est) future
until we work out how to split `Handle` into several traits, each
representing a different type of WASI resource. In this particular
case, this would be a resource which would implement the interface
required for a handle to act as a stdio (with appropriate rights, etc.).

* Use OsFile in c-api

* Add some documention to the types exposed by this PR, and a few others

Signed-off-by: Jakub Konka <kubkon@jakubkonka.com>

* Add construction examples and missing docs for Handle trait

* Fix example on Windows

* Merge wasi_preview_builder into create_preview1_instance

Co-authored-by: Pat Hickey <pat@moreproductive.org>

crates/c-api/src/wasi.rs

@@ -201,63 +201,77 @@ enum WasiInstance {
     Snapshot0(WasiSnapshot0),
 }
 
-macro_rules! config_to_builder {
+fn create_snapshot0_instance(store: &Store, config: wasi_config_t) -> Result<WasiInstance> {
-    ($builder:ident, $config:ident) => {{
+    let mut builder = WasiSnapshot0CtxBuilder::new();
-        let mut builder = $builder::new();
+    if config.inherit_args {
-
+        builder.inherit_args();
-        if $config.inherit_args {
+    } else if !config.args.is_empty() {
-            builder.inherit_args();
+        builder.args(config.args);
-        } else if !$config.args.is_empty() {
+    }
-            builder.args($config.args);
+    if config.inherit_env {
-        }
+        builder.inherit_env();
-
+    } else if !config.env.is_empty() {
-        if $config.inherit_env {
+        builder.envs(config.env);
-            builder.inherit_env();
+    }
-        } else if !$config.env.is_empty() {
+    if config.inherit_stdin {
-            builder.envs($config.env);
+        builder.inherit_stdin();
-        }
+    } else if let Some(file) = config.stdin {
-
+        builder.stdin(file);
-        if $config.inherit_stdin {
+    }
-            builder.inherit_stdin();
+    if config.inherit_stdout {
-        } else if let Some(file) = $config.stdin {
+        builder.inherit_stdout();
-            builder.stdin(file);
+    } else if let Some(file) = config.stdout {
-        }
+        builder.stdout(file);
-
+    }
-        if $config.inherit_stdout {
+    if config.inherit_stderr {
-            builder.inherit_stdout();
+        builder.inherit_stderr();
-        } else if let Some(file) = $config.stdout {
+    } else if let Some(file) = config.stderr {
-            builder.stdout(file);
+        builder.stderr(file);
-        }
+    }
-
+    for preopen in config.preopens {
-        if $config.inherit_stderr {
+        builder.preopened_dir(preopen.0, preopen.1);
-            builder.inherit_stderr();
+    }
-        } else if let Some(file) = $config.stderr {
-            builder.stderr(file);
-        }
-
-        for preopen in $config.preopens {
-            builder.preopened_dir(preopen.0, preopen.1);
-        }
-
-        builder
-    }};
-}
-
-fn create_snapshot0_instance(store: &Store, config: wasi_config_t) -> Result<WasiInstance, String> {
     Ok(WasiInstance::Snapshot0(WasiSnapshot0::new(
         store,
-        config_to_builder!(WasiSnapshot0CtxBuilder, config)
+        builder.build()?,
-            .build()
-            .map_err(|e| e.to_string())?,
     )))
 }
 
-fn create_preview1_instance(store: &Store, config: wasi_config_t) -> Result<WasiInstance, String> {
+fn create_preview1_instance(store: &Store, config: wasi_config_t) -> Result<WasiInstance> {
+    use std::convert::TryFrom;
+    use wasi_common::OsFile;
+    let mut builder = WasiPreview1CtxBuilder::new();
+    if config.inherit_args {
+        builder.inherit_args();
+    } else if !config.args.is_empty() {
+        builder.args(config.args);
+    }
+    if config.inherit_env {
+        builder.inherit_env();
+    } else if !config.env.is_empty() {
+        builder.envs(config.env);
+    }
+    if config.inherit_stdin {
+        builder.inherit_stdin();
+    } else if let Some(file) = config.stdin {
+        builder.stdin(OsFile::try_from(file)?);
+    }
+    if config.inherit_stdout {
+        builder.inherit_stdout();
+    } else if let Some(file) = config.stdout {
+        builder.stdout(OsFile::try_from(file)?);
+    }
+    if config.inherit_stderr {
+        builder.inherit_stderr();
+    } else if let Some(file) = config.stderr {
+        builder.stderr(OsFile::try_from(file)?);
+    }
+    for preopen in config.preopens {
+        builder.preopened_dir(preopen.0, preopen.1);
+    }
     Ok(WasiInstance::Preview1(WasiPreview1::new(
         store,
-        config_to_builder!(WasiPreview1CtxBuilder, config)
+        builder.build()?,
-            .build()
-            .map_err(|e| e.to_string())?,
     )))
 }
 
@@ -286,8 +300,10 @@ pub unsafe extern "C" fn wasi_instance_new(
     let store = &store.store;
 
     let result = match CStr::from_ptr(name).to_str().unwrap_or("") {
-        "wasi_snapshot_preview1" => create_preview1_instance(store, *config),
+        "wasi_snapshot_preview1" => {
-        "wasi_unstable" => create_snapshot0_instance(store, *config),
+            create_preview1_instance(store, *config).map_err(|e| e.to_string())
+        }
+        "wasi_unstable" => create_snapshot0_instance(store, *config).map_err(|e| e.to_string()),
         _ => Err("unsupported WASI version".into()),
     };
 

crates/test-programs/tests/wasm_tests/runtime.rs

@@ -1,7 +1,8 @@
 use anyhow::Context;
+use std::convert::TryFrom;
 use std::fs::File;
 use std::path::Path;
-use wasi_common::VirtualDirEntry;
+use wasi_common::{OsOther, VirtualDirEntry};
 use wasmtime::{Linker, Module, Store};
 
 #[derive(Clone, Copy, Debug)]
@@ -46,7 +47,9 @@ pub fn instantiate(
     // where `stdin` is never ready to be read. In some CI systems, however,
     // stdin is closed which causes tests to fail.
     let (reader, _writer) = os_pipe::pipe()?;
-    builder.stdin(reader_to_file(reader));
+    let file = reader_to_file(reader);
+    let handle = OsOther::try_from(file).context("failed to create OsOther from PipeReader")?;
+    builder.stdin(handle);
     let snapshot1 = wasmtime_wasi::Wasi::new(&store, builder.build()?);
 
     let mut linker = Linker::new(&store);

crates/wasi-common/src/ctx.rs

@@ -47,7 +47,7 @@ type WasiCtxBuilderResult<T> = std::result::Result<T, WasiCtxBuilderError>;
 
 enum PendingEntry {
     Thunk(fn() -> io::Result<Box<dyn Handle>>),
-    OsHandle(File),
+    Handle(Box<dyn Handle>),
 }
 
 impl std::fmt::Debug for PendingEntry {
@@ -58,7 +58,7 @@ impl std::fmt::Debug for PendingEntry {
                 "PendingEntry::Thunk({:p})",
                 f as *const fn() -> io::Result<Box<dyn Handle>>
             ),
-            Self::OsHandle(f) => write!(fmt, "PendingEntry::OsHandle({:?})", f),
+            Self::Handle(handle) => write!(fmt, "PendingEntry::Handle({:p})", handle),
         }
     }
 }
@@ -247,21 +247,21 @@ impl WasiCtxBuilder {
         self
     }
 
-    /// Provide a File to use as stdin
+    /// Provide a `Handle` to use as stdin
-    pub fn stdin(&mut self, file: File) -> &mut Self {
+    pub fn stdin<T: Handle + 'static>(&mut self, handle: T) -> &mut Self {
-        self.stdin = Some(PendingEntry::OsHandle(file));
+        self.stdin = Some(PendingEntry::Handle(Box::new(handle)));
         self
     }
 
-    /// Provide a File to use as stdout
+    /// Provide a `Handle` to use as stdout
-    pub fn stdout(&mut self, file: File) -> &mut Self {
+    pub fn stdout<T: Handle + 'static>(&mut self, handle: T) -> &mut Self {
-        self.stdout = Some(PendingEntry::OsHandle(file));
+        self.stdout = Some(PendingEntry::Handle(Box::new(handle)));
         self
     }
 
-    /// Provide a File to use as stderr
+    /// Provide a `Handle` to use as stderr
-    pub fn stderr(&mut self, file: File) -> &mut Self {
+    pub fn stderr<T: Handle + 'static>(&mut self, handle: T) -> &mut Self {
-        self.stderr = Some(PendingEntry::OsHandle(file));
+        self.stderr = Some(PendingEntry::Handle(Box::new(handle)));
         self
     }
 
@@ -368,9 +368,8 @@ impl WasiCtxBuilder {
                         .insert(entry)
                         .ok_or(WasiCtxBuilderError::TooManyFilesOpen)?
                 }
-                PendingEntry::OsHandle(f) => {
+                PendingEntry::Handle(handle) => {
-                    let handle = OsOther::try_from(f)?;
+                    let handle = EntryHandle::from(handle);
-                    let handle = EntryHandle::new(handle);
                     let entry = Entry::new(handle);
                     entries
                         .insert(entry)

crates/wasi-common/src/entry.rs

@@ -8,6 +8,7 @@ use std::rc::Rc;
 pub(crate) struct EntryHandle(Rc<dyn Handle>);
 
 impl EntryHandle {
+    #[allow(dead_code)]
     pub(crate) fn new<T: Handle + 'static>(handle: T) -> Self {
         Self(Rc::new(handle))
     }

crates/wasi-common/src/handle.rs

@@ -6,38 +6,49 @@ use std::io::{self, SeekFrom};
 
 /// Represents rights of a `Handle`, either already held or required.
 #[derive(Debug, Copy, Clone)]
-pub(crate) struct HandleRights {
+pub struct HandleRights {
     pub(crate) base: Rights,
     pub(crate) inheriting: Rights,
 }
 
 impl HandleRights {
-    pub(crate) fn new(base: Rights, inheriting: Rights) -> Self {
+    /// Creates new `HandleRights` instance from `base` and `inheriting` rights.
+    pub fn new(base: Rights, inheriting: Rights) -> Self {
         Self { base, inheriting }
     }
 
-    /// Create new `HandleRights` instance from `base` rights only, keeping
+    /// Creates new `HandleRights` instance from `base` rights only, keeping
     /// `inheriting` set to none.
-    pub(crate) fn from_base(base: Rights) -> Self {
+    pub fn from_base(base: Rights) -> Self {
         Self {
             base,
             inheriting: Rights::empty(),
         }
     }
 
-    /// Create new `HandleRights` instance with both `base` and `inheriting`
+    /// Creates new `HandleRights` instance with both `base` and `inheriting`
     /// rights set to none.
-    pub(crate) fn empty() -> Self {
+    pub fn empty() -> Self {
         Self {
             base: Rights::empty(),
             inheriting: Rights::empty(),
         }
     }
 
-    /// Check if `other` is a subset of those rights.
+    /// Checks if `other` is a subset of those rights.
-    pub(crate) fn contains(&self, other: &Self) -> bool {
+    pub fn contains(&self, other: &Self) -> bool {
         self.base.contains(&other.base) && self.inheriting.contains(&other.inheriting)
     }
+
+    /// Returns base rights.
+    pub fn base(&self) -> Rights {
+        self.base
+    }
+
+    /// Returns inheriting rights.
+    pub fn inheriting(&self) -> Rights {
+        self.inheriting
+    }
 }
 
 impl fmt::Display for HandleRights {
@@ -50,7 +61,25 @@ impl fmt::Display for HandleRights {
     }
 }
 
-pub(crate) trait Handle {
+/// Generic interface for all WASI-compatible handles. We currently group these into two groups:
+/// * OS-based resources (actual, real resources): `OsFile`, `OsDir`, `OsOther`, and `Stdio`,
+/// * virtual files and directories: VirtualDir`, and `InMemoryFile`.
+///
+/// # Constructing `Handle`s representing OS-based resources
+///
+/// Each type of handle can either be constructed directly (see docs entry for a specific handle
+/// type such as `OsFile`), or you can let the `wasi_common` crate's machinery work it out
+/// automatically for you using `std::convert::TryInto` from `std::fs::File`:
+///
+/// ```rust,no_run
+/// use std::convert::TryInto;
+/// use wasi_common::Handle;
+/// use std::fs::OpenOptions;
+///
+/// let some_file = OpenOptions::new().read(true).open("some_file").unwrap();
+/// let wasi_handle: Box<dyn Handle> = some_file.try_into().unwrap();
+/// ```
+pub trait Handle {
     fn as_any(&self) -> &dyn Any;
     fn try_clone(&self) -> io::Result<Box<dyn Handle>>;
     fn get_file_type(&self) -> types::Filetype;

crates/wasi-common/src/lib.rs

@@ -36,5 +36,9 @@ mod virtfs;
 pub mod wasi;
 
 pub use ctx::{WasiCtx, WasiCtxBuilder, WasiCtxBuilderError};
+pub use handle::{Handle, HandleRights};
+pub use sys::osdir::OsDir;
+pub use sys::osfile::OsFile;
+pub use sys::osother::{OsOther, OsOtherExt};
 pub use sys::preopen_dir;
 pub use virtfs::{FileContents, VirtualDirEntry};

crates/wasi-common/src/sys/osdir.rs

@@ -10,7 +10,7 @@ use std::ops::Deref;
 // TODO could this be cleaned up?
 // The actual `OsDir` struct is OS-dependent, therefore we delegate
 // its definition to OS-specific modules.
-pub(crate) use super::sys_impl::osdir::OsDir;
+pub use super::sys_impl::osdir::OsDir;
 
 impl Deref for OsDir {
     type Target = RawOsHandle;

crates/wasi-common/src/sys/osfile.rs

@@ -9,7 +9,24 @@ use std::io::{self, Read, Seek, SeekFrom, Write};
 use std::ops::Deref;
 
 #[derive(Debug)]
-pub(crate) struct OsFile {
+/// A file backed by the operating system's file system. Dereferences to a
+/// `RawOsHandle`.  Its impl of `Handle` uses Rust's `std` to implement all
+/// file descriptor operations.
+///
+/// # Constructing `OsFile`
+///
+/// `OsFile` can currently only be constructed from `std::fs::File` using
+/// the `std::convert::TryFrom` trait:
+///
+/// ```rust,no_run
+/// use std::fs::OpenOptions;
+/// use std::convert::TryFrom;
+/// use wasi_common::OsFile;
+///
+/// let file = OpenOptions::new().read(true).open("some_file").unwrap();
+/// let os_file = OsFile::try_from(file).unwrap();
+/// ```
+pub struct OsFile {
     rights: Cell<HandleRights>,
     handle: RawOsHandle,
 }

crates/wasi-common/src/sys/osother.rs

@@ -10,7 +10,9 @@ use std::fs::File;
 use std::io::{self, Read, Write};
 use std::ops::Deref;
 
-pub(crate) trait OsOtherExt {
+/// Extra methods for `OsOther` that are only available when configured for
+/// some operating systems.
+pub trait OsOtherExt {
     /// Create `OsOther` as `dyn Handle` from null device.
     fn from_null() -> io::Result<Box<dyn Handle>>;
 }
@@ -20,8 +22,22 @@ pub(crate) trait OsOtherExt {
 /// sockets, streams, etc. As such, when redirecting stdio within `WasiCtxBuilder`, the redirected
 /// pipe should be encapsulated within this instance _and not_ `OsFile` which represents a regular
 /// OS file.
+///
+/// # Constructing `OsOther`
+///
+/// `OsOther` can currently only be constructed from `std::fs::File` using
+/// the `std::convert::TryFrom` trait:
+///
+/// ```rust,no_run
+/// use std::fs::OpenOptions;
+/// use std::convert::TryFrom;
+/// use wasi_common::OsOther;
+///
+/// let pipe = OpenOptions::new().read(true).open("a_pipe").unwrap();
+/// let os_other = OsOther::try_from(pipe).unwrap();
+/// ```
 #[derive(Debug)]
-pub(crate) struct OsOther {
+pub struct OsOther {
     file_type: Filetype,
     rights: Cell<HandleRights>,
     handle: RawOsHandle,

crates/wasi-common/src/sys/unix/bsd/osdir.rs

@@ -6,7 +6,24 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
-pub(crate) struct OsDir {
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in `sys::osdir`. This type is exposed to all other modules as
+/// `sys::osdir::OsDir` when configured.
+///
+/// # Constructing `OsDir`
+///
+/// `OsDir` can currently only be constructed from `std::fs::File` using
+/// the `std::convert::TryFrom` trait:
+///
+/// ```rust,no_run
+/// use std::fs::OpenOptions;
+/// use std::convert::TryFrom;
+/// use wasi_common::OsDir;
+///
+/// let dir = OpenOptions::new().read(true).open("some_dir").unwrap();
+/// let os_dir = OsDir::try_from(dir).unwrap();
+/// ```
+pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
     // When the client makes a `fd_readdir` syscall on this descriptor,
@@ -39,7 +56,9 @@ impl OsDir {
             stream_ptr,
         })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck
+    /// typing: sys::unix::fd::readdir expects the configured OsDir to have
+    /// this method.
     pub(crate) fn stream_ptr(&self) -> Result<RefMut<Dir>> {
         Ok(self.stream_ptr.borrow_mut())
     }

crates/wasi-common/src/sys/unix/linux/osdir.rs

@@ -6,7 +6,24 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
-pub(crate) struct OsDir {
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in `sys::osdir`. This type is exposed to all other modules as
+/// `sys::osdir::OsDir` when configured.
+///
+/// # Constructing `OsDir`
+///
+/// `OsDir` can currently only be constructed from `std::fs::File` using
+/// the `std::convert::TryFrom` trait:
+///
+/// ```rust,no_run
+/// use std::fs::OpenOptions;
+/// use std::convert::TryFrom;
+/// use wasi_common::OsDir;
+///
+/// let dir = OpenOptions::new().read(true).open("some_dir").unwrap();
+/// let os_dir = OsDir::try_from(dir).unwrap();
+/// ```
+pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
 }
@@ -16,7 +33,8 @@ impl OsDir {
         let rights = Cell::new(rights);
         Ok(Self { rights, handle })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck typing:
+    /// sys::unix::fd::readdir expects the configured OsDir to have this method.
     pub(crate) fn stream_ptr(&self) -> Result<Box<Dir>> {
         // We need to duplicate the handle, because `opendir(3)`:
         //     After a successful call to fdopendir(), fd is used internally by the implementation,

crates/wasi-common/src/sys/unix/osdir.rs

@@ -6,7 +6,7 @@ use std::fs::File;
 use std::io;
 use std::os::unix::prelude::{AsRawFd, FromRawFd, IntoRawFd};
 
-pub(crate) use super::sys_impl::osdir::OsDir;
+pub use super::sys_impl::osdir::OsDir;
 
 impl TryFrom<File> for OsDir {
     type Error = io::Error;

crates/wasi-common/src/sys/unix/oshandle.rs

@@ -3,7 +3,7 @@ use std::io;
 use std::os::unix::prelude::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
 
 #[derive(Debug)]
-pub(crate) struct RawOsHandle(File);
+pub struct RawOsHandle(File);
 
 impl RawOsHandle {
     /// Tries clone `self`.

crates/wasi-common/src/sys/windows/osdir.rs

@@ -8,7 +8,26 @@ use std::io;
 use std::os::windows::prelude::{AsRawHandle, FromRawHandle, IntoRawHandle};
 
 #[derive(Debug)]
-pub(crate) struct OsDir {
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in `sys::osdir`. This type is exposed to all other modules as
+/// `sys::osdir::OsDir` when configured.
+///
+/// # Constructing `OsDir`
+///
+/// `OsDir` can currently only be constructed from `std::fs::File` using
+/// the `std::convert::TryFrom` trait:
+///
+/// ```rust,no_run
+/// use std::fs::OpenOptions;
+/// use std::convert::TryFrom;
+/// use std::os::windows::fs::OpenOptionsExt;
+/// use wasi_common::OsDir;
+/// use winapi::um::winbase::FILE_FLAG_BACKUP_SEMANTICS;
+///
+/// let dir = OpenOptions::new().read(true).attributes(FILE_FLAG_BACKUP_SEMANTICS).open("some_dir").unwrap();
+/// let os_dir = OsDir::try_from(dir).unwrap();
+/// ```
+pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
 }

crates/wasi-common/src/sys/windows/oshandle.rs

@@ -6,7 +6,7 @@ use std::mem::ManuallyDrop;
 use std::os::windows::prelude::{AsRawHandle, FromRawHandle, IntoRawHandle, RawHandle};
 
 #[derive(Debug)]
-pub(crate) struct RawOsHandle(Cell<RawHandle>);
+pub struct RawOsHandle(Cell<RawHandle>);
 
 impl RawOsHandle {
     /// Tries cloning `self`.

crates/wasi-common/src/virtfs.rs

@@ -11,12 +11,16 @@ use std::io::SeekFrom;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
 
+/// An entry in a virtual filesystem
 pub enum VirtualDirEntry {
+    /// The contents of a child directory
     Directory(HashMap<String, VirtualDirEntry>),
+    /// A file
     File(Box<dyn FileContents>),
 }
 
 impl VirtualDirEntry {
+    /// Construct an empty directory
     pub fn empty_directory() -> Self {
         Self::Directory(HashMap::new())
     }