Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make Module::deserialize's version check optional via Config #2945

Merged
merged 6 commits into from
Jun 4, 2021

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented May 27, 2021

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)

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator wasmtime:c-api Issues pertaining to the C API. labels May 27, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@pchickey pchickey marked this pull request as draft May 27, 2021 23:01
@pchickey pchickey requested a review from alexcrichton May 27, 2021 23:03
@@ -329,7 +329,7 @@ impl<'a> SerializedModule<'a> {
Ok(bytes)
}

pub fn from_bytes(bytes: &[u8]) -> Result<Self> {
pub fn from_bytes(bytes: &[u8], check_version: bool) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me! I was thinking as well though that we'd just omit the version header entirely on serialization too?

@@ -329,7 +329,7 @@ impl<'a> SerializedModule<'a> {
Ok(bytes)
}

pub fn from_bytes(bytes: &[u8]) -> Result<Self> {
pub fn from_bytes(bytes: &[u8], check_version: bool) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved
pchickey added 3 commits June 3, 2021 15:04
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)
@pchickey pchickey force-pushed the pch/validate_module_version branch from 9d15c09 to c270b39 Compare June 3, 2021 22:06
@pchickey pchickey marked this pull request as ready for review June 3, 2021 22:06
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton merged commit 895ee2b into main Jun 4, 2021
@alexcrichton alexcrichton deleted the pch/validate_module_version branch June 4, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants