Merge pull request #3493 from wasmerio/fix_instancehandle_memoryleak

Fixed some memory leak issue with InstanceHandle

lib/api/src/sys/instance.rs

@@ -3,7 +3,7 @@ use crate::sys::module::Module;
 use crate::sys::{LinkError, RuntimeError};
 use std::fmt;
 use thiserror::Error;
-use wasmer_vm::{InstanceHandle, StoreHandle};
+use wasmer_vm::{StoreHandle, VMInstance};
 
 #[cfg(feature = "compiler")]
 use super::store::AsStoreMut;
@@ -20,7 +20,7 @@ use crate::sys::{externals::Extern, imports::Imports};
 /// Spec: <https://webassembly.github.io/spec/core/exec/runtime.html#module-instances>
 #[derive(Clone)]
 pub struct Instance {
-    _handle: StoreHandle<InstanceHandle>,
+    _handle: StoreHandle<VMInstance>,
     module: Module,
     /// The exports for an instance.
     pub exports: Exports,

lib/api/src/sys/module.rs

@@ -17,7 +17,7 @@ use wasmer_types::{ExportType, ImportType};
 #[cfg(feature = "compiler")]
 use crate::{sys::InstantiationError, AsStoreMut, AsStoreRef, IntoBytes};
 #[cfg(feature = "compiler")]
-use wasmer_vm::InstanceHandle;
+use wasmer_vm::VMInstance;
 
 /// IO Error on a Module Compilation
 #[derive(Error, Debug)]
@@ -316,7 +316,7 @@ impl Module {
         &self,
         store: &mut impl AsStoreMut,
         imports: &[crate::Extern],
-    ) -> Result<InstanceHandle, InstantiationError> {
+    ) -> Result<VMInstance, InstantiationError> {
         // Ensure all imports come from the same context.
         for import in imports {
             if !import.is_from_store(store) {

lib/compiler/src/engine/artifact.rs

@@ -34,7 +34,7 @@ use wasmer_types::{
 use wasmer_types::{CompileModuleInfo, Target};
 use wasmer_types::{SerializableModule, SerializeError};
 use wasmer_vm::{FunctionBodyPtr, MemoryStyle, TableStyle, VMSharedSignatureIndex, VMTrampoline};
-use wasmer_vm::{InstanceAllocator, InstanceHandle, StoreObjects, TrapHandlerFn, VMExtern};
+use wasmer_vm::{InstanceAllocator, StoreObjects, TrapHandlerFn, VMExtern, VMInstance};
 
 /// A compiled wasm module, ready to be instantiated.
 pub struct Artifact {
@@ -307,13 +307,13 @@ impl Artifact {
     ///
     /// # Safety
     ///
-    /// See [`InstanceHandle::new`].
+    /// See [`VMInstance::new`].
     pub unsafe fn instantiate(
         &self,
         tunables: &dyn Tunables,
         imports: &[VMExtern],
         context: &mut StoreObjects,
-    ) -> Result<InstanceHandle, InstantiationError> {
+    ) -> Result<VMInstance, InstantiationError> {
         // Validate the CPU features this module was compiled with against the
         // host CPU features.
         let host_cpu_features = CpuFeature::for_host();
@@ -367,7 +367,7 @@ impl Artifact {
 
         self.register_frame_info();
 
-        let handle = InstanceHandle::new(
+        let handle = VMInstance::new(
             allocator,
             module,
             context,
@@ -383,15 +383,15 @@ impl Artifact {
         Ok(handle)
     }
 
-    /// Finishes the instantiation of a just created `InstanceHandle`.
+    /// Finishes the instantiation of a just created `VMInstance`.
     ///
     /// # Safety
     ///
-    /// See [`InstanceHandle::finish_instantiation`].
+    /// See [`VMInstance::finish_instantiation`].
     pub unsafe fn finish_instantiation(
         &self,
         trap_handler: Option<*const TrapHandlerFn<'static>>,
-        handle: &mut InstanceHandle,
+        handle: &mut VMInstance,
     ) -> Result<(), InstantiationError> {
         let data_initializers = self
             .data_initializers()

lib/compiler/src/traits.rs

@@ -33,7 +33,7 @@ pub trait ArtifactCreate: Send + Sync + Upcastable {
     /// Returns the table plans associated with this `Artifact`.
     fn table_styles(&self) -> &PrimaryMap<TableIndex, TableStyle>;
 
-    /// Returns data initializers to pass to `InstanceHandle::initialize`
+    /// Returns data initializers to pass to `VMInstance::initialize`
     fn data_initializers(&self) -> &[OwnedDataInitializer];
 
     /// Serializes an artifact into bytes

lib/types/src/serialize.rs

@@ -142,7 +142,7 @@ impl SerializableModule {
         EnumSet::from_u64(self.cpu_features)
     }
 
-    /// Returns data initializers to pass to `InstanceHandle::initialize`
+    /// Returns data initializers to pass to `VMInstance::initialize`
     pub fn data_initializers(&self) -> &[OwnedDataInitializer] {
         &self.data_initializers
     }

lib/vm/src/instance/allocator.rs

@@ -1,4 +1,4 @@
-use super::{Instance, InstanceHandle};
+use super::{Instance, VMInstance};
 use crate::vmcontext::VMTableDefinition;
 use crate::VMMemoryDefinition;
 use std::alloc::{self, Layout};
@@ -59,15 +59,15 @@ impl Drop for InstanceAllocator {
 }
 
 impl InstanceAllocator {
-    /// Allocates instance data for use with [`InstanceHandle::new`].
+    /// Allocates instance data for use with [`VMInstance::new`].
     ///
     /// Returns a wrapper type around the allocation and 2 vectors of
     /// pointers into the allocated buffer. These lists of pointers
     /// correspond to the location in memory for the local memories and
     /// tables respectively. These pointers should be written to before
-    /// calling [`InstanceHandle::new`].
+    /// calling [`VMInstance::new`].
     ///
-    /// [`InstanceHandle::new`]: super::InstanceHandle::new
+    /// [`VMInstance::new`]: super::VMInstance::new
     pub fn new(
         module: &ModuleInfo,
     ) -> (
@@ -188,15 +188,15 @@ impl InstanceAllocator {
 
     /// Finish preparing by writing the [`Instance`] into memory, and
     /// consume this `InstanceAllocator`.
-    pub(crate) fn write_instance(mut self, instance: Instance) -> InstanceHandle {
+    pub(crate) fn into_vminstance(mut self, instance: Instance) -> VMInstance {
         // Prevent the old state's drop logic from being called as we
         // transition into the new state.
         self.consumed = true;
 
         unsafe {
             // `instance` is moved at `Self.instance_ptr`. This
             // pointer has been allocated by `Self::allocate_instance`
-            // (so by `InstanceHandle::allocate_instance`).
+            // (so by `VMInstance::allocate_instance`).
             ptr::write(self.instance_ptr.as_ptr(), instance);
             // Now `instance_ptr` is correctly initialized!
         }
@@ -205,7 +205,7 @@ impl InstanceAllocator {
 
         // This is correct because of the invariants of `Self` and
         // because we write `Instance` to the pointer in this function.
-        InstanceHandle {
+        VMInstance {
             instance,
             instance_layout,
         }

lib/vm/src/instance/mod.rs

@@ -3,7 +3,7 @@
 
 //! An `Instance` contains all the runtime state used by execution of
 //! a WebAssembly module (except its callstack and register state). An
-//! `InstanceHandle` is a wrapper around `Instance` that manages
+//! `VMInstance` is a wrapper around `Instance` that manages
 //! how it is allocated and deallocated.
 
 mod allocator;
@@ -1024,7 +1024,7 @@ impl Instance {
 /// This is more or less a public facade of the private `Instance`,
 /// providing useful higher-level API.
 #[derive(Debug, Clone, Eq, PartialEq)]
-pub struct InstanceHandle {
+pub struct VMInstance {
     /// The layout of `Instance` (which can vary).
     instance_layout: Layout,
 
@@ -1040,8 +1040,24 @@ pub struct InstanceHandle {
     instance: NonNull<Instance>,
 }
 
-impl InstanceHandle {
-    /// Create a new `InstanceHandle` pointing at a new [`InstanceRef`].
+/// VMInstance are created with an InstanceAllocator
+/// and it will "consume" the memory
+/// So the Drop here actualy free it (else it would be leaked)
+impl Drop for VMInstance {
+    fn drop(&mut self) {
+        let instance_ptr = self.instance.as_ptr();
+
+        unsafe {
+            // Need to drop all the actual Instance members
+            instance_ptr.drop_in_place();
+            // And then free the memory allocated for the Instance itself
+            std::alloc::dealloc(instance_ptr as *mut u8, self.instance_layout);
+        }
+    }
+}
+
+impl VMInstance {
+    /// Create a new `VMInstance` pointing at a new [`InstanceRef`].
     ///
     /// # Safety
     ///
@@ -1051,7 +1067,7 @@ impl InstanceHandle {
     /// method is a low-overhead way of saying “this is an extremely unsafe type
     /// to work with”.
     ///
-    /// Extreme care must be taken when working with `InstanceHandle` and it's
+    /// Extreme care must be taken when working with `VMInstance` and it's
     /// recommended to have relatively intimate knowledge of how it works
     /// internally if you'd like to do so. If possible it's recommended to use
     /// the `wasmer` crate API rather than this type since that is vetted for
@@ -1114,7 +1130,7 @@ impl InstanceHandle {
                 })),
             };
 
-            let mut instance_handle = allocator.write_instance(instance);
+            let mut instance_handle = allocator.into_vminstance(instance);
 
             // Set the funcrefs after we've built the instance
             {
@@ -1586,3 +1602,10 @@ fn build_funcrefs(
         imported_func_refs.into_boxed_slice(),
     )
 }
+
+/// This type is deprecated, it has been replaced by VMinstance.
+#[deprecated(
+    since = "3.2.0",
+    note = "InstanceHandle has been replaced by VMInstance"
+)]
+pub type InstanceHandle = VMInstance;

lib/vm/src/instance/ref.rs

@@ -92,12 +92,12 @@ unsafe impl Sync for InstanceInner {}
 /// simplified version of `std::sync::Arc`.
 ///
 /// Note for the curious reader: [`InstanceAllocator::new`]
-/// and [`InstanceHandle::new`] will respectively allocate a proper
+/// and [`VMInstance::new`] will respectively allocate a proper
 /// `Instance` and will fill it correctly.
 ///
 /// A little bit of background: The initial goal was to be able to
-/// share an [`Instance`] between an [`InstanceHandle`] and the module
-/// exports, so that one can drop a [`InstanceHandle`] but still being
+/// share an [`Instance`] between an [`VMInstance`] and the module
+/// exports, so that one can drop a [`VMInstance`] but still being
 /// able to use the exports properly.
 #[derive(Debug, PartialEq, Clone)]
 pub struct InstanceRef(Arc<InstanceInner>);

lib/vm/src/lib.rs

@@ -44,7 +44,8 @@ pub use crate::extern_ref::{VMExternObj, VMExternRef};
 pub use crate::function_env::VMFunctionEnvironment;
 pub use crate::global::*;
 pub use crate::imports::Imports;
-pub use crate::instance::{InstanceAllocator, InstanceHandle};
+#[allow(deprecated)]
+pub use crate::instance::{InstanceAllocator, InstanceHandle, VMInstance};
 pub use crate::memory::{
     initialize_memory_with_data, LinearMemory, VMMemory, VMOwnedMemory, VMSharedMemory,
 };

lib/vm/src/libcalls.rs

@@ -18,7 +18,7 @@
 //!   function frame, we need to raise it. This involves some nasty and quite
 //!   unsafe code under the covers! Notable, after raising the trap, drops
 //!   **will not** be run for local variables! This can lead to things like
-//!   leaking `InstanceHandle`s which leads to never deallocating JIT code,
+//!   leaking `VMInstance`s which leads to never deallocating JIT code,
 //!   instances, and modules! Therefore, always use nested blocks to ensure
 //!   drops run before raising a trap:
 //!

lib/vm/src/store.rs

@@ -9,7 +9,7 @@ use std::{
 };
 
 use crate::{
-    InstanceHandle, VMExternObj, VMFunction, VMFunctionEnvironment, VMGlobal, VMMemory, VMTable,
+    VMExternObj, VMFunction, VMFunctionEnvironment, VMGlobal, VMInstance, VMMemory, VMTable,
 };
 
 /// Unique ID to identify a context.
@@ -54,7 +54,7 @@ impl_context_object! {
     functions => VMFunction,
     tables => VMTable,
     globals => VMGlobal,
-    instances => InstanceHandle,
+    instances => VMInstance,
     memories => VMMemory,
     extern_objs => VMExternObj,
     function_environments => VMFunctionEnvironment,
@@ -68,7 +68,7 @@ pub struct StoreObjects {
     tables: Vec<VMTable>,
     globals: Vec<VMGlobal>,
     functions: Vec<VMFunction>,
-    instances: Vec<InstanceHandle>,
+    instances: Vec<VMInstance>,
     extern_objs: Vec<VMExternObj>,
     function_environments: Vec<VMFunctionEnvironment>,
 }