fix(vm) Fix memory leak of InstanceHandle

[Hywan]

Description

In wasmer_engine::Artifact::instantiate, the engine artifact
allocates a new instance by calling
wasmer_vm::InstanceHandle::allocate_instance. To make it short, this
function calculates the Layout of an wasmer_vm::Instance, and
allocates space for it by calling alloc. This last part is
important because it means we are responsible to deallocate it with
dealloc.

The pointer to this wasmer_vm::Instance is stored in the
wasmer_vm::InstanceHandle structure. That's the handle that is
returned by the engine artifact Artifact::instantiate method.

This instance handle is then stored in wasmer::Instance (through the
intervention of wasmer::Module).

How is it freed? It wasn't. That's the leak. dealloc was never
called.

How to free it? We must call dealloc. There is even a
wasmer_vm::InstanceHandle::deallocate helper to do that
properly. Neat!

When to free it? That's the tricky part. A wasmer::Instance can be
clonable. To do so, wasmer_vm::InstanceHandle must be clonable
too. There was a Clone implementation, that was constructing a new
wasmer_vm:InstanceHandle by using the same pointer to
wasmer_vm::Instance. That's annoying because it's the best way to
get many pointers that point to the same wasmer_vm::Instance in the
nature, and it's difficult to track them.

This patch changes the paradigm. There is only one and unique
wasmer_vm::InstanceHandle per wasmer::Instance, including its
clones. The handle is now stored inside a
Arc<Mutex<wasmer_vm::InstanceHandle>>. Consequently, when a
wasmer::Instance is cloned, it uses the same
wasmer_vm::InstanceHandle, not a clone of it.

Bonus: wasmer::Instance continues to be Send + Sync.

So. Let's back to our question. When to free
wasmer_vm::InstanceHandle? Response: When wasmer::Instance is
dropped. Right? There is a unique path from wasmer::Instance, to
wasmer_vm::InstanceHandle, to wasmer_vm::Instance now. So we just
need to call wasmer_vm::InstanceHandle::dealloc in a specific Drop
implementation for wasmer_vm::InstanceHandle, and the Rust borrow
checker does the rest.

Yes. … No. There is another use case: It is possible to create a
wasmer_vm::InstanceHandle with InstanceHandle::from_vmctx. Indeed,
a wasmer_vm::VMContext also stores a pointer to
wasmer_vm::Instance. In this, we consider wasmer_vm::VMContext
owns the instance pointer, somehow, and is responsible to free it
properly.

Consequently, we need another flag inside wasmer_vm::InstanceHandle
to know whether this structure owns the pointer to
wasmer_vm::Instance or not.

So. Let's back to our question. When to free
wasmer_vm::InstanceHandle? Response: Inside the Drop
implementation of wasmer_vm::InstanceHandle with its Self::dealloc
method if and only if the handle owns the pointer to
wasmer_vm::Instance.

Testing with Valgrind shows that the leak has been removed.

Review

• Add a short description of the the change to the CHANGELOG.md file

[Hywan]

Note: The first 2 commits were about explorations of this leak. They are part of the PR but they may disturb the reviewer. Feel free to review 8016a34 alone.

[MarkMcCaskey]

Looks generally good to me, I have some concerns about use-after-free with this change: my understanding is that this shouldn't be that hard. I'll try to trigger it and post the code here.

[MarkMcCaskey]

error[E0609]: no field `module_translation` on type `wasmer_compiler::ModuleInfoTranslation<'_>`
   --> lib/engine-object-file/src/artifact.rs:131:25
    |
131 |             translation.module_translation,
    |                         ^^^^^^^^^^^^^^^^^^ help: a field with a similar name exists: `module_translation_state`

error: aborting due to previous error

Heads up I got an error while building locally, it's easy to fix though

[MarkMcCaskey]

By the way here's some code that compiles and runs and I believe it's a memory leak:

use wasmer::{imports, wat2wasm, Instance, Module, Store, Value};
use wasmer_compiler_cranelift::Cranelift;
use wasmer_engine_jit::JIT;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let wasm_bytes = wat2wasm(
        r#"
(module
  (type $sum_t (func (param i32 i32) (result i32)))
  (func $sum_f (type $sum_t) (param $x i32) (param $y i32) (result i32)
    local.get $x
    local.get $y
    i32.add)
  (export "sum" (func $sum_f)))
"#
        .as_bytes(),
    )?;

    let sum = {
        let compiler = Cranelift::default();
        let store = Store::new(&JIT::new(&compiler).engine());
        let module = Module::new(&store, wasm_bytes)?;
        let import_object = imports! {};
        let instance = Instance::new(&module, &import_object)?;
        let sum = instance.exports.get_function("sum")?;
        sum.clone()
    };

    println!("Calling `sum` function...");
    let results = sum.call(&[Value::I32(1), Value::I32(2)])?;

    println!("Results: {:?}", results);
    assert_eq!(results.to_vec(), vec![Value::I32(3)]);

    Ok(())
}

The idea here is to just call a local function after the Instance has been freed. I've verified that the instance is actually being freed by adding a print statement to InstanceHandle::dealloc giving us:

[lib/vm/src/instance.rs:1149] "FREEING INSTANCE DATA" = "FREEING INSTANCE DATA"
Calling `sum` function...
Results: [I32(3)]

[Hywan]

The behavior you're describing is also a “bug”: it was working of this bug.

Let's fix that. As discussed internally, we will keep this behavior.

[Hywan]

error[E0609]: no field `module_translation` on type `wasmer_compiler::ModuleInfoTranslation<'_>`
   --> lib/engine-object-file/src/artifact.rs:131:25
    |
131 |             translation.module_translation,
    |                         ^^^^^^^^^^^^^^^^^^ help: a field with a similar name exists: `module_translation_state`

error: aborting due to previous error

Heads up I got an error while building locally, it's easy to fix though

Fixed. Thanks.

[syrusakbary]

bors r+

[bors]

Build succeeded:

• Test on linux-aarch64
• Test on macos
• Test cross-compile on macos
• Test cross-compile on linux
• Audit
• Code lint
• Test on linux
• Test on windows

[Hywan]

Thanks for merging that. I've been working on another PR to fix the “use exports after freed instance” feature.