Merge #806

806: Fix issue 653 (panic in MemoryDescriptor) + remove panic in UnsharedMemory r=syrusakbary a=pventuzelo

#Description

Based on comment from @syrusakbary [here](#654 (comment)) and previous pull request from @bjfish.
* Reformat MemoryDescriptor (new structure field, function new, function memory_type)
* Remove panic for MemoryDescriptor
* Remove panic for UnsharedMemory

# Review

- [ ] Create a short description of the the change in the CHANGELOG.md file


Co-authored-by: Patrick Ventuzelo <[email protected]>
Co-authored-by: Patrick Ventuzelo <[email protected]>

Author: bors[bot]

lib/emscripten/src/lib.rs

@@ -474,11 +474,7 @@ impl EmscriptenGlobals {
         let (memory_min, memory_max, shared) = get_emscripten_memory_size(&module)?;
 
         // Memory initialization
-        let memory_type = MemoryDescriptor {
-            minimum: memory_min,
-            maximum: memory_max,
-            shared: shared,
-        };
+        let memory_type = MemoryDescriptor::new(memory_min, memory_max, shared)?;
         let memory = Memory::new(memory_type).unwrap();
 
         let table_type = TableDescriptor {

lib/runtime-c-api/src/memory.rs

@@ -1,6 +1,6 @@
 //! Create, read, write, grow, destroy memory of an instance.
 
-use crate::{error::update_last_error, wasmer_limits_t, wasmer_result_t};
+use crate::{error::update_last_error, error::CApiError, wasmer_limits_t, wasmer_result_t};
 use std::cell::Cell;
 use wasmer_runtime::Memory;
 use wasmer_runtime_core::{
@@ -31,12 +31,17 @@ pub unsafe extern "C" fn wasmer_memory_new(
     } else {
         None
     };
-    let desc = MemoryDescriptor {
-        minimum: Pages(limits.min),
-        maximum: max,
-        shared: false,
+    let desc = MemoryDescriptor::new(Pages(limits.min), max, false);
+    let new_desc = match desc {
+        Ok(desc) => desc,
+        Err(error) => {
+            update_last_error(CApiError {
+                msg: error.to_string(),
+            });
+            return wasmer_result_t::WASMER_ERROR;
+        }
     };
-    let result = Memory::new(desc);
+    let result = Memory::new(new_desc);
     let new_memory = match result {
         Ok(memory) => memory,
         Err(error) => {

lib/runtime-core/src/memory/mod.rs

@@ -50,16 +50,12 @@ impl Memory {
     /// # use wasmer_runtime_core::memory::Memory;
     /// # use wasmer_runtime_core::error::Result;
     /// # use wasmer_runtime_core::units::Pages;
-    /// # fn create_memory() -> Result<()> {
-    /// let descriptor = MemoryDescriptor {
-    ///     minimum: Pages(10),
-    ///     maximum: None,
-    ///     shared: false,
-    /// };
+    /// fn create_memory() -> Result<()> {
+    ///     let descriptor = MemoryDescriptor::new(Pages(10), None, false).unwrap();
     ///
-    /// let memory = Memory::new(descriptor)?;
-    /// # Ok(())
-    /// # }
+    ///     let memory = Memory::new(descriptor)?;
+    ///     Ok(())
+    /// }
     /// ```
     pub fn new(desc: MemoryDescriptor) -> Result<Self, CreationError> {
         if let Some(max) = desc.maximum {
@@ -174,7 +170,7 @@ impl fmt::Debug for Memory {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum MemoryType {
     Dynamic,
     Static,
@@ -232,7 +228,11 @@ impl UnsharedMemory {
             MemoryType::Static => {
                 UnsharedMemoryStorage::Static(StaticMemory::new(desc, &mut local)?)
             }
-            MemoryType::SharedStatic => panic!("attempting to create shared unshared memory"),
+            MemoryType::SharedStatic => {
+                return Err(CreationError::InvalidDescriptor(
+                    "attempting to create shared unshared memory".to_string(),
+                ));
+            }
         };
 
         Ok(Self {
@@ -350,24 +350,16 @@ mod memory_tests {
 
     #[test]
     fn test_initial_memory_size() {
-        let unshared_memory = Memory::new(MemoryDescriptor {
-            minimum: Pages(10),
-            maximum: Some(Pages(20)),
-            shared: false,
-        })
-        .unwrap();
+        let memory_desc = MemoryDescriptor::new(Pages(10), Some(Pages(20)), false).unwrap();
+        let unshared_memory = Memory::new(memory_desc).unwrap();
         assert_eq!(unshared_memory.size(), Pages(10));
     }
 
     #[test]
     fn test_invalid_descriptor_returns_error() {
-        let result = Memory::new(MemoryDescriptor {
-            minimum: Pages(10),
-            maximum: None,
-            shared: true,
-        });
+        let memory_desc = MemoryDescriptor::new(Pages(10), None, true);
         assert!(
-            result.is_err(),
+            memory_desc.is_err(),
             "Max number of pages is required for shared memory"
         )
     }

lib/runtime-core/src/parse.rs

@@ -136,11 +136,13 @@ pub fn read_module<
                             .push((import_name, table_desc));
                     }
                     ImportSectionEntryType::Memory(memory_ty) => {
-                        let mem_desc = MemoryDescriptor {
-                            minimum: Pages(memory_ty.limits.initial),
-                            maximum: memory_ty.limits.maximum.map(|max| Pages(max)),
-                            shared: memory_ty.shared,
-                        };
+                        let mem_desc = MemoryDescriptor::new(
+                            Pages(memory_ty.limits.initial),
+                            memory_ty.limits.maximum.map(|max| Pages(max)),
+                            memory_ty.shared,
+                        )
+                        .map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
+
                         info.write()
                             .unwrap()
                             .imported_memories
@@ -172,11 +174,12 @@ pub fn read_module<
                 info.write().unwrap().tables.push(table_desc);
             }
             ParserState::MemorySectionEntry(memory_ty) => {
-                let mem_desc = MemoryDescriptor {
-                    minimum: Pages(memory_ty.limits.initial),
-                    maximum: memory_ty.limits.maximum.map(|max| Pages(max)),
-                    shared: memory_ty.shared,
-                };
+                let mem_desc = MemoryDescriptor::new(
+                    Pages(memory_ty.limits.initial),
+                    memory_ty.limits.maximum.map(|max| Pages(max)),
+                    memory_ty.shared,
+                )
+                .map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
 
                 info.write().unwrap().memories.push(mem_desc);
             }

lib/runtime-core/src/types.rs

@@ -326,7 +326,7 @@ pub struct GlobalInit {
     pub init: Initializer,
 }
 
-/// A wasm memory.
+/// A wasm memory descriptor.
 #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)]
 pub struct MemoryDescriptor {
     /// The minimum number of allowed pages.
@@ -335,16 +335,30 @@ pub struct MemoryDescriptor {
     pub maximum: Option<Pages>,
     /// This memory can be shared between wasm threads.
     pub shared: bool,
+    /// The type of the memory
+    pub memory_type: MemoryType,
 }
 
 impl MemoryDescriptor {
-    pub fn memory_type(self) -> MemoryType {
-        match (self.maximum.is_some(), self.shared) {
+    pub fn new(minimum: Pages, maximum: Option<Pages>, shared: bool) -> Result<Self, String> {
+        let memory_type = match (maximum.is_some(), shared) {
             (true, true) => MemoryType::SharedStatic,
             (true, false) => MemoryType::Static,
             (false, false) => MemoryType::Dynamic,
-            (false, true) => panic!("shared memory without a max is not allowed"),
-        }
+            (false, true) => {
+                return Err("Max number of pages is required for shared memory".to_string());
+            }
+        };
+        Ok(MemoryDescriptor {
+            minimum,
+            maximum,
+            shared,
+            memory_type,
+        })
+    }
+
+    pub fn memory_type(&self) -> MemoryType {
+        self.memory_type
     }
 
     pub(crate) fn fits_in_imported(&self, imported: MemoryDescriptor) -> bool {

lib/spectests/examples/simple/main.rs

@@ -42,12 +42,8 @@ fn main() -> error::Result<()> {
 
     let inner_module = wasmer_runtime_core::compile_with(&wasm_binary, &get_compiler())?;
 
-    let memory = Memory::new(MemoryDescriptor {
-        minimum: Pages(1),
-        maximum: Some(Pages(1)),
-        shared: false,
-    })
-    .unwrap();
+    let memory_desc = MemoryDescriptor::new(Pages(1), Some(Pages(1)), false).unwrap();
+    let memory = Memory::new(memory_desc).unwrap();
 
     let global = Global::new(Value::I32(42));
 

lib/spectests/tests/spectest.rs

@@ -1021,12 +1021,8 @@ mod tests {
     fn get_spectest_import_object(
         registered_modules: &HashMap<String, Arc<Mutex<Instance>>>,
     ) -> ImportObject {
-        let memory = Memory::new(MemoryDescriptor {
-            minimum: Pages(1),
-            maximum: Some(Pages(2)),
-            shared: false,
-        })
-        .unwrap();
+        let memory_desc = MemoryDescriptor::new(Pages(1), Some(Pages(2)), false).unwrap();
+        let memory = Memory::new(memory_desc).unwrap();
 
         let global_i32 = Global::new(wasmer_runtime_core::types::Value::I32(666));
         let global_f32 = Global::new(wasmer_runtime_core::types::Value::F32(666.0));