From e2daf4f5970b3e329c3279499db1ab1fa3909d15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 14:39:28 +0200 Subject: [PATCH 01/10] Test metadata slice len before accessing to prevent panic --- lib/compiler/src/engine/artifact.rs | 25 ++++++++++++++++++++++++- lib/types/src/error.rs | 8 ++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 2608723a037..f362ed4de83 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -116,9 +116,32 @@ impl Artifact { "The provided bytes are not wasmer-universal".to_string(), )); } + + if bytes.len() < ArtifactBuild::MAGIC_HEADER.len() { + return Err(DeserializeError::InvalidByteLength { + expected: ArtifactBuild::MAGIC_HEADER.len(), + got: bytes.len(), + }); + } + let bytes = &bytes[ArtifactBuild::MAGIC_HEADER.len()..]; let metadata_len = MetadataHeader::parse(bytes)?; - let metadata_slice: &[u8] = &bytes[MetadataHeader::LEN..][..metadata_len]; + if bytes.len() < MetadataHeader::LEN { + return Err(DeserializeError::InvalidByteLength { + expected: MetadataHeader::LEN, + got: bytes.len(), + }); + } + + let metadata_slice: &[u8] = &bytes[MetadataHeader::LEN..]; + if metadata_slice.len() < metadata_len { + return Err(DeserializeError::InvalidByteLength { + expected: metadata_len + MetadataHeader::LEN, + got: bytes.len(), + }); + } + + let metadata_slice: &[u8] = &metadata_slice[..metadata_len]; let serializable = SerializableModule::deserialize(metadata_slice)?; let artifact = ArtifactBuild::from_serializable(serializable); let mut inner_engine = engine.inner_mut(); diff --git a/lib/types/src/error.rs b/lib/types/src/error.rs index 0e0a1eb12fd..8f99f9419aa 100644 --- a/lib/types/src/error.rs +++ b/lib/types/src/error.rs @@ -35,6 +35,14 @@ pub enum DeserializeError { /// trying to allocate the required resources. #[error(transparent)] Compiler(#[from] CompileError), + /// Input artifact bytes have an invalid length + #[error("invalid input bytes: expected {expected} bytes, got {got}")] + InvalidByteLength { + /// How many bytes were expected + expected: usize, + /// How many bytes the artifact contained + got: usize, + } } /// Error type describing things that can go wrong when operating on Wasm Memories. From 9ea5c941b4ed5ffd927ada37fd88a0713e7f142d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 14:56:07 +0200 Subject: [PATCH 02/10] Prevent more panics in Artifact::deserialize --- lib/compiler/src/engine/artifact.rs | 86 +++++++++++++++++++++++++---- lib/types/src/error.rs | 2 +- 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index f362ed4de83..6446ee05ed6 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -132,7 +132,7 @@ impl Artifact { got: bytes.len(), }); } - + let metadata_slice: &[u8] = &bytes[MetadataHeader::LEN..]; if metadata_slice.len() < metadata_len { return Err(DeserializeError::InvalidByteLength { @@ -140,7 +140,7 @@ impl Artifact { got: bytes.len(), }); } - + let metadata_slice: &[u8] = &metadata_slice[..metadata_len]; let serializable = SerializableModule::deserialize(metadata_slice)?; let artifact = ArtifactBuild::from_serializable(serializable); @@ -596,6 +596,26 @@ impl Artifact { )) } + fn get_byte_slice(input: &[u8], start: usize, end: usize) -> Option<&[u8]> { + if start == end && input.len() > start { + return Some(&input[start..end]); + } else if start < end && input.len() > start && input.len() >= end { + Some(&input[start..end]) + } else { + None + } + } + + fn get_byte_slice_mut(input: &mut [u8], start: usize, end: usize) -> Option<&mut [u8]> { + if start == end && input.len() > start { + return Some(&mut input[start..end]); + } else if start < end && input.len() > start && input.len() >= end { + Some(&mut input[start..end]) + } else { + None + } + } + /// Deserialize a ArtifactBuild from an object file /// /// # Safety @@ -606,15 +626,27 @@ impl Artifact { bytes: &[u8], ) -> Result { let metadata_len = MetadataHeader::parse(bytes)?; + let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len()).ok_or( + DeserializeError::InvalidByteLength { + expected: MetadataHeader::LEN, + got: bytes.len(), + }, + )?; - let metadata_slice = &bytes[MetadataHeader::LEN..][..metadata_len]; let metadata: ModuleMetadata = ModuleMetadata::deserialize(metadata_slice)?; const WORD_SIZE: usize = mem::size_of::(); let mut byte_buffer = [0u8; WORD_SIZE]; let mut cur_offset = MetadataHeader::LEN + metadata_len; - byte_buffer[0..WORD_SIZE].clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + + let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; + + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let num_finished_functions = usize::from_ne_bytes(byte_buffer); @@ -626,8 +658,15 @@ impl Artifact { // read finished functions in order now... for _i in 0..num_finished_functions { - byte_buffer[0..WORD_SIZE] - .clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + let byte_buffer_slice = + Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( + DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + }, + )?; + + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; @@ -648,12 +687,24 @@ impl Artifact { // read trampolines in order let mut finished_function_call_trampolines = PrimaryMap::new(); - byte_buffer[0..WORD_SIZE].clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + + let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; + byte_buffer[0..WORD_SIZE].clone_from_slice(&byte_buffer_slice); cur_offset += WORD_SIZE; let num_function_trampolines = usize::from_ne_bytes(byte_buffer); for _ in 0..num_function_trampolines { - byte_buffer[0..WORD_SIZE] - .clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + let byte_buffer_slice = + Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( + DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + }, + )?; + byte_buffer[0..WORD_SIZE].clone_from_slice(&byte_buffer_slice); cur_offset += WORD_SIZE; let trampoline_ptr_bytes = usize::from_ne_bytes(byte_buffer); let trampoline = mem::transmute::(trampoline_ptr_bytes); @@ -663,12 +714,23 @@ impl Artifact { // read dynamic function trampolines in order now... let mut finished_dynamic_function_trampolines = PrimaryMap::new(); - byte_buffer[0..WORD_SIZE].clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let num_dynamic_trampoline_functions = usize::from_ne_bytes(byte_buffer); for _i in 0..num_dynamic_trampoline_functions { - byte_buffer[0..WORD_SIZE] - .clone_from_slice(&bytes[cur_offset..(cur_offset + WORD_SIZE)]); + let byte_buffer_slice = + Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( + DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + }, + )?; + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; diff --git a/lib/types/src/error.rs b/lib/types/src/error.rs index 8f99f9419aa..e9b8dcd9e07 100644 --- a/lib/types/src/error.rs +++ b/lib/types/src/error.rs @@ -42,7 +42,7 @@ pub enum DeserializeError { expected: usize, /// How many bytes the artifact contained got: usize, - } + }, } /// Error type describing things that can go wrong when operating on Wasm Memories. From 6988025cd0fc9e0d42ce5852fef2f9e5e8f200d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 15:24:13 +0200 Subject: [PATCH 03/10] Fix make lint --- lib/cli/src/c_gen/mod.rs | 4 +- .../src/artifact_builders/artifact_builder.rs | 2 +- lib/compiler/src/engine/artifact.rs | 67 +++++++------------ lib/emscripten/src/lib.rs | 2 +- lib/types/src/error.rs | 2 +- lib/types/src/serialize.rs | 2 +- lib/vm/src/instance/mod.rs | 4 +- lib/vm/src/libcalls.rs | 26 +++---- lib/wasi/src/syscalls/mod.rs | 6 +- 9 files changed, 50 insertions(+), 65 deletions(-) diff --git a/lib/cli/src/c_gen/mod.rs b/lib/cli/src/c_gen/mod.rs index addc30a8c76..dbb56490a9e 100644 --- a/lib/cli/src/c_gen/mod.rs +++ b/lib/cli/src/c_gen/mod.rs @@ -125,7 +125,7 @@ impl CType { #[allow(clippy::borrowed_box)] let ret: CType = return_value .as_ref() - .map(|i: &Box| (&**i).clone()) + .map(|i: &Box| (**i).clone()) .unwrap_or_default(); ret.generate_c(w); w.push(' '); @@ -183,7 +183,7 @@ impl CType { #[allow(clippy::borrowed_box)] let ret: CType = return_value .as_ref() - .map(|i: &Box| (&**i).clone()) + .map(|i: &Box| (**i).clone()) .unwrap_or_default(); ret.generate_c(w); w.push(' '); diff --git a/lib/compiler/src/artifact_builders/artifact_builder.rs b/lib/compiler/src/artifact_builders/artifact_builder.rs index aef2db98879..098cd0b8fd4 100644 --- a/lib/compiler/src/artifact_builders/artifact_builder.rs +++ b/lib/compiler/src/artifact_builders/artifact_builder.rs @@ -201,7 +201,7 @@ impl ArtifactCreate for ArtifactBuild { } fn data_initializers(&self) -> &[OwnedDataInitializer] { - &*self.serializable.data_initializers + &self.serializable.data_initializers } fn memory_styles(&self) -> &PrimaryMap { diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 6446ee05ed6..40bf5c3715a 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -366,7 +366,7 @@ impl Artifact { // Get pointers to where metadata about local tables should live in VM memory. let (allocator, memory_definition_locations, table_definition_locations) = - InstanceAllocator::new(&*module); + InstanceAllocator::new(&module); let finished_memories = tunables .create_memories( context, @@ -423,7 +423,7 @@ impl Artifact { .iter() .map(|init| DataInitializer { location: init.location.clone(), - data: &*init.data, + data: &init.data, }) .collect::>(); handle @@ -596,26 +596,17 @@ impl Artifact { )) } + #[cfg(feature = "static-artifact-load")] fn get_byte_slice(input: &[u8], start: usize, end: usize) -> Option<&[u8]> { - if start == end && input.len() > start { - return Some(&input[start..end]); - } else if start < end && input.len() > start && input.len() >= end { + if (start == end && input.len() > start) + || (start < end && input.len() > start && input.len() >= end) + { Some(&input[start..end]) } else { None } } - fn get_byte_slice_mut(input: &mut [u8], start: usize, end: usize) -> Option<&mut [u8]> { - if start == end && input.len() > start { - return Some(&mut input[start..end]); - } else if start < end && input.len() > start && input.len() >= end { - Some(&mut input[start..end]) - } else { - None - } - } - /// Deserialize a ArtifactBuild from an object file /// /// # Safety @@ -640,7 +631,7 @@ impl Artifact { let mut cur_offset = MetadataHeader::LEN + metadata_len; - let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) .ok_or(DeserializeError::InvalidByteLength { expected: cur_offset + WORD_SIZE, got: bytes.len(), @@ -658,13 +649,11 @@ impl Artifact { // read finished functions in order now... for _i in 0..num_finished_functions { - let byte_buffer_slice = - Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( - DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - }, - )?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); @@ -688,23 +677,21 @@ impl Artifact { // read trampolines in order let mut finished_function_call_trampolines = PrimaryMap::new(); - let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) .ok_or(DeserializeError::InvalidByteLength { expected: cur_offset + WORD_SIZE, got: bytes.len(), })?; - byte_buffer[0..WORD_SIZE].clone_from_slice(&byte_buffer_slice); + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let num_function_trampolines = usize::from_ne_bytes(byte_buffer); for _ in 0..num_function_trampolines { - let byte_buffer_slice = - Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( - DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - }, - )?; - byte_buffer[0..WORD_SIZE].clone_from_slice(&byte_buffer_slice); + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; + byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let trampoline_ptr_bytes = usize::from_ne_bytes(byte_buffer); let trampoline = mem::transmute::(trampoline_ptr_bytes); @@ -714,7 +701,7 @@ impl Artifact { // read dynamic function trampolines in order now... let mut finished_dynamic_function_trampolines = PrimaryMap::new(); - let byte_buffer_slice = Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE) + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) .ok_or(DeserializeError::InvalidByteLength { expected: cur_offset + WORD_SIZE, got: bytes.len(), @@ -723,13 +710,11 @@ impl Artifact { cur_offset += WORD_SIZE; let num_dynamic_trampoline_functions = usize::from_ne_bytes(byte_buffer); for _i in 0..num_dynamic_trampoline_functions { - let byte_buffer_slice = - Self::get_byte_slice(&bytes, cur_offset, cur_offset + WORD_SIZE).ok_or( - DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - }, - )?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) + .ok_or(DeserializeError::InvalidByteLength { + expected: cur_offset + WORD_SIZE, + got: bytes.len(), + })?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; diff --git a/lib/emscripten/src/lib.rs b/lib/emscripten/src/lib.rs index 1e4b7af80ea..216171e3510 100644 --- a/lib/emscripten/src/lib.rs +++ b/lib/emscripten/src/lib.rs @@ -105,7 +105,7 @@ impl EmEnv { /// Get a reference to the memory pub fn memory(&self, _mem_idx: u32) -> Memory { - (&*self.memory.read().unwrap()).as_ref().cloned().unwrap() + (*self.memory.read().unwrap()).as_ref().cloned().unwrap() } pub fn set_functions(&mut self, funcs: EmscriptenFunctions) { diff --git a/lib/types/src/error.rs b/lib/types/src/error.rs index e9b8dcd9e07..66f5ce765cc 100644 --- a/lib/types/src/error.rs +++ b/lib/types/src/error.rs @@ -46,7 +46,7 @@ pub enum DeserializeError { } /// Error type describing things that can go wrong when operating on Wasm Memories. -#[derive(Error, Debug, Clone, PartialEq, Hash)] +#[derive(Error, Debug, Clone, PartialEq, Eq, Hash)] pub enum MemoryError { /// Low level error with mmap. #[error("Error when allocating memory: {0}")] diff --git a/lib/types/src/serialize.rs b/lib/types/src/serialize.rs index 4bf7bfac000..7df4fca9b5f 100644 --- a/lib/types/src/serialize.rs +++ b/lib/types/src/serialize.rs @@ -144,7 +144,7 @@ impl SerializableModule { /// Returns data initializers to pass to `InstanceHandle::initialize` pub fn data_initializers(&self) -> &[OwnedDataInitializer] { - &*self.data_initializers + &self.data_initializers } /// Returns the memory styles associated with this `Artifact`. diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index c91a8f63fdc..0f189a75e75 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -116,7 +116,7 @@ impl Instance { } pub(crate) fn module_ref(&self) -> &ModuleInfo { - &*self.module + &self.module } fn context(&self) -> &StoreObjects { @@ -868,7 +868,7 @@ impl InstanceHandle { let instance = instance_handle.instance_mut(); let vmctx_ptr = instance.vmctx_ptr(); (instance.funcrefs, instance.imported_funcrefs) = build_funcrefs( - &*instance.module, + &instance.module, context, &imports, &instance.functions, diff --git a/lib/vm/src/libcalls.rs b/lib/vm/src/libcalls.rs index f9b1b28709d..9274237f167 100644 --- a/lib/vm/src/libcalls.rs +++ b/lib/vm/src/libcalls.rs @@ -189,7 +189,7 @@ pub unsafe extern "C" fn wasmer_vm_imported_memory32_grow( /// `vmctx` must be dereferenceable. #[no_mangle] pub unsafe extern "C" fn wasmer_vm_memory32_size(vmctx: *mut VMContext, memory_index: u32) -> u32 { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let memory_index = LocalMemoryIndex::from_u32(memory_index); instance.memory_size(memory_index).0 @@ -205,7 +205,7 @@ pub unsafe extern "C" fn wasmer_vm_imported_memory32_size( vmctx: *mut VMContext, memory_index: u32, ) -> u32 { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let memory_index = MemoryIndex::from_u32(memory_index); instance.imported_memory_size(memory_index).0 @@ -303,7 +303,7 @@ pub unsafe extern "C" fn wasmer_vm_table_fill( /// `vmctx` must be dereferenceable. #[no_mangle] pub unsafe extern "C" fn wasmer_vm_table_size(vmctx: *mut VMContext, table_index: u32) -> u32 { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let table_index = LocalTableIndex::from_u32(table_index); instance.table_size(table_index) @@ -319,7 +319,7 @@ pub unsafe extern "C" fn wasmer_vm_imported_table_size( vmctx: *mut VMContext, table_index: u32, ) -> u32 { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let table_index = TableIndex::from_u32(table_index); instance.imported_table_size(table_index) @@ -336,7 +336,7 @@ pub unsafe extern "C" fn wasmer_vm_table_get( table_index: u32, elem_index: u32, ) -> RawTableElement { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let table_index = LocalTableIndex::from_u32(table_index); // TODO: type checking, maybe have specialized accessors @@ -495,7 +495,7 @@ pub unsafe extern "C" fn wasmer_vm_func_ref( vmctx: *mut VMContext, function_index: u32, ) -> VMFuncRef { - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); let function_index = FunctionIndex::from_u32(function_index); instance.func_ref(function_index).unwrap() @@ -510,7 +510,7 @@ pub unsafe extern "C" fn wasmer_vm_func_ref( pub unsafe extern "C" fn wasmer_vm_elem_drop(vmctx: *mut VMContext, elem_index: u32) { on_host_stack(|| { let elem_index = ElemIndex::from_u32(elem_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.elem_drop(elem_index); }) } @@ -530,7 +530,7 @@ pub unsafe extern "C" fn wasmer_vm_memory32_copy( ) { let result = { let memory_index = LocalMemoryIndex::from_u32(memory_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.local_memory_copy(memory_index, dst, src, len) }; if let Err(trap) = result { @@ -553,7 +553,7 @@ pub unsafe extern "C" fn wasmer_vm_imported_memory32_copy( ) { let result = { let memory_index = MemoryIndex::from_u32(memory_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.imported_memory_copy(memory_index, dst, src, len) }; if let Err(trap) = result { @@ -576,7 +576,7 @@ pub unsafe extern "C" fn wasmer_vm_memory32_fill( ) { let result = { let memory_index = LocalMemoryIndex::from_u32(memory_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.local_memory_fill(memory_index, dst, val, len) }; if let Err(trap) = result { @@ -599,7 +599,7 @@ pub unsafe extern "C" fn wasmer_vm_imported_memory32_fill( ) { let result = { let memory_index = MemoryIndex::from_u32(memory_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.imported_memory_fill(memory_index, dst, val, len) }; if let Err(trap) = result { @@ -624,7 +624,7 @@ pub unsafe extern "C" fn wasmer_vm_memory32_init( let result = { let memory_index = MemoryIndex::from_u32(memory_index); let data_index = DataIndex::from_u32(data_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.memory_init(memory_index, data_index, dst, src, len) }; if let Err(trap) = result { @@ -641,7 +641,7 @@ pub unsafe extern "C" fn wasmer_vm_memory32_init( pub unsafe extern "C" fn wasmer_vm_data_drop(vmctx: *mut VMContext, data_index: u32) { on_host_stack(|| { let data_index = DataIndex::from_u32(data_index); - let instance = (&*vmctx).instance(); + let instance = (*vmctx).instance(); instance.data_drop(data_index) }) } diff --git a/lib/wasi/src/syscalls/mod.rs b/lib/wasi/src/syscalls/mod.rs index 0c5266e6efc..e972946c018 100644 --- a/lib/wasi/src/syscalls/mod.rs +++ b/lib/wasi/src/syscalls/mod.rs @@ -307,7 +307,7 @@ pub fn args_get( let env = ctx.data(); let (memory, mut state) = env.get_memory_and_wasi_state(&ctx, 0); - let result = write_buffer_array(&memory, &*state.args, argv, argv_buf); + let result = write_buffer_array(&memory, &state.args, argv, argv_buf); debug!( "=> args:\n{}", @@ -433,7 +433,7 @@ pub fn environ_get( let (memory, mut state) = env.get_memory_and_wasi_state(&ctx, 0); trace!(" -> State envs: {:?}", state.envs); - write_buffer_array(&memory, &*state.envs, environ, environ_buf) + write_buffer_array(&memory, &state.envs, environ, environ_buf) } /// ### `environ_sizes_get()` @@ -5555,7 +5555,7 @@ pub unsafe fn sock_send_file( sock, __WASI_RIGHT_SOCK_SEND, |socket| { - let buf = (&buf[..]).to_vec(); + let buf = (buf[..]).to_vec(); socket.send_bytes::(Bytes::from(buf)) } )); From e00be2380d0157f0d6dd89b7b7e063b520c3182c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 16:10:12 +0200 Subject: [PATCH 04/10] Remove manual checks in Artifact::deserialize, prefer function instead --- lib/compiler/src/engine/artifact.rs | 34 +++++++++++++---------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 40bf5c3715a..d98dbe4e294 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -117,29 +117,26 @@ impl Artifact { )); } - if bytes.len() < ArtifactBuild::MAGIC_HEADER.len() { - return Err(DeserializeError::InvalidByteLength { + let bytes = Self::get_byte_slice(bytes, 0, ArtifactBuild::MAGIC_HEADER.len()).ok_or( + DeserializeError::InvalidByteLength { expected: ArtifactBuild::MAGIC_HEADER.len(), got: bytes.len(), - }); - } + }, + )?; - let bytes = &bytes[ArtifactBuild::MAGIC_HEADER.len()..]; let metadata_len = MetadataHeader::parse(bytes)?; - if bytes.len() < MetadataHeader::LEN { - return Err(DeserializeError::InvalidByteLength { - expected: MetadataHeader::LEN, - got: bytes.len(), - }); - } - - let metadata_slice: &[u8] = &bytes[MetadataHeader::LEN..]; - if metadata_slice.len() < metadata_len { - return Err(DeserializeError::InvalidByteLength { - expected: metadata_len + MetadataHeader::LEN, + let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len()).ok_or( + DeserializeError::InvalidByteLength { + expected: ArtifactBuild::MAGIC_HEADER.len(), got: bytes.len(), - }); - } + }, + )?; + let metadata_slice = Self::get_byte_slice(metadata_slice, 0, metadata_len).ok_or( + DeserializeError::InvalidByteLength { + expected: metadata_len, + got: metadata_slice.len(), + }, + )?; let metadata_slice: &[u8] = &metadata_slice[..metadata_len]; let serializable = SerializableModule::deserialize(metadata_slice)?; @@ -596,7 +593,6 @@ impl Artifact { )) } - #[cfg(feature = "static-artifact-load")] fn get_byte_slice(input: &[u8], start: usize, end: usize) -> Option<&[u8]> { if (start == end && input.len() > start) || (start < end && input.len() > start && input.len() >= end) From 21342e540f3dfe5579dbab13b4cadd63c8baa406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 16:25:08 +0200 Subject: [PATCH 05/10] Adress review comment: Use Result<...> instead of Option for quick return --- lib/compiler/src/engine/artifact.rs | 73 +++++------------------------ 1 file changed, 13 insertions(+), 60 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index d98dbe4e294..08e598b603a 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -117,26 +117,11 @@ impl Artifact { )); } - let bytes = Self::get_byte_slice(bytes, 0, ArtifactBuild::MAGIC_HEADER.len()).ok_or( - DeserializeError::InvalidByteLength { - expected: ArtifactBuild::MAGIC_HEADER.len(), - got: bytes.len(), - }, - )?; + let bytes = Self::get_byte_slice(bytes, 0, ArtifactBuild::MAGIC_HEADER.len())?; let metadata_len = MetadataHeader::parse(bytes)?; - let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len()).ok_or( - DeserializeError::InvalidByteLength { - expected: ArtifactBuild::MAGIC_HEADER.len(), - got: bytes.len(), - }, - )?; - let metadata_slice = Self::get_byte_slice(metadata_slice, 0, metadata_len).ok_or( - DeserializeError::InvalidByteLength { - expected: metadata_len, - got: metadata_slice.len(), - }, - )?; + 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 metadata_slice: &[u8] = &metadata_slice[..metadata_len]; let serializable = SerializableModule::deserialize(metadata_slice)?; @@ -593,13 +578,13 @@ impl Artifact { )) } - fn get_byte_slice(input: &[u8], start: usize, end: usize) -> Option<&[u8]> { + fn get_byte_slice(input: &[u8], start: usize, end: usize) -> Result<&[u8], DeserializeError> { if (start == end && input.len() > start) || (start < end && input.len() > start && input.len() >= end) { - Some(&input[start..end]) + Ok(&input[start..end]) } else { - None + Err(DeserializeError::InvalidByteLength { expected: end - start, got: input.len() }) } } @@ -613,13 +598,7 @@ impl Artifact { bytes: &[u8], ) -> Result { let metadata_len = MetadataHeader::parse(bytes)?; - let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len()).ok_or( - DeserializeError::InvalidByteLength { - expected: MetadataHeader::LEN, - got: bytes.len(), - }, - )?; - + let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len())?; let metadata: ModuleMetadata = ModuleMetadata::deserialize(metadata_slice)?; const WORD_SIZE: usize = mem::size_of::(); @@ -627,12 +606,7 @@ impl Artifact { let mut cur_offset = MetadataHeader::LEN + metadata_len; - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; - + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; @@ -645,12 +619,7 @@ impl Artifact { // read finished functions in order now... for _i in 0..num_finished_functions { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; - + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; @@ -673,20 +642,12 @@ impl Artifact { // read trampolines in order let mut finished_function_call_trampolines = PrimaryMap::new(); - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let num_function_trampolines = usize::from_ne_bytes(byte_buffer); for _ in 0..num_function_trampolines { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let trampoline_ptr_bytes = usize::from_ne_bytes(byte_buffer); @@ -697,20 +658,12 @@ impl Artifact { // read dynamic function trampolines in order now... let mut finished_dynamic_function_trampolines = PrimaryMap::new(); - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let num_dynamic_trampoline_functions = usize::from_ne_bytes(byte_buffer); for _i in 0..num_dynamic_trampoline_functions { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE) - .ok_or(DeserializeError::InvalidByteLength { - expected: cur_offset + WORD_SIZE, - got: bytes.len(), - })?; + let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; From 0df69e4a6479a16667e5f1f3aa96edbdc8d2b70c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 16:27:54 +0200 Subject: [PATCH 06/10] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7756a471e61..68ab2b5e892 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Looking for changes that affect our C API? See the [C API Changelog](lib/c-api/C ### Changed ### Fixed +- #[3130](https://github.com/wasmerio/wasmer/pull/3130) Remove panics from Artifact::deserialize ## 3.0.0-beta - 2022/08/08 From f659a35229bd7f26a9537e698050c8b9d428c7bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 16:30:50 +0200 Subject: [PATCH 07/10] Make "make lint" pass --- lib/compiler/src/engine/artifact.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 08e598b603a..b721c120ad7 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -584,7 +584,10 @@ impl Artifact { { Ok(&input[start..end]) } else { - Err(DeserializeError::InvalidByteLength { expected: end - start, got: input.len() }) + Err(DeserializeError::InvalidByteLength { + expected: end - start, + got: input.len(), + }) } } @@ -619,7 +622,8 @@ impl Artifact { // read finished functions in order now... for _i in 0..num_finished_functions { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; + let byte_buffer_slice = + Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; @@ -647,7 +651,8 @@ impl Artifact { cur_offset += WORD_SIZE; let num_function_trampolines = usize::from_ne_bytes(byte_buffer); for _ in 0..num_function_trampolines { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; + let byte_buffer_slice = + Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); cur_offset += WORD_SIZE; let trampoline_ptr_bytes = usize::from_ne_bytes(byte_buffer); @@ -663,7 +668,8 @@ impl Artifact { cur_offset += WORD_SIZE; let num_dynamic_trampoline_functions = usize::from_ne_bytes(byte_buffer); for _i in 0..num_dynamic_trampoline_functions { - let byte_buffer_slice = Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; + let byte_buffer_slice = + Self::get_byte_slice(bytes, cur_offset, cur_offset + WORD_SIZE)?; byte_buffer[0..WORD_SIZE].clone_from_slice(byte_buffer_slice); let fp = FunctionBodyPtr(usize::from_ne_bytes(byte_buffer) as _); cur_offset += WORD_SIZE; From 18abab157c6ae5cffd673db3ab1066b65c9f7d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 16:56:16 +0200 Subject: [PATCH 08/10] Add sanity test for Artifact::deserialize --- tests/compilers/serialize.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/compilers/serialize.rs b/tests/compilers/serialize.rs index 2610f83da1e..4bca9000fb9 100644 --- a/tests/compilers/serialize.rs +++ b/tests/compilers/serialize.rs @@ -1,6 +1,13 @@ use anyhow::Result; use wasmer::*; +#[test] +fn sanity_test_artifact_deserialize() { + let engine = Engine::headless(); + let result = unsafe { Artifact::deserialize(&engine, &[]) }; + assert!(result.is_err()); +} + #[compiler_test(serialize)] fn test_serialize(config: crate::Config) -> Result<()> { let mut store = config.store(); From ff8f43b0aec4d21a37ec9e2724514ffa01e8f0be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Tue, 23 Aug 2022 18:03:13 +0200 Subject: [PATCH 09/10] Fix error in MetadataHeader::parse --- lib/compiler/src/engine/artifact.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index b721c120ad7..d4978012783 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -117,7 +117,7 @@ impl Artifact { )); } - let bytes = Self::get_byte_slice(bytes, 0, ArtifactBuild::MAGIC_HEADER.len())?; + let bytes = Self::get_byte_slice(bytes, ArtifactBuild::MAGIC_HEADER.len(), bytes.len())?; let metadata_len = MetadataHeader::parse(bytes)?; let metadata_slice = Self::get_byte_slice(bytes, MetadataHeader::LEN, bytes.len())?; From d8ed8d7c6c0c8dbe784ea1b0f547968b947bf66b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Sch=C3=BCtt?= Date: Thu, 25 Aug 2022 12:39:08 +0200 Subject: [PATCH 10/10] Fix issue in Artifact::deserialize --- lib/compiler/src/engine/artifact.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index d4978012783..b436c2da8d6 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -123,7 +123,6 @@ 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 metadata_slice: &[u8] = &metadata_slice[..metadata_len]; let serializable = SerializableModule::deserialize(metadata_slice)?; let artifact = ArtifactBuild::from_serializable(serializable); let mut inner_engine = engine.inner_mut(); @@ -602,6 +601,7 @@ impl Artifact { ) -> Result { let metadata_len = MetadataHeader::parse(bytes)?; 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 metadata: ModuleMetadata = ModuleMetadata::deserialize(metadata_slice)?; const WORD_SIZE: usize = mem::size_of::();