alexcrichton opened PR #2977 from pre-guard
to main
:
This commit implements a safety feature for Wasmtime to place guard
pages before the allocation of all linear memories. Guard pages placed
after linear memories are typically present for performance (at least)
because it can help elide bounds checks. Guard pages before a linear
memory, however, are never strictly needed for performance or features.
The intention of a preceding guard page is to help insulate against bugs
in Cranelift or other code generators, such as CVE-2021-32629.This commit adds a
Config::guard_before_linear_memory
configuration
option, defaulting totrue
, which indicates whether guard pages should
be present both before linear memories as well as afterwards. Guard
regions continue to be controlled by
{static,dynamic}_memory_guard_size
methods.The implementation here affects both on-demand allocated memories as
well as the pooling allocator for memories. For on-demand memories this
adjusts the size of the allocation as well as adjusts the calculations
for the base pointer of the wasm memory. For the pooling allocator this
will place a singular extra guard region at the very start of the
allocation for memories. Since linear memories in the pooling allocator
are contiguous every memory already had a preceding guard region in
memory, it was just the previous memory's guard region afterwards. Only
the first memory needed this extra guard.I've attempted to write some tests to help test all this, but this is
all somewhat tricky to test because the settings are pretty far away
from the actual behavior. I think, though, that the tests added here
should help cover various use cases and help us have confidence in
tweaking the variousConfig
settings beyond their defaults.Note that this also contains a semantic change where
InstanceLimits::memory_reservation_size
has been removed. Instead this
field is now inferred from thestatic_memory_maximum_size
and guard
size settings. This should hopefully remove some duplication in these
settings, canonicalizing on the guard-size/static-size settings as the
way to control memory sizes and virtual reservations.<!--
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.
-->
alexcrichton updated PR #2977 from pre-guard
to main
.
alexcrichton updated PR #2977 from pre-guard
to main
.
alexcrichton updated PR #2977 from pre-guard
to main
.
alexcrichton updated PR #2977 from pre-guard
to main
.
alexcrichton requested peterhuene for a review on PR #2977.
peterhuene submitted PR review.
peterhuene created PR review comment:
This default value of 1 GiB for 32-bit hosts was the reason the pooling allocator didn't use
static_memory_bound
, as there isn't enough address space for a default of 1000 instances (and2
seems like a really silly default for the count as an alternative).Thoughts on this?
peterhuene submitted PR review.
peterhuene created PR review comment:
This name is a little confusing to me (specifically "unguarded"); perhaps simply
mapping_start
?
peterhuene submitted PR review.
peterhuene created PR review comment:
Nit: this looks good, but while we're here can we rename
mapped_bytes
toaccessible_bytes
(seemingly incorrectly named prior to these changes)?
peterhuene created PR review comment:
peterhuene submitted PR review.
peterhuene submitted PR review.
peterhuene created PR review comment:
peterhuene submitted PR review.
peterhuene created PR review comment:
peterhuene submitted PR review.
peterhuene edited PR review comment.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
AFAIK wasmtime doesn't actually even run on any 32-bit platforms right now, or at least we definitely don't test it. Any objection to changing the default in
Tunables
to 10MB like it is here? Only being able to have 2 instances is also a little silly
peterhuene created PR review comment:
I have no objections; worth it certainly if it removes a duplicate configuration point.
peterhuene submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Looking back at this I have no idea what I was thinking
alexcrichton updated PR #2977 from pre-guard
to main
.
alexcrichton merged PR #2977.
uweigand submitted PR review.
uweigand created PR review comment:
This unconditionally adds a SIMD operation, which we don't support on s390x yet, causing the test to fail.
Can we make this part architecture-dependent for now?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh sure yeah, it's also fine to just comment this out since it's not really the most critical part of the test anyway
Last updated: Dec 23 2024 at 12:05 UTC