Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuration of default memory #1360

Closed
webmaster128 opened this issue Apr 7, 2020 · 17 comments
Closed

Allow configuration of default memory #1360

webmaster128 opened this issue Apr 7, 2020 · 17 comments
Labels
🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates

Comments

@webmaster128
Copy link
Contributor

Motivation

I'd like to be able to limit the memory consumption of a Wasm sandbox. It seems like MemoryDescriptor can do that. However, I don't see a way to configure the default memory (memory(0)), which is created for me automatically.

Proposed solution

After digging into the code, I found a way to patch the memory limit into Wasmer (0.16.2) like this:

diff --git a/lib/runtime-core/src/lib.rs b/lib/runtime-core/src/lib.rs
index 7b6168c5f..a9cd4189a 100644
--- a/lib/runtime-core/src/lib.rs
+++ b/lib/runtime-core/src/lib.rs
@@ -103,6 +103,9 @@ pub mod prelude {
     pub use crate::{func, imports};
 }
 
+use crate::types::{LocalMemoryIndex};
+use crate::units::{Pages};
+
 /// Compile a [`Module`] using the provided compiler from
 /// WebAssembly binary code. This function is useful if it
 /// is necessary to a compile a module before it can be instantiated
@@ -117,6 +120,11 @@ pub fn compile_with(
         .map(|mut inner| {
             let inner_info: &mut crate::module::ModuleInfo = &mut inner.info;
             inner_info.import_custom_sections(wasm).unwrap();
+            let keys: Vec<LocalMemoryIndex> = inner_info.memories.iter().map(|(k, _)| k).collect();
+            for key in keys {
+                inner_info.memories[key].maximum = Some(Pages(512)); // 32 MiB
+            }
+            // println!("Module memories: {:?}", inner_info.memories);
             module::Module::new(Arc::new(inner))
         })
 }

The loop iterates over one key only. For some visibility reasons I was not able to construct LocalMemoryIndex(0) directly.

This patch changes the default memory from

Memory descriptor: MemoryDescriptor { minimum: 17 pages, maximum: None, shared: false, memory_type: Dynamic }; size: 18 pages (1179648 bytes)

to

Memory descriptor: MemoryDescriptor { minimum: 17 pages, maximum: Some(512 pages), shared: false, memory_type: Dynamic }; size: 18 pages (1179648 bytes)

and in my tests this new maximum seems to be respected.

The problem with this is primarily, that compiler.compile is explicitely designed to be crate internal and I don't see an API that allows for setting memory limits.

Alternatives

I don't know if this is the correct approach to set memory limits.

Additional context

@ethanfrey
Copy link
Contributor

A clean solution I could imagine would be to modify compile_with_config and then extend CompilerConfig to also include Option<MemoryDescriptor>.

The backends would then add support to use the MemoryDescriptor when instantiating memory:

@ethanfrey
Copy link
Contributor

Looking more, I am not sure that is where Memories are created, but rather in the VM Instance...

LocalBacking::generate_memories uses the info from modules.info to decide how big to make the memory. So we just need to set this info when compiling, as Simon did above. If we did the same logic, but only on compile_with_config if a MemoryDescriptor was set, would that make an acceptable PR to merge upstream?

@syrusakbary
Copy link
Member

I think there are two ways of solving this:

  1. Enable custom memory generators (so when a wasm module has a memory, the developer can specify how this memory is getting created, and therefor specify the maximum if it's not present).
  2. Add middleware to modify the memory descriptor that the compiler receives

However, there are a few considerations to make before jumping into an implementation:

  • What happens if a memory created in a wasm side has a maximum that is over what the implementor wants to allow? Should it fail? Should it silently replace the memory max with the lowest value?
  • Do we want to modify just the descriptor metadata provided by the wasm instead?

@webmaster128
Copy link
Contributor Author

@syrusakbary Thanks for your response and I agree this should not be rushed. I think I don't understand Wasm internals and memory well enough to contribute to those specific questions. Could you maybe help by explaining where the first, unlimited memory(0) is created (or specified)? I just observe that it is there, my data lives in there, I can access it from the VM and it can grow to 4 GB.

@syrusakbary
Copy link
Member

syrusakbary commented Apr 8, 2020

So, when no maximum memory is specified (memory 0), it's on the implementor side define what is that maximum.

By default, this will mean that the memory is dynamically allocated (as is less expensive for creation). However, dynamically allocated memory pushes more effort into the compiler since it have to do bounds-checking each time it does an operation in memory.
Because of that, most of the wasm implementors assume a static memory, if no maximum is set (with a max of 4Gb).

Having said that, in the wasm side if the memory maximum is not provided, we can easily decide what to do with it (meaning: we can easily allow a custom maximum, if not set in the wasm module).

This is the ideal scenario since the implementation would be quite simple. However, it's important to be aware that this solution will not work if the user set's a different custom maximum in the memory module.

@webmaster128
Copy link
Contributor Author

Thanks!

This is the ideal scenario since the implementation would be quite simple. However, it's important to be aware that this solution will not work if the user set's a different custom maximum in the memory module.

In this case "the user" is the person who wrote the Wasm bytecode? Since we don't trust this entity (at least for browser and blockchain use cases), I think we need a way to detect and reject that Wasm.

@ethanfrey
Copy link
Contributor

Assuming this is deterministic amount of memory used, I agree that we should fail with a WasmerRuntimeError or such when the contract tries to exceed the limit. This is a requirement for all blockchain projects I believe.

@syrusakbary
Copy link
Member

syrusakbary commented Apr 8, 2020

we should fail with a WasmerRuntimeError or such when the contract tries to exceed the limit

If the wasm contract has a (memory 0 {LARGE_MAXIMUM}) it should also fail, right?
(even if is just "allocating the memory").

If that sounds right, I think we can get into an easy solution :)

@webmaster128
Copy link
Contributor Author

webmaster128 commented Apr 8, 2020

If the wasm contract has a (memory 0 {LARGE_MAXIMUM}) it should also fail, right?
(even if is just "allocating the memory").

Yes. At least in our case we are able to define the rule: memory maximum must be undefined or below X. Non-compliant contracts can be rejected. Then the host sets the maximum.

@Hywan Hywan added the 📦 lib-deprecated About the deprecated crates label Apr 9, 2020
@Hywan
Copy link
Contributor

Hywan commented Apr 9, 2020

I think the idea proposed by @syrusakbary is the correct one:

  1. On one side, being able to define the maximum bound of a memory when undefined (either to None, which is the current behavior —more or less—, or Some(n)),
  2. On another side, use a contract to check that all maximum bounds are lower than a specific limit.

I think the best way to address (1) is to use wasmer_runtime_core::backend::CompilerConfig, and compile_with_config. What do you think?

Possible connection to the following PR: #1299.

@MaksymZavershynskyi
Copy link

At Near we use pwasm-utils to limit memory consumption: https://github.com/nearprotocol/nearcore/blob/4bc4b74aeaab3231866ddb7de822e8892a13d9dc/runtime/near-vm-runner/src/prepare.rs#L31

@ethanfrey
Copy link
Contributor

Thanks for the input @nearmax

So you check and rewrite the memory request of the contract before sending it to wasmer. This seems quite solid. It would be nice to allow this to be done with a config option or a middleware, but a working solution is great.

@webmaster128
Copy link
Contributor Author

webmaster128 commented Apr 11, 2020

Thank you @nearmax, very helpful to understand the flow! If possible, I'd like to find a solution that avoids manipulating and re-serializing the orginial contract. However, this is just a personal feeling with no specific reason.

The more I learn about memory management in Wasm, the more I give up the idea of a simple max memory setting on the VM. I guess some kind of callback allowing to inject custom commands like this one or a middleware solution would be nice.

@syrusakbary
Copy link
Member

After some iteration of this, we got into a nice way of solving the custom memory generation with a very clean API.
We will close this once we merge the refactor into master.

@syrusakbary
Copy link
Member

This should be very easily solved by the refactor, that is now in master thanks to a custom implementation of Tunables that can be provided when creating a Store.

Closing the issue

@ethanfrey
Copy link
Contributor

Sounds good. Can you please link to the relevant code for those of us who do not know the details? (Ideally a testcase to see this in action)

bors bot added a commit that referenced this issue Oct 28, 2020
1730: Create example for limiting memory with Tunables r=syrusakbary a=webmaster128

Closes #1588

# Description

This is an attempt for solving #1360 with the new Tunables API. It could be developed further to address #1588 if you like.

I'd appreciate a code review to ensure I'm on the right track.

Open questions:
1. Is it expected that I had to add `wasmer-vm`?
2. Should I really create a new `Target` for this use case when engine already has one set?
3. I used `BaseTunables` for the base implementation and sticked with `Tunables` for the trait name, because it makes most sense to me. However, in `lib/api/src/tunables.rs` it is done the other way round. Any thoughts on the naming issue?
4. The import collision between `wamer::Memory` and `wasmer_vm::Memory` is inconvenient. Can it be avoided or do I do it correctly here?


# Review

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


Co-authored-by: Simon Warta <[email protected]>
@webmaster128
Copy link
Contributor Author

webmaster128 commented Oct 29, 2020

For those who find this thread: We now have an example that shows how to limit memory consumption by adjusting the contract's imported memory's maximum via the new Tunables API (see also #1730 and #1775). The exports required to copy LimitingTunables into your project will be available from Wasmer 1.0.0-alpha5 on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates
Projects
None yet
Development

No branches or pull requests

5 participants