Fix memory leak in host function envs #1865

MarkMcCaskey · 2020-12-03T00:57:20Z

TODO: link to issue

This PR contains a number of changes:

Make WasmerEnv: Clone
Store a pointer to the clone function when creating a host function (Notably this is a feature that wouldn't work even if we could use a proper trait object because you can't have a Sized trait object and Clone: Sized).
Store a pointer to the drop function when creating a host function.
Clone the env via pointer every time an Instance is made. Therefore each Instance gets its own, unique Env per host function with Env.
Add reference counting and drop logic to a sub-field of wasmer_export::ExportFunction which frees the original version of the Env (the thing that gets cloned each time an Instance is made) with the drop function pointer.
Change some logic in vm::Instance from SoA (struct of arrays) to AoS (array of structs): this uses more memory but is a bit less error prone and can be easily changed later.
Add logic on this new struct (vm::ImportEnv) that contains the function pointers for each import in Instance to drop (with the drop fn pointer) when the vm::Instance is being dropped. This fixes the original memory leak.
Add wrapper functions inside the host function creation functions which makes the layout of the user supplied env-pointer the responsibility of each function. Thus, rather than drop being Env::drop, it's a function which frees all wrapper types, traverses indirections and frees the internal Env with Env::drop. This simplifies code at the cost of making the host_env pointer (vmctx) not consistent in terms of what it actually points to. This change fixes another memory leak related to the creation of host functions.

tl;dr: we're leaning into manually doing virtual method dispatch on WasmerEnvs and it actually works great! The biggest issue I have with the PR as-is is that the code isn't as clean/readable/robust as I'd ideally like it to be.

Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667.

Review

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

lib/engine/src/export.rs

syrusakbary

A function can be exported twice. How do you verify that host_env_drop_fn is only called once?

MarkMcCaskey · 2020-12-05T01:41:45Z

bors try

bors · 2020-12-05T01:42:46Z

try

Build failed:

Code lint

Cargo.toml

lib/api/src/env.rs

lib/api/src/externals/function.rs

lib/engine/src/export.rs

Hywan

I like the PR. In some places it simplifies code, in other, it's a little bit harder to understand. I think another refactoring round would be required in a close future, but let's keep the priority as is: fixing the memory leak.

There is a lot of TODO in this PR. Are you going to fix them all? I believe a majority should be fixed before merging this PR.

Hywan · 2020-12-10T12:13:10Z

lib/api/src/externals/function.rs

-            func: Box::new(func),
-            function_type: ty.clone(),
-        });
+        let dynamic_ctx: VMDynamicFunctionContext<DynamicFunctionWithoutEnv> =


I bet this type annotation isn't required (regardless, I prefer having type annotation in this kind of code, so it's perfectly fine to keep it, just wondered whether it changes something).

Not strictly no, I thought it was helpful for understanding what's happening with the unsafe block here though.

Actually all these wrapper functions can be created by a helper function, so I should probably just do that

Hywan · 2020-12-10T12:32:01Z

lib/api/src/externals/function.rs

-    // This field _must_ come first in this struct.
-    env: Box<Env>,


Why env is no longer required to be at first position?

The reason before was due to a bad hack where we'd double deref a pointer ignoring the fact that it was in a struct.

We now have each function that is called on the env traverse things correctly, so for example we create a wrapper function where we walk through the types and find what we need. So if we need to operate on the Box<env> itself, we can find it in each function. I think this is a simpler solution because then all the functions always just work on the env, we had an issue where the clone and drop function needed to be called on the top level the wrapper dynamic func context thing but the initialize function needed to be called on the inner env. This fixes that by making the thing each function gets consistent.

Hywan · 2020-12-10T12:32:50Z

lib/api/src/types.rs

