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

Add option to define maximum memory #417

Closed
inkeliz opened this issue Mar 30, 2022 · 6 comments · Fixed by #419
Closed

Add option to define maximum memory #417

inkeliz opened this issue Mar 30, 2022 · 6 comments · Fixed by #419

Comments

@inkeliz
Copy link
Contributor

inkeliz commented Mar 30, 2022

Currently, seems that any code can increase the memory without any soft-limit. That means that one WASM with for { r = append(r, make([]byte, 10_000)...) } will run and will take, eventually, all the memory available.

I would like to limit each runtime/wasm to have ~10MB, for example. Then the wasm must panic/crash due to "Out-Of-Memory" and the grow call must fail.

Also, would be interesting to limit the CPU usage, but I think it's hard to track the CPU usage, that could be more useful when WASM supports thread directly.

@inkeliz inkeliz changed the title Add option to maximum memory/cpu Add option to define maximum memory/cpu Mar 30, 2022
@codefromthecrypt
Copy link
Contributor

I suspect that growing to memory.max (pages aka 64KB/page) is enforced per module. So, I suppose what you mean is to have a runtime config for a max for any instantiated module. Since memory so often leaves max undefined[1], it may be the case that this has to fail at runtime instead of instantiation time. Do I understand correctly?

[1] https://www.w3.org/TR/wasm-core-1/#memory-types%E2%91%A0

@codefromthecrypt
Copy link
Contributor

Another way, if you aren't using multiple modules per runtime, is to allow a configuration of memory.max when it wasn't set by the source wasm. This would be simpler and have the same effect.

@codefromthecrypt
Copy link
Contributor

ex.

m, err := r.CompileModule(source)
m.WithMemoryMax(5) // 5*64KiB max unless the existing max was less.

@inkeliz
Copy link
Contributor Author

inkeliz commented Mar 30, 2022

I suspect that growing to memory.max (pages aka 64KB/page) is enforced per module. So, I suppose what you mean is to have a runtime config for a max for any instantiated module. Since memory so often leaves max undefined[1], it may be the case that this has to fail at runtime instead of instantiation time. Do I understand correctly?

[1] https://www.w3.org/TR/wasm-core-1/#memory-types%E2%91%A0

I think you get the idea. I would like to enforce that any code can only take ~10MB, for example. It either crashes on initialization (when the minimum memory is larger than the maximum) or during runtime (when calling grow).


I did one test modifying the internal/wasm/memory:

MemoryMaxPages = MemoryPageSize

Modifying to MemoryMaxPages = uint32(192) seems to force my wasm to crash when using ~8MB, which is expected. So, it seems to work. However, that package is internal/private.

@codefromthecrypt
Copy link
Contributor

So, the limit is applied per-memory and since there can only be one memory per module, the simplest and cheapest way is to limit this per module. I'll raise a configuration to backfill memory.max via runtimeconfig with notes on this later today. thanks for the elaboration!

codefromthecrypt pushed a commit that referenced this issue Mar 30, 2022
This allows users to reduce the memory limit per module below 4 Gi. This
is often needed because Wasm routinely leaves off the max, which implies
spec max (4 Gi). This uses Ki Gi etc in error messages because the spec
chooses to, though we can change to make it less awkward.

This also fixes an issue where we instantiated an engine inside config.

Fixes #417

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor

#419

@codefromthecrypt codefromthecrypt changed the title Add option to define maximum memory/cpu Add option to define maximum memory Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants