Stream: git-wasmtime

Topic: wasmtime / PR #9571 Add `Module::deserialize_open_file`

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 14:52):

adambratschikaye edited PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 14:58):

adambratschikaye submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 14:58):

adambratschikaye created PR review comment:

Creating this function seems a bit messy and I'm wondering if a cross-platform open is fine for Windows anyway. We do say that deserialize_file is unsafe if the file is modified while in use.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 15:02):

adambratschikaye has marked PR #9571 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 15:02):

adambratschikaye requested wasmtime-core-reviewers for a review on PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 15:02):

adambratschikaye requested fitzgen for a review on PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 15:02):

adambratschikaye requested bjorn3 for a review on PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 17:10):

fitzgen submitted PR review:

LGTM, but I am not sure I understand what you are getting at with regards to windows, so we should resolve that before merging. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 06 2024 at 17:10):

fitzgen created PR review comment:

It isn't clear to me exactly what you are suggesting/asking. However, it seems like you've already written the code to use modes that disallow concurrent modifications while we have the file open, so I don't think it makes sense to remove that, even though we do say that it is unsafe to modify the file while it is in use.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 07:34):

adambratschikaye submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 07:34):

adambratschikaye created PR review comment:

Yeah I was suggesting we remove this additional code since we say it's unsafe to modify the file. But I'm good leaving it in.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 16:26):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 16:26):

alexcrichton created PR review comment:

IIRC this is required on Windows not necessarily for safety but just to get anything working. I have a vague recollection that "just" doing File::open causes some mmap operation down the line to return an error or similar. I could be misremembering though (and CI should turn up any issues if there are some)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 19:05):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 19:05):

fitzgen created PR review comment:

Gotcha. In general, when we can easily and without imposing overhead protect against bad things happening when an unsafe contract is violated, we should do it. So let's leave these bits in, even if it turns out they aren't strictly necessary for correctness.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 21:07):

alexcrichton commented on PR #9571:

Not necessarily obvious to find but the failure is a Windows-specific one

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 09:09):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 09:16):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 09:28):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 09:52):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:03):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:26):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:32):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:32):

bjorn3 created PR review comment:

*already

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:37):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:37):

adambratschikaye updated PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:38):

adambratschikaye submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:38):

adambratschikaye created PR review comment:

fixed

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:54):

adambratschikaye commented on PR #9571:

Yeah @alexcrichton is right - it looks like a simple File::open doesn't work for windows because the open options need to be compatible with the options used when creating the mapping. I also had to fix up the new test for this and I added a comment on deserialize_open_file on how the file should be opened for windows.

Also ran the full CI on the branch now, so the previous windows failure should be good.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 10:54):

adambratschikaye requested fitzgen for a review on PR #9571.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 21:07):

fitzgen submitted PR review:

I think there is a writable vs readable swaparoo in the docs. AFAICT, it is readable and executable, not writable and executable, and if that is true then this is good to merge with that doc comment fixed.

If my understanding is off, and we are actually mapping as writable and executable, then I think we want to revisit what we're doing here, since that is a pretty scary widget to expose!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2024 at 21:07):

fitzgen created PR review comment:

Does it need to be writable? Seems like it should be (and maybe actually is?) readable and executable?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2024 at 09:27):

adambratschikaye submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2024 at 09:27):

adambratschikaye created PR review comment:

In windows we create the mapping with protection PAGE_EXECUTE_WRITECOPY which only requires the file handle was created with FILE_GENERIC_READ and FILE_GENERIC_EXECUTE access (docs here). It looks like write access would only be required if writes were being persisted to the file, but since we use copy-on-write it's not needed. Maybe I should add a link to these windows docs in comment for further clarity?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2024 at 21:03):

alexcrichton commented on PR #9571:

Ah I think Nick is on vacation right now so I'll take this over and flag for merge. I think the last doc bit is fine for now and we can always update it as necessary later on.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2024 at 21:24):

alexcrichton merged PR #9571.

Last updated: Nov 22 2024 at 17:03 UTC