Wasmer cache can throw panic if cache file has been corrupted

[grishasobol]

wasmer/lib/engine-universal/src/artifact.rs

Lines 85 to 103 in 66c1c00

	/// # Safety
	/// This function is unsafe because rkyv reads directly without validating
	/// the data.
	pub unsafe fn deserialize(
	engine: &UniversalEngine,
	bytes: &[u8],
	) -> Result<Self, DeserializeError> {
	if !UniversalArtifactBuild::is_deserializable(bytes) {
	return Err(DeserializeError::Incompatible(
	"The provided bytes are not wasmer-universal".to_string(),
	));
	}
	let bytes = &bytes[UniversalArtifactBuild::MAGIC_HEADER.len()..];
	let metadata_len = MetadataHeader::parse(bytes)?;
	let metadata_slice: &[u8] = &bytes[MetadataHeader::LEN..][..metadata_len];
	let serializable = SerializableModule::deserialize(metadata_slice)?;
	let artifact = UniversalArtifactBuild::from_serializable(serializable);
	let mut inner_engine = engine.inner_mut();
	Self::from_parts(&mut inner_engine, artifact).map_err(DeserializeError::Compiler)

Hello guys, as I can see in code above, you work with string slice without any checks before. As a result this may cause problems in some cases. For example: if we would corrupt file in compiled wasm cache, then this part of code may throw panic, which is not suitable behaviour. I think the best way is to return error in the case.

[epilys]

Hello, thanks for your ticket!

This function is unsafe for this reason as explained in the doc comment: the byte slice is not validated. I think the best way to deal with the problem you're saying is providing an opt-in validation option.

[grishasobol]

is providing an opt-in validation option.

If you mean - check bytes before calling this method, then it's not the most optimal way. Because then we have to copy some logic from this method and do it before calling it, in order to be sure that it wont throw panic.
Much simple way is if this method returns error in all cases when bytes are in inconsistent state. Even if it's unsafe method =)

[epilys]

I meant a validate: bool argument. You might not want to revalidate an immutable bytes slice after a first validation. Would that satisfy your usecase?

[grishasobol]

I meant a validate: bool argument. You might not want to revalidate an immutable bytes slice after a first validation. Would that satisfy your use case.

Yep, if you will use validate = true by default in wasmer functions which uses this method, or propagate validate argument up for all functions which uses this method.

[epilys]

Propagating the option makes sense to me. We're redesigning the API for the next major release, 3.0, at the moment. So we will take this into consideration, thanks for writing the ticket!

[silwol]

If we strive to follow the API guidelines, I think it makes sense to rather expose a deserialize_unchecked(…) method that may panic, and a deserialize(…) method that does checking and returns a Result<…>. The latter may call the former of course, or both call an additional private function that has a validate: bool parameter - depending on what makes more sense for the implementation.

[epilys]

Tracked in #2925

[epilys]

Actually keeping this open because it might get lost in #2925. But it depends on it.

[syrusakbary]

We need to make sure that deserialize that validates the data and never panics.