Validate Module Artifacts (CLI + WASIX ModuleCache)

[theduke]

• wasix: Use validation when loading serialized modules
• cli: Use validation when loading module artifacts

[syrusakbary]

This is not solving the caching problem in the proper way.

We should be fixing this function:
https://github.com/wasmerio/wasmer/blob/master/lib/types/src/serialize.rs#L103-L118

To use: rkyv::check_archived_value instead (note that you may need to enable the validation feature in rkyv)

[ptitSeb]

I think this #3695 is relevant here.

[theduke]

Important modifications:

commit d4c65d3 (HEAD -> wasix-no-unsafe-archive, origin/wasix-no-unsafe-archive)
Author: Christoph Herzog [email protected]
Date: Thu Mar 30 15:36:00 2023 +0200

Switch wasi module cache and CLI to use validated module deserialization

Prevents undefined behaviour when loading modules.

This is a much saner default option, since loading modules without
validation can cause UB.

commit 6c7d172
Author: Christoph Herzog [email protected]
Date: Thu Mar 30 15:32:51 2023 +0200

feat: Implement validation for serialized modules

Implement validation of rykv serialized modules with the "validation"
feature which uses bytecheck.

Bumps the MAGIC VERSION of the serialization header , because some
format changes were required.
(mostly setting an explicit repr() for enums)

Also updates various APIs to add unsafe methods as
"dangerous_deserialize_unchecked", deprecate the old unsafe "deserialize" methods
and to add new "deserialize_checked" methods

[theduke]

Also enabled rkyv strict mode by default, as requested in #3695 .

I think we want this on by default.

[ptitSeb]

I think the dangerous_ prefix is too much. The _checked/_uncheckedis enough I think (especialy since it was "unchecked" since the beggining...

[theduke]

I rather like the dangerous_ prefix, because it really makes the user notice "hey, really don't do this unless you are sure you want it".

But I don't mind much either way.
Thoughts, @syrusakbary ?

[syrusakbary]

I have a better take. Rather than making deserialize unsafe and deprecated, we will make deserialize to call deserialize_checked and deserialize_unchecked as dangerous_deserialize_unchecked. Thoughts @theduke ?

[theduke]

I don't think we want to change the behaviour of an existing funciton in a minor bump.

I'll just revert the renames and we can tackle that for 4.0.

Properly implemented now.

[ptitSeb]

This is quite the oposite as the "dangerous_" one now. I would have created _unchecked high level variant with the current code, left the current one to just call the _unchecked and mark the current one as deprecatedand to use eithet _unchecked or _checked (to realy introduce the new versions)
Else, I'm unsure many will une the newly _checked version.

But I'm fine with current version, is just that I'm unsure it's enough this time.

[theduke]

Syrus didn't want to deprecate methods, and I think adding new methods without deprecating the old ones makes the API too confusing and messy.

[ptitSeb]

fine with me

[theduke]

4.0 shouldn't be too far off anyway.

[ptitSeb]

4.0 shouldn't be too far off anyway.

Yes, but 4.0 is too late to remove function if they have not been marked as deprecated before.
That means it's for the 5.0 for now.