Skip to content

Commit

Permalink
Rework Module deserialize methods
Browse files Browse the repository at this point in the history
Rework the deserialize/deserialize_checked methods for module
deserialization.

* Make deserialize() methods use artifact validation
* Make checked methods unsafe again, because loading executable memory
  is inherently unsafe
* Rename methods that skip artifact validation to deserialize_unchecked

Closes #3727
  • Loading branch information
theduke committed May 23, 2023
1 parent d6d3a4e commit d184831
Show file tree
Hide file tree
Showing 12 changed files with 138 additions and 70 deletions.
45 changes: 39 additions & 6 deletions lib/api/src/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,24 +135,57 @@ impl Engine {
))
}

#[deprecated(since = "3.2.0")]
#[cfg(all(feature = "sys", not(target_arch = "wasm32")))]
/// Deserializes a WebAssembly module
/// Deserializes a WebAssembly module which was previously serialized with
/// `Module::serialize`.
///
/// NOTE: you should almost always prefer [`Self::deserialize`].
///
/// # Safety
/// See [`Artifact::deserialize_unchecked`].
pub unsafe fn deserialize_unchecked(
&self,
bytes: &[u8],
) -> Result<Arc<Artifact>, DeserializeError> {
Ok(Arc::new(Artifact::deserialize_unchecked(&self.0, bytes)?))
}

#[cfg(all(feature = "sys", not(target_arch = "wasm32")))]
/// Deserializes a WebAssembly module which was previously serialized with
/// `Module::serialize`.
///
/// The serialized content must represent a serialized WebAssembly module.
/// # Safety
/// See [`Artifact::deserialize`].
pub unsafe fn deserialize(&self, bytes: &[u8]) -> Result<Arc<Artifact>, DeserializeError> {
Ok(Arc::new(Artifact::deserialize(&self.0, bytes)?))
}

#[deprecated(since = "3.2.0")]
#[cfg(all(feature = "sys", not(target_arch = "wasm32")))]
/// Deserializes a WebAssembly module from a path
/// Load a serialized WebAssembly module from a file and deserialize it.
///
/// NOTE: you should almost always prefer [`Self::deserialize_from_file`].
///
/// # Safety
/// See [`Artifact::deserialize_unchecked`].
pub unsafe fn deserialize_from_file_unchecked(
&self,
file_ref: &Path,
) -> Result<Arc<Artifact>, DeserializeError> {
let mut file = std::fs::File::open(file_ref)?;
let mut buffer = Vec::new();
// read the whole file
file.read_to_end(&mut buffer)?;
Ok(Arc::new(Artifact::deserialize_unchecked(
&self.0,
buffer.as_slice(),
)?))
}

#[cfg(all(feature = "sys", not(target_arch = "wasm32")))]
/// Load a serialized WebAssembly module from a file and deserialize it.
///
/// The file's content must represent a serialized WebAssembly module.
/// # Safety
/// See [`Artifact::deserialize`].
pub unsafe fn deserialize_from_file(
&self,
file_ref: &Path,
Expand Down
10 changes: 5 additions & 5 deletions lib/api/src/js/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ impl Module {
));
}

