Add HostAlignedByteCount to enforce alignment at compile time #9620
Conversation
sunshowers
requested review from
alexcrichton
and removed request for
a team
| let stack_size = self.stack_size.byte_count() - self.page_size.byte_count(); | ||
| let bottom_of_stack = top - stack_size; | ||
| let start_of_stack = bottom_of_stack - self.page_size; | ||
| let start_of_stack = bottom_of_stack - self.page_size.byte_count(); | ||
| assert!(start_of_stack >= base && start_of_stack < (base + len)); | ||
| assert!((start_of_stack - base) % self.stack_size == 0); | ||
| assert!((start_of_stack - base) % self.stack_size.byte_count() == 0); |
There was a problem hiding this comment.
This code and the similar section below would be good to convert over, but I decided to do this in a followup.
sunshowers
force-pushed
the
host-aligned
branch
2 times, most recently
from
a64e06d
to
7546b03
Compare
github-actions
bot
added
the
wasmtime:api
|
I started working on a followup as well: 03efe42 Unfortunately GitHub's review flow does not let you manage stacked PRs unless you have push access to the destination repo, so I'll have to put these up one at a time. Sigh :( |
|
As a heads up I'm adding #9614 to the merge queue which is going to have a number of conflicts with this. It should (hopefully) be just a few |
…maps In the wasmtime codebase there are two kinds of mmaps: those that are always backed by anonymous memory and are always aligned, and those that are possibly file-backed and so the length might be unaligned. In bytecodealliance#9620 I accidentally mixed the two up in a way that was a ticking time bomb. To resolve this, add a type parameter to `Mmap` to distinguish between the cases. All of wasmtime's users are clearly either one or the other, so it's quite simple in the end.
…maps (#9639) In the wasmtime codebase there are two kinds of mmaps: those that are always backed by anonymous memory and are always aligned, and those that are possibly file-backed and so the length might be unaligned. In #9620 I accidentally mixed the two up in a way that was a ticking time bomb. To resolve this, add a type parameter to `Mmap` to distinguish between the cases. All of wasmtime's users are clearly either one or the other, so it's quite simple in the end.
sunshowers
force-pushed
the
host-aligned
branch
2 times, most recently
from
9070ae2
to
ef285a4
Compare
|
(Apologies for the force push -- had to do it as part of the rebase. GH makes me cry) |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
sunshowers
force-pushed
the
host-aligned
branch
from
ef285a4
to
277670b
Compare
|
All right, think I fixed the Miri issue. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
|
||
| // If the Wasm memory's page size is smaller than the host's page | ||
| // size, then we might not need to actually change permissions, | ||
| // since we are forced to round our accessible range up to the | ||
| // host's page size. | ||
| if new_accessible > self.accessible() { | ||
| if let Ok(difference) = new_accessible.checked_sub(self.accessible()) { |
There was a problem hiding this comment.
I think this may be the source of the bug, albeit it's a bit indirect. Previously the > condition is now morally becoming >= since new_accessible == self.accessible() didn't run this block before but it's now running this block with difference == 0. I think then it may be the case that mprotect for 0 bytes is succeeding on unix but failing on Windows.
On Linux I see mprotect(0x740d36001000, 0, PROT_READ|PROT_WRITE) = 0 in strace with this patch when running the failing spec test and while I haven't executed this on Windows this is what I would suspect is going on.
Another option is to update the Mmap type to skip make_accessible when the byte size is zero which I think would also be reasonable.
There was a problem hiding this comment.
Oh that's terrifying -- I'm going to fix this (probably by making make_accessible ignore zero-sized regions) and go through this code again to make sure I didn't miss any other sign changes. Thanks for diagnosing this!
There was a problem hiding this comment.
Made make_accessible a no-op on zero-length allocations, as well as the other mprotect calls (make_executable and make_readonly). They also had this as a lurking bug because they had assertions that range.start <= range.end, not range.start < range.end.
I've also added tests for zero-length calls, and re-reviewed all of the sign changes -- didn't see any other errors.
As part of the work to allow mmaps to be backed by other implementations, I realized that we didn't have any way to track whether a particular usize is host-page-aligned at compile time. Add a `HostAlignedByteCount` which tracks that a particular usize is aligned to the host page size. This also does not expose safe unchecked arithmetic operations, to ensure that overflows always error out. With `HostAlignedByteCount`, a lot of runtime checks can go away thanks to the type-level assertion. In the interest of keeping the diff relatively small, I haven't converted everything over yet. More can be converted over as time permits.
sunshowers
force-pushed
the
host-aligned
branch
from
277670b
to
93e23db
Compare
As part of the work to allow mmaps to be backed by other implementations (#9544), I realized that we didn't have any way to track whether a particular usize is host-page-aligned at compile time.
Add a
HostAlignedByteCountwhich tracks that a particular usize is aligned to the host page size. This also does not expose safe unchecked arithmetic operations, to ensure that overflows are always handled.With
HostAlignedByteCount, a lot of runtime checks can go away thanks to the type-level assertion.In the interest of keeping the diff relatively small, I haven't converted everything over yet. More code can be converted over as we go along.