alexcrichton opened PR #3766 from better-memory-init
to main
:
This commit consolidates the few locations that we have which are
performing memory initialization. Namely the uffd logic for creating
paged memory as well as the memfd logic for creating a memory image now
share an implementation to avoid duplicating bounds-checks or other
validation conditions. The main purpose of this commit is to fix a
fuzz-bug where a multiplication overflowed. The overflow itself was
benign but it seemed better to fix the overflow in only one place
instead of multiple.The overflow in question is specifically when an initializer is checked
to be statically out-of-bounds and multiplies a memory's minimum size by
the wasm page size, returning the result as au64
. For
memory64-memories of size1 << 48
this multiplication will overflow.
This was actually a preexisting bug with thetry_paged_init
function
which was copied for memfd, but cropped up here since memfd is used more
often than paged initialization. The fix here is to skip validation of
theend
index if the size of memory is1 << 64
since if theend
index can be represented as au64
then it's in-bounds. This is
somewhat of an esoteric case, though, since a memory of minimum size1 << 64
can't ever exist (we can't even ask the os for that much memory,
and even if we could it would fail).<!--
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 #3766 from better-memory-init
to main
.
alexcrichton updated PR #3766 from better-memory-init
to main
.
alexcrichton updated PR #3766 from better-memory-init
to main
.
alexcrichton requested cfallin for a review on PR #3766.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
get_cur_size_pages
orget_cur_size_in_pages
maybe?get_cur_page_size
is otherwise ambiguous to me ("sure, the current page size is 64KiB").
cfallin created PR review comment:
Comments with arg-names (the
/* get_cur_page_size = */ ...
idiom) might be useful here to make clearer what each closure is used for. (Or at least that's a style I like, but subjective, feel free to ignore!)
cfallin created PR review comment:
if !self.module.memory_initialization.is_segmented() { return; }
maybe, with suitable helper on the enum?
cfallin created PR review comment:
Could we have doc-comments here to describe what each return-code means?
cfallin created PR review comment:
Could we avoid the conditional-on-overflow logic by testing for
(end / WASM_PAGE_SIZE) > get_cur_page_size(memory_index)
instead, i.e., doing the comparison in the Wasm-page-count domain?(Or I guess, the above but rounding up, so
((end + WASM_PAGE_SIZE - 1) / WASM_PAGE_SIZE)
, maybe with that wrapped in a helper...)
cfallin created PR review comment:
I wonder if there is a way to refactor this so that we write the segments directly to the memfd -- the advantage of that scheme is that we don't pay RSS cost for zero-filled holes in the middle of the image. Here we will write the whole image into the memfd, including the zeroes, so the kernel (which afaik doesn't check written pages for "all zeroes, can maintain as a hole") needs to keep around all the pages.
cfallin created PR review comment:
Against that, though, is the fact that for a lot of images one big
pwrite
to the memfd is probably faster than a lot of little ones. Maybe worth benchmarking the memfd creation (by continually loading and throwing away aModule
and either instantiating once, or forcing memfd creation eagerly) for some module with a big heap?And also, this approach is much cleaner and easier to maintain, and IIRC the zero-filled holes in the memfd will be filled in by fresh zero-pages the first time someone CoWs each one, so perhaps it's reasonable to just frontload that work once we commit to having a memfd image. Will leave this up to you...
cfallin created PR review comment:
interesting idiom here -- any reason not to do
[offset..(offset+data.len())]
? This is kind of elegant but it took me a second to parse (my brain wanted to see it as a 2D array somehow at first).
alexcrichton created PR review comment:
Ah yeah my hope here was to avoid having to deal with rounding logic and being off-by-one by accident. I think this would work if
end
were rounded up but that's still tricky checked arithmetic to do as well.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I've done this in a few other places in Wasmtime as well but the main benefits in my opinion are:
- Slightly more ergonomic (don't have to repeat the base twice)
- Guaranteed no overflow (no additions which could accidentally overflow)
Although I agree that if it's the first time seeing this it can be a bit odd.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah yeah I forgot to mention this in the PR description, my bad, but I changed this for the main reason if theoretically future-proofing at some point. Eventually implementing https://github.com/bytecodealliance/wasmtime/issues/3758 will entail having doing this logic, all the time, at compile time and storing the
.data
section appropriately. In light of that I figured it'd be good to avoid the memfd dependence in building this image so it can be moved towasmtime-environ
or something like that in the future.Otherwise with https://github.com/bytecodealliance/wasmtime/issues/3758 we'd ideally only need to create the memfd, do a single write, and then start mmap-ing from there.
In terms of the rss cost for this I'm not personally too too worried because we effectively already do this for the paged initialization transition where we get most of the data segments resident in our own rss. If this becomes an issue though we can tweak to improve various metrics though.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh I meant to come back to this but I ended up not using anything but
Ok
for testing, so I went ahead and switched this to abool
.
alexcrichton updated PR #3766 from better-memory-init
to main
.
alexcrichton created PR review comment:
I'm not opposed to such an idiom but personally I feel it's more motivation for a different API (it's a pain otherwise to ensure all call-sites have comments like this), so I brought back the
InitMemory
enum as a way to encapsulate the first two arguments here so there's only one closure-as-argument.
alexcrichton submitted PR review.
alexcrichton updated PR #3766 from better-memory-init
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Wow, nice, the enum-of-named-args is fantastic. Thanks!
cfallin submitted PR review.
cfallin created PR review comment:
Ah, yes, that's a great point; I had forgotten that paged initialization already effectively does this. I think this is definitely reasonable overhead given that.
cfallin submitted PR review.
cfallin created PR review comment:
I'll take this as a "learned a nice Rust idiom from Alex" moment then :-)
cfallin submitted PR review.
alexcrichton merged PR #3766.
Last updated: Dec 23 2024 at 12:05 UTC