wasmi-labs / wasmi

WebAssembly (Wasm) interpreter.
https://wasmi-labs.github.io/wasmi/
Apache License 2.0
1.55k stars 278 forks source link

How to limit memory usage? #720

Closed jayz22 closed 1 year ago

jayz22 commented 1 year ago

https://github.com/paritytech/wasmi/issues/643 introduces native fuel metering that allows fuel (proxy to cpu instructions) consumption to be metered and limited, and the limit is configurable.

Is there plan to enhance the metering system to cover memory usage? It would be useful to have native support for configurable memory usage limiting, similar to the fuel metering.

The Memory module contains a maximum_pages and is enforced during the memory grow operation. However, AFAIK the maximum page is specified by the WASM bytecode and is not configurable by Wasmi. Is there a way for Wasmi to natively limit the memory usage and allow that limit to be configurable?

Robbepop commented 1 year ago

The Memory module contains a maximum_pages and is enforced during the memory grow operation. However, AFAIK the maximum page is specified by the WASM bytecode and is not configurable by Wasmi. Is there a way for Wasmi to natively limit the memory usage and allow that limit to be configurable?

This is true only for Memory instances that are internal to a Wasm module. You can definitely control and customize the maximum pages of a Memory instance if it is managed by the host side and imported by the Wasm module.

Is there plan to enhance the metering system to cover memory usage? It would be useful to have native support for configurable memory usage limiting, similar to the fuel metering.

So far this was never requested. Also I am not aware of any other Wasm runtime that provides this feature. I might be not up to date though. With the above information about imported Memory instances, is this still required for you?

jayz22 commented 1 year ago

This is true only for Memory instances that are internal to a Wasm module. You can definitely control and customize the maximum pages of a Memory instance if it is managed by the host side and imported by the Wasm module.

Thanks @Robbepop. If the memory instance can be host controlled that would be great and would address my issue.

If I understood it correctly, the host would create a Memory instance and pass it to Wasmi during Module instantiation (or create the module first and import the Memory)? Will the imported memory take the place of the internal one, such that any linear memory operation (e.g. memory grow) will take place on the host-created memory? Or will both the internal and external memory exist simultaneously.

I've been having a bit of hard time figuring out how to make it work, if you have any code examples that shows how memory importing works that would helpful greatly.

jayz22 commented 1 year ago

the host would create a Memory instance and pass it to Wasmi during Module instantiation (or create the module first and import the Memory)?

I think I figured this part out, I first create a new Memory in the host and use linker.define to link the memory to the store (same way extern Func works).

Also if I'm not mistaken, it appears the external and internal memory will exist together, so it's still possible for the VM to grow its internal memory without the host control / limit?

Robbepop commented 1 year ago

I think I figured this part out, I first create a new Memory in the host and use linker.define to link the memory to the store (same way extern Func works).

Happy you figured it out!

Also if I'm not mistaken, it appears the external and internal memory will exist together, so it's still possible for the VM to grow its internal memory without the host control / limit?

Yes, the Wasm side can still grow the imported Memory instance. However, only within the bounds set by the host side.

jayz22 commented 1 year ago

Also if I'm not mistaken, it appears the external and internal memory will exist together, so it's still possible for the VM to grow its internal memory without the host control / limit?

Yes, the Wasm side can still grow the imported Memory instance. However, only within the bounds set by the host side.

Apologies if my question wasn't clear. When we link in an external memory, I understood the guest can only grow the external memory within the host-defined limit. However, the internal memory will still exist when the module is created from parsing the WASM bytecode, and it can be still grown by WASM without the host's control/lilmit? If that's the case, we don't have a mechanism to limit the total memory consumption (internal + external) from the VM by the host? Or am I mistaken, and the internal memory doesn't exist any more (it doesn't appear like it from looking at the code)?

Just to give a bit of background on what we were trying to do and why memory limiting/metering is important. In Soroban smart contract platform. The smart contract is the WASM bytecode running inside the VM, the host contains functionalities shared among all contracts and exposed as host functions. We need a mechanism for the host to meter both the cpu instruction (fuel), as well as memory usage by each contract, and charge a proper fee for the resource consumption. (You can read more about our metering and fees approach in here. We are currently using our custom fork of Wasmi as our WASM runtime and would love to switch to the upstream.

huntc commented 1 year ago

I've been playing around with this too. I can see how to define a Memory object, but I'm unsure what I should supply to the module and name parameters when calling define on the linker. Thanks for any further pointers.

Robbepop commented 1 year ago

Also if I'm not mistaken, it appears the external and internal memory will exist together, so it's still possible for the VM to grow its internal memory without the host control / limit?

Yes, the Wasm side can still grow the imported Memory instance. However, only within the bounds set by the host side.

Apologies if my question wasn't clear. When we link in an external memory, I understood the guest can only grow the external memory within the host-defined limit. However, the internal memory will still exist when the module is created from parsing the WASM bytecode, and it can be still grown by WASM without the host's control/lilmit? If that's the case, we don't have a mechanism to limit the total memory consumption (internal + external) from the VM by the host? Or am I mistaken, and the internal memory doesn't exist any more (it doesn't appear like it from looking at the code)?

