Skip to content

Technically &mut Instance is not safe #10933

@alexcrichton

Description

@alexcrichton

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    wasm-proposal:component-modelIssues related to the WebAssembly Component Model proposal

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions