-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Description
Currently in Wasmtime it's safe to acquire &mut wasmtime::runtime::vm::Instance
in various locations and this is used pervasively. Technically though this alone is not sound as it allows mem::swap
-ing two instances. This would mean that the offsets, used to calculate the size of the allocation used for deallocation, could get mixed up (among other issues). Ideally we would have an abstraction which allows mutable access but disallows usage of mem::swap
. In a sense this is similar to StoreContextMut
which basically entirely exists to prevent mem::swap
-ing two stores. This approach has a lot of ergonomic downsides though and it's not fun to write against.
Another alternative is to assume that this basically doesn't happen (it's an internal type in Wasmtime, and it's pretty unlikely we reach for doing this naturally). That would mean we'd document on some unsafe
block somewhere "this requires auditing the entire codebase" which practically wouldn't happen but would in-practice work alright.
Another possible alternative would be something like:
struct InstanceFields { /* ... */ } // what `struct Instance` is today
pub struct Instance {
fields: InstanceFields,
_something_forcing_mut_ref_to_be_dst: [u8],
}
We'd keep methods and such on impl Instance { ... }
and Instance
would itself be a dynamically sized type which would disallow overwriting the entire structure via the type system. Runtime-wise this would have a slight cost as passing it by reference passes two values and one value is always zero (the _something_forcing_mut_ref_to_be_dst
would always be zero-length, we wouldn't want it to encompass the VMContext
as that would inaccurately model how &Instance
actually has mutable provenance to VMContext
, but if [u8]
covered the whole field then it would also seemingly be only shared provenance). LLVM might be able to figure that out though with enough inlining?
Perosnally I'm tempted to pursue this last solution of a dynamically-sized type. It would still require auditing the entire vm/instance.rs
module (as you could still overwrite InstanceFields
in there) but that's a much less daunting task than the entire codebase.