Just to give a bit of background on what we were trying to do and why memory limiting/metering is important. In Soroban smart contract platform. The smart contract is the WASM bytecode running inside the VM, the host contains functionalities shared among all contracts and exposed as host functions. We need a mechanism for the host to meter both the cpu instruction (fuel), as well as memory usage by each contract, and charge a proper fee for the resource consumption. (You can read more about our metering and fees approach in here. We are currently using our custom fork of Wasmi as our WASM runtime and would love to switch to the upstream.

I partially read the metering description and was wondering as to what exactly are you metering. Are you metering the number of accesses, the number of read or written bytes, the number of accesses Wasm pages, the maximum Wasm pages accessed, etc. for the metering?

In our contracts-pallet which we use at Substrate/Polkadot we do not need linear memory metering because we simply provide every smart contract execution with 1MB (or something like that) of linear memory. If the smart contract needs more, then you are screwed. So far I am not aware of a single report to provide more linear memory to smart contract executions. The reason probably being that smart contract executions usually are short lived. However, we do not persist linear memory of a smart contract in between executions. This might be different from your use case though, for example if you are using an actor based execution approach.

I've been playing around with this too. I can see how to define a Memory object, but I'm unsure what I should supply to the module and name parameters when calling define on the linker. Thanks for any further pointers.

I warmly recommend looking into the Wasmtime API docs since they are pretty decent and extremely similar to what you'd do in wasmi since wasmi goes a long way mirroring the Wasmtime API so people only have to learn one API to deal with Wasm runtimes. https://docs.rs/wasmtime/9.0.1/wasmtime/ (It is also full of examples. We clearly can do a better job at providing examples in wasmi docs.)

To answer your question: You want to create an Engine, a Store, a Linker and a Memory. The Memory is given the min and max limits to your wishes. Then you define this Memory instance in the Linker. In Wasm all imports have a module and field namespace. These are just 2 different names, for example a module name could be something like env or std and a field name could be mem in this case, so module: "env", field: "mem" to define your Memory instance in the Linker. Then in your actual Wasm file you need to import this Memory using exactly these 2 names.

If you import a Memory instance the lifetime of the instance is bound by the host side. So you can re-use the same Memory instance by different smart contract executions if you clear the bytes in between calls. In Substrate's pallet-contracts we simply spin up a new Engine, Store etc. for every smart contract execution and do not share anything. But we only do it this way for technical/architectural reasons.

huntc commented 1 year ago

Then in your actual Wasm file you need to import this Memory using exactly these 2 names.

Hopefully, I’m not hijacking the original question, but how about if there is no control over the supply of the wasm file, and therefore the ability to import env::mem?

Robbepop commented 1 year ago

You need some kind of control. At Parity for example we still pre-analyze incoming Wasm blobs before execution via wasmi for some properties. This way it is possible to use non-imported Memory inherent to the Wasm module as long as the pre-analysis determines that at most N pages are used where N is the set limit. Otherwise the Wasm blob is discarded and never executed.

huntc commented 1 year ago

Thanks for that. Here's the control I ended up with. I hope that it is useful for someone else:

#[derive(Debug)]
enum Error {
    MaximumMemoryExceeded(u32),
    MaximumMemoryUnspecified,
}
impl Display for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Error::MaximumMemoryExceeded(pages) => write!(
                f,
                "WASM file has a maximum memory of {} pages, but must not exceed {} pages",
                pages, MAX_MEMORY_PAGES
            ),
            Error::MaximumMemoryUnspecified => write!(
                f,
                "WASM file must be compiled with a maximum memory of {} pages or less",
                MAX_MEMORY_PAGES
            ),
        }
    }
}
impl std::error::Error for Error {}

fn validate_module(module: &Module) -> Result<(), Error> {
    let Some(max_pages) = module.exports().find_map(|e| if e.name() == "memory" {
        e.ty().memory().and_then(|m| m.maximum_pages())
    } else {
        None
    }) else {
        return Err(Error::MaximumMemoryUnspecified)
    };
    if max_pages > MAX_MEMORY_PAGES.into() {
        return Err(Error::MaximumMemoryExceeded(max_pages.into()));
    }
    Ok(())
}

