Stream: git-wasmtime

Topic: wasmtime / PR #2945 make Module::deserialize's version ch...

Wasmtime GitHub notifications bot (May 27 2021 at 22:18):

pchickey opened PR #2945 from pch/validate_module_version to main:

Based on #2897

A SerializedModule contains the CARGO_PKG_VERSION string, which is
checked for equality when loading. This is a great guard-rail but
some users may want to disable this check (e.g. so they can implement
their own versioning scheme)

<!--

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.
-->

Wasmtime GitHub notifications bot (May 27 2021 at 23:01):

pchickey edited PR #2945 from pch/validate_module_version to main:

Based on #2897 - draft until that PR lands.

A SerializedModule contains the CARGO_PKG_VERSION string, which is
checked for equality when loading. This is a great guard-rail but
some users may want to disable this check (e.g. so they can implement
their own versioning scheme)

<!--

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.
-->

Wasmtime GitHub notifications bot (May 27 2021 at 23:03):

pchickey requested alexcrichton for a review on PR #2945.

Wasmtime GitHub notifications bot (May 28 2021 at 08:30):

bjorn3 created PR review comment:

I think the check_version = false version should be unsafe as it is much easier to get wasmtime to interpret caches from a different wasmtime version to be interpreted incorrectly in such a way that you get UB.

Wasmtime GitHub notifications bot (May 28 2021 at 08:30):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 28 2021 at 14:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 28 2021 at 14:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 28 2021 at 14:54):

alexcrichton created PR review comment:

I disagree, the deserialize function is already unsafe, so this should just add caveat there to the documentation.

Wasmtime GitHub notifications bot (May 28 2021 at 14:54):

alexcrichton created PR review comment:

To bikeshed this slightly, this sounds like it's dealing with the wasm binary's version header in one way or another so I think we want to scope this differently perhaps. Some other possibilities:

• precompiled_module_emit_version
• deserialize_check_wasmtime_verison
• omit_wasmtime_version

or... others?

Wasmtime GitHub notifications bot (May 28 2021 at 17:00):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 28 2021 at 17:00):

pchickey created PR review comment:

I didn't mess with the serialization side because I figured it only changed behavior at deserialization, so it wasn't harmful to leave a version string in the binary that we didn't care about. But it is easy enough to turn it off emitting it if you think it is useful (maybe for someone who wants to binary diff modules across different builds?)

I like deserialize_check_wasmtime_version as a name for what is implemented - module has a lot of meanings so that is more clear.

Wasmtime GitHub notifications bot (May 28 2021 at 23:53):

pchickey updated PR #2945 from pch/validate_module_version to main.

Wasmtime GitHub notifications bot (May 28 2021 at 23:54):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 28 2021 at 23:54):

pchickey created PR review comment:

renamed c27e152

Wasmtime GitHub notifications bot (Jun 01 2021 at 14:44):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 01 2021 at 14:44):

alexcrichton created PR review comment:

Nah yeah this seems fine to have for now. Can you add a test for this as well?

Wasmtime GitHub notifications bot (Jun 01 2021 at 21:23):

pchickey updated PR #2945 from pch/validate_module_version to main.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:06):

pchickey updated PR #2945 from pch/validate_module_version to main.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:06):

pchickey has marked PR #2945 as ready for review.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2021 at 23:35):

pchickey updated PR #2945 from pch/validate_module_version to main.

Wasmtime GitHub notifications bot (Jun 04 2021 at 17:27):

pchickey updated PR #2945 from pch/validate_module_version to main.

Wasmtime GitHub notifications bot (Jun 04 2021 at 19:18):

alexcrichton merged PR #2945.

Last updated: Dec 23 2024 at 13:07 UTC