pub unsafe fn deserialize(
pub unsafe fn deserialize_unchecked(
_engine: &impl AsEngineRef,
_bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
Expand All @@ -239,7 +239,7 @@ impl Module {
return Err(DeserializeError::Generic("You need to enable the `js-serializable-module` feature flag to deserialize a `Module`".to_string()));
}

pub fn deserialize_checked(
pub unsafe fn deserialize(
_engine: &impl AsEngineRef,
_bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
Expand All @@ -251,20 +251,20 @@ impl Module {
return Err(DeserializeError::Generic("You need to enable the `js-serializable-module` feature flag to deserialize a `Module`".to_string()));
}

pub unsafe fn deserialize_from_file(
pub unsafe fn deserialize_from_file_unchecked(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
let bytes = std::fs::read(path.as_ref())?;
Self::deserialize(engine, bytes)
}

pub fn deserialize_from_file_checked(
pub unsafe fn deserialize_from_file(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
let bytes = std::fs::read(path.as_ref())?;
Self::deserialize_checked(engine, bytes)
Self::deserialize(engine, bytes)
}

pub fn set_name(&mut self, name: &str) -> bool {
Expand Down
12 changes: 6 additions & 6 deletions lib/api/src/jsc/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,35 +183,35 @@ impl Module {
));
}

pub unsafe fn deserialize(
pub unsafe fn deserialize_unchecked(
_engine: &impl AsEngineRef,
_bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
return Self::from_binary(_engine, &_bytes.into_bytes())
.map_err(|e| DeserializeError::Compiler(e));
}

pub fn deserialize_checked(
pub unsafe fn deserialize(
_engine: &impl AsEngineRef,
_bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
unimplemented!();
}

pub unsafe fn deserialize_from_file(
pub unsafe fn deserialize_from_file_unchecked(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
let bytes = std::fs::read(path.as_ref())?;
Self::deserialize(engine, bytes)
Self::deserialize_unchecked(engine, bytes)
}

pub fn deserialize_from_file_checked(
pub unsafe fn deserialize_from_file(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
let bytes = std::fs::read(path.as_ref())?;
Self::deserialize_checked(engine, bytes)
Self::deserialize(engine, bytes)
}

pub fn set_name(&mut self, name: &str) -> bool {
Expand Down
42 changes: 27 additions & 15 deletions lib/api/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ impl Module {
Ok(())
}

/// Deserializes a serialized Module binary into a `Module`.
/// Deserializes a serialized module binary into a `Module`.
///
/// Note: You should usually prefer the safe [`Module::deserialize_checked`].
/// Note: You should usually prefer the safer [`Module::deserialize`].
///
/// # Important
///
Expand Down Expand Up @@ -250,11 +250,13 @@ impl Module {
/// # Ok(())
/// # }
/// ```
pub unsafe fn deserialize(
pub unsafe fn deserialize_unchecked(
engine: &impl AsEngineRef,
bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
Ok(Self(module_imp::Module::deserialize(engine, bytes)?))
Ok(Self(module_imp::Module::deserialize_unchecked(
engine, bytes,
)?))
}

/// Deserializes a serialized Module binary into a `Module`.
Expand All @@ -272,17 +274,21 @@ impl Module {
/// # use wasmer::*;
/// # fn main() -> anyhow::Result<()> {
/// # let mut store = Store::default();
/// let module = Module::deserialize_checked(&store, serialized_data)?;
/// let module = Module::deserialize(&store, serialized_data)?;
/// # Ok(())
/// # }
/// ```
pub fn deserialize_checked(
///
/// # Safety
/// This function is inherently **unsafe**, because it loads executable code
/// into memory.
/// The loaded bytes must be trusted to contain a valid artifact previously
/// built with [`Self::serialize`].
pub unsafe fn deserialize(
engine: &impl AsEngineRef,
bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
Ok(Self(module_imp::Module::deserialize_checked(
engine, bytes,
)?))
Ok(Self(module_imp::Module::deserialize(engine, bytes)?))
}

/// Deserializes a a serialized Module located in a `Path` into a `Module`.
Expand All @@ -298,37 +304,43 @@ impl Module {
/// # Ok(())
/// # }
/// ```
pub fn deserialize_from_file_checked(
///
/// # Safety
///
/// See [`Self::deserialize`].
pub unsafe fn deserialize_from_file(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
Ok(Self(module_imp::Module::deserialize_from_file_checked(
Ok(Self(module_imp::Module::deserialize_from_file(
engine, path,
)?))
}

/// Deserializes a a serialized Module located in a `Path` into a `Module`.
/// > Note: the module has to be serialized before with the `serialize` method.
///
/// You should usually prefer the safer [`Module::deserialize_from_file`].
///
/// # Safety
///
/// Please check [`Module::deserialize`].
/// Please check [`Module::deserialize_unchecked`].
///
/// # Usage
///
/// ```ignore
/// # use wasmer::*;
/// # let mut store = Store::default();
/// # fn main() -> anyhow::Result<()> {
/// let module = Module::deserialize_from_file(&store, path)?;
/// let module = Module::deserialize_from_file_unchecked(&store, path)?;
/// # Ok(())
/// # }
/// ```
pub unsafe fn deserialize_from_file(
pub unsafe fn deserialize_from_file_unchecked(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
Ok(Self(module_imp::Module::deserialize_from_file(
Ok(Self(module_imp::Module::deserialize_from_file_unchecked(
engine, path,
)?))
}
Expand Down
16 changes: 6 additions & 10 deletions lib/api/src/sys/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl Module {
self.artifact.serialize().map(|bytes| bytes.into())
}

pub unsafe fn deserialize(
pub unsafe fn deserialize_unchecked(
engine: &impl AsEngineRef,
bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
Expand All @@ -78,20 +78,16 @@ impl Module {
Ok(Self::from_artifact(artifact))
}

pub fn deserialize_checked(
pub unsafe fn deserialize(
engine: &impl AsEngineRef,
bytes: impl IntoBytes,
) -> Result<Self, DeserializeError> {
let bytes = bytes.into_bytes();
let artifact = engine
.as_engine_ref()
.engine()
.0
.deserialize_checked(&bytes)?;
let artifact = engine.as_engine_ref().engine().0.deserialize(&bytes)?;
Ok(Self::from_artifact(artifact))
}

pub unsafe fn deserialize_from_file(
pub unsafe fn deserialize_from_file_unchecked(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
Expand All @@ -103,15 +99,15 @@ impl Module {
Ok(Self::from_artifact(artifact))
}

pub fn deserialize_from_file_checked(
pub unsafe fn deserialize_from_file(
engine: &impl AsEngineRef,
path: impl AsRef<Path>,
) -> Result<Self, DeserializeError> {
let artifact = engine
.as_engine_ref()
.engine()
.0
.deserialize_from_file_checked(path.as_ref())?;
.deserialize_from_file(path.as_ref())?;
Ok(Self::from_artifact(artifact))
}

Expand Down
2 changes: 1 addition & 1 deletion lib/cache/src/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl Cache for FileSystemCache {
key.to_string()
};
let path = self.path.join(filename);
let ret = Module::deserialize_from_file_checked(engine, path.clone());
let ret = Module::deserialize_from_file(engine, path.clone());
if ret.is_err() {
// If an error occurs while deserializing then we can not trust it anymore
// so delete the cache file
Expand Down
2 changes: 1 addition & 1 deletion lib/cli/src/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ impl RunWithPathBuf {
if wasmer_compiler::Artifact::is_deserializable(&contents) {
let engine = wasmer_compiler::EngineBuilder::headless();
let store = Store::new(engine);
let module = Module::deserialize_from_file_checked(&store, &self.path)?;
let module = unsafe { Module::deserialize_from_file(&store, &self.path)? };
return Ok((store, module));
}
let (store, compiler_type) = self.store.get_store()?;
Expand Down
26 changes: 18 additions & 8 deletions lib/compiler/src/engine/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,15 @@ impl Artifact {
))
}

/// Deserialize a ArtifactBuild
/// Deserialize a serialized artifact.
///
/// # Safety
/// This function is unsafe because rkyv reads directly without validating
/// the data.
pub fn deserialize_checked(engine: &Engine, bytes: &[u8]) -> Result<Self, DeserializeError> {
/// This function loads executable code into memory.
/// You must trust the loaded bytes to be valid for the chosen engine and
/// for the host CPU architecture.
/// In contrast to [`Self::deserialize_unchecked`] the artifact layout is
/// validated, which increases safety.
pub unsafe fn deserialize(engine: &Engine, bytes: &[u8]) -> Result<Self, DeserializeError> {
if !ArtifactBuild::is_deserializable(bytes) {
return Err(DeserializeError::Incompatible(
"Magic header not found".to_string(),
Expand All @@ -161,18 +164,25 @@ impl Artifact {
let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len())?;
let metadata_slice = Self::get_byte_slice(metadata_slice, 0, metadata_len)?;

let serializable = SerializableModule::deserialize_checked(metadata_slice)?;
let serializable = SerializableModule::deserialize(metadata_slice)?;
let artifact = ArtifactBuild::from_serializable(serializable);
let mut inner_engine = engine.inner_mut();
Self::from_parts(&mut inner_engine, artifact, engine.target())
.map_err(DeserializeError::Compiler)
}

/// Deserialize a ArtifactBuild
/// Deserialize a serialized artifact.
///
/// NOTE: You should prefer [`Self::deserialize`].
///
/// # Safety
/// This function is unsafe because rkyv reads directly without validating the data.
pub unsafe fn deserialize(engine: &Engine, bytes: &[u8]) -> Result<Self, DeserializeError> {
/// See [`Self::deserialize`].
/// In contrast to the above, this function skips artifact layout validation,
/// which increases the risk of loading invalid artifacts.
pub unsafe fn deserialize_unchecked(
engine: &Engine,
bytes: &[u8],
) -> Result<Self, DeserializeError> {
if !ArtifactBuild::is_deserializable(bytes) {
let static_artifact = Self::deserialize_object(engine, bytes);
match static_artifact {
Expand Down
Loading

0 comments on commit d184831

Please sign in to comment.