wiggle: adapt Wiggle guest slices for unsafe shared use

[abrown]

When multiple threads can concurrently modify a WebAssembly shared memory, the underlying data for a Wiggle GuestSlice and GuestSliceMut could change due to access from other threads. This breaks Rust guarantees when &[T] and &mut [T] slices are handed out. This change modifies GuestPtr to make as_slice and as_slice_mut return an Option which is None when the underlying WebAssembly memory is shared.

But WASI implementations still need access to the underlying WebAssembly memory, both to read to it and write from it. This change adds new APIs:

• GuestPtr::to_vec copies the bytes from WebAssembly memory (from which we can safely take a &[T])
• GuestPtr::as_unsafe_slice_mut returns a wrapper struct from which we can unsafe-ly return a mutable slice (users must accept the unsafety of concurrently modifying a &mut [T])

This approach allows us to maintain Wiggle's borrow-checking infrastructure, which enforces the guarantee that Wiggle will not modify overlapping regions, e.g. This is important because the underlying system calls may expect this. Other threads may modify the same underlying region and this is impossible to prevent; at least Wiggle will not be able to do so.

Finally, the changes to Wiggle's API are propagated to all WASI implementations in Wasmtime. For now, code locations that attempt to get a guest slice will panic if the underlying memory is shared. Note that Wiggle is not enabled for shared memory (that will come later in something like #5054), but when it is, these panics will be clear indicators of locations that must be re-implemented in a thread-safe way.

[bjorn3]

Would it make sense to have a method that returns &UnsafeCell<[T]> or &Cell<[T]>?

[github-actions]

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

• kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[abrown]

Would it make sense to have a method that returns &UnsafeCell<[T]> or &Cell<[T]>?

I'm open to it; not sure if all of the functions exposed are necessary, though. Let's see what @alexcrichton and @pchickey think?

[alexcrichton]

I like the idea of the Unsafe* wrapper you added here and I left a comment about how ideally I think we could use that to implement the safe bits and bobs.

Otherwise I think it should be able to implement:

impl<T> Deref for UnsafeGuestSlice<'_, T> {
    type Target = [UnsafeCell<T>];
    // ...
}

which could actually be relatively convenient to work with.

[abrown]

Otherwise I think it should be able to implement:

impl<T> Deref for UnsafeGuestSlice<'_, T> {
    type Target = [UnsafeCell<T>];
    // ...
}

which could actually be relatively convenient to work with.

Thinking about this more, I don't think accessing T through an UnsafeCell is right: UnsafeCell::get_mut would give us a &mut T and the documentation of that method claims that "this call borrows the UnsafeCell mutably (at compile-time) which guarantees that we possess the only reference," which is incorrect. It's incorrect because UnsafeGuestSlice is pointing to shared memory which could be modified at any time by any WASI thread, outside of the Rust borrowing world. @alexcrichton, what do you think?

[alexcrichton]

The get_mut method requires &mut self so we won't have to worry about that here since we'll never hand out &mut UnsafeCell<T>. By more-or-less handing out &[UnsafeCell<T>] we're basically saying "here's a 100% valid region of memory in terms of in-bounds, you have access to it, and it's all aligned correctly and whatnot -- but the contents of memory are ruled by other mechanisms"

The only way to interact with the contents of &UnsafeCell<T> is to have unsafe, which is the goal here, so I think it'll work out well. For unshared cases the wrapper methods to go to safe slices will work, and for shared cases it's clear that atomics/raw intrinsics/etc need to get used (and is something we'll document).

[abrown]

The get_mut method requires &mut self

Ah, yeah, I see what you mean; this makes more sense. I'm not sure how/when this would be used, though; I guess this would be yet another way to modify a slice besides the unsafe UnsafeGuestSlice::get_mut? (I guess for completeness we could also expose an unsafe UnsafeGuestSlice::get which would return a &[T]). If this isn't going to be used just yet, should we implement this in this PR or wait until it is needed?

[alexcrichton]

Oh I don't think the Deref impl is really all that important to have, it's just a "might as well" sort of thing. I think though having an interim UnsafeGuestSlice get constructed on the way to a GuestSlice{,Mut} will exercise it and make it useful. Only for niche cases will UnsafeGuestSlice be useful. Note though we could have UnsafeGuestSlice::copy_from_slice as a safe method which only fails due to active borrows.