Merge pull request #18 from wasmerio/fix/memory-not-growing

Debug memory not growing bug

Cargo.lock

@@ -1359,10 +1359,12 @@ dependencies = [
 name = "test-utils"
 version = "0.16.2"
 dependencies = [
+ "wasmer",
  "wasmer-compiler",
  "wasmer-compiler-cranelift",
  "wasmer-compiler-llvm",
  "wasmer-compiler-singlepass",
+ "wasmer-engine-jit",
 ]
 
 [[package]]

lib/api/src/externals.rs

@@ -11,8 +11,8 @@ use wasm_common::{Bytes, HostFunction, Pages, ValueType, WasmTypeList, WithEnv,
 use wasmer_engine::Engine as _;
 use wasmer_runtime::{
     wasmer_call_trampoline, Export, ExportFunction, ExportGlobal, ExportMemory, ExportTable,
-    LinearMemory, Table as RuntimeTable, VMCallerCheckedAnyfunc, VMContext, VMFunctionBody,
-    VMGlobalDefinition, VMMemoryDefinition, VMTrampoline,
+    LinearMemory, MemoryError, Table as RuntimeTable, VMCallerCheckedAnyfunc, VMContext,
+    VMFunctionBody, VMGlobalDefinition, VMMemoryDefinition, VMTrampoline,
 };
 
 #[derive(Clone)]
@@ -344,25 +344,25 @@ pub struct Memory {
 }
 
 impl Memory {
-    pub fn new(store: &Store, ty: MemoryType) -> Memory {
+    pub fn new(store: &Store, ty: MemoryType) -> Result<Memory, MemoryError> {
         let tunables = store.engine().tunables();
         let memory_plan = tunables.memory_plan(ty);
-        let memory = tunables.create_memory(memory_plan).unwrap();
+        let memory = tunables.create_memory(memory_plan)?;
 
         let definition = memory.vmmemory();
 
-        Memory {
+        Ok(Memory {
             store: store.clone(),
             owned_by_store: true,
             exported: ExportMemory {
                 from: Box::leak(Box::new(memory)),
                 definition: Box::leak(Box::new(definition)),
             },
-        }
+        })
     }
 
-    fn definition(&self) -> &VMMemoryDefinition {
-        unsafe { &*self.exported.definition }
+    fn definition(&self) -> VMMemoryDefinition {
+        self.memory().vmmemory()
     }
 
     pub fn ty(&self) -> &MemoryType {
@@ -391,14 +391,14 @@ impl Memory {
     }
 
     pub fn size(&self) -> Pages {
-        Bytes(self.data_size()).into()
+        self.memory().size()
     }
 
     fn memory(&self) -> &LinearMemory {
         unsafe { &*self.exported.from }
     }
 
-    pub fn grow(&self, delta: Pages) -> Option<Pages> {
+    pub fn grow(&self, delta: Pages) -> Result<Pages, MemoryError> {
         self.memory().grow(delta)
     }
 

lib/api/src/lib.rs

@@ -33,6 +33,7 @@ pub use wasmer_compiler::{Features, Target};
 pub use wasmer_engine::{
     DeserializeError, Engine, InstantiationError, LinkError, RuntimeError, SerializeError,
 };
+pub use wasmer_runtime::MemoryError;
 
 // The compilers are mutually exclusive
 #[cfg(any(

lib/api/src/tunables.rs

@@ -3,6 +3,7 @@ use std::cmp::min;
 use target_lexicon::{OperatingSystem, PointerWidth, Triple, HOST};
 use wasm_common::{MemoryType, Pages, TableType};
 use wasmer_engine::Tunables as BaseTunables;
+use wasmer_runtime::MemoryError;
 use wasmer_runtime::{LinearMemory, Table};
 use wasmer_runtime::{MemoryPlan, MemoryStyle, TablePlan, TableStyle};
 
@@ -93,7 +94,7 @@ impl BaseTunables for Tunables {
     }
 
     /// Create a memory given a memory type
-    fn create_memory(&self, plan: MemoryPlan) -> Result<LinearMemory, String> {
+    fn create_memory(&self, plan: MemoryPlan) -> Result<LinearMemory, MemoryError> {
         LinearMemory::new(&plan)
     }
 

lib/c-api/src/import/wasi.rs

@@ -213,7 +213,7 @@ pub unsafe extern "C" fn wasmer_wasi_generate_default_import_object() -> *mut wa
     let mut wasi_env = wasi::WasiEnv::new(wasi_state);
     // this API will now leak a `Memory`
     let memory_type = MemoryType::new(0, None, false);
-    let memory = Memory::new(store, memory_type);
+    let memory = Memory::new(store, memory_type).expect("create memory");
     wasi_env.set_memory(&memory);
     // TODO(mark): review lifetime of `Memory` here
     let import_object = Box::new(wasi::generate_import_object_from_env(

lib/c-api/src/memory.rs

@@ -69,9 +69,18 @@ pub unsafe extern "C" fn wasmer_memory_new(
     };
     let store = crate::get_global_store();
     let desc = MemoryType::new(Pages(limits.min), max, false);
-    let new_memory = Memory::new(store, desc);
-    *memory = Box::into_raw(Box::new(new_memory)) as *mut wasmer_memory_t;
-    wasmer_result_t::WASMER_OK
+    match Memory::new(store, desc) {
+        Ok(new_memory) => {
+            *memory = Box::into_raw(Box::new(new_memory)) as *mut wasmer_memory_t;
+            wasmer_result_t::WASMER_OK
+        }
+        Err(err) => {
+            update_last_error(CApiError {
+                msg: err.to_string(),
+            });
+            wasmer_result_t::WASMER_ERROR
+        }
+    }
 }
 
 /// Grows a memory by the given number of pages (of 65Kb each).
@@ -105,8 +114,13 @@ pub extern "C" fn wasmer_memory_grow(memory: *mut wasmer_memory_t, delta: u32) -
     let grow_result = memory.grow(Pages(delta));
 
     match grow_result {
-        Some(_) => wasmer_result_t::WASMER_OK,
-        _ => wasmer_result_t::WASMER_ERROR,
+        Ok(_) => wasmer_result_t::WASMER_OK,
+        Err(err) => {
+            update_last_error(CApiError {
+                msg: err.to_string(),
+            });
+            wasmer_result_t::WASMER_ERROR
+        }
     }
 }
 

lib/c-api/tests/CMakeLists.txt

@@ -111,8 +111,7 @@ add_test(test-instantiate test-instantiate)
 
 target_link_libraries(test-memory general ${WASMER_LIB})
 target_compile_options(test-memory PRIVATE ${COMPILER_OPTIONS})
-# TODO: reenable this test
-#add_test(test-memory test-memory)
+add_test(test-memory test-memory)
 
 target_link_libraries(test-module general ${WASMER_LIB})
 target_compile_options(test-module PRIVATE ${COMPILER_OPTIONS})

lib/c-api/tests/test-memory.c

@@ -40,7 +40,7 @@ int main()
     char *error_str = malloc(error_len);
     wasmer_last_error_message(error_str, error_len);
     printf("Error str: `%s`\n", error_str);
-    assert(0 == strcmp(error_str, "Failed to add pages because would exceed maximum number of pages for the memory. Left: 22, Added: 15"));
+    assert(0 == strcmp(error_str, "The memory could not grow: current size 12 pages, requested increase: 10 pages"));
     free(error_str);
 
     wasmer_memory_t *bad_memory = NULL;
@@ -58,7 +58,7 @@ int main()
     char *error_str2 = malloc(error_len2);
     wasmer_last_error_message(error_str2, error_len2);
     printf("Error str 2: `%s`\n", error_str2);
-    assert(0 == strcmp(error_str2, "Unable to create because the supplied descriptor is invalid: \"Max number of memory pages is less than the minimum number of pages\""));
+    assert(0 == strcmp(error_str2, "The memory plan is invalid because the maximum (10 pages) is less than the minimum (15 pages)"));
     free(error_str2);
 
     printf("Destroy memory\n");

lib/engine/src/tunables.rs

@@ -4,6 +4,7 @@ use wasm_common::{
     GlobalIndex, LocalGlobalIndex, LocalMemoryIndex, LocalTableIndex, MemoryIndex, MemoryType,
     TableIndex, TableType,
 };
+use wasmer_runtime::MemoryError;
 use wasmer_runtime::{LinearMemory, Module, Table, VMGlobalDefinition};
 use wasmer_runtime::{MemoryPlan, TablePlan};
 
@@ -16,7 +17,7 @@ pub trait Tunables {
     fn table_plan(&self, table: TableType) -> TablePlan;
 
     /// Create a memory given a memory type
-    fn create_memory(&self, memory_type: MemoryPlan) -> Result<LinearMemory, String>;
+    fn create_memory(&self, memory_type: MemoryPlan) -> Result<LinearMemory, MemoryError>;
 
     /// Create a memory given a memory type
     fn create_table(&self, table_type: TablePlan) -> Result<Table, String>;
@@ -32,7 +33,10 @@ pub trait Tunables {
             PrimaryMap::with_capacity(module.memories.len() - num_imports);
         for index in num_imports..module.memories.len() {
             let plan = memory_plans[MemoryIndex::new(index)].clone();
-            memories.push(self.create_memory(plan).map_err(LinkError::Resource)?);
+            memories.push(
+                self.create_memory(plan)
+                    .map_err(|e| LinkError::Resource(format!("Failed to create memory: {}", e)))?,
+            );
         }
         Ok(memories)
     }

lib/runtime/src/instance.rs

@@ -3,7 +3,7 @@
 //! `InstanceHandle` is a reference-counting handle for an `Instance`.
 use crate::export::Export;
 use crate::imports::Imports;
-use crate::memory::LinearMemory;
+use crate::memory::{LinearMemory, MemoryError};
 use crate::table::Table;
 use crate::trap::{catch_traps, init_traps, Trap, TrapCode};
 use crate::vmcontext::{
@@ -431,7 +431,7 @@ impl Instance {
         &self,
         memory_index: LocalMemoryIndex,
         delta: IntoPages,
-    ) -> Option<Pages>
+    ) -> Result<Pages, MemoryError>
     where
         IntoPages: Into<Pages>,
     {
@@ -459,7 +459,7 @@ impl Instance {
         &self,
         memory_index: MemoryIndex,
         delta: IntoPages,
-    ) -> Option<Pages>
+    ) -> Result<Pages, MemoryError>
     where
         IntoPages: Into<Pages>,
     {
@@ -979,7 +979,7 @@ impl InstanceHandle {
         &self,
         memory_index: LocalMemoryIndex,
         delta: IntoPages,
-    ) -> Option<Pages>
+    ) -> Result<Pages, MemoryError>
     where
         IntoPages: Into<Pages>,
     {

lib/runtime/src/lib.rs

@@ -39,7 +39,7 @@ pub mod libcalls;
 pub use crate::export::*;
 pub use crate::imports::Imports;
 pub use crate::instance::InstanceHandle;
-pub use crate::memory::LinearMemory;
+pub use crate::memory::{LinearMemory, MemoryError};
 pub use crate::mmap::Mmap;
 pub use crate::module::{
     ExportsIterator, ImportsIterator, MemoryPlan, MemoryStyle, Module, TableElements, TablePlan,

lib/runtime/src/memory.rs

@@ -7,7 +7,34 @@ use crate::module::{MemoryPlan, MemoryStyle};
 use crate::vmcontext::VMMemoryDefinition;
 use more_asserts::{assert_ge, assert_le};
 use std::cell::RefCell;
-use wasm_common::Pages;
+use thiserror::Error;
+use wasm_common::{Bytes, Pages};
+
+/// Error type describing things that can go wrong when operating on Wasm Memories.
+#[derive(Error, Debug, Clone, PartialEq, Hash)]
+pub enum MemoryError {
+    /// Low level error with mmap.
+    #[error("Error when allocating memory: {0}")]
+    Region(String),
+    /// The operation would cause the size of the memory to exceed the maximum or would cause
+    /// an overflow leading to unindexable memory.
+    #[error("The memory could not grow: current size {} pages, requested increase: {} pages", current.0, attempted_delta.0)]
+    CouldNotGrow {
+        /// The current size in pages.
+        current: Pages,
+        /// The attempted amount to grow by in pages.
+        attempted_delta: Pages,
+    },
+    /// The operation would cause the size of the memory size exceed the maximum.
+    #[error("The memory plan is invalid because {}", reason)]
+    InvalidMemoryPlan {
+        /// The reason why the memory plan is invalid.
+        reason: String,
+    },
+    /// A user defined error value, used for error cases not listed above.
+    #[error("A user-defined error occurred: {0}")]
+    Generic(String),
+}
 
 /// A linear memory instance.
 #[derive(Debug)]
@@ -40,13 +67,23 @@ struct WasmMmap {
 
 impl LinearMemory {
     /// Create a new linear memory instance with specified minimum and maximum number of wasm pages.
-    pub fn new(plan: &MemoryPlan) -> Result<Self, String> {
+    pub fn new(plan: &MemoryPlan) -> Result<Self, MemoryError> {
         // `maximum` cannot be set to more than `65536` pages.
         assert_le!(plan.memory.minimum, Pages::max_value());
         assert!(
             plan.memory.maximum.is_none() || plan.memory.maximum.unwrap() <= Pages::max_value()
         );
 
+        if plan.memory.maximum.is_some() && plan.memory.maximum.unwrap() < plan.memory.minimum {
+            return Err(MemoryError::InvalidMemoryPlan {
+                reason: format!(
+                    "the maximum ({} pages) is less than the minimum ({} pages)",
+                    plan.memory.maximum.unwrap().0,
+                    plan.memory.minimum.0
+                ),
+            });
+        }
+
         let offset_guard_bytes = plan.offset_guard_size as usize;
 
         // If we have an offset guard, or if we're doing the static memory
@@ -71,7 +108,8 @@ impl LinearMemory {
         let mapped_bytes = mapped_pages.bytes();
 
         let mmap = WasmMmap {
-            alloc: Mmap::accessible_reserved(mapped_bytes.0, request_bytes)?,
+            alloc: Mmap::accessible_reserved(mapped_bytes.0, request_bytes)
+                .map_err(MemoryError::Region)?,
             size: plan.memory.minimum,
         };
 
@@ -98,7 +136,7 @@ impl LinearMemory {
     ///
     /// Returns `None` if memory can't be grown by the specified amount
     /// of wasm pages.
-    pub fn grow<IntoPages>(&self, delta: IntoPages) -> Option<Pages>
+    pub fn grow<IntoPages>(&self, delta: IntoPages) -> Result<Pages, MemoryError>
     where
         IntoPages: Into<Pages>,
     {
@@ -106,20 +144,24 @@ impl LinearMemory {
         let delta: Pages = delta.into();
         let mut mmap = self.mmap.borrow_mut();
         if delta.0 == 0 {
-            return Some(mmap.size);
+            return Ok(mmap.size);
         }
 
-        let new_pages = match mmap.size.checked_add(delta) {
-            Some(new_pages) => new_pages,
-            // Linear memory size overflow.
-            None => return None,
-        };
+        let new_pages = mmap
+            .size
+            .checked_add(delta)
+            .ok_or_else(|| MemoryError::CouldNotGrow {
+                current: mmap.size,
+                attempted_delta: delta,
+            })?;
         let prev_pages = mmap.size;
 
         if let Some(maximum) = self.maximum {
             if new_pages > maximum {
-                // Linear memory size would exceed the declared maximum.
-                return None;
+                return Err(MemoryError::CouldNotGrow {
+                    current: mmap.size,
+                    attempted_delta: delta,
+                });
             }
         }
 
@@ -128,7 +170,10 @@ impl LinearMemory {
         // limit here.
         if new_pages >= Pages::max_value() {
             // Linear memory size would exceed the index range.
-            return None;
+            return Err(MemoryError::CouldNotGrow {
+                current: mmap.size,
+                attempted_delta: delta,
+            });
         }
 
         let delta_bytes = delta.bytes().0;
@@ -139,9 +184,16 @@ impl LinearMemory {
             // If the new size is within the declared maximum, but needs more memory than we
             // have on hand, it's a dynamic heap and it can move.
             let guard_bytes = self.offset_guard_size;
-            let request_bytes = new_bytes.checked_add(guard_bytes)?;
+            let request_bytes =
+                new_bytes
+                    .checked_add(guard_bytes)
+                    .ok_or_else(|| MemoryError::CouldNotGrow {
+                        current: new_pages,
+                        attempted_delta: Bytes(guard_bytes).into(),
+                    })?;
 
-            let mut new_mmap = Mmap::accessible_reserved(new_bytes, request_bytes).ok()?;
+            let mut new_mmap =
+                Mmap::accessible_reserved(new_bytes, request_bytes).map_err(MemoryError::Region)?;
 
             let copy_len = mmap.alloc.len() - self.offset_guard_size;
             new_mmap.as_mut_slice()[..copy_len].copy_from_slice(&mmap.alloc.as_slice()[..copy_len]);
@@ -149,12 +201,14 @@ impl LinearMemory {
             mmap.alloc = new_mmap;
         } else if delta_bytes > 0 {
             // Make the newly allocated pages accessible.
-            mmap.alloc.make_accessible(prev_bytes, delta_bytes).ok()?;
+            mmap.alloc
+                .make_accessible(prev_bytes, delta_bytes)
+                .map_err(MemoryError::Region)?;
         }
 
         mmap.size = new_pages;
 
-        Some(prev_pages)
+        Ok(prev_pages)
     }
 
     /// Return a `VMMemoryDefinition` for exposing the memory to compiled wasm code.

tests/lib/test-utils/Cargo.toml

@@ -13,6 +13,8 @@ wasmer-compiler = { path = "../../../lib/compiler", version = "0.16.2" }
 wasmer-compiler-singlepass = { path = "../../../lib/compiler-singlepass", version = "0.16.2", optional = true }
 wasmer-compiler-cranelift = { path = "../../../lib/compiler-cranelift", version = "0.16.2", optional = true }
 wasmer-compiler-llvm = { path = "../../../lib/compiler-llvm", version = "0.16.2", optional = true }
+wasmer-engine-jit = { path = "../../../lib/engine-jit", version = "0.16.2" }
+wasmer = { path = "../../../lib/api", version = "0.16.2", deafult-features = false }
 
 [features]
 compiler = []

tests/lib/test-utils/src/lib.rs

@@ -1,6 +1,9 @@
 #![cfg(feature = "compiler")]
 
+use std::sync::Arc;
+use wasmer::{Store, Tunables};
 use wasmer_compiler::{CompilerConfig, Features, Target};
+use wasmer_engine_jit::JITEngine;
 
 pub fn get_compiler_config_from_str(
     compiler_name: &str,
@@ -34,3 +37,10 @@ pub fn get_compiler_config_from_str(
         _ => panic!("Compiler {} not supported", compiler_name),
     }
 }
+
+/// for when you need a store but you don't care about the details
+pub fn get_default_store() -> Store {
+    let compiler_config = get_compiler_config_from_str("cranelift", false, Features::default());
+    let tunables = Tunables::for_target(compiler_config.target().triple());
+    Store::new(Arc::new(JITEngine::new(&*compiler_config, tunables)))
+}

tests/lib/wast/src/spectest.rs

@@ -26,7 +26,7 @@ pub fn spectest_importobject(store: &Store) -> ImportObject {
     let table = Table::new(store, ty, Val::AnyRef(AnyRef::Null)).unwrap();
 
     let ty = MemoryType::new(1, Some(2), false);
-    let memory = Memory::new(store, ty);
+    let memory = Memory::new(store, ty).unwrap();
 
     imports! {
         "spectest" => {

tests/misc.rs

@@ -0,0 +1,41 @@
+use test_utils::get_default_store;
+use wasmer::{Memory, MemoryError, MemoryType, Pages};
+
+#[test]
+fn growing_memory_with_api() {
+    let desc = MemoryType::new(Pages(10), Some(Pages(16)), false);
+    let store = get_default_store();
+
+    let memory = Memory::new(&store, desc).unwrap();
+
+    assert_eq!(memory.size(), Pages(10));
+    let result = memory.grow(Pages(2)).unwrap();
+    assert_eq!(result, Pages(10));
+    assert_eq!(memory.size(), Pages(12));
+
+    let result = memory.grow(Pages(10));
+    assert_eq!(
+        result,
+        Err(MemoryError::CouldNotGrow {
+            current: 12.into(),
+            attempted_delta: 10.into(),
+        })
+    );
+
+    let bad_desc = MemoryType::new(Pages(15), Some(Pages(10)), false);
+    let bad_result = Memory::new(&store, bad_desc);
+
+    // due to stack overflow with a modern nightly, we can't update CI to use a version of nightly which will make this work
+    /*assert!(matches!(
+        bad_result,
+        Err(MemoryError::InvalidMemoryPlan { .. })
+    ));*/
+
+    assert!(
+        if let Err(MemoryError::InvalidMemoryPlan { .. }) = bad_result {
+            true
+        } else {
+            false
+        }
+    );
+}