From eb1cf8b0a16e5914f5dc5c04a046fd545299f339 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Mon, 13 May 2019 15:47:38 -0700 Subject: [PATCH] Make users of `dec_slice_of` safe. Make `dec_slice_of` return a slice rather than a pointer-length pair, freeing its users from having to call the unsafe `slice::from_raw_parts`. This requires splitting `dec_slice_of` and `dec_ptr` into mut and non-mut versions, and reorganizing poll_oneoff a little to avoid borrow-checker errors -- decoded slices do alias the main memory, so make sure functions only need one or the other. --- src/hostcalls.rs | 63 ++++++++++++++++------------------------ src/memory.rs | 75 +++++++++++++++++++++++++++++++++++++----------- 2 files changed, 82 insertions(+), 56 deletions(-) diff --git a/src/hostcalls.rs b/src/hostcalls.rs index 89c41d97d5..d377dbf90a 100644 --- a/src/hostcalls.rs +++ b/src/hostcalls.rs @@ -542,13 +542,12 @@ pub fn path_open( } let path = match dec_slice_of::(memory, path_ptr, path_len) { - Ok((ptr, len)) => OsStr::from_bytes(unsafe { std::slice::from_raw_parts(ptr, len) }), + Ok(slice) => OsStr::from_bytes(slice), Err(e) => return enc_errno(e), }; let (dir, path) = match path_get( wasi_ctx, - memory, dirfd, dirflags, path, @@ -619,14 +618,11 @@ pub fn random_get( ) -> wasm32::__wasi_errno_t { use rand::{thread_rng, RngCore}; - let buf_len = dec_usize(buf_len); - let buf_ptr = match dec_ptr(memory, buf_ptr, buf_len) { - Ok(ptr) => ptr, + let buf = match dec_slice_of_mut::(memory, buf_ptr, buf_len) { + Ok(buf) => buf, Err(e) => return enc_errno(e), }; - let buf = unsafe { std::slice::from_raw_parts_mut(buf_ptr, buf_len) }; - thread_rng().fill_bytes(buf); return wasm32::__WASI_ESUCCESS; @@ -643,16 +639,14 @@ pub fn poll_oneoff( return wasm32::__WASI_EINVAL; } enc_pointee(memory, nevents, 0).unwrap(); - let input_slice_ = + let input_slice = dec_slice_of::(memory, input, nsubscriptions).unwrap(); - let input_slice = unsafe { slice::from_raw_parts(input_slice_.0, input_slice_.1) }; - - let output_slice_ = - dec_slice_of::(memory, output, nsubscriptions).unwrap(); - let output_slice = unsafe { slice::from_raw_parts_mut(output_slice_.0, output_slice_.1) }; let input: Vec<_> = input_slice.iter().map(|x| dec_subscription(x)).collect(); + let output_slice = + dec_slice_of_mut::(memory, output, nsubscriptions).unwrap(); + let timeout = input .iter() .filter_map(|event| match event { @@ -713,11 +707,16 @@ pub fn poll_oneoff( Ok(ready) => break ready as usize, } }; - if ready == 0 { - return poll_oneoff_handle_timeout_event(memory, output_slice, nevents, timeout); + let events_count = if ready == 0 { + poll_oneoff_handle_timeout_event(output_slice, nevents, timeout) + } else { + let events = fd_events.iter().zip(poll_fds.iter()).take(ready); + poll_oneoff_handle_fd_event(output_slice, nevents, events) + }; + if let Err(e) = enc_pointee(memory, nevents, events_count) { + return enc_errno(e); } - let events = fd_events.iter().zip(poll_fds.iter()).take(ready); - poll_oneoff_handle_fd_event(memory, output_slice, nevents, events) + wasm32::__WASI_ESUCCESS } pub fn fd_filestat_get( @@ -761,12 +760,11 @@ pub fn path_filestat_get( let dirfd = dec_fd(dirfd); let dirflags = dec_lookupflags(dirflags); let path = match dec_slice_of::(memory, path_ptr, path_len) { - Ok((ptr, len)) => OsStr::from_bytes(unsafe { std::slice::from_raw_parts(ptr, len) }), + Ok(slice) => OsStr::from_bytes(slice), Err(e) => return enc_errno(e), }; let (dir, path) = match path_get( wasi_ctx, - memory, dirfd, dirflags, path, @@ -804,12 +802,11 @@ pub fn path_create_directory( let dirfd = dec_fd(dirfd); let path = match dec_slice_of::(memory, path_ptr, path_len) { - Ok((ptr, len)) => OsStr::from_bytes(unsafe { std::slice::from_raw_parts(ptr, len) }), + Ok(slice) => OsStr::from_bytes(slice), Err(e) => return enc_errno(e), }; let (dir, path) = match path_get( wasi_ctx, - memory, dirfd, 0, path, @@ -843,12 +840,11 @@ pub fn path_unlink_file( let dirfd = dec_fd(dirfd); let path = match dec_slice_of::(memory, path_ptr, path_len) { - Ok((ptr, len)) => OsStr::from_bytes(unsafe { std::slice::from_raw_parts(ptr, len) }), + Ok(slice) => OsStr::from_bytes(slice), Err(e) => return enc_errno(e), }; let (dir, path) = match path_get( wasi_ctx, - memory, dirfd, 0, path, @@ -1143,11 +1139,10 @@ struct FdEventData { } fn poll_oneoff_handle_timeout_event( - memory: &mut [u8], output_slice: &mut [wasm32::__wasi_event_t], nevents: wasm32::uintptr_t, timeout: Option, -) -> wasm32::__wasi_errno_t { +) -> wasm32::size_t { if let Some(ClockEventData { userdata, .. }) = timeout { let output_event = host::__wasi_event_t { userdata, @@ -1161,24 +1156,18 @@ fn poll_oneoff_handle_timeout_event( }, }; output_slice[0] = enc_event(output_event); - if let Err(e) = enc_pointee(memory, nevents, 1) { - return enc_errno(e); - } + 1 } else { // shouldn't happen - if let Err(e) = enc_pointee(memory, nevents, 0) { - return enc_errno(e); - } + 0 } - wasm32::__WASI_ESUCCESS } fn poll_oneoff_handle_fd_event<'t>( - memory: &mut [u8], output_slice: &mut [wasm32::__wasi_event_t], nevents: wasm32::uintptr_t, events: impl Iterator, -) -> wasm32::__wasi_errno_t { +) -> wasm32::size_t { let mut output_slice_cur = output_slice.iter_mut(); let mut revents_count = 0; for (fd_event, poll_fd) in events { @@ -1250,10 +1239,7 @@ fn poll_oneoff_handle_fd_event<'t>( *output_slice_cur.next().unwrap() = enc_event(output_event); revents_count += 1; } - if let Err(e) = enc_pointee(memory, nevents, revents_count) { - return enc_errno(e); - } - wasm32::__WASI_ESUCCESS + revents_count } /// Normalizes a path to ensure that the target path is located under the directory provided. @@ -1261,7 +1247,6 @@ fn poll_oneoff_handle_fd_event<'t>( /// This is a workaround for not having Capsicum support in the OS. pub fn path_get>( wasi_ctx: &WasiCtx, - memory: &mut [u8], dirfd: host::__wasi_fd_t, dirflags: host::__wasi_lookupflags_t, path: P, diff --git a/src/memory.rs b/src/memory.rs index 5a2f6b6512..0b28ebd1ca 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -23,7 +23,22 @@ macro_rules! bail_errno { }; } -pub fn dec_ptr( +fn dec_ptr( + memory: &[u8], + ptr: wasm32::uintptr_t, + len: usize, +) -> Result<*const u8, host::__wasi_errno_t> { + // check for overflow + let checked_len = (ptr as usize).checked_add(len).ok_or(host::__WASI_EFAULT)?; + + // translate the pointer + memory + .get(ptr as usize..checked_len) + .ok_or(host::__WASI_EFAULT) + .map(|mem| mem.as_ptr()) +} + +fn dec_ptr_mut( memory: &mut [u8], ptr: wasm32::uintptr_t, len: usize, @@ -38,7 +53,19 @@ pub fn dec_ptr( .map(|mem| mem.as_mut_ptr()) } -pub fn dec_ptr_to<'memory, T>( +fn dec_ptr_to<'memory, T>( + memory: &'memory [u8], + ptr: wasm32::uintptr_t, +) -> Result<&'memory T, host::__wasi_errno_t> { + // check that the ptr is aligned + if ptr as usize % align_of::() != 0 { + bail_errno!(__WASI_EINVAL); + } + + dec_ptr(memory, ptr, size_of::()).map(|p| unsafe { &*(p as *const T) }) +} + +fn dec_ptr_to_mut<'memory, T>( memory: &'memory mut [u8], ptr: wasm32::uintptr_t, ) -> Result<&'memory mut T, host::__wasi_errno_t> { @@ -47,13 +74,10 @@ pub fn dec_ptr_to<'memory, T>( bail_errno!(__WASI_EINVAL); } - dec_ptr(memory, ptr, size_of::()).map(|p| unsafe { &mut *(p as *mut T) }) + dec_ptr_mut(memory, ptr, size_of::()).map(|p| unsafe { &mut *(p as *mut T) }) } -pub fn dec_pointee( - memory: &mut [u8], - ptr: wasm32::uintptr_t, -) -> Result { +pub fn dec_pointee(memory: &[u8], ptr: wasm32::uintptr_t) -> Result { dec_ptr_to::(memory, ptr).map(|p| unsafe { ptr::read(p) }) } @@ -62,14 +86,13 @@ pub fn enc_pointee( ptr: wasm32::uintptr_t, t: T, ) -> Result<(), host::__wasi_errno_t> { - dec_ptr_to::(memory, ptr).map(|p| unsafe { ptr::write(p, t) }) + dec_ptr_to_mut::(memory, ptr).map(|p| unsafe { ptr::write(p, t) }) } -pub fn dec_slice_of<'memory, T>( - memory: &'memory mut [u8], +fn check_slice_of( ptr: wasm32::uintptr_t, len: wasm32::size_t, -) -> Result<(&'memory mut T, usize), host::__wasi_errno_t> { +) -> Result<(usize, usize), host::__wasi_errno_t> { // check alignment, and that length doesn't overflow if ptr as usize % align_of::() != 0 { return Err(host::__WASI_EINVAL); @@ -81,8 +104,27 @@ pub fn dec_slice_of<'memory, T>( return Err(host::__WASI_EOVERFLOW); }; - let ptr = dec_ptr(memory, ptr, len_bytes)? as *mut T; - Ok((unsafe { &mut *ptr }, len)) + Ok((len, len_bytes)) +} + +pub fn dec_slice_of<'memory, T>( + memory: &'memory [u8], + ptr: wasm32::uintptr_t, + len: wasm32::size_t, +) -> Result<&'memory [T], host::__wasi_errno_t> { + let (len, len_bytes) = check_slice_of::(ptr, len)?; + let ptr = dec_ptr(memory, ptr, len_bytes)? as *const T; + Ok(unsafe { slice::from_raw_parts(ptr, len) }) +} + +pub fn dec_slice_of_mut<'memory, T>( + memory: &'memory mut [u8], + ptr: wasm32::uintptr_t, + len: wasm32::size_t, +) -> Result<&'memory mut [T], host::__wasi_errno_t> { + let (len, len_bytes) = check_slice_of::(ptr, len)?; + let ptr = dec_ptr_mut(memory, ptr, len_bytes)? as *mut T; + Ok(unsafe { slice::from_raw_parts_mut(ptr, len) }) } pub fn enc_slice_of( @@ -102,7 +144,7 @@ pub fn enc_slice_of( }; // get the pointer into guest memory, and copy the bytes - let ptr = dec_ptr(memory, ptr, len_bytes)? as *mut libc::c_void; + let ptr = dec_ptr_mut(memory, ptr, len_bytes)? as *mut libc::c_void; unsafe { libc::memcpy(ptr, slice.as_ptr() as *const libc::c_void, len_bytes); } @@ -138,7 +180,7 @@ macro_rules! dec_enc_scalar { } pub fn dec_ciovec( - memory: &mut [u8], + memory: &[u8], ciovec: &wasm32::__wasi_ciovec_t, ) -> Result { let len = dec_usize(ciovec.buf_len); @@ -149,12 +191,11 @@ pub fn dec_ciovec( } pub fn dec_ciovec_slice( - memory: &mut [u8], + memory: &[u8], ptr: wasm32::uintptr_t, len: wasm32::size_t, ) -> Result, host::__wasi_errno_t> { let slice = dec_slice_of::(memory, ptr, len)?; - let slice = unsafe { slice::from_raw_parts(slice.0, slice.1) }; slice.iter().map(|iov| dec_ciovec(memory, iov)).collect() }