Skip to content

Commit

Permalink
adapter: eliminate State::with_mut (bytecodealliance#6973)
Browse files Browse the repository at this point in the history
There is no need to get exclusive access to State:
* All mutation of State and Descriptors is handled internally with cells.
`descriptors_mut` does not need exclusive access to State, and eliminating
that constraint means we can trivially shorten all uses of `with_mut` to `with`.
* recursive access to State is required for implementing the cabi_realloc
functions. These functions do not need exclusive access to State, but if
another call has already taken an exclusive borrow, realloc will trap.

I recently found a bug that would occur if `path_open` is the first call into
the adapter, the State will be borrowed exclusively and `descriptors_mut` will
initialize the descriptors by calling out to get the preopens. Because preopens
are a list, the runtime needs to call cabi_realloc to return them, and that
call will fail where cabi_import_realloc tries to borrow the exclusively-
borrowed State.
  • Loading branch information
Pat Hickey authored Sep 6, 2023
1 parent a10802e commit 4274a3d
Showing 1 changed file with 6 additions and 18 deletions.
24 changes: 6 additions & 18 deletions crates/wasi-preview1-component-adapter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ pub unsafe extern "C" fn cabi_export_realloc(
unreachable!();
}
let mut ret = null_mut::<u8>();
State::with_mut(|state| {
State::with(|state| {
ret = state.long_lived_arena.alloc(align, new_size);
Ok(())
});
Expand Down Expand Up @@ -444,7 +444,7 @@ pub unsafe extern "C" fn fd_allocate(fd: Fd, offset: Filesize, len: Filesize) ->
/// Note: This is similar to `close` in POSIX.
#[no_mangle]
pub unsafe extern "C" fn fd_close(fd: Fd) -> Errno {
State::with_mut(|state| {
State::with(|state| {
// If there's a dirent cache entry for this file descriptor then drop
// it since the descriptor is being closed and future calls to
// `fd_readdir` should return an error.
Expand Down Expand Up @@ -601,7 +601,7 @@ pub unsafe extern "C" fn fd_fdstat_set_flags(fd: Fd, flags: Fdflags) -> Errno {
return wasi::ERRNO_INVAL;
}

State::with_mut(|state| {
State::with(|state| {
let mut ds = state.descriptors_mut();
let file = match ds.get_mut(fd)? {
Descriptor::Streams(Streams {
Expand Down Expand Up @@ -1165,7 +1165,7 @@ pub unsafe extern "C" fn fd_readdir(
/// would disappear if `dup2()` were to be removed entirely.
#[no_mangle]
pub unsafe extern "C" fn fd_renumber(fd: Fd, to: Fd) -> Errno {
State::with_mut(|state| state.descriptors_mut().renumber(fd, to))
State::with(|state| state.descriptors_mut().renumber(fd, to))
}

/// Move the offset of a file descriptor.
Expand Down Expand Up @@ -1441,7 +1441,7 @@ pub unsafe extern "C" fn path_open(
let mode = filesystem::Modes::READABLE | filesystem::Modes::WRITABLE;
let append = fdflags & wasi::FDFLAGS_APPEND == wasi::FDFLAGS_APPEND;

State::with_mut(|state| {
State::with(|state| {
let mut ds = state.descriptors_mut();
let file = ds.get_dir(fd)?;
let result = filesystem::open_at(file.fd, at_flags, path, o_flags, flags, mode)?;
Expand Down Expand Up @@ -2322,18 +2322,6 @@ impl State {
}
}

fn with_mut(f: impl FnOnce(&mut State) -> Result<(), Errno>) -> Errno {
let ptr = State::ptr();
let mut ptr = ptr.try_borrow_mut().unwrap_or_else(|_| unreachable!());
assert_eq!(ptr.magic1, MAGIC);
assert_eq!(ptr.magic2, MAGIC);
let ret = f(&mut *ptr);
match ret {
Ok(()) => ERRNO_SUCCESS,
Err(err) => err,
}
}

fn ptr() -> &'static RefCell<State> {
unsafe {
let mut ptr = get_state_ptr();
Expand Down Expand Up @@ -2416,7 +2404,7 @@ impl State {
}

/// Mut accessor for the descriptors member that ensures it is properly initialized
fn descriptors_mut<'a>(&'a mut self) -> impl DerefMut + Deref<Target = Descriptors> + 'a {
fn descriptors_mut<'a>(&'a self) -> impl DerefMut + Deref<Target = Descriptors> + 'a {
let mut d = self
.descriptors
.try_borrow_mut()
Expand Down

0 comments on commit 4274a3d

Please sign in to comment.