From 87222d6db0dbaf788b14b298820ebd776e0cdfa5 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 24 Aug 2020 12:26:02 -0700 Subject: [PATCH 01/13] wasi-common: move GuestPtr deref out of path::get --- crates/wasi-common/src/path.rs | 8 +- .../src/snapshots/wasi_snapshot_preview1.rs | 168 +++++++++++------- 2 files changed, 103 insertions(+), 73 deletions(-) diff --git a/crates/wasi-common/src/path.rs b/crates/wasi-common/src/path.rs index a5b7304186..2f6da351b5 100644 --- a/crates/wasi-common/src/path.rs +++ b/crates/wasi-common/src/path.rs @@ -4,7 +4,6 @@ use crate::wasi::types; use crate::{Error, Result}; use std::path::{Component, Path}; use std::str; -use wiggle::GuestPtr; pub(crate) use crate::sys::path::{from_host, open_rights}; @@ -15,15 +14,12 @@ pub(crate) fn get( entry: &Entry, required_rights: &HandleRights, dirflags: types::Lookupflags, - path_ptr: &GuestPtr<'_, str>, + path: &str, needs_final_component: bool, ) -> Result<(Box, String)> { const MAX_SYMLINK_EXPANSIONS: usize = 128; - // Extract path as &str from guest's memory. - let path = path_ptr.as_str()?; - - tracing::trace!(path = &*path); + tracing::trace!(path = path); if path.contains('\0') { // if contains NUL, return Ilseq diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index e2d6c0eef0..3643cf9107 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -6,6 +6,7 @@ use crate::wasi::{types, AsBytes}; use crate::{path, poll, Error, Result, WasiCtx}; use std::convert::TryInto; use std::io::{self, SeekFrom}; +use std::ops::Deref; use tracing::{debug, trace}; use wiggle::{GuestPtr, GuestSlice}; @@ -438,11 +439,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { types::Rights::PATH_OPEN | types::Rights::PATH_CREATE_DIRECTORY, ); let entry = self.get_entry(dirfd)?; + let path = path.as_str()?; let (dirfd, path) = path::get( &entry, &required_rights, types::Lookupflags::empty(), - path, + path.deref(), false, )?; dirfd.create_directory(&path) @@ -456,7 +458,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result { let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_GET); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get(&entry, &required_rights, flags, path, false)?; + let path = path.as_str()?; + let (dirfd, path) = path::get(&entry, &required_rights, flags, path.deref(), false)?; let host_filestat = dirfd.filestat_get_at(&path, flags.contains(&types::Lookupflags::SYMLINK_FOLLOW))?; Ok(host_filestat) @@ -473,7 +476,8 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_SET_TIMES); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get(&entry, &required_rights, flags, path, false)?; + let path = path.as_str()?; + let (dirfd, path) = path::get(&entry, &required_rights, flags, path.deref(), false)?; dirfd.filestat_set_times_at( &path, atim, @@ -494,22 +498,30 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_LINK_SOURCE); let old_entry = self.get_entry(old_fd)?; - let (old_dirfd, old_path) = path::get( - &old_entry, - &required_rights, - types::Lookupflags::empty(), - old_path, - false, - )?; + let (old_dirfd, old_path) = { + // Borrow old_path for just this scope + let old_path = old_path.as_str()?; + path::get( + &old_entry, + &required_rights, + types::Lookupflags::empty(), + old_path.deref(), + false, + )? + }; let required_rights = HandleRights::from_base(types::Rights::PATH_LINK_TARGET); let new_entry = self.get_entry(new_fd)?; - let (new_dirfd, new_path) = path::get( - &new_entry, - &required_rights, - types::Lookupflags::empty(), - new_path, - false, - )?; + let (new_dirfd, new_path) = { + // Borrow new_path for just this scope + let new_path = new_path.as_str()?; + path::get( + &new_entry, + &required_rights, + types::Lookupflags::empty(), + new_path.deref(), + false, + )? + }; old_dirfd.link( &old_path, new_dirfd, @@ -535,13 +547,16 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ); trace!(" | needed_rights={}", needed_rights); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get( - &entry, - &needed_rights, - dirflags, - path, - oflags & types::Oflags::CREAT != types::Oflags::empty(), - )?; + let (dirfd, path) = { + let path = path.as_str()?; + path::get( + &entry, + &needed_rights, + dirflags, + path.deref(), + oflags & types::Oflags::CREAT != types::Oflags::empty(), + )? + }; // which open mode do we need? let read = fs_rights_base & (types::Rights::FD_READ | types::Rights::FD_READDIR) != types::Rights::empty(); @@ -577,13 +592,17 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result { let required_rights = HandleRights::from_base(types::Rights::PATH_READLINK); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - path, - false, - )?; + let (dirfd, path) = { + // borrow path for just this scope + let path = path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + path.deref(), + false, + )? + }; let mut slice = buf.as_array(buf_len).as_slice()?; let host_bufused = dirfd.readlink(&path, &mut *slice)?.try_into()?; Ok(host_bufused) @@ -592,13 +611,16 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { fn path_remove_directory(&self, dirfd: types::Fd, path: &GuestPtr<'_, str>) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_REMOVE_DIRECTORY); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - path, - true, - )?; + let (dirfd, path) = { + let path = path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + path.deref(), + true, + )? + }; dirfd.remove_directory(&path) } @@ -611,22 +633,28 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_RENAME_SOURCE); let entry = self.get_entry(old_fd)?; - let (old_dirfd, old_path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - old_path, - true, - )?; + let (old_dirfd, old_path) = { + let old_path = old_path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + old_path.deref(), + true, + )? + }; let required_rights = HandleRights::from_base(types::Rights::PATH_RENAME_TARGET); let entry = self.get_entry(new_fd)?; - let (new_dirfd, new_path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - new_path, - true, - )?; + let (new_dirfd, new_path) = { + let new_path = new_path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + new_path.deref(), + true, + )? + }; old_dirfd.rename(&old_path, new_dirfd, &new_path) } @@ -638,28 +666,34 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_SYMLINK); let entry = self.get_entry(dirfd)?; - let (new_fd, new_path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - new_path, - true, - )?; + let (new_fd, new_path) = { + let new_path = new_path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + new_path.deref(), + true, + )? + }; let old_path = old_path.as_str()?; - trace!(old_path = &*old_path); + trace!(old_path = old_path.deref()); new_fd.symlink(&old_path, &new_path) } fn path_unlink_file(&self, dirfd: types::Fd, path: &GuestPtr<'_, str>) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::PATH_UNLINK_FILE); let entry = self.get_entry(dirfd)?; - let (dirfd, path) = path::get( - &entry, - &required_rights, - types::Lookupflags::empty(), - path, - false, - )?; + let (dirfd, path) = { + let path = path.as_str()?; + path::get( + &entry, + &required_rights, + types::Lookupflags::empty(), + path.deref(), + false, + )? + }; dirfd.unlink_file(&path)?; Ok(()) } From 6213a05c8557b8b1263d9c2a9ef0e6a3579f8ee9 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 24 Aug 2020 16:38:09 -0700 Subject: [PATCH 02/13] wiggle: add array indexing methods to GuestPtr<[T]> --- crates/wiggle/src/lib.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index b7c646a09a..d4eabacb9a 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -542,6 +542,29 @@ impl<'a, T> GuestPtr<'a, [T]> { pub fn as_ptr(&self) -> GuestPtr<'a, T> { GuestPtr::new(self.mem, self.offset_base()) } + + pub fn get(&self, index: u32) -> Option> { + if index < self.len() { + Some(GuestPtr::new(self.mem, self.offset_base() + index)) + } else { + None + } + } + + pub fn get_range(&self, r: std::ops::Range) -> Option> { + let range_length = r.end - r.start; + if r.start < self.len() && r.end < self.len() { + Some(GuestPtr::new( + self.mem, + ( + self.offset_base() + r.start, + self.offset_base() + range_length, + ), + )) + } else { + None + } + } } impl<'a> GuestPtr<'a, str> { From 51d88f7899ff8d1c5dc3032a7aac114d265cc905 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 24 Aug 2020 16:38:40 -0700 Subject: [PATCH 03/13] wasi-common: move implementation of args, env methods into StringArrayWriter --- crates/wasi-common/src/ctx.rs | 9 ++++ crates/wasi-common/src/lib.rs | 1 + .../src/snapshots/wasi_snapshot_preview1.rs | 47 +++------------- crates/wasi-common/src/string_array_writer.rs | 53 +++++++++++++++++++ 4 files changed, 70 insertions(+), 40 deletions(-) create mode 100644 crates/wasi-common/src/string_array_writer.rs diff --git a/crates/wasi-common/src/ctx.rs b/crates/wasi-common/src/ctx.rs index 08584dbaf5..346af12c28 100644 --- a/crates/wasi-common/src/ctx.rs +++ b/crates/wasi-common/src/ctx.rs @@ -1,6 +1,7 @@ use crate::entry::{Entry, EntryHandle}; use crate::fdpool::FdPool; use crate::handle::Handle; +use crate::string_array_writer::StringArrayWriter; use crate::sys::osdir::OsDir; use crate::sys::stdio::NullDevice; use crate::sys::stdio::{Stderr, StderrExt, Stdin, StdinExt, Stdout, StdoutExt}; @@ -491,4 +492,12 @@ impl WasiCtx { pub(crate) fn remove_entry(&self, fd: types::Fd) -> Result> { self.entries.borrow_mut().remove(fd).ok_or(Error::Badf) } + + pub(crate) fn args(&self) -> &impl StringArrayWriter { + &self.args + } + + pub(crate) fn env(&self) -> &impl StringArrayWriter { + &self.env + } } diff --git a/crates/wasi-common/src/lib.rs b/crates/wasi-common/src/lib.rs index 76e64bb3c0..c2beccdeb6 100644 --- a/crates/wasi-common/src/lib.rs +++ b/crates/wasi-common/src/lib.rs @@ -32,6 +32,7 @@ mod path; mod poll; mod sandboxed_tty_writer; pub mod snapshots; +mod string_array_writer; mod sys; pub mod virtfs; pub mod wasi; diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index 3643cf9107..be8994c24d 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -1,5 +1,6 @@ use crate::entry::{Entry, EntryHandle}; use crate::handle::HandleRights; +use crate::string_array_writer::StringArrayWriter; use crate::sys::clock; use crate::wasi::wasi_snapshot_preview1::WasiSnapshotPreview1; use crate::wasi::{types, AsBytes}; @@ -16,29 +17,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { argv: &GuestPtr<'b, GuestPtr<'b, u8>>, argv_buf: &GuestPtr<'b, u8>, ) -> Result<()> { - let mut argv = argv.clone(); - let mut argv_buf = argv_buf.clone(); - - for arg in &self.args { - let arg_bytes = arg.as_bytes_with_nul(); - let elems = arg_bytes.len().try_into()?; - argv_buf.as_array(elems).copy_from_slice(arg_bytes)?; - argv.write(argv_buf)?; - argv = argv.add(1)?; - argv_buf = argv_buf.add(elems)?; - } - - Ok(()) + self.args().write_to_guest(argv_buf, argv) } fn args_sizes_get(&self) -> Result<(types::Size, types::Size)> { - let argc = self.args.len().try_into()?; - let mut argv_size: types::Size = 0; - for arg in &self.args { - let arg_len = arg.as_bytes_with_nul().len().try_into()?; - argv_size = argv_size.checked_add(arg_len).ok_or(Error::Overflow)?; - } - Ok((argc, argv_size)) + let args = self.args(); + Ok((args.number_elements()?, args.cumulative_size()?)) } fn environ_get<'b>( @@ -46,29 +30,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { environ: &GuestPtr<'b, GuestPtr<'b, u8>>, environ_buf: &GuestPtr<'b, u8>, ) -> Result<()> { - let mut environ = environ.clone(); - let mut environ_buf = environ_buf.clone(); - - for e in &self.env { - let environ_bytes = e.as_bytes_with_nul(); - let elems = environ_bytes.len().try_into()?; - environ_buf.as_array(elems).copy_from_slice(environ_bytes)?; - environ.write(environ_buf)?; - environ = environ.add(1)?; - environ_buf = environ_buf.add(elems)?; - } - - Ok(()) + self.env().write_to_guest(environ_buf, environ) } fn environ_sizes_get(&self) -> Result<(types::Size, types::Size)> { - let environ_count = self.env.len().try_into()?; - let mut environ_size: types::Size = 0; - for environ in &self.env { - let env_len = environ.as_bytes_with_nul().len().try_into()?; - environ_size = environ_size.checked_add(env_len).ok_or(Error::Overflow)?; - } - Ok((environ_count, environ_size)) + let args = self.args(); + Ok((args.number_elements()?, args.cumulative_size()?)) } fn clock_res_get(&self, id: types::Clockid) -> Result { diff --git a/crates/wasi-common/src/string_array_writer.rs b/crates/wasi-common/src/string_array_writer.rs new file mode 100644 index 0000000000..8f44253858 --- /dev/null +++ b/crates/wasi-common/src/string_array_writer.rs @@ -0,0 +1,53 @@ +use crate::{Error, Result}; +use std::convert::TryInto; +use std::ffi::CString; +use wiggle::GuestPtr; + +pub trait StringArrayWriter { + fn number_elements(&self) -> Result; + fn cumulative_size(&self) -> Result; + fn write_to_guest<'a>( + &self, + buffer: &GuestPtr<'a, u8>, + elements: &GuestPtr<'a, GuestPtr<'a, u8>>, + ) -> Result<()>; +} + +impl StringArrayWriter for Vec { + fn number_elements(&self) -> Result { + let elems = self.len().try_into()?; + Ok(elems) + } + fn cumulative_size(&self) -> Result { + let mut total: u32 = 0; + for elem in self.iter() { + let elem_bytes = elem.as_bytes_with_nul().len().try_into()?; + total = total.checked_add(elem_bytes).ok_or(Error::Overflow)?; + } + Ok(total) + } + fn write_to_guest<'a>( + &self, + buffer: &GuestPtr<'a, u8>, + element_heads: &GuestPtr<'a, GuestPtr<'a, u8>>, + ) -> Result<()> { + let element_heads = element_heads.as_array(self.number_elements()?); + let buffer = buffer.as_array(self.cumulative_size()?); + let mut cursor = 0; + for (elem, head) in self.iter().zip(element_heads.iter()) { + let bytes = elem.as_bytes_with_nul(); + let len: u32 = bytes.len().try_into()?; + let elem_buffer = buffer + .get_range(cursor..(cursor + len)) + .ok_or(Error::Overflow)?; + elem_buffer.copy_from_slice(bytes)?; + head?.write( + elem_buffer + .get(0) + .expect("all elem buffers at least length 1"), + )?; + cursor += len; + } + Ok(()) + } +} From a2eae907010f17347147844b8bd89f6afe41588c Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 24 Aug 2020 16:41:24 -0700 Subject: [PATCH 04/13] test-all.sh: wasimtime-wasi-c does not exist anymore --- scripts/test-all.sh | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/scripts/test-all.sh b/scripts/test-all.sh index 9ebfb5926d..29ef441f88 100755 --- a/scripts/test-all.sh +++ b/scripts/test-all.sh @@ -64,13 +64,7 @@ RUST_BACKTRACE=1 cargo test \ --package wiggle-generate \ --package wiggle-test \ --package wiggle-macro \ - --package wasi-common \ - -# Test wasmtime-wasi-c, which doesn't support Windows. -if [ "${OS:-Not}" != "Windows_NT" ]; then - RUST_BACKTRACE=1 cargo test \ - --package wasmtime-wasi-c -fi + --package wasi-common # Make sure the documentation builds. banner "Rust documentation: $topdir/target/doc/wasmtime/index.html" From fe5d6d59e6989dfc4e76586a1f8b142a1305a901 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Tue, 1 Sep 2020 14:34:12 -0700 Subject: [PATCH 05/13] wasi example: theres lots of useful tracing messages for debugging --- Cargo.lock | 2 ++ Cargo.toml | 2 ++ examples/wasi/main.rs | 5 +++++ 3 files changed, 9 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index b2acc2ea90..c79c06601b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2419,6 +2419,8 @@ dependencies = [ "target-lexicon", "tempfile", "test-programs", + "tracing", + "tracing-subscriber", "wasi-common", "wasmtime", "wasmtime-cache", diff --git a/Cargo.toml b/Cargo.toml index ba36d92e89..666f918b14 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,8 @@ tempfile = "3.1.0" test-programs = { path = "crates/test-programs" } wasmtime-fuzzing = { path = "crates/fuzzing" } wasmtime-runtime = { path = "crates/runtime" } +tracing = "0.1.12" +tracing-subscriber = "0.2.0" [build-dependencies] anyhow = "1.0.19" diff --git a/examples/wasi/main.rs b/examples/wasi/main.rs index 35cb948135..5769069443 100644 --- a/examples/wasi/main.rs +++ b/examples/wasi/main.rs @@ -8,6 +8,11 @@ use wasmtime::*; use wasmtime_wasi::{Wasi, WasiCtx}; fn main() -> Result<()> { + tracing_subscriber::FmtSubscriber::builder() + .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) + .with_ansi(true) + .init(); + let store = Store::default(); let mut linker = Linker::new(&store); From 7eb607b07679fda289b4dbd8ba18a4184e7d943a Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Tue, 1 Sep 2020 17:53:44 -0700 Subject: [PATCH 06/13] bugfix! --- crates/wiggle/src/lib.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index d4eabacb9a..9c36c274f3 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -553,13 +553,10 @@ impl<'a, T> GuestPtr<'a, [T]> { pub fn get_range(&self, r: std::ops::Range) -> Option> { let range_length = r.end - r.start; - if r.start < self.len() && r.end < self.len() { + if r.start <= self.len() && r.end <= self.len() { Some(GuestPtr::new( self.mem, - ( - self.offset_base() + r.start, - self.offset_base() + range_length, - ), + (self.offset_base() + r.start, range_length), )) } else { None From ddd50f7443ee6f362008e0c302e2d0a489dd3332 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Wed, 2 Sep 2020 13:13:13 -0700 Subject: [PATCH 07/13] fix error report --- crates/wasi-common/src/string_array_writer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasi-common/src/string_array_writer.rs b/crates/wasi-common/src/string_array_writer.rs index 8f44253858..47a9761bf7 100644 --- a/crates/wasi-common/src/string_array_writer.rs +++ b/crates/wasi-common/src/string_array_writer.rs @@ -39,7 +39,7 @@ impl StringArrayWriter for Vec { let len: u32 = bytes.len().try_into()?; let elem_buffer = buffer .get_range(cursor..(cursor + len)) - .ok_or(Error::Overflow)?; + .ok_or(Error::Inval)?; // Elements don't fit in buffer provided elem_buffer.copy_from_slice(bytes)?; head?.write( elem_buffer From 82b367295634be310ea07813e83b912154167331 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Wed, 2 Sep 2020 13:18:09 -0700 Subject: [PATCH 08/13] delete extra dep --- Cargo.lock | 1 - Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c79c06601b..9be4f475c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2419,7 +2419,6 @@ dependencies = [ "target-lexicon", "tempfile", "test-programs", - "tracing", "tracing-subscriber", "wasi-common", "wasmtime", diff --git a/Cargo.toml b/Cargo.toml index 666f918b14..610a4bad6c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,6 @@ tempfile = "3.1.0" test-programs = { path = "crates/test-programs" } wasmtime-fuzzing = { path = "crates/fuzzing" } wasmtime-runtime = { path = "crates/runtime" } -tracing = "0.1.12" tracing-subscriber = "0.2.0" [build-dependencies] From b63e974014d2f04f2dd5dbaab6f54f5e2136182d Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Wed, 2 Sep 2020 13:39:32 -0700 Subject: [PATCH 09/13] code review fixes --- crates/wiggle/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index 9c36c274f3..3e4e4c97ea 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -449,7 +449,7 @@ impl<'a, T> GuestPtr<'a, [T]> { self.pointer.0 } - /// For slices, returns the length of the slice, in units. + /// For slices, returns the length of the slice, in elements. pub fn len(&self) -> u32 { self.pointer.1 } @@ -552,6 +552,9 @@ impl<'a, T> GuestPtr<'a, [T]> { } pub fn get_range(&self, r: std::ops::Range) -> Option> { + if r.end < r.start { + return None; + } let range_length = r.end - r.start; if r.start <= self.len() && r.end <= self.len() { Some(GuestPtr::new( From 39f1c9716cf50264d8b72afd6c1de90bd2b9c60c Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Wed, 2 Sep 2020 14:11:32 -0700 Subject: [PATCH 10/13] restructure StringArrayWriter trait into StringArray struct --- crates/wasi-common/src/ctx.rs | 56 ++++++------- crates/wasi-common/src/lib.rs | 2 +- .../src/snapshots/wasi_snapshot_preview1.rs | 11 +-- crates/wasi-common/src/string_array.rs | 80 +++++++++++++++++++ crates/wasi-common/src/string_array_writer.rs | 53 ------------ 5 files changed, 111 insertions(+), 91 deletions(-) create mode 100644 crates/wasi-common/src/string_array.rs delete mode 100644 crates/wasi-common/src/string_array_writer.rs diff --git a/crates/wasi-common/src/ctx.rs b/crates/wasi-common/src/ctx.rs index 346af12c28..db7e617097 100644 --- a/crates/wasi-common/src/ctx.rs +++ b/crates/wasi-common/src/ctx.rs @@ -1,7 +1,7 @@ use crate::entry::{Entry, EntryHandle}; use crate::fdpool::FdPool; use crate::handle::Handle; -use crate::string_array_writer::StringArrayWriter; +use crate::string_array::{StringArray, StringArrayError}; use crate::sys::osdir::OsDir; use crate::sys::stdio::NullDevice; use crate::sys::stdio::{Stderr, StderrExt, Stdin, StdinExt, Stdout, StdoutExt}; @@ -12,7 +12,7 @@ use std::borrow::Borrow; use std::cell::RefCell; use std::collections::HashMap; use std::convert::TryFrom; -use std::ffi::{self, CString, OsString}; +use std::ffi::OsString; use std::fs::File; use std::path::{Path, PathBuf}; use std::rc::Rc; @@ -33,9 +33,12 @@ pub enum WasiCtxBuilderError { /// This error is expected to only occur on Windows hosts. #[error("provided sequence is not valid UTF-16: {0}")] InvalidUtf16(#[from] string::FromUtf16Error), - /// Provided sequence of bytes contained an unexpected NUL byte. - #[error("provided sequence contained an unexpected NUL byte")] - UnexpectedNul(#[from] ffi::NulError), + /// Error constructing arguments + #[error("while constructing arguments: {0}")] + Args(#[source] StringArrayError), + /// Error constructing environment + #[error("while constructing environment: {0}")] + Env(#[source] StringArrayError), /// The root of a VirtualDirEntry tree must be a VirtualDirEntry::Directory. #[error("the root of a VirtualDirEntry tree at {} must be a VirtualDirEntry::Directory", .0.display())] VirtualDirEntryRootNotADirectory(PathBuf), @@ -65,24 +68,24 @@ impl std::fmt::Debug for PendingEntry { } #[derive(Debug, Eq, Hash, PartialEq)] -enum PendingCString { +enum PendingString { Bytes(Vec), OsString(OsString), } -impl From> for PendingCString { +impl From> for PendingString { fn from(bytes: Vec) -> Self { Self::Bytes(bytes) } } -impl From for PendingCString { +impl From for PendingString { fn from(s: OsString) -> Self { Self::OsString(s) } } -impl PendingCString { +impl PendingString { fn into_string(self) -> WasiCtxBuilderResult { let res = match self { Self::Bytes(v) => String::from_utf8(v)?, @@ -100,13 +103,6 @@ impl PendingCString { }; Ok(res) } - - /// Create a `CString` containing valid UTF-8. - fn into_utf8_cstring(self) -> WasiCtxBuilderResult { - let s = self.into_string()?; - let s = CString::new(s)?; - Ok(s) - } } struct PendingPreopen(Box WasiCtxBuilderResult>>); @@ -130,8 +126,8 @@ pub struct WasiCtxBuilder { stdout: Option, stderr: Option, preopens: Option>, - args: Option>, - env: Option>, + args: Option>, + env: Option>, } impl WasiCtxBuilder { @@ -180,7 +176,7 @@ impl WasiCtxBuilder { pub fn inherit_args(&mut self) -> &mut Self { let args = self.args.as_mut().unwrap(); args.clear(); - args.extend(env::args_os().map(PendingCString::OsString)); + args.extend(env::args_os().map(PendingString::OsString)); self } @@ -324,16 +320,16 @@ impl WasiCtxBuilder { /// If any of the arguments or environment variables in this builder cannot be converted into /// `CString`s, either due to NUL bytes or Unicode conversions, this will fail. pub fn build(&mut self) -> WasiCtxBuilderResult { - // Process arguments and environment variables into `CString`s, failing quickly if they + // Process arguments and environment variables into `String`s, failing quickly if they // contain any NUL bytes, or if conversion from `OsString` fails. let args = self .args .take() .unwrap() .into_iter() - .map(|arg| arg.into_utf8_cstring()) - .collect::>>()?; - + .map(|arg| arg.into_string()) + .collect::>>()?; + let args = StringArray::new(args).map_err(WasiCtxBuilderError::Args)?; let env = self .env .take() @@ -344,14 +340,12 @@ impl WasiCtxBuilder { v.into_string().and_then(|v| { pair.push('='); pair.push_str(v.as_str()); - // We have valid UTF-8, but the keys and values have not yet been checked - // for NULs, so we do a final check here. - let s = CString::new(pair)?; - Ok(s) + Ok(pair) }) }) }) - .collect::>>()?; + .collect::>>()?; + let env = StringArray::new(env).map_err(WasiCtxBuilderError::Env)?; let mut entries = EntryTable::new(); // Populate the non-preopen entries. @@ -441,8 +435,8 @@ impl EntryTable { pub struct WasiCtx { entries: RefCell, - pub(crate) args: Vec, - pub(crate) env: Vec, + pub(crate) args: StringArray, + pub(crate) env: StringArray, } impl WasiCtx { @@ -493,6 +487,7 @@ impl WasiCtx { self.entries.borrow_mut().remove(fd).ok_or(Error::Badf) } + /* pub(crate) fn args(&self) -> &impl StringArrayWriter { &self.args } @@ -500,4 +495,5 @@ impl WasiCtx { pub(crate) fn env(&self) -> &impl StringArrayWriter { &self.env } + */ } diff --git a/crates/wasi-common/src/lib.rs b/crates/wasi-common/src/lib.rs index c2beccdeb6..013106cb7c 100644 --- a/crates/wasi-common/src/lib.rs +++ b/crates/wasi-common/src/lib.rs @@ -32,7 +32,7 @@ mod path; mod poll; mod sandboxed_tty_writer; pub mod snapshots; -mod string_array_writer; +mod string_array; mod sys; pub mod virtfs; pub mod wasi; diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index be8994c24d..0836491265 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -1,6 +1,5 @@ use crate::entry::{Entry, EntryHandle}; use crate::handle::HandleRights; -use crate::string_array_writer::StringArrayWriter; use crate::sys::clock; use crate::wasi::wasi_snapshot_preview1::WasiSnapshotPreview1; use crate::wasi::{types, AsBytes}; @@ -17,12 +16,11 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { argv: &GuestPtr<'b, GuestPtr<'b, u8>>, argv_buf: &GuestPtr<'b, u8>, ) -> Result<()> { - self.args().write_to_guest(argv_buf, argv) + self.args.write_to_guest(argv_buf, argv) } fn args_sizes_get(&self) -> Result<(types::Size, types::Size)> { - let args = self.args(); - Ok((args.number_elements()?, args.cumulative_size()?)) + Ok((self.args.number_elements, self.args.cumulative_size)) } fn environ_get<'b>( @@ -30,12 +28,11 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { environ: &GuestPtr<'b, GuestPtr<'b, u8>>, environ_buf: &GuestPtr<'b, u8>, ) -> Result<()> { - self.env().write_to_guest(environ_buf, environ) + self.env.write_to_guest(environ_buf, environ) } fn environ_sizes_get(&self) -> Result<(types::Size, types::Size)> { - let args = self.args(); - Ok((args.number_elements()?, args.cumulative_size()?)) + Ok((self.env.number_elements, self.env.cumulative_size)) } fn clock_res_get(&self, id: types::Clockid) -> Result { diff --git a/crates/wasi-common/src/string_array.rs b/crates/wasi-common/src/string_array.rs new file mode 100644 index 0000000000..a671f6a2a1 --- /dev/null +++ b/crates/wasi-common/src/string_array.rs @@ -0,0 +1,80 @@ +use crate::Error; +use std::convert::TryInto; +use std::ffi::CString; +use wiggle::GuestPtr; + +pub struct StringArray { + elems: Vec, + pub number_elements: u32, + pub cumulative_size: u32, +} + +#[derive(Debug, thiserror::Error)] +pub enum StringArrayError { + /// Provided sequence of bytes contained an unexpected NUL byte. + #[error("provided sequence contained an unexpected NUL byte")] + Nul(#[from] std::ffi::NulError), + /// Too many elements: must fit into u32 + #[error("too many elements")] + NumElements, + /// Element size: must fit into u32 + #[error("element too big")] + ElemSize, + /// Cumulative element size: must fit into u32 + #[error("cumulative element size too big")] + CumElemSize, +} + +impl StringArray { + pub fn new(elems: Vec) -> Result { + let elems = elems + .into_iter() + .map(|s| CString::new(s)) + .collect::, _>>()?; + let number_elements = elems + .len() + .try_into() + .map_err(|_| StringArrayError::NumElements)?; + let mut cumulative_size: u32 = 0; + for elem in elems.iter() { + let elem_bytes = elem + .as_bytes_with_nul() + .len() + .try_into() + .map_err(|_| StringArrayError::ElemSize)?; + cumulative_size = cumulative_size + .checked_add(elem_bytes) + .ok_or(StringArrayError::CumElemSize)?; + } + Ok(Self { + elems, + number_elements, + cumulative_size, + }) + } + + pub fn write_to_guest<'a>( + &self, + buffer: &GuestPtr<'a, u8>, + element_heads: &GuestPtr<'a, GuestPtr<'a, u8>>, + ) -> Result<(), Error> { + let element_heads = element_heads.as_array(self.number_elements); + let buffer = buffer.as_array(self.cumulative_size); + let mut cursor = 0; + for (elem, head) in self.elems.iter().zip(element_heads.iter()) { + let bytes = elem.as_bytes_with_nul(); + let len: u32 = bytes.len().try_into()?; + let elem_buffer = buffer + .get_range(cursor..(cursor + len)) + .ok_or(Error::Inval)?; // Elements don't fit in buffer provided + elem_buffer.copy_from_slice(bytes)?; + head?.write( + elem_buffer + .get(0) + .expect("all elem buffers at least length 1"), + )?; + cursor += len; + } + Ok(()) + } +} diff --git a/crates/wasi-common/src/string_array_writer.rs b/crates/wasi-common/src/string_array_writer.rs deleted file mode 100644 index 47a9761bf7..0000000000 --- a/crates/wasi-common/src/string_array_writer.rs +++ /dev/null @@ -1,53 +0,0 @@ -use crate::{Error, Result}; -use std::convert::TryInto; -use std::ffi::CString; -use wiggle::GuestPtr; - -pub trait StringArrayWriter { - fn number_elements(&self) -> Result; - fn cumulative_size(&self) -> Result; - fn write_to_guest<'a>( - &self, - buffer: &GuestPtr<'a, u8>, - elements: &GuestPtr<'a, GuestPtr<'a, u8>>, - ) -> Result<()>; -} - -impl StringArrayWriter for Vec { - fn number_elements(&self) -> Result { - let elems = self.len().try_into()?; - Ok(elems) - } - fn cumulative_size(&self) -> Result { - let mut total: u32 = 0; - for elem in self.iter() { - let elem_bytes = elem.as_bytes_with_nul().len().try_into()?; - total = total.checked_add(elem_bytes).ok_or(Error::Overflow)?; - } - Ok(total) - } - fn write_to_guest<'a>( - &self, - buffer: &GuestPtr<'a, u8>, - element_heads: &GuestPtr<'a, GuestPtr<'a, u8>>, - ) -> Result<()> { - let element_heads = element_heads.as_array(self.number_elements()?); - let buffer = buffer.as_array(self.cumulative_size()?); - let mut cursor = 0; - for (elem, head) in self.iter().zip(element_heads.iter()) { - let bytes = elem.as_bytes_with_nul(); - let len: u32 = bytes.len().try_into()?; - let elem_buffer = buffer - .get_range(cursor..(cursor + len)) - .ok_or(Error::Inval)?; // Elements don't fit in buffer provided - elem_buffer.copy_from_slice(bytes)?; - head?.write( - elem_buffer - .get(0) - .expect("all elem buffers at least length 1"), - )?; - cursor += len; - } - Ok(()) - } -} From 1d410d655995e1116958c6752fecb02c53145f1e Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Wed, 2 Sep 2020 17:16:39 -0700 Subject: [PATCH 11/13] move more constructor stuff into stringarray --- crates/wasi-common/src/ctx.rs | 88 ++++---------------------- crates/wasi-common/src/string_array.rs | 77 ++++++++++++++++++++-- 2 files changed, 82 insertions(+), 83 deletions(-) diff --git a/crates/wasi-common/src/ctx.rs b/crates/wasi-common/src/ctx.rs index db7e617097..1b77b093c4 100644 --- a/crates/wasi-common/src/ctx.rs +++ b/crates/wasi-common/src/ctx.rs @@ -1,22 +1,21 @@ use crate::entry::{Entry, EntryHandle}; use crate::fdpool::FdPool; use crate::handle::Handle; -use crate::string_array::{StringArray, StringArrayError}; +use crate::string_array::{PendingString, StringArray, StringArrayError}; use crate::sys::osdir::OsDir; use crate::sys::stdio::NullDevice; use crate::sys::stdio::{Stderr, StderrExt, Stdin, StdinExt, Stdout, StdoutExt}; use crate::virtfs::{VirtualDir, VirtualDirEntry}; use crate::wasi::types; -use crate::{Error, Result}; +use crate::Error; use std::borrow::Borrow; use std::cell::RefCell; use std::collections::HashMap; use std::convert::TryFrom; -use std::ffi::OsString; use std::fs::File; use std::path::{Path, PathBuf}; use std::rc::Rc; -use std::{env, io, string}; +use std::{env, io}; /// Possible errors when `WasiCtxBuilder` fails building /// `WasiCtx`. @@ -25,14 +24,6 @@ pub enum WasiCtxBuilderError { /// General I/O error was encountered. #[error("general I/O error encountered: {0}")] Io(#[from] io::Error), - /// Provided sequence of bytes was not a valid UTF-8. - #[error("provided sequence is not valid UTF-8: {0}")] - InvalidUtf8(#[from] string::FromUtf8Error), - /// Provided sequence of bytes was not a valid UTF-16. - /// - /// This error is expected to only occur on Windows hosts. - #[error("provided sequence is not valid UTF-16: {0}")] - InvalidUtf16(#[from] string::FromUtf16Error), /// Error constructing arguments #[error("while constructing arguments: {0}")] Args(#[source] StringArrayError), @@ -67,44 +58,6 @@ impl std::fmt::Debug for PendingEntry { } } -#[derive(Debug, Eq, Hash, PartialEq)] -enum PendingString { - Bytes(Vec), - OsString(OsString), -} - -impl From> for PendingString { - fn from(bytes: Vec) -> Self { - Self::Bytes(bytes) - } -} - -impl From for PendingString { - fn from(s: OsString) -> Self { - Self::OsString(s) - } -} - -impl PendingString { - fn into_string(self) -> WasiCtxBuilderResult { - let res = match self { - Self::Bytes(v) => String::from_utf8(v)?, - #[cfg(unix)] - Self::OsString(s) => { - use std::os::unix::ffi::OsStringExt; - String::from_utf8(s.into_vec())? - } - #[cfg(windows)] - Self::OsString(s) => { - use std::os::windows::ffi::OsStrExt; - let bytes: Vec = s.encode_wide().collect(); - String::from_utf16(&bytes)? - } - }; - Ok(res) - } -} - struct PendingPreopen(Box WasiCtxBuilderResult>>); impl PendingPreopen { @@ -322,30 +275,11 @@ impl WasiCtxBuilder { pub fn build(&mut self) -> WasiCtxBuilderResult { // Process arguments and environment variables into `String`s, failing quickly if they // contain any NUL bytes, or if conversion from `OsString` fails. - let args = self - .args - .take() - .unwrap() - .into_iter() - .map(|arg| arg.into_string()) - .collect::>>()?; - let args = StringArray::new(args).map_err(WasiCtxBuilderError::Args)?; - let env = self - .env - .take() - .unwrap() - .into_iter() - .map(|(k, v)| { - k.into_string().and_then(|mut pair| { - v.into_string().and_then(|v| { - pair.push('='); - pair.push_str(v.as_str()); - Ok(pair) - }) - }) - }) - .collect::>>()?; - let env = StringArray::new(env).map_err(WasiCtxBuilderError::Env)?; + let args = + StringArray::from_pending_vec(self.args.take().expect("WasiCtxBuilder has args")) + .map_err(WasiCtxBuilderError::Args)?; + let env = StringArray::from_pending_map(self.env.take().expect("WasiCtxBuilder has env")) + .map_err(WasiCtxBuilderError::Env)?; let mut entries = EntryTable::new(); // Populate the non-preopen entries. @@ -461,7 +395,7 @@ impl WasiCtx { } /// Get an immutable `Entry` corresponding to the specified raw WASI `fd`. - pub(crate) fn get_entry(&self, fd: types::Fd) -> Result> { + pub(crate) fn get_entry(&self, fd: types::Fd) -> Result, Error> { match self.entries.borrow().get(&fd) { Some(entry) => Ok(entry), None => Err(Error::Badf), @@ -472,7 +406,7 @@ impl WasiCtx { /// /// The `Entry` will automatically get another free raw WASI `fd` assigned. Note that /// the two subsequent free raw WASI `fd`s do not have to be stored contiguously. - pub(crate) fn insert_entry(&self, entry: Entry) -> Result { + pub(crate) fn insert_entry(&self, entry: Entry) -> Result { self.entries.borrow_mut().insert(entry).ok_or(Error::Mfile) } @@ -483,7 +417,7 @@ impl WasiCtx { } /// Remove `Entry` corresponding to the specified raw WASI `fd` from the `WasiCtx` object. - pub(crate) fn remove_entry(&self, fd: types::Fd) -> Result> { + pub(crate) fn remove_entry(&self, fd: types::Fd) -> Result, Error> { self.entries.borrow_mut().remove(fd).ok_or(Error::Badf) } diff --git a/crates/wasi-common/src/string_array.rs b/crates/wasi-common/src/string_array.rs index a671f6a2a1..cfb3d75af2 100644 --- a/crates/wasi-common/src/string_array.rs +++ b/crates/wasi-common/src/string_array.rs @@ -1,12 +1,45 @@ use crate::Error; +use std::collections::HashMap; use std::convert::TryInto; -use std::ffi::CString; +use std::ffi::{CString, OsString}; use wiggle::GuestPtr; -pub struct StringArray { - elems: Vec, - pub number_elements: u32, - pub cumulative_size: u32, +#[derive(Debug, Eq, Hash, PartialEq)] +pub enum PendingString { + Bytes(Vec), + OsString(OsString), +} + +impl From> for PendingString { + fn from(bytes: Vec) -> Self { + Self::Bytes(bytes) + } +} + +impl From for PendingString { + fn from(s: OsString) -> Self { + Self::OsString(s) + } +} + +impl PendingString { + pub fn into_string(self) -> Result { + let res = match self { + Self::Bytes(v) => String::from_utf8(v)?, + #[cfg(unix)] + Self::OsString(s) => { + use std::os::unix::ffi::OsStringExt; + String::from_utf8(s.into_vec())? + } + #[cfg(windows)] + Self::OsString(s) => { + use std::os::windows::ffi::OsStrExt; + let bytes: Vec = s.encode_wide().collect(); + String::from_utf16(&bytes)? + } + }; + Ok(res) + } } #[derive(Debug, thiserror::Error)] @@ -23,10 +56,42 @@ pub enum StringArrayError { /// Cumulative element size: must fit into u32 #[error("cumulative element size too big")] CumElemSize, + /// Provided sequence of bytes was not a valid UTF-8. + #[error("provided sequence is not valid UTF-8: {0}")] + InvalidUtf8(#[from] std::string::FromUtf8Error), + /// Provided sequence of bytes was not a valid UTF-16. + /// + /// This error is expected to only occur on Windows hosts. + #[error("provided sequence is not valid UTF-16: {0}")] + InvalidUtf16(#[from] std::string::FromUtf16Error), } +pub struct StringArray { + elems: Vec, + pub number_elements: u32, + pub cumulative_size: u32, +} impl StringArray { - pub fn new(elems: Vec) -> Result { + pub fn from_pending_vec(elems: Vec) -> Result { + let elems = elems + .into_iter() + .map(|arg| arg.into_string()) + .collect::, StringArrayError>>()?; + Self::from_strings(elems) + } + pub fn from_pending_map( + elems: HashMap, + ) -> Result { + let mut pairs = Vec::new(); + for (k, v) in elems.into_iter() { + let mut pair = k.into_string()?; + pair.push('='); + pair.push_str(&v.into_string()?); + pairs.push(pair); + } + Self::from_strings(pairs) + } + pub fn from_strings(elems: Vec) -> Result { let elems = elems .into_iter() .map(|s| CString::new(s)) From 580c236dee05992985e0c5aeb18a632ff78c8640 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Thu, 3 Sep 2020 16:40:45 -0700 Subject: [PATCH 12/13] wiggle: implement array get/get_range in terms of ptr/add/as_array hard to get it wrong when you use the safe primitives i already made! --- crates/wiggle/src/lib.rs | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index 3e4e4c97ea..fe2b172498 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -543,24 +543,36 @@ impl<'a, T> GuestPtr<'a, [T]> { GuestPtr::new(self.mem, self.offset_base()) } - pub fn get(&self, index: u32) -> Option> { + pub fn get(&self, index: u32) -> Option> + where + T: GuestType<'a>, + { if index < self.len() { - Some(GuestPtr::new(self.mem, self.offset_base() + index)) + Some( + self.as_ptr() + .add(index) + .expect("just performed bounds check"), + ) } else { None } } - pub fn get_range(&self, r: std::ops::Range) -> Option> { + pub fn get_range(&self, r: std::ops::Range) -> Option> + where + T: GuestType<'a>, + { if r.end < r.start { return None; } let range_length = r.end - r.start; if r.start <= self.len() && r.end <= self.len() { - Some(GuestPtr::new( - self.mem, - (self.offset_base() + r.start, range_length), - )) + Some( + self.as_ptr() + .add(r.start) + .expect("just performed bounds check") + .as_array(range_length), + ) } else { None } From 776f12ae3c0c21e8cda0b7c251c998d35ef0a1c0 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Thu, 3 Sep 2020 16:41:10 -0700 Subject: [PATCH 13/13] tests: exercise array getters --- crates/wiggle/test-helpers/src/lib.rs | 9 +- crates/wiggle/tests/arrays.rs | 191 +++++++++++++++++++++++++- crates/wiggle/tests/arrays.witx | 19 +++ crates/wiggle/tests/strings.rs | 8 +- crates/wiggle/tests/structs.rs | 1 + 5 files changed, 221 insertions(+), 7 deletions(-) diff --git a/crates/wiggle/test-helpers/src/lib.rs b/crates/wiggle/test-helpers/src/lib.rs index 7c9f8840ef..6164e4f92f 100644 --- a/crates/wiggle/test-helpers/src/lib.rs +++ b/crates/wiggle/test-helpers/src/lib.rs @@ -102,10 +102,11 @@ impl HostMemory { out } - pub fn byte_slice_strat(size: u32, exclude: &MemAreas) -> BoxedStrategy { + pub fn byte_slice_strat(size: u32, align: u32, exclude: &MemAreas) -> BoxedStrategy { let available: Vec = Self::invert(exclude) .iter() .flat_map(|a| a.inside(size)) + .filter(|a| a.ptr % align == 0) .collect(); Just(available) @@ -256,18 +257,18 @@ mod test { s2: u32, s3: u32, ) -> BoxedStrategy<(MemArea, MemArea, MemArea)> { - HostMemory::byte_slice_strat(s1, &MemAreas::new()) + HostMemory::byte_slice_strat(s1, 1, &MemAreas::new()) .prop_flat_map(move |a1| { ( Just(a1), - HostMemory::byte_slice_strat(s2, &MemAreas::from(&[a1])), + HostMemory::byte_slice_strat(s2, 1, &MemAreas::from(&[a1])), ) }) .prop_flat_map(move |(a1, a2)| { ( Just(a1), Just(a2), - HostMemory::byte_slice_strat(s3, &MemAreas::from(&[a1, a2])), + HostMemory::byte_slice_strat(s3, 1, &MemAreas::from(&[a1, a2])), ) }) .boxed() diff --git a/crates/wiggle/tests/arrays.rs b/crates/wiggle/tests/arrays.rs index 9bfad46e7b..5f660fce69 100644 --- a/crates/wiggle/tests/arrays.rs +++ b/crates/wiggle/tests/arrays.rs @@ -1,6 +1,6 @@ use proptest::prelude::*; -use wiggle::{GuestMemory, GuestPtr}; -use wiggle_test::{impl_errno, HostMemory, MemArea, WasiCtx}; +use wiggle::{GuestMemory, GuestPtr, GuestType}; +use wiggle_test::{impl_errno, HostMemory, MemArea, MemAreas, WasiCtx}; wiggle::from_witx!({ witx: ["$CARGO_MANIFEST_DIR/tests/arrays.witx"], @@ -208,3 +208,190 @@ proptest! { e.test() } } + +impl<'a> array_traversal::ArrayTraversal for WasiCtx<'a> { + fn sum_of_element( + &self, + elements: &GuestPtr<[types::PairInts]>, + index: u32, + ) -> Result { + let elem_ptr = elements.get(index).ok_or(types::Errno::InvalidArg)?; + let pair = elem_ptr.read().map_err(|_| types::Errno::DontWantTo)?; + Ok(pair.first.wrapping_add(pair.second)) + } + fn sum_of_elements( + &self, + elements: &GuestPtr<[types::PairInts]>, + start: u32, + end: u32, + ) -> Result { + let elem_range = elements + .get_range(start..end) + .ok_or(types::Errno::InvalidArg)?; + let mut sum: i32 = 0; + for e in elem_range.iter() { + let pair = e + .map_err(|_| types::Errno::DontWantTo)? + .read() + .map_err(|_| types::Errno::PhysicallyUnable)?; + sum = sum.wrapping_add(pair.first).wrapping_add(pair.second); + } + Ok(sum) + } +} + +impl types::PairInts { + pub fn strat() -> BoxedStrategy { + (prop::num::i32::ANY, prop::num::i32::ANY) + .prop_map(|(first, second)| types::PairInts { first, second }) + .boxed() + } +} + +#[derive(Debug)] +struct SumElementsExercise { + elements: Vec, + element_loc: MemArea, + return_loc: MemArea, + start_ix: u32, + end_ix: u32, +} + +impl SumElementsExercise { + pub fn strat() -> BoxedStrategy { + ( + prop::collection::vec(types::PairInts::strat(), 1..256), + HostMemory::mem_area_strat(4), + ) + .prop_flat_map(|(elements, return_loc)| { + let len = elements.len() as u32; + ( + Just(elements), + HostMemory::byte_slice_strat( + len * types::PairInts::guest_size(), + types::PairInts::guest_size(), + &MemAreas::from([return_loc]), + ), + Just(return_loc), + 0..len, + 0..len, + ) + }) + .prop_map( + |(elements, element_loc, return_loc, start_ix, end_ix)| SumElementsExercise { + elements, + element_loc, + return_loc, + start_ix, + end_ix, + }, + ) + .boxed() + } + pub fn test(&self) { + let ctx = WasiCtx::new(); + let host_memory = HostMemory::new(); + + // Populate array + let ptr = host_memory + .ptr::<[types::PairInts]>((self.element_loc.ptr, self.elements.len() as u32)); + for (ptr, val) in ptr.iter().zip(&self.elements) { + ptr.expect("should be valid pointer") + .write(val.clone()) + .expect("failed to write value"); + } + + let res = array_traversal::sum_of_element( + &ctx, + &host_memory, + self.element_loc.ptr as i32, + self.elements.len() as i32, + self.start_ix as i32, + self.return_loc.ptr as i32, + ); + assert_eq!(res, types::Errno::Ok.into(), "sum_of_element errno"); + let result_ptr = host_memory.ptr::(self.return_loc.ptr); + let result = result_ptr.read().expect("read result"); + + let e = self + .elements + .get(self.start_ix as usize) + .expect("start_ix must be in bounds"); + assert_eq!(result, e.first.wrapping_add(e.second), "sum of element"); + + // Off the end of the array: + let res = array_traversal::sum_of_element( + &ctx, + &host_memory, + self.element_loc.ptr as i32, + self.elements.len() as i32, + self.elements.len() as i32, + self.return_loc.ptr as i32, + ); + assert_eq!( + res, + types::Errno::InvalidArg.into(), + "out of bounds sum_of_element errno" + ); + + let res = array_traversal::sum_of_elements( + &ctx, + &host_memory, + self.element_loc.ptr as i32, + self.elements.len() as i32, + self.start_ix as i32, + self.end_ix as i32, + self.return_loc.ptr as i32, + ); + if self.start_ix <= self.end_ix { + assert_eq!( + res, + types::Errno::Ok.into(), + "expected ok sum_of_elements errno" + ); + let result_ptr = host_memory.ptr::(self.return_loc.ptr); + let result = result_ptr.read().expect("read result"); + + let mut expected_sum: i32 = 0; + for elem in self + .elements + .get(self.start_ix as usize..self.end_ix as usize) + .unwrap() + .iter() + { + expected_sum = expected_sum + .wrapping_add(elem.first) + .wrapping_add(elem.second); + } + assert_eq!(result, expected_sum, "sum of elements"); + } else { + assert_eq!( + res, + types::Errno::InvalidArg.into(), + "expected error out-of-bounds sum_of_elements" + ); + } + + // Index an array off the end of the array: + let res = array_traversal::sum_of_elements( + &ctx, + &host_memory, + self.element_loc.ptr as i32, + self.elements.len() as i32, + self.start_ix as i32, + self.elements.len() as i32 + 1, + self.return_loc.ptr as i32, + ); + assert_eq!( + res, + types::Errno::InvalidArg.into(), + "out of bounds sum_of_elements errno" + ); + } +} +proptest! { + #[test] + fn sum_elements(e in SumElementsExercise::strat()) { + e.test() + } +} diff --git a/crates/wiggle/tests/arrays.witx b/crates/wiggle/tests/arrays.witx index e8a81cee95..4b6a1f14c5 100644 --- a/crates/wiggle/tests/arrays.witx +++ b/crates/wiggle/tests/arrays.witx @@ -15,3 +15,22 @@ (result $error $errno) ) ) + +(typename $pair_ints + (struct + (field $first s32) + (field $second s32))) + +(module $array_traversal + (@interface func (export "sum_of_element") + (param $elements (array $pair_ints)) + (param $index (@witx usize)) + (result $error $errno) + (result $sum s32)) + + (@interface func (export "sum_of_elements") + (param $elements (array $pair_ints)) + (param $start (@witx usize)) + (param $end (@witx usize)) + (result $error $errno) + (result $sum s32))) diff --git a/crates/wiggle/tests/strings.rs b/crates/wiggle/tests/strings.rs index e68a569ed6..cff55d92d0 100644 --- a/crates/wiggle/tests/strings.rs +++ b/crates/wiggle/tests/strings.rs @@ -125,7 +125,11 @@ impl MultiStringExercise { Just(a.clone()), Just(b.clone()), Just(c.clone()), - HostMemory::byte_slice_strat(a.len() as u32, &MemAreas::from([return_ptr_loc])), + HostMemory::byte_slice_strat( + a.len() as u32, + 1, + &MemAreas::from([return_ptr_loc]), + ), Just(return_ptr_loc), ) }) @@ -137,6 +141,7 @@ impl MultiStringExercise { Just(sa_ptr_loc), HostMemory::byte_slice_strat( b.len() as u32, + 1, &MemAreas::from([sa_ptr_loc, return_ptr_loc]), ), Just(return_ptr_loc), @@ -151,6 +156,7 @@ impl MultiStringExercise { Just(sb_ptr_loc), HostMemory::byte_slice_strat( c.len() as u32, + 1, &MemAreas::from([sa_ptr_loc, sb_ptr_loc, return_ptr_loc]), ), Just(return_ptr_loc), diff --git a/crates/wiggle/tests/structs.rs b/crates/wiggle/tests/structs.rs index ad2e43a2db..27b7b865c1 100644 --- a/crates/wiggle/tests/structs.rs +++ b/crates/wiggle/tests/structs.rs @@ -473,6 +473,7 @@ impl SumArrayExercise { Just(inputs.clone()), HostMemory::byte_slice_strat( inputs.len() as u32, + 1, &MemAreas::from([input_struct_loc, output_loc]), ), Just(input_struct_loc.clone()),