@@ -76,7 +76,7 @@ impl ValFuncRef for Val {
        let export = wasmer_engine::ExportFunction {
            // TODO:
            // figure out if we ever need a value here: need testing with complicated import patterns
-            import_init_function_ptr: None,


Is the TODO above still valid?

Yeah, I'm not 100% confident in this code which is why that comment is still there, I can investigate it later... I'm not aware of a good way to test this yet, I can investigate that though

We should investigate. I bet is fine as None, but it will be good to verify

lib/engine/src/export.rs

Hywan · 2020-12-10T12:35:13Z

lib/engine/src/export.rs

+    ///
+    /// See `wasmer_vm::export::VMExportFunction::vmctx` for the version of
+    /// this pointer that is used by the VM when creating an `Instance`.
+    pub host_env: *mut std::ffi::c_void,


Idea: Create a new constructor method rather than having all fields pub. I think it could be “dangerous”, it's better to hide everything here. Thoughts?

Same for all the fields in this type.

lib/engine/src/export.rs

lib/vm/src/instance.rs

1902: Fix memory leak in `InstanceAllocator` on failure r=MarkMcCaskey a=MarkMcCaskey This code needs to be cleaned up, just hacked it out really quickly. The idea is to use a builder pattern to create a type-safe state machine for setting up things for the `InstanceAllocator`; this should allow us to remove `unsafe` in our "public" API because we can guarantee certain invariants because of the restricted nature of these types. The names for the new types are long and don't really make sense but I didn't want to get blocked on that: suggestions welcome! In combination with #1865 , this PR should fix the all the memory leaks seen by asan while running `make test-cranelift-jit` 🎉 🎉 🎉 Co-authored-by: Mark McCaskey <[email protected]>

syrusakbary · 2020-12-11T22:29:51Z

lib/api/src/env.rs

+impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicU32 {}
+impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicI64 {}
+impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicUsize {}
+impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicIsize {}


Why we need the <'a> ?

<'a> is needed because of the &: trait impls on things with lifetimes can't omit them. And the reason we need & is to make these types shared somehow, otherwise the impl WasmerEnv for X can't be used here unless we have recursive definitions.

These impls on std types definitely aren't critical; these actually are only work with 'static lifetimes because of other constraints, so this only really works when putting an atomic in a static variable or otherwise doing unsafe things.

I'd be fine to remove them

lib/vm/src/instance.rs

lib/engine/src/artifact.rs

lib/vm/src/instance.rs

lib/vm/src/imports.rs

syrusakbary · 2020-12-11T22:40:55Z

lib/api/src/types.rs

@@ -76,7 +76,7 @@ impl ValFuncRef for Val {
        let export = wasmer_engine::ExportFunction {
            // TODO:
            // figure out if we ever need a value here: need testing with complicated import patterns
-            import_init_function_ptr: None,


We should investigate. I bet is fine as None, but it will be good to verify

syrusakbary · 2020-12-14T22:58:31Z

lib/vm/src/instance/mod.rs

+pub enum ImportFunctionEnv {
+    /// The `vmctx` pointer does not refer to a host env, there is no
+    /// metadata about it.
+    NoEnv,


I think is better to name this: VMEnv.
And have the HostEnv to be an enum with all the different options:

NoEnv

Env

Also, the VM should not be aware of dynamic functions or not. So I believe DynamicHostEnvWithNoInnerEnv should be done via HostEnv

Same thought on the VM prefix, I'm not sure it makes sense to use it on types that aren't related to the VM.

Yeah, the extra case here is just to make the multiple cases more explicit. Otherwise there's data that's sometimes there and sometimes not and it's not exactly clear why. This enum is more descriptive than prescriptive: it's describing the state of the world as it is, not what it should be. Perhaps we should move this type out of vm, there's no reason it has to be defined in VM.

For the VM, there is not such thing as DynamicHostEnv. This is something that we do in the engine level, not VM

Meaning: the vm only handles the functions with their end ABI. Is the engine the one that creates the trampolines and so.

Note: there is some "dangling" code in the VM that is aware of dynamic functions. But thanks to the PR where we move most of the function stuff to the engine, it should be easy to move the rest as well.

lib/vm/src/imports.rs

MarkMcCaskey · 2020-12-15T20:20:00Z

bors r+

@Hywan

1865: Fix memory leak in host function envs r=MarkMcCaskey a=MarkMcCaskey TODO: link to issue This PR contains a number of changes: 1. Make `WasmerEnv: Clone` 2. Store a pointer to the `clone` function when creating a host function (Notably this is a feature that wouldn't work even if we _could_ use a proper trait object because you can't have a `Sized` trait object and `Clone: Sized`). 3. Store a pointer to the `drop` function when creating a host function. 4. Clone the env via pointer every time an `Instance` is made. Therefore each `Instance` gets its own, unique `Env` per host function with `Env`. 5. Add reference counting and drop logic to a sub-field of `wasmer_export::ExportFunction` which frees the original version of the `Env` (the thing that gets cloned each time an `Instance` is made) with the `drop` function pointer. 6. Change some logic in `vm::Instance` from SoA (struct of arrays) to AoS (array of structs): this uses more memory but is a bit less error prone and can be easily changed later. 7. Add logic on this new struct (`vm::ImportEnv`) that contains the function pointers for each import in `Instance` to drop (with the `drop` fn pointer) when the `vm::Instance` is being dropped. This fixes the original memory leak. 8. Add wrapper functions inside the host function creation functions which makes the layout of the user supplied env-pointer the responsibility of each function. Thus, rather than `drop` being `Env::drop`, it's a function which frees all wrapper types, traverses indirections and frees the internal `Env` with `Env::drop`. This simplifies code at the cost of making the `host_env` pointer (`vmctx`) not consistent in terms of what it actually points to. This change fixes another memory leak related to the creation of host functions. tl;dr: we're leaning into manually doing virtual method dispatch on `WasmerEnv`s and it actually works great! The biggest issue I have with the PR as-is is that the code isn't as clean/readable/robust as I'd ideally like it to be. Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667. # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

bors · 2020-12-15T20:33:19Z

Build failed:

Test on linux-aarch64

syrusakbary · 2020-12-15T21:45:22Z

examples/imports_function_env.rs

-        counter: Arc<RefCell<i32>>,
+        counter: Arc<Mutex<i32>>,


Why do we need to change the examples? The previous examples should work as is. Or am I missing something?

Yeah, the previous examples were incorrect, we just didn't enforce it. WasmerEnv needs to be Send and Sync because Instance is Send and Sync and we don't synchronize access to functions. So it's possible in safe Rust to get a NativeFunc on multiple threads that points to the same underlying function. In order for the WasmerEnv to be able to handle being used this way, it must be both Send and Sync. This shouldn't affect much, if any, real code. For example, WASI was not affected at all by this change. Emscripten was only affected because it was doing some weird stuff with pointers.

MarkMcCaskey · 2020-12-15T22:37:24Z

bors r+

bors · 2020-12-15T23:12:21Z

Build succeeded:

webmaster128 · 2020-12-15T23:53:36Z

Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667.

Note that GitHub only closes one ticket per keyword. In order to connect all of them, you need something like: fixes #1584, fixes #1714, fixes #1865, fixes #1667.

Fix memory leak in host function envs

1eaea6e

MarkMcCaskey requested a review from Hywan December 3, 2020 00:57

MarkMcCaskey requested review from nlewycky and syrusakbary as code owners December 3, 2020 00:57

syrusakbary reviewed Dec 3, 2020

View reviewed changes

lib/engine/src/export.rs Outdated Show resolved Hide resolved

syrusakbary reviewed Dec 3, 2020

View reviewed changes

jubianchi added 1.0 Wasmer at 1.0 🎉 enhancement New feature! labels Dec 4, 2020

Experimental: clone host envs during construction

e43d9d2

MarkMcCaskey requested a review from jubianchi as a code owner December 5, 2020 00:04

Mark McCaskey added 2 commits December 4, 2020 17:07

Fix issue with dynamic host env

4ec8c9e

Fix tests in C API and deprecated API

ca4736c

bors bot added a commit that referenced this pull request Dec 5, 2020

Try #1865:

af7c60a

Merge branch 'master' into fix/host-func-env-memory-leak

476e8d1

nlewycky reviewed Dec 7, 2020

View reviewed changes

Address feedback

6e95c50

This was referenced Dec 9, 2020

Fix memory leak in InstanceAllocator on failure #1902

Merged

Add test showing issue with reusing import objects w/ WasmerEnv #1876

Closed

Mark McCaskey added 2 commits December 9, 2020 11:55

Merge branch 'master' into fix/host-func-env-memory-leak

99e0357

Make NativeFunc Clone

be3c381

Hywan reviewed Dec 10, 2020

View reviewed changes

Address feedback, make code more explicit about the different cases

ec4d6e6

MarkMcCaskey force-pushed the fix/host-func-env-memory-leak branch from 7c247bc to ec4d6e6 Compare December 11, 2020 21:00

syrusakbary reviewed Dec 11, 2020

View reviewed changes

Mark McCaskey added 2 commits December 11, 2020 15:47

Address feedback

bd01d6a

Merge branch 'master' into fix/host-func-env-memory-leak

f798f34

syrusakbary reviewed Dec 14, 2020

View reviewed changes

lib/vm/src/imports.rs Show resolved Hide resolved

Mark McCaskey added 2 commits December 15, 2020 08:31

Remove variant from ImportFunctionEnv

27f7e7e

Merge branch 'master' into fix/host-func-env-memory-leak

62d15fa

syrusakbary approved these changes Dec 15, 2020

View reviewed changes

Mark McCaskey added 2 commits December 15, 2020 13:35

Fix up tests, make WasmerEnv: Send

6a21169

Add WasmerEnv: Sync

8081aae

syrusakbary reviewed Dec 15, 2020

View reviewed changes

bors bot merged commit 22c7bc8 into master Dec 15, 2020

bors bot deleted the fix/host-func-env-memory-leak branch December 15, 2020 23:12

jubianchi mentioned this pull request Dec 16, 2020

Environment passed to Function::new_native_with_env is never dropped and leaks memory #1667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in host function envs #1865

Fix memory leak in host function envs #1865

MarkMcCaskey commented Dec 3, 2020 •

edited by Hywan

Loading

syrusakbary left a comment

MarkMcCaskey commented Dec 5, 2020

bors bot commented Dec 5, 2020

Hywan left a comment

Hywan Dec 10, 2020

MarkMcCaskey Dec 10, 2020

Hywan Dec 10, 2020

MarkMcCaskey Dec 10, 2020

Hywan Dec 10, 2020

MarkMcCaskey Dec 10, 2020

syrusakbary Dec 11, 2020

Hywan Dec 10, 2020

Hywan Dec 10, 2020

syrusakbary Dec 11, 2020

MarkMcCaskey Dec 11, 2020

syrusakbary Dec 11, 2020

syrusakbary Dec 14, 2020

MarkMcCaskey Dec 14, 2020

syrusakbary Dec 14, 2020

syrusakbary Dec 14, 2020

syrusakbary Dec 14, 2020

MarkMcCaskey commented Dec 15, 2020

bors bot commented Dec 15, 2020

syrusakbary Dec 15, 2020

MarkMcCaskey Dec 15, 2020 •

edited

Loading

MarkMcCaskey commented Dec 15, 2020

bors bot commented Dec 15, 2020

webmaster128 commented Dec 15, 2020

		// This field _must_ come first in this struct.
		env: Box<Env>,

Fix memory leak in host function envs #1865

Fix memory leak in host function envs #1865

Conversation

MarkMcCaskey commented Dec 3, 2020 • edited by Hywan Loading

Review

syrusakbary left a comment

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 5, 2020

bors bot commented Dec 5, 2020

try

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 15, 2020

bors bot commented Dec 15, 2020

Choose a reason for hiding this comment

MarkMcCaskey Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 15, 2020

bors bot commented Dec 15, 2020

webmaster128 commented Dec 15, 2020

MarkMcCaskey commented Dec 3, 2020 •

edited by Hywan

Loading

MarkMcCaskey Dec 15, 2020 •

edited

Loading