Fix soundness issue in vm::Global #1590
Conversation
The `unsafe impl` for `Send` and `Sync` relies on the global being locked when it's being accessed; the get and get_mut functions did not (and could not) do this, so we replace them with `get` and `set` methods which operate on `Value`s directly. Additionally the `get_mut` function took a `&self` and returned a `&mut` to the underlying data which allows for aliased mutable references to the same data which is another soundness issue.
CHANGELOG.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| # Changelog | |||
|
|
|||
| ## **[Unreleased]** | |||
| - [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API on vm::Global | |||
There was a problem hiding this comment.
| - [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API on vm::Global | |
| - [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API of vm::Global |
lib/vm/src/global.rs
Outdated
| /// The caller should also ensure that this global is synchronized. Otherwise, use | ||
| /// `set` instead. | ||
| pub unsafe fn set_unchecked<T>(&self, val: Value<T>) -> Result<(), GlobalError> { | ||
| // ideally we'd use atomics here |
There was a problem hiding this comment.
Is this a TODO? Do you mean that you wish the assignment into *definition was atomic? The comment mentions that this should be synchronized, do we still need atomics here even if it's synchronized, or are those separate?
There was a problem hiding this comment.
Yeah, the idea is that we'd use atomics instead of a mutex on platforms with atomics that go up to 16 bytes... if there are any platforms that have that
There was a problem hiding this comment.
I don't think it's particularly actionable, but I also don't think leaving that comment there is hurting anything, it might inspire a future person to try something!
There was a problem hiding this comment.
I just think the comment is terse, it doesn't describe why an atomic is better than not atomic. Like, generally we don't use atomics and using atomics is slower on CPU, and is less clean code. So why the suggestion to use one here? What would it help? I'm trying to improve the comment "// TODO: if we used atomics here, we could ..." but I don't know enough to write that comment myself.
|
bors r+ |
1590: Fix soundness issue in vm::Global r=MarkMcCaskey a=MarkMcCaskey The `unsafe impl` for `Send` and `Sync` relies on the global being locked when it's being accessed; the get and get_mut functions did not (and could not) do this, so we replace them with `get` and `set` methods which operate on `Value`s directly. Additionally the `get_mut` function took a `&self` and returned a `&mut` to the underlying data which allows for aliased mutable references to the same data which is another soundness issue. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <mark@wasmer.io>
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
|
Note that might be useful for the future: Prior to this PR, the logic that was handling how to get/set values in Globals was done in the end layer (the wasmer API). We did that because once we add support for reference types, the final implementation of it will also be living on the wasmer API (not on the runtime). Moving it to the runtime might cause a leak of logic down the stream. Moving the logic to the runtime (as this PR is doing) might cause difficulties on the long term when approaching reference types. But I guess we can solve for that when the time comes. |
The
unsafe implforSendandSyncrelies on the global beinglocked when it's being accessed; the get and get_mut functions did
not (and could not) do this, so we replace them with
getandsetmethods which operate on
Values directly.Additionally the
get_mutfunction took a&selfand returned a&mutto the underlying data which allows for aliased mutablereferences to the same data which is another soundness issue.
Review