Would there be interest in a PR that provides Store with ResourceLimiter in the spirit of wasmtime? Despite the above, I think it is important that a WASM runtime contain memory usage from a security stance.

Robbepop commented 1 year ago

@huntc Thank you for kindly sharing your solution with others! :)

Would there be interest in a PR that provides Store with ResourceLimiter in the spirit of wasmtime? Despite the above, I think it is important that a WASM runtime contain memory usage from a security stance.

I seriously was not aware of this API so far and thus have not dug into it. We probably want this API in wasmi but I first have to take a look at it.

Robbepop commented 1 year ago

@jayz22 If your questions are resolved I'd be happy if you close the issue or respond with a follow up. :)

Robbepop commented 1 year ago

@huntc I added an issue for ResourceLimiter API for wasmi: https://github.com/paritytech/wasmi/issues/728

jayz22 commented 1 year ago

To answer your question: You want to create an Engine, a Store, a Linker and a Memory. The Memory is given the min and max limits to your wishes. Then you define this Memory instance in the Linker. In Wasm all imports have a module and field namespace. These are just 2 different names, for example a module name could be something like env or std and a field name could be mem in this case, so module: "env", field: "mem" to define your Memory instance in the Linker. Then in your actual Wasm file you need to import this Memory using exactly these 2 names.

@Robbepop Thank you for the detailed explanation. I have a couple of followup questions:

I added an issue for ResourceLimiter API for wasmi: https://github.com/paritytech/wasmi/issues/728

I'm not aware of this API but this looks very promising. If I understood it correctly, this ResourceLimiter will be added to the Store such that any all memory grow, both imported and exported, will be limited? If so that would be exactly what I was looking for!

Robbepop commented 1 year ago

Hey @jayz22,

I am not aware of any Rust compiler or Cargo flags that can be applied to alter the generated Wasm blob. There might be options and settings, although I would not consider using them if they were not marked as stable or production ready.

At Parity we are compiling our ink! smart contracts (written in Rust, compiled to Wasm) via our own custom tool named cargo-contract: https://crates.io/crates/cargo-contract

This tool alters the compilation process for us, applies even more optimizations, e.g. via Binaryen's wasm-opt, precompiles Rust standard library to further decrease Wasm blob size and also performs some mutation on its own on the generated Wasm as well as automatically generating metadata for the ink! smart contract.

Therefore we have everything available to us via our own tooling which is super flexible. Users simply always use cargo-contract to build ink! smart contracts and our promise to our users is that then everything just works out of the box.

jayz22 commented 1 year ago

Thanks @Robbepop, I think this has answered my questions regarding memory limiting/metering. Here are some of my main takeaways which maybe useful for someone else:

(feel free correct me or add to it if necessary)

I'm definitely very interesting in having the ResourceLimiter feature, and will follow up with questions there. Thanks @Robbepop @huntc for all the answers and info!

graydon commented 1 year ago

You need some kind of control. At Parity for example we still pre-analyze incoming Wasm blobs before execution via wasmi for some properties. This way it is possible to use non-imported Memory inherent to the Wasm module as long as the pre-analysis determines that at most N pages are used where N is the set limit. Otherwise the Wasm blob is discarded and never executed.

@Robbepop I just wanted to follow up a bit on this: I went looking for the code that does this so we could at least try to do some similar analysis / filtering and it looks to me like you're talking about this code -- is that what you meant?

If so, it's a little awkward for us to do something similar using wasmi's current API, as indeed that code is not doing, it's parsing the module separately from wasmi, using parity_wasm, which provides access to (say) the memory sections of the module. Since wasmi has subsequently changed away from using parity_wasm for parsing at all, to wasmparser, clients of wasmi are no longer in a position to inspect the memory sections (or other similar structures you limit in that code above, say the various tables).

Would you object to a few additional APIs added to wasmi::Module to facilitate doing this sort of post-parse analysis of the structures of a wasmi::Module directly, before instantiating it, without having to parse twice using parity_wasm?

Robbepop commented 1 year ago

@graydon Hi

I just wanted to follow up a bit on this: I went looking for the code that does this so we could at least try to do some similar analysis / filtering and it looks to me like you're talking about this code -- is that what you meant?

Actually I was referring to code in our ink! build tool: https://github.com/paritytech/cargo-contract/blob/master/crates/build/src/lib.rs#L580

As you can see it is also using parity-wasm at the moment. This is pretty bad since just recently LLVM/Rust enabled some non-MVP features of Wasm by default (that cannot be disabled) which parity-wasm does not support. Generally we think that the ecosystem is currently lacking a proper replacment for parity-wasm and in my personal opinion it was not the best idea to let it rot and not care for Wasm proposals at all but here we are.

In my opinion the ecosystem needs or will need such a tool again that is capable to stay up-to-date with modern Wasm and I do not even think that it would be too hard to build such a tool based on wasmparser, wasm-encoder and wasm-printer which are all crates driven by the BytecodeAlliance and kept up to date by them. However, we haven't found a proper solution for this, yet from what I can tell. @ascjones @athei please correct me if I am wrong.

If so, it's a little awkward for us to do something similar using wasmi's current API, as indeed that code is not doing, it's parsing the module separately from wasmi, using parity_wasm, which provides access to (say) the memory sections of the module.

The separate parsing is not a problem in our own case since it is done at different stages of the smart contract life cycle.

  1. build: cargo-contract inspects and manipulates the .wasm blob that comes out of Rust/LLVM right after compilation. This is done off-chain and kinda automated so users do not have to think about any of this at all. Until now this step was done by parity-wasm.
  2. upload: contracts-pallet basically checks some invariants about .wasm blobs once when they are uploaded to the chain. This is done exactly once and also performed via parity-wasm so far. Additionally contracts-pallet sends the .wasm blob to wasmi to check if wasmi can handle it properly since not only Wasm validation can fail but also certain limitations of wasmi itself could be out of bounds. Mostly malicious contracts won't compile here. In earlier times we also added fuel metering to the .wasm blob at this stage but now/soon wasmi will do this as part of stage 3.
  3. execute: When a contract is executed only wasmi is used to validate the Wasm blob (again) and execute it. Technically we could get rid of the validation phase here but it wouldn't be wise to do so for the sake of correctness and safety. This step is by far executed the most of the 3 stages since a smart contract is usually compiled locally (so no problem), uploaded once onto the chain and executed many times. Therefore optimizations should be done to improve this stage However, in our case this stage is already only making use of wasmi due to the Wasm blob inspections and manipulations at the other 2 stages.

Therefore parity-wasm was used to inspect and manipulate the Wasm and wasmi was used to check for specific wasmi invariants (can wasmi handle it?).

Would you object to a few additional APIs added to wasmi::Module to facilitate doing this sort of post-parse analysis of the structures of a wasmi::Module directly, before instantiating it, without having to parse twice using parity_wasm?

Generally this kind of post-parse analysis is already possible for imported Wasm objects via Module::get_export. This is one of the reasons why we use imported memory which is one of the main purposes of imported Wasm objects: so that the host side can take control over them. So I guess your question is more about inspecting internal Wasm objects. Technically it should be fairly easy to add those APIs. However, I feel like this is going to be a rat's tail: people will obviously depend on wasmi for its ability to inspect a Wasm module and want more and more control and inspectability until the point where wasmi becomes the crate that parity-wasm actually was from a functionality point of view. This is contrary to one of the main selling points of wasmi being a Wasm executor and doing its best to mirroring the Wasmtime API to act as a drop-in replacement in both directions which already many people are using for exactly this purpose.

I am very open to add Wasmtime APIs that do not yet exist in wasmi, for example the ResourceLimiter API. What exactly is not possible with it that was possible if wasmi supported inspection of internal Wasm objects? As a non-technical practical argument against bloating up wasmi is that I am currently working alone on this projects, so there is a natural urge to keep its supported functionality and API surface as minimal as possible while striking a practical balance for users and I found that Wasmtime API mirroring was a decent line here.

If smart contracts on your platform are using internal memory instead of imported memory: what is the reason? Is it historical or purposeful? If it is just historical I guess a decent idea is to performance a phase transition where new smart contracts should always be built using imported memory and old contracts silently get converted to those via a tool such as parity-wasm. So over time the dependency on a tool such as parity-wasm (and its associated functionality to inspect internal Wasm objects) will nullify.

graydon commented 1 year ago

@graydon Hi

Actually I was referring to code in our ink! build tool: https://github.com/paritytech/cargo-contract/blob/master/crates/build/src/lib.rs#L580

Well, ok, but that has to be paired with a component that's in the contract runtime, right? Like someone can upload a contract that wasn't built with that build tool, and you have to detect and reject whatever badness the user chose to upload. That's the component I'm concerned with (am currently writing).

We can theoretically modify the build side to do something different too -- we do have a build wrapper command though we would prefer our contract builds to stay as close to cargo build as possible -- but we still need a way to validate assumptions in the runtime.

In my opinion the ecosystem needs or will need such a tool again that is capable to stay up-to-date with modern Wasm and I do not even think that it would be too hard to build such a tool based on wasmparser, wasm-encoder and wasm-printer which are all crates driven by the BytecodeAlliance and kept up to date by them. However, we haven't found a proper solution for this, yet from what I can tell. @ascjones @athei please correct me if I am wrong.

At the moment I'm just hoping to avoid paying the CPU cost of parsing the code twice using two different libraries just to perform very basic inspection on modules that I can already do in wasmi if wasmi just reveals a little more information about the module it's storing in memory. Wasmi has already parsed it during module construction, the data is right there, it seems a good time to look at the module and reject it if it's bad.

The separate parsing is not a problem in our own case since it is done at different stages of the smart contract life cycle. ... Therefore parity-wasm was used to inspect and manipulate the Wasm and wasmi was used to check for specific wasmi invariants (can wasmi handle it?).

Ok. I .. generally undersand these phases, it's just not really clear to me that I need a lot of information from parity_wasm that the wasmi::Module can't easily provide. To be concrete, I'm mostly just talking about adding, say, an accessor that returns the list of MemoryTypes. Like:

fn get_memories() -> &[MemoryType] { &self.memories }

I was hoping this isn't such a maintenance burden that it'd cause headaches (the wasmi::Module API surface already isn't exactly the same as the wasmtime::Module surface) but if it's too much to ask, I understand.

Generally this kind of post-parse analysis is already possible for imported Wasm objects via Module::get_export.

Hmm, wait, I thought that only lets me get exports? Here you say it lets me get imports.

What I'm concerned about is if the user maliciously includes a definition of a very large, non-exported memory (just to DoS the server) we can't find it that way.

This is one of the reasons why we use imported memory which is one of the main purposes of imported Wasm objects: so that the host side can take control over them. So I guess your question is more about inspecting internal Wasm objects. Technically it should be fairly easy to add those APIs. However, I feel like this is going to be a rat's tail: people will obviously depend on wasmi for its ability to inspect a Wasm module and want more and more control and inspectability until the point where wasmi becomes the crate that parity-wasm actually was from a functionality point of view. This is contrary to one of the main selling points of wasmi being a Wasm executor and doing its best to mirroring the Wasmtime API to act as a drop-in replacement in both directions which already many people are using for exactly this purpose.

I mean, I don't know what to say here -- obviously I can't tell you which goals to prioritize or how to spend maintenance time on the project, so if the answer is "no" I understand. I would define the methods myself as an extension trait and happily maintain them, but Rust is picky about that and won't let me -- they'd need to access fields of wasmi::Module that are not public. The best alternative for me is to just maintain a fork in perpetuity, which is a little unfortunate but well within our own budget. I just thought I'd ask.

I am very open to add Wasmtime APIs that do not yet exist in wasmi, for example the ResourceLimiter API.

Sure, and longer-term I think that's probably the right way for all this to go, and we'll be happy to spend some time helping with that too (if doing so doesn't step on your feet). I'm concerned with shipping a mitigation today that I can accomplish with a single line of code added.

What exactly is not possible with it that was possible if wasmi supported inspection of internal Wasm objects?

Oh, probably nothing. I mean, assuming it gets called during initial memory instantiation -- it's not clear to me if the memory_growing method is called while instantiating memories initially -- it's just a bunch of code that doesn't exist today and I need to do something today. I can maintain it in our fork until then if this isn't an acceptable proposal to upstream to you.

As a non-technical practical argument against bloating up wasmi is that I am currently working alone on this projects, so there is a natural urge to keep its supported functionality and API surface as minimal as possible while striking a practical balance for users and I found that Wasmtime API mirroring was a decent line here.

Of course. I've been in your shoes many times and am 100% not trying to demand your labor on something you're uncomfortable with.

If smart contracts on your platform are using internal memory instead of imported memory: what is the reason?

Three reasons:

  1. It's not what rustc does by default, even forcing it to occur appear to require use of a specific linker and linker flags, so it's one more thing our users can get wrong when building (and/or one more thing we have to enforce by making them use our build wrapper)
  2. As far as I know, it's not compatible with basic wasm allocators -- they want to define and grow their own memory. Maybe I'm wrong here.
  3. Most importantly: even if we make changes to our build system and assume users import a memory we define in our host, we have to check that assumption! Malicious users might still upload a wasm that defines its own gigantic memory, to attack the system. We need a way to check.
Robbepop commented 1 year ago

From what I understood the ResourceLimiter API would solve this problem for you if it already prevented malicious memory growth during Wasm module instantiations. Is that correct? If so I could see myself working on that issue with high priority to get your fix ready asap. You get your feature and wasmi improves its Wasmtime mirroring. :)