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.
-->
pchickey updated PR #3349 from pch/limiter
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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 (sinceResourceLimiter
is reexported inwasmtime
). 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 toOption<T: ResourceLimiter>
instead of having&mut dyn
here
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
alexcrichton created PR review comment:
I'm a bit wary about saturating here for
new_byte_size
. I think I'd prefer to keepnew_byte_size
asOption
since it's the precise page-aligned size requested by the wasm, and then when we call the limiter it'd benew_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.
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 thebase.len()
return value is the maximal size of the memory that the host is willing to allocate
pchickey submitted PR review.
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.
pchickey submitted PR review.
pchickey created PR review comment:
I decided against this and just defined two auxiliary functions in memory.rs instead.
pchickey submitted PR review.
pchickey created PR review comment:
Agreed
pchickey updated PR #3349 from pch/limiter
to main
.
pchickey submitted PR review.
pchickey created PR review comment:
Agreed
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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 intoMmap::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 haveusize::MAX
here truncated to the OS page alignment (which is still an un-satisfiable request) or something like that.
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?
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 withabsolute_max
as calculated in thenew
function. That wayusize::MAX
doesn't have to get specifically tested for.
alexcrichton created PR review comment:
but is still what?!?!
pchickey updated PR #3349 from pch/limiter
to main
.
alexcrichton submitted PR review.
pchickey updated PR #3349 from pch/limiter
to main
.
pchickey updated PR #3349 from pch/limiter
to main
.
Last updated: Dec 23 2024 at 12:05 UTC