Stream: git-wasmtime

Topic: wasmtime / PR #3349 wip: memory grow limiter

Wasmtime GitHub notifications bot (Sep 14 2021 at 18:24):

pchickey opened PR #3349 from pch/limiter to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:18):

pchickey updated PR #3349 from pch/limiter to main.

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton created PR review comment:

This I'm a bit wary of but only because this is part of the public API of the wasmtime crate (since ResourceLimiter is reexported in wasmtime). This honestly seems kinda ok for an implementation though if anyone else really wants to use it, all I'd say that that it should switch to Option<T: ResourceLimiter> instead of having &mut dyn here

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton created PR review comment:

I think that having a different error-per-overflow may be a bit confusing here, but an error message for any overflow all seems reasonable

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton created PR review comment:

I'm a bit wary about saturating here for new_byte_size. I think I'd prefer to keep new_byte_size as Option since it's the precise page-aligned size requested by the wasm, and then when we call the limiter it'd be new_byte_size.unwrap_or(usize::MAX) or something like that.

Otherwise this saturated value could leak into grow_to below which runs a risk of getting confused if the size isn't page-aligned.

Wasmtime GitHub notifications bot (Sep 14 2021 at 21:59):

alexcrichton created PR review comment:

FWIW this is arguably a case that may want to get memory_grow_failed as well. This is akin to "malloc returned NULL" because the base.len() return value is the maximal size of the memory that the host is willing to allocate

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:13):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:13):

pchickey created PR review comment:

usize::MAX is not page aligned though, its 2^32-1 or 2^64-1. I can change this to checked operations that unwrap to usize::MAX, but grow_to will still need a special case.

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:16):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:16):

pchickey created PR review comment:

I decided against this and just defined two auxiliary functions in memory.rs instead.

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:16):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 00:16):

pchickey created PR review comment:

Agreed

Wasmtime GitHub notifications bot (Sep 15 2021 at 01:11):

pchickey updated PR #3349 from pch/limiter to main.

Wasmtime GitHub notifications bot (Sep 15 2021 at 01:30):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 01:30):

pchickey created PR review comment:

Agreed

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:26):

alexcrichton created PR review comment:

I'd ideally prefer to not have a one-off check for something like this and have it fall out of other checks if possible. The risk I'm worried about is that if this is usize::MAX then it flows into Mmap::accessible_reserved which goes straight to the OS and will return an error about this being non-page-aligned. I think it'd be best to either have usize::MAX here truncated to the OS page alignment (which is still an un-satisfiable request) or something like that.

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:26):

alexcrichton created PR review comment:

Mind expanding this a bit to explain that it's an overflow trying to calculate the size of the allocation of the linear memory?

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:26):

alexcrichton created PR review comment:

Along the lines of my comment up above, I think actually these usize::MAX values might be good to replace with absolute_max as calculated in the new function. That way usize::MAX doesn't have to get specifically tested for.

Wasmtime GitHub notifications bot (Sep 15 2021 at 14:27):

alexcrichton created PR review comment:

but is still what?!?!

Wasmtime GitHub notifications bot (Sep 15 2021 at 16:50):

pchickey updated PR #3349 from pch/limiter to main.

Wasmtime GitHub notifications bot (Sep 15 2021 at 17:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2021 at 18:46):

pchickey updated PR #3349 from pch/limiter to main.

Wasmtime GitHub notifications bot (Sep 15 2021 at 18:50):

pchickey updated PR #3349 from pch/limiter to main.

Last updated: Nov 22 2024 at 17:03 UTC