From 488b21ccef3d907d8b0ec8747293180bca280284 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Wed, 18 Jan 2023 12:47:59 +0100 Subject: [PATCH 1/5] Fixed some memory leak issue with InstanceHandle (some more leak are still present) --- lib/vm/src/instance/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index 45224b64ee3..bfd8c4ad3d2 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -1040,6 +1040,19 @@ pub struct InstanceHandle { instance: NonNull, } +/// InstanceHandle 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 InstanceHandle { + fn drop(&mut self) { + let instance_ptr = self.instance.as_ptr(); + + unsafe { + std::alloc::dealloc(instance_ptr as *mut u8, self.instance_layout); + } + } +} + impl InstanceHandle { /// Create a new `InstanceHandle` pointing at a new [`InstanceRef`]. /// From 0dc0ce1f0dd8e1fcf23cafbb53572759391454e5 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Wed, 25 Jan 2023 12:06:09 +0100 Subject: [PATCH 2/5] Renamed InstanceHandle to VMinstance, and InstanceAllocator::write_instance to into_vminstance --- lib/api/src/sys/instance.rs | 4 ++-- lib/api/src/sys/module.rs | 4 ++-- lib/compiler/src/engine/artifact.rs | 14 +++++++------- lib/compiler/src/traits.rs | 2 +- lib/types/src/serialize.rs | 2 +- lib/vm/src/instance/allocator.rs | 14 +++++++------- lib/vm/src/instance/mod.rs | 16 ++++++++-------- lib/vm/src/instance/ref.rs | 6 +++--- lib/vm/src/lib.rs | 2 +- lib/vm/src/libcalls.rs | 2 +- lib/vm/src/store.rs | 6 +++--- 11 files changed, 36 insertions(+), 36 deletions(-) diff --git a/lib/api/src/sys/instance.rs b/lib/api/src/sys/instance.rs index 4e0d90d3f18..a1572c681af 100644 --- a/lib/api/src/sys/instance.rs +++ b/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: #[derive(Clone)] pub struct Instance { - _handle: StoreHandle, + _handle: StoreHandle, module: Module, /// The exports for an instance. pub exports: Exports, diff --git a/lib/api/src/sys/module.rs b/lib/api/src/sys/module.rs index 9d49179265e..e6405272dbe 100644 --- a/lib/api/src/sys/module.rs +++ b/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 { + ) -> Result { // Ensure all imports come from the same context. for import in imports { if !import.is_from_store(store) { diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index b9ac9029227..b6ce5a6aa31 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/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 { @@ -308,13 +308,13 @@ impl Artifact { /// /// # Safety /// - /// See [`InstanceHandle::new`]. + /// See [`VMInstance::new`]. pub unsafe fn instantiate( &self, tunables: &dyn Tunables, imports: &[VMExtern], context: &mut StoreObjects, - ) -> Result { + ) -> Result { // Validate the CPU features this module was compiled with against the // host CPU features. let host_cpu_features = CpuFeature::for_host(); @@ -368,7 +368,7 @@ impl Artifact { self.register_frame_info(); - let handle = InstanceHandle::new( + let handle = VMInstance::new( allocator, module, context, @@ -384,15 +384,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() diff --git a/lib/compiler/src/traits.rs b/lib/compiler/src/traits.rs index 170a436cf12..d6ce67eda09 100644 --- a/lib/compiler/src/traits.rs +++ b/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; - /// 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 diff --git a/lib/types/src/serialize.rs b/lib/types/src/serialize.rs index 4b8d3be2bb4..e74151c687a 100644 --- a/lib/types/src/serialize.rs +++ b/lib/types/src/serialize.rs @@ -145,7 +145,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 } diff --git a/lib/vm/src/instance/allocator.rs b/lib/vm/src/instance/allocator.rs index ae3f360e871..78f41fbadbb 100644 --- a/lib/vm/src/instance/allocator.rs +++ b/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,7 +188,7 @@ 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; @@ -196,7 +196,7 @@ impl InstanceAllocator { 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, } diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index bfd8c4ad3d2..b835af4854e 100644 --- a/lib/vm/src/instance/mod.rs +++ b/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,10 +1040,10 @@ pub struct InstanceHandle { instance: NonNull, } -/// InstanceHandle are created with an InstanceAllocator +/// 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 InstanceHandle { +impl Drop for VMInstance { fn drop(&mut self) { let instance_ptr = self.instance.as_ptr(); @@ -1053,8 +1053,8 @@ impl Drop for InstanceHandle { } } -impl InstanceHandle { - /// Create a new `InstanceHandle` pointing at a new [`InstanceRef`]. +impl VMInstance { + /// Create a new `VMInstance` pointing at a new [`InstanceRef`]. /// /// # Safety /// @@ -1064,7 +1064,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 @@ -1127,7 +1127,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 { diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index 6d8b7c57898..5eafc337c32 100644 --- a/lib/vm/src/instance/ref.rs +++ b/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); diff --git a/lib/vm/src/lib.rs b/lib/vm/src/lib.rs index 3b1abc55127..f1bafdfb045 100644 --- a/lib/vm/src/lib.rs +++ b/lib/vm/src/lib.rs @@ -44,7 +44,7 @@ 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}; +pub use crate::instance::{InstanceAllocator, VMInstance}; pub use crate::memory::{ initialize_memory_with_data, LinearMemory, VMMemory, VMOwnedMemory, VMSharedMemory, }; diff --git a/lib/vm/src/libcalls.rs b/lib/vm/src/libcalls.rs index 67523a14454..1c747cb41f0 100644 --- a/lib/vm/src/libcalls.rs +++ b/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: //! diff --git a/lib/vm/src/store.rs b/lib/vm/src/store.rs index 4e0c4676525..c28421474d8 100644 --- a/lib/vm/src/store.rs +++ b/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, globals: Vec, functions: Vec, - instances: Vec, + instances: Vec, extern_objs: Vec, function_environments: Vec, } From 51eff3229437d33574313e5b9fdc4680b4813158 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Wed, 25 Jan 2023 13:28:34 +0100 Subject: [PATCH 3/5] Added InstanceHandle as a deprecated alias to VMInstance, to avoid API-breaking change --- lib/vm/src/instance/mod.rs | 7 +++++++ lib/vm/src/lib.rs | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index b835af4854e..ace1222a40b 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -1599,3 +1599,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; diff --git a/lib/vm/src/lib.rs b/lib/vm/src/lib.rs index f1bafdfb045..af1bbaa5101 100644 --- a/lib/vm/src/lib.rs +++ b/lib/vm/src/lib.rs @@ -44,7 +44,7 @@ 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, VMInstance}; +pub use crate::instance::{InstanceAllocator, InstanceHandle, VMInstance}; pub use crate::memory::{ initialize_memory_with_data, LinearMemory, VMMemory, VMOwnedMemory, VMSharedMemory, }; From ec368e7c08f152499e00a89e8a9bee83e08de2eb Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Wed, 25 Jan 2023 13:43:37 +0100 Subject: [PATCH 4/5] Removed a warning --- lib/vm/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/vm/src/lib.rs b/lib/vm/src/lib.rs index af1bbaa5101..f4fe1a18ea2 100644 --- a/lib/vm/src/lib.rs +++ b/lib/vm/src/lib.rs @@ -44,6 +44,7 @@ pub use crate::extern_ref::{VMExternObj, VMExternRef}; pub use crate::function_env::VMFunctionEnvironment; pub use crate::global::*; pub use crate::imports::Imports; +#[allow(deprecated)] pub use crate::instance::{InstanceAllocator, InstanceHandle, VMInstance}; pub use crate::memory::{ initialize_memory_with_data, LinearMemory, VMMemory, VMOwnedMemory, VMSharedMemory, From df6bc6ba3b81a580afaa4ea2029ea2cd32d4420e Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Wed, 25 Jan 2023 14:06:43 +0100 Subject: [PATCH 5/5] Fixed the remaining memory leaks --- lib/vm/src/instance/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index ace1222a40b..cb0d38f3291 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -1048,6 +1048,9 @@ impl Drop for VMInstance { 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); } }