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

feat: when deserialization of module from filesystem cache fails, remove from cache #138

Merged
merged 15 commits into from
Jan 7, 2025

Conversation

mattyg
Copy link
Member

@mattyg mattyg commented Dec 24, 2024

Resolves #137

Merging into feat/rm-serialized-module-cache for a clean diff.

I'm not sure if this is the right approach, so let me know if you think differently.

/// Wasmer failed to serialize a Module to bytes.
ModuleSerialize(String),
/// Wasmer failed to deserialize a Module from bytes.
ModuleDeserialize(String),
Copy link
Member Author

Choose a reason for hiding this comment

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

Additional error types for clarity

crates/host/src/module.rs Outdated Show resolved Hide resolved
crates/host/src/module/wasmer_sys.rs Show resolved Hide resolved
crates/host/src/module/wasmer_sys.rs Outdated Show resolved Hide resolved
crates/host/src/module.rs Outdated Show resolved Hide resolved
unsafe { Module::deserialize(&self.runtime_engine, serialized_module.clone()) };

// If deserialization fails, we assume the file is corrupt,
// so it is removed from the filesystem cache.
Copy link
Member

Choose a reason for hiding this comment

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

Will a module that deserializes always run? With a binary, there are interlinked references and it should break if you chop off the end. Is that true with these WASM binaries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into it briefly it looks like a wasm binary is made up of pre-defined "sections". Each section starts with an identifier for the section, and then the size of the contents of that section. The wasm is validated before it is executed, so it would catch that a section is not its expected size and fail to run.

It looks there is validation of the "artifact layout" during deserialization in wasmer -- its not clear to me exactly what this is doing though, or if further validation occurs before execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that wasmer exposes a "validate" function on Module -- maybe worth digging in a bit deeper to make sure we are validating wasms if necessary: https://docs.rs/wasmer/latest/wasmer/struct.Module.html#method.validate

crates/host/src/module/wasmer_sys.rs Outdated Show resolved Hide resolved
Base automatically changed from feat/rm-serialized-module-cache to main January 7, 2025 20:16
@mattyg mattyg merged commit dbbda5b into main Jan 7, 2025
10 checks passed
@mattyg mattyg deleted the fix/corrupted-cached-file branch January 7, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Purge serialized wasm from filesystem cache if deserialization fails
3 participants