wiggle: copy guest slices back to shared memory#5471
Merged
abrown merged 4 commits intobytecodealliance:mainfrom Jan 3, 2023
Merged
wiggle: copy guest slices back to shared memory#5471abrown merged 4 commits intobytecodealliance:mainfrom
abrown merged 4 commits intobytecodealliance:mainfrom
Conversation
Subscribe to Label Actioncc @kubkon DetailsThis issue or pull request has been labeled: "wasi"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Dec 19, 2022
Along the same lines as bytecodealliance#5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Dec 19, 2022
Along the same lines as bytecodealliance#5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared.
This change upgrades `UnsafeGuestSlice` in Wiggle to expose more functionality to be able to use `std::ptr::copy` for writing bytes into Wasm shared memory. Additionally, it adds a new `GuestCow` type for delineating between Wasm memory regions that can be borrowed (non-shared memory) or must be copied (shared memory) in order to maintain Rust guarantees. With these in place, it is now possible to implement the `preview1` "read" functions for shared memory. Previously, these would panic if attempting to copy to a shared memory. This change removes the panic and introduces some (rather complex) logic for handling both the shared and non-shared cases: - if reading into a Wasm non-shared memory, Wiggle guarantees that no other guest pointers will touch the memory region and, in the absence of concurrency, a WASI function can write directly to this memory - if reading into a Wasm shared memory, the memory region can be concurrently modified. At @alexcrichton's request re: Rust safety, this change copies all of the bytes into an intermediate buffer before using `std::ptr::copy` to move them into Wasm memory. This change only applies to the `preview0` and `preview1` implementations of `wasi-common`. Fixing up other WASI implementations (esp. wasi-crypto) is left for later.
abrown
force-pushed
the
wiggle-copied-reads
branch
from
bca68da to
30cc65d
Compare
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Dec 20, 2022
Along the same lines as bytecodealliance#5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared. This change updates the uses of Wiggle strings in both wasi-common and wasi-crypto.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Dec 20, 2022
Along the same lines as bytecodealliance#5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared. This change updates the uses of Wiggle strings in both wasi-common and wasi-crypto.
alexcrichton
approved these changes
Jan 3, 2023
Comment on lines
+1116
to
+1122
| // If the Wasm memory is shared, copy to an intermediate buffer to | ||
| // avoid Rust unsafety (i.e., the called function could rely on | ||
| // `&mut [u8]`'s exclusive ownership which is not guaranteed due to | ||
| // potential access from other threads). | ||
| let mut tmp = vec![0; buf.len().min(MAX_SHARED_BUFFER_SIZE)]; | ||
| self.random.try_fill_bytes(&mut tmp)?; | ||
| buf.copy_from_slice(&tmp)?; |
Member
There was a problem hiding this comment.
Since random_get fills the entire buffer I think this will need to loop or otherwise this will return an error since tmp may be shorter than buf
Member
Author
There was a problem hiding this comment.
You might want to take a look at 652988f since I changed the GuestPtr API slightly.
alexcrichton
approved these changes
Jan 3, 2023
Member
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Looks good!
If you're up for it I also just notice that the read_vectored pattern is here 4 times, so possibly refactoring to just one definition might be a good idea but not a blocker.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Jan 3, 2023
Along the same lines as bytecodealliance#5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared. This change updates the uses of Wiggle strings in both wasi-common and wasi-crypto.
alexcrichton
pushed a commit
that referenced
this pull request
Jan 4, 2023
* wiggle: copy guest strings from shared memory Along the same lines as #5471, this change adds a new smart pointer, `GuestStrCow`, to copy the string bytes over from Wasm memory to the host when the string is found in shared memory. This is necessary to maintain Rust guarantees: with shared memory, the bytes backing a `GuestStr` could be altered by another thread and this would invalidate the assumption that we can dereference at any point to `&str`. `GuestStrCow` is essentially a wrapper around `GuestStr` when the memory is not shared but copies the memory region into a `String` when the memory is shared. This change updates the uses of Wiggle strings in both wasi-common and wasi-crypto. * review: perform UTF-8 check on `GuestStr` construction
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change upgrades
UnsafeGuestSlicein Wiggle to expose morefunctionality to be able to use
std::ptr::copyfor writing bytes intoWasm shared memory. Additionally, it adds a new
GuestCowtype fordelineating between Wasm memory regions that can be borrowed (non-shared
memory) or must be copied (shared memory) in order to maintain Rust
guarantees.
With these in place, it is now possible to implement the
preview1"read" functions for shared memory. Previously, these would panic if
attempting to copy to a shared memory. This change removes the panic and
introduces some (rather complex) logic for handling both the shared and
non-shared cases:
other guest pointers will touch the memory region and, in the absence
of concurrency, a WASI function can write directly to this memory
concurrently modified. At @alexcrichton's request re: Rust safety,
this change copies all of the bytes into an intermediate buffer before
using
std::ptr::copyto move them into Wasm memory.This change only applies to the
preview0andpreview1implementations of
wasi-common. Fixing up other WASI implementations(esp. wasi-crypto) is left for later.