From 1f6890e07066036e4d0d753f03801bd4fbb9bd91 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Wed, 25 Mar 2020 14:00:52 +0100 Subject: [PATCH] [wasi-common] Clean up the use of mutable Entry (#1395) * Clean up the use of mutable Entry Until now, several syscalls including `fd_pwrite` etc. were relying on mutating `&mut Entry` by mutating its inner file handle. This is unnecessary in almost all cases since all methods mutating `std::fs::File` in Rust's libstd are also implemented for `&std::fs::File`. While here, I've also modified `OsHandle` in BSD to include `RefCell>` rather than `Option>` as was until now. While `RefCell` could easily be replaced with `RefCell`, since going multithreading will require a lot of (probably even) conceptual changes to `wasi-common`, I thought it'd be best not to mix single- with multithreading contexts and swap all places at once when it comes to it. I've also had to make some modifications to virtual FS which mainly swapped mutability for interior mutability in places. * Use one-liners wherever convenient --- crates/wasi-common/src/entry.rs | 17 +----- .../src/snapshots/wasi_snapshot_preview1.rs | 58 ++++++++++--------- crates/wasi-common/src/sys/unix/bsd/fd.rs | 37 +++++------- .../wasi-common/src/sys/unix/bsd/oshandle.rs | 17 +++--- crates/wasi-common/src/sys/unix/fd.rs | 2 +- crates/wasi-common/src/sys/unix/linux/fd.rs | 2 +- .../src/sys/unix/linux/oshandle.rs | 8 +-- crates/wasi-common/src/sys/windows/entry.rs | 8 +-- crates/wasi-common/src/virtfs.rs | 42 ++++++++------ 9 files changed, 81 insertions(+), 110 deletions(-) diff --git a/crates/wasi-common/src/entry.rs b/crates/wasi-common/src/entry.rs index f4a7264f3e..8bd5072ce3 100644 --- a/crates/wasi-common/src/entry.rs +++ b/crates/wasi-common/src/entry.rs @@ -5,7 +5,7 @@ use crate::wasi::types::{Filetype, Rights}; use crate::wasi::{Errno, Result}; use std::marker::PhantomData; use std::mem::ManuallyDrop; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; use std::path::PathBuf; use std::{fmt, fs, io}; @@ -63,15 +63,6 @@ impl Descriptor { } } - /// Like `as_file`, but return a mutable reference. - pub(crate) fn as_file_mut<'descriptor>(&'descriptor mut self) -> Result<&'descriptor mut Self> { - match self { - Self::OsHandle(_) => Ok(self), - Self::VirtualFile(_) => Ok(self), - _ => Err(Errno::Badf), - } - } - /// Return an `OsHandle`, which may be a stream or socket file descriptor. pub(crate) fn as_os_handle<'descriptor>(&'descriptor self) -> OsHandleRef<'descriptor> { descriptor_as_oshandle(self) @@ -263,9 +254,3 @@ impl<'descriptor> Deref for OsHandleRef<'descriptor> { &self.handle } } - -impl<'descriptor> DerefMut for OsHandleRef<'descriptor> { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.handle - } -} diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index aafdbe8017..b19638bc36 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -7,8 +7,8 @@ use crate::{clock, fd, path, poll}; use log::{debug, error, trace}; use std::cell::Ref; use std::convert::TryInto; +use std::fs::File; use std::io::{self, Read, Seek, SeekFrom, Write}; -use std::ops::DerefMut; use wiggle_runtime::{GuestBorrows, GuestPtr}; impl<'a> WasiSnapshotPreview1 for WasiCtx { @@ -124,10 +124,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { len, advice ); - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut(types::Rights::FD_ADVISE, types::Rights::empty())? - .as_file_mut()?; + .as_descriptor(types::Rights::FD_ADVISE, types::Rights::empty())? + .as_file()?; match file { Descriptor::OsHandle(fd) => fd::advise(&fd, advice, offset, len), Descriptor::VirtualFile(virt) => virt.advise(advice, offset, len), @@ -361,13 +361,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { buf.push(io::IoSliceMut::new(slice)); } - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut( + .as_descriptor( types::Rights::FD_READ | types::Rights::FD_SEEK, types::Rights::empty(), )? - .as_file_mut()?; + .as_file()?; if offset > i64::max_value() as u64 { return Err(Errno::Io); @@ -375,6 +375,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let host_nread = match file { Descriptor::OsHandle(fd) => { + let mut fd: &File = fd; let cur_pos = fd.seek(SeekFrom::Current(0))?; fd.seek(SeekFrom::Start(offset))?; let nread = fd.read_vectored(&mut buf)?; @@ -474,13 +475,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { buf.push(io::IoSlice::new(slice)); } - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut( + .as_descriptor( types::Rights::FD_WRITE | types::Rights::FD_SEEK, types::Rights::empty(), )? - .as_file_mut()?; + .as_file()?; if offset > i64::max_value() as u64 { return Err(Errno::Io); @@ -488,6 +489,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let host_nwritten = match file { Descriptor::OsHandle(fd) => { + let mut fd: &File = fd; let cur_pos = fd.seek(SeekFrom::Current(0))?; fd.seek(SeekFrom::Start(offset))?; let nwritten = fd.write_vectored(&buf)?; @@ -524,10 +526,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { slices.push(io::IoSliceMut::new(slice)); } - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let host_nread = - match entry.as_descriptor_mut(types::Rights::FD_READ, types::Rights::empty())? { - Descriptor::OsHandle(file) => file.read_vectored(&mut slices)?, + match entry.as_descriptor(types::Rights::FD_READ, types::Rights::empty())? { + Descriptor::OsHandle(file) => (file as &File).read_vectored(&mut slices)?, Descriptor::VirtualFile(virt) => virt.read_vectored(&mut slices)?, Descriptor::Stdin => io::stdin().read_vectored(&mut slices)?, _ => return Err(Errno::Badf), @@ -554,10 +556,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { cookie, ); - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut(types::Rights::FD_READDIR, types::Rights::empty())? - .as_file_mut()?; + .as_descriptor(types::Rights::FD_READDIR, types::Rights::empty())? + .as_file()?; fn copy_entities>>( iter: T, @@ -643,17 +645,17 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { } else { types::Rights::FD_SEEK | types::Rights::FD_TELL }; - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut(rights, types::Rights::empty())? - .as_file_mut()?; + .as_descriptor(rights, types::Rights::empty())? + .as_file()?; let pos = match whence { types::Whence::Cur => SeekFrom::Current(offset), types::Whence::End => SeekFrom::End(offset), types::Whence::Set => SeekFrom::Start(offset as u64), }; let host_newoffset = match file { - Descriptor::OsHandle(fd) => fd.seek(pos)?, + Descriptor::OsHandle(fd) => (fd as &File).seek(pos)?, Descriptor::VirtualFile(virt) => virt.seek(pos)?, _ => { unreachable!( @@ -689,12 +691,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { fn fd_tell(&self, fd: types::Fd) -> Result { trace!("fd_tell(fd={:?})", fd); - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let file = entry - .as_descriptor_mut(types::Rights::FD_TELL, types::Rights::empty())? - .as_file_mut()?; + .as_descriptor(types::Rights::FD_TELL, types::Rights::empty())? + .as_file()?; let host_offset = match file { - Descriptor::OsHandle(fd) => fd.seek(SeekFrom::Current(0))?, + Descriptor::OsHandle(fd) => (fd as &File).seek(SeekFrom::Current(0))?, Descriptor::VirtualFile(virt) => virt.seek(SeekFrom::Current(0))?, _ => { unreachable!( @@ -726,15 +728,15 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { } // perform unbuffered writes - let mut entry = self.get_entry_mut(fd)?; + let entry = self.get_entry(fd)?; let isatty = entry.isatty(); - let desc = entry.as_descriptor_mut(types::Rights::FD_WRITE, types::Rights::empty())?; + let desc = entry.as_descriptor(types::Rights::FD_WRITE, types::Rights::empty())?; let host_nwritten = match desc { Descriptor::OsHandle(file) => { if isatty { - SandboxedTTYWriter::new(file.deref_mut()).write_vectored(&slices)? + SandboxedTTYWriter::new(&mut (file as &File)).write_vectored(&slices)? } else { - file.write_vectored(&slices)? + (file as &File).write_vectored(&slices)? } } Descriptor::VirtualFile(virt) => { diff --git a/crates/wasi-common/src/sys/unix/bsd/fd.rs b/crates/wasi-common/src/sys/unix/bsd/fd.rs index 11dfd274f5..884ccf0c6b 100644 --- a/crates/wasi-common/src/sys/unix/bsd/fd.rs +++ b/crates/wasi-common/src/sys/unix/bsd/fd.rs @@ -1,26 +1,21 @@ use crate::sys::entry::OsHandle; use crate::wasi::Result; -use std::sync::{Mutex, MutexGuard}; +use std::cell::RefMut; use yanix::dir::Dir; -pub(crate) fn get_dir_from_os_handle<'a>( - os_handle: &'a mut OsHandle, -) -> Result> { - let dir = match os_handle.dir { - Some(ref mut dir) => dir, - None => { - // We need to duplicate the fd, because `opendir(3)`: - // Upon successful return from fdopendir(), the file descriptor is under - // control of the system, and if any attempt is made to close the file - // descriptor, or to modify the state of the associated description other - // than by means of closedir(), readdir(), readdir_r(), or rewinddir(), - // the behaviour is undefined. - let fd = (*os_handle).try_clone()?; - let dir = Dir::from(fd)?; - os_handle.dir.get_or_insert(Mutex::new(dir)) - } - }; - // Note that from this point on, until the end of the parent scope (i.e., enclosing this - // function), we're locking the `Dir` member of this `OsHandle`. - Ok(dir.lock().unwrap()) +pub(crate) fn get_dir_from_os_handle(os_handle: &OsHandle) -> Result> { + if os_handle.dir.borrow().is_none() { + // We need to duplicate the fd, because `opendir(3)`: + // Upon successful return from fdopendir(), the file descriptor is under + // control of the system, and if any attempt is made to close the file + // descriptor, or to modify the state of the associated description other + // than by means of closedir(), readdir(), readdir_r(), or rewinddir(), + // the behaviour is undefined. + let fd = (*os_handle).try_clone()?; + let d = Dir::from(fd)?; + *os_handle.dir.borrow_mut() = Some(d); + } + Ok(RefMut::map(os_handle.dir.borrow_mut(), |dir| { + dir.as_mut().unwrap() + })) } diff --git a/crates/wasi-common/src/sys/unix/bsd/oshandle.rs b/crates/wasi-common/src/sys/unix/bsd/oshandle.rs index 70d1a8c4e8..1918a2af34 100644 --- a/crates/wasi-common/src/sys/unix/bsd/oshandle.rs +++ b/crates/wasi-common/src/sys/unix/bsd/oshandle.rs @@ -1,7 +1,7 @@ +use std::cell::RefCell; use std::fs; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; use std::os::unix::prelude::{AsRawFd, RawFd}; -use std::sync::Mutex; use yanix::dir::Dir; #[derive(Debug)] @@ -18,12 +18,15 @@ pub(crate) struct OsHandle { // > of the DIR pointer, dirp, from which they are derived. // > If the directory is closed and then reopened, prior values // > returned by telldir() will no longer be valid. - pub(crate) dir: Option>, + pub(crate) dir: RefCell>, } impl From for OsHandle { fn from(file: fs::File) -> Self { - Self { file, dir: None } + Self { + file, + dir: RefCell::new(None), + } } } @@ -40,9 +43,3 @@ impl Deref for OsHandle { &self.file } } - -impl DerefMut for OsHandle { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.file - } -} diff --git a/crates/wasi-common/src/sys/unix/fd.rs b/crates/wasi-common/src/sys/unix/fd.rs index dd7eec5f07..a2732f831f 100644 --- a/crates/wasi-common/src/sys/unix/fd.rs +++ b/crates/wasi-common/src/sys/unix/fd.rs @@ -44,7 +44,7 @@ pub(crate) fn filestat_get(file: &std::fs::File) -> Result { } pub(crate) fn readdir<'a>( - os_handle: &'a mut OsHandle, + os_handle: &'a OsHandle, cookie: types::Dircookie, ) -> Result> + 'a> { use yanix::dir::{DirIter, Entry, EntryExt, SeekLoc}; diff --git a/crates/wasi-common/src/sys/unix/linux/fd.rs b/crates/wasi-common/src/sys/unix/linux/fd.rs index 6017edaef2..f480294a4e 100644 --- a/crates/wasi-common/src/sys/unix/linux/fd.rs +++ b/crates/wasi-common/src/sys/unix/linux/fd.rs @@ -2,7 +2,7 @@ use crate::sys::entry::OsHandle; use crate::wasi::Result; use yanix::dir::Dir; -pub(crate) fn get_dir_from_os_handle(os_handle: &mut OsHandle) -> Result> { +pub(crate) fn get_dir_from_os_handle(os_handle: &OsHandle) -> Result> { // We need to duplicate the fd, because `opendir(3)`: // After a successful call to fdopendir(), fd is used internally by the implementation, // and should not otherwise be used by the application. diff --git a/crates/wasi-common/src/sys/unix/linux/oshandle.rs b/crates/wasi-common/src/sys/unix/linux/oshandle.rs index 8104d9d5e3..bdf75e7f68 100644 --- a/crates/wasi-common/src/sys/unix/linux/oshandle.rs +++ b/crates/wasi-common/src/sys/unix/linux/oshandle.rs @@ -1,5 +1,5 @@ use std::fs; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; use std::os::unix::prelude::{AsRawFd, RawFd}; #[derive(Debug)] @@ -24,9 +24,3 @@ impl Deref for OsHandle { &self.0 } } - -impl DerefMut for OsHandle { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} diff --git a/crates/wasi-common/src/sys/windows/entry.rs b/crates/wasi-common/src/sys/windows/entry.rs index d2d0c04497..e4d386188f 100644 --- a/crates/wasi-common/src/sys/windows/entry.rs +++ b/crates/wasi-common/src/sys/windows/entry.rs @@ -3,7 +3,7 @@ use crate::wasi::{types, RightsExt}; use std::fs::File; use std::io; use std::mem::ManuallyDrop; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; use std::os::windows::prelude::{AsRawHandle, FromRawHandle, RawHandle}; #[derive(Debug)] @@ -29,12 +29,6 @@ impl Deref for OsHandle { } } -impl DerefMut for OsHandle { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - impl AsRawHandle for Descriptor { fn as_raw_handle(&self) -> RawHandle { match self { diff --git a/crates/wasi-common/src/virtfs.rs b/crates/wasi-common/src/virtfs.rs index f6247aa805..7a2068ab6b 100644 --- a/crates/wasi-common/src/virtfs.rs +++ b/crates/wasi-common/src/virtfs.rs @@ -86,7 +86,7 @@ pub(crate) trait VirtualFile: MovableFile { Err(Errno::Badf) } - fn write_vectored(&mut self, _iovs: &[io::IoSlice]) -> Result { + fn write_vectored(&self, _iovs: &[io::IoSlice]) -> Result { Err(Errno::Badf) } @@ -98,7 +98,7 @@ pub(crate) trait VirtualFile: MovableFile { Err(Errno::Badf) } - fn seek(&mut self, _offset: SeekFrom) -> Result { + fn seek(&self, _offset: SeekFrom) -> Result { Err(Errno::Badf) } @@ -131,7 +131,7 @@ pub(crate) trait VirtualFile: MovableFile { Err(Errno::Badf) } - fn read_vectored(&mut self, _iovs: &mut [io::IoSliceMut]) -> Result { + fn read_vectored(&self, _iovs: &mut [io::IoSliceMut]) -> Result { Err(Errno::Badf) } @@ -254,7 +254,7 @@ impl VecFileContents { /// a filesystem wherein a file descriptor is one view into a possibly-shared underlying collection /// of data and permissions on a filesystem. pub struct InMemoryFile { - cursor: types::Filesize, + cursor: RefCell, parent: Rc>>>, fd_flags: types::Fdflags, data: Rc>>, @@ -263,7 +263,7 @@ pub struct InMemoryFile { impl InMemoryFile { pub fn memory_backed() -> Self { Self { - cursor: 0, + cursor: RefCell::new(0), parent: Rc::new(RefCell::new(None)), fd_flags: types::Fdflags::empty(), data: Rc::new(RefCell::new(Box::new(VecFileContents::new()))), @@ -272,7 +272,7 @@ impl InMemoryFile { pub fn new(contents: Box) -> Self { Self { - cursor: 0, + cursor: RefCell::new(0), fd_flags: types::Fdflags::empty(), parent: Rc::new(RefCell::new(None)), data: Rc::new(RefCell::new(contents)), @@ -293,7 +293,7 @@ impl VirtualFile for InMemoryFile { fn try_clone(&self) -> io::Result> { Ok(Box::new(Self { - cursor: 0, + cursor: RefCell::new(0), fd_flags: self.fd_flags, parent: Rc::clone(&self.parent), data: Rc::clone(&self.data), @@ -356,7 +356,7 @@ impl VirtualFile for InMemoryFile { Ok(()) } - fn write_vectored(&mut self, iovs: &[io::IoSlice]) -> Result { + fn write_vectored(&self, iovs: &[io::IoSlice]) -> Result { trace!("write_vectored(iovs={:?})", iovs); let mut data = self.data.borrow_mut(); @@ -367,7 +367,7 @@ impl VirtualFile for InMemoryFile { let write_start = if append_mode { data.size() } else { - self.cursor + *self.cursor.borrow() }; let max_size = iovs @@ -396,16 +396,16 @@ impl VirtualFile for InMemoryFile { // If we are not appending, adjust the cursor appropriately for the write, too. This can't // overflow, as we checked against that before writing any data. if !append_mode { - self.cursor += written as u64; + *self.cursor.borrow_mut() += written as u64; } Ok(written) } - fn read_vectored(&mut self, iovs: &mut [io::IoSliceMut]) -> Result { + fn read_vectored(&self, iovs: &mut [io::IoSliceMut]) -> Result { trace!("read_vectored(iovs={:?})", iovs); trace!(" | *read_start={:?}", self.cursor); - self.data.borrow_mut().preadv(iovs, self.cursor) + self.data.borrow_mut().preadv(iovs, *self.cursor.borrow()) } fn preadv(&self, buf: &mut [io::IoSliceMut], offset: types::Filesize) -> Result { @@ -416,32 +416,36 @@ impl VirtualFile for InMemoryFile { self.data.borrow_mut().pwritev(buf, offset) } - fn seek(&mut self, offset: SeekFrom) -> Result { + fn seek(&self, offset: SeekFrom) -> Result { let content_len = self.data.borrow().size(); match offset { SeekFrom::Current(offset) => { let new_cursor = if offset < 0 { self.cursor + .borrow() .checked_sub(offset.wrapping_neg() as u64) .ok_or(Errno::Inval)? } else { - self.cursor.checked_add(offset as u64).ok_or(Errno::Inval)? + self.cursor + .borrow() + .checked_add(offset as u64) + .ok_or(Errno::Inval)? }; - self.cursor = std::cmp::min(content_len, new_cursor); + *self.cursor.borrow_mut() = std::cmp::min(content_len, new_cursor); } SeekFrom::End(offset) => { // A negative offset from the end would be past the end of the file, let offset: u64 = offset.try_into().map_err(|_| Errno::Inval)?; - self.cursor = content_len.saturating_sub(offset); + *self.cursor.borrow_mut() = content_len.saturating_sub(offset); } SeekFrom::Start(offset) => { // A negative offset from the end would be before the start of the file. let offset: u64 = offset.try_into().map_err(|_| Errno::Inval)?; - self.cursor = std::cmp::min(content_len, offset); + *self.cursor.borrow_mut() = std::cmp::min(content_len, offset); } } - Ok(self.cursor) + Ok(*self.cursor.borrow()) } fn advise( @@ -749,7 +753,7 @@ impl VirtualFile for VirtualDir { } } - fn write_vectored(&mut self, _iovs: &[io::IoSlice]) -> Result { + fn write_vectored(&self, _iovs: &[io::IoSlice]) -> Result { Err(Errno::Badf) }