Adjust behavior of 4gb memories with custom page sizes (#12884)

* Adjust behavior of 4gb memories with custom page sizes

This commit adjust what happens when a linear memory grows up to 4gb
large when custom page sizes are used. This is an open question in the
upstream proposal at WebAssembly/custom-page-sizes#45 but without any
special handling a return value of -1 is ambiguous if it succeeded or
failed. For now eagerly trap memory operations reaching these conditions
while the upstream specification question is resolved.

* Fix CI

* Debug CI failure

prtest:full

* Fix 32-bit platforms

Author: alexcrichton

crates/environ/src/types.rs

@@ -2320,6 +2320,36 @@ impl Memory {
         let max = self.maximum_byte_size().unwrap_or(u64::MAX);
         max > tunables.memory_reservation
     }
+
+    /// Tests whether this memory type is allowed to grow up to `size` bytes.
+    ///
+    /// This is only applicable to custom-page-size memories which have a page
+    /// size of a single byte. In that situation growth beyond `-1i32 as u32`
+    /// bytes is not allowed because at that point memory growth succeeding and
+    /// failing would be indistinguishable in the return value of `memory.grow`,
+    /// for example. To handle this 32-bit memories are only allowed to grow to
+    /// `-2i32 as u32`, for example, and 64-bit memories with a page size of 1
+    /// are allowed to grow up to the maximum size.
+    pub fn allow_growth_to(&self, size: usize) -> bool {
+        if self.page_size_log2 != 0 {
+            return true;
+        }
+        match self.idx_type {
+            // For a 32-bit memory using 1-byte pages the last 2 bytes of the
+            // 32-bit address space are addressable but disallowed for now.  A
+            // memory that is 4GiB in size cannot report its size via
+            // `memory.size`, and a memory that is 4GiB-1 bytes in size cannot
+            // be distinguished when 1 byte is added from an allocation
+            // failure.  To handle this the memory is capped at 4GiB-2 which
+            // means that all memory-related instructions and such will have
+            // unambiguous return codes.
+            IndexType::I32 => size < 0xffff_ffff,
+
+            // Assume that for a 64-bit memory using 1-byte pages it's going to
+            // exhaust system resources before a limit is actually reached.
+            IndexType::I64 => true,
+        }
+    }
 }
 
 #[derive(Copy, Clone, Debug)]

crates/test-util/src/wast.rs

@@ -457,9 +457,11 @@ impl WastTest {
 
         // Some tests are known to fail with the pooling allocator
         if config.pooling {
+            // allocates too much memory for the pooling configuration here
+            if self.config.hogs_memory() {
+                return true;
+            }
             let unsupported = [
-                // allocates too much memory for the pooling configuration here
-                "misc_testsuite/memory64/more-than-4gb.wast",
                 // shared memories + pooling allocator aren't supported yet
                 "misc_testsuite/memory-combos.wast",
                 "misc_testsuite/threads/atomics-end-of-memory.wast",

crates/wasmtime/src/runtime/vm/memory.rs

@@ -360,15 +360,19 @@ impl Memory {
             )
         })?;
 
+        if !ty.allow_growth_to(minimum) {
+            bail!(
+                "memory minimum size of {} pages exceeds memory limits",
+                ty.limits.min
+            );
+        }
+
         Ok((minimum, maximum))
     }
 
     /// Returns this memory's page size, in bytes.
     pub fn page_size(&self) -> u64 {
-        match self {
-            Memory::Local(mem) => mem.page_size(),
-            Memory::Shared(mem) => mem.page_size(),
-        }
+        self.ty().page_size()
     }
 
     /// Returns the size of this memory, in bytes.
@@ -411,6 +415,39 @@ impl Memory {
         delta_pages: u64,
         limiter: Option<&mut StoreResourceLimiter<'_>>,
     ) -> Result<Option<usize>, Error> {
+        let new_size = delta_pages
+            .checked_mul(self.page_size())
+            .and_then(|new_bytes| {
+                let new_bytes = usize::try_from(new_bytes).ok()?;
+                self.byte_size().checked_add(new_bytes)
+            });
+        match new_size {
+            Some(new_size) => {
+                // FIXME(WebAssembly/custom-page-sizes#45) - what should the
+                // behavior here be exactly? A trap? Return -1? A smaller limit?
+                // Unclear!
+                //
+                // Trap for now to make this as noisy/conservative as possible.
+                if !self.ty().allow_growth_to(new_size) {
+                    bail!(
+                        "disallowing growth to {new_size:#x} bytes based on \
+                         page size"
+                    )
+                }
+            }
+
+            // If the new size in memory isn't representable in a `usize` then
+            // there's no need to actually try to grow it to that size. It's
+            // impossible to succeed so just fail it early.
+            None => {
+                if let Some(limiter) = limiter {
+                    let err = crate::format_err!("memory growth exceeds address space");
+                    limiter.memory_grow_failed(err)?;
+                }
+                return Ok(None);
+            }
+        }
+
         let result = match self {
             Memory::Local(mem) => mem.grow(delta_pages, limiter).await?,
             Memory::Shared(mem) => mem.grow(delta_pages)?,
@@ -513,6 +550,13 @@ impl Memory {
             Memory::Shared(mem) => mem.wasm_accessible(),
         }
     }
+
+    fn ty(&self) -> &wasmtime_environ::Memory {
+        match self {
+            Memory::Local(mem) => mem.ty(),
+            Memory::Shared(mem) => mem.ty(),
+        }
+    }
 }
 
 /// An owned allocation of a wasm linear memory.
@@ -579,8 +623,8 @@ impl LocalMemory {
         })
     }
 
-    pub fn page_size(&self) -> u64 {
-        self.ty.page_size()
+    pub fn ty(&self) -> &wasmtime_environ::Memory {
+        &self.ty
     }
 
     /// Grows a memory by `delta_pages`.
@@ -601,7 +645,7 @@ impl LocalMemory {
             return Ok(Some((old_byte_size, old_byte_size)));
         }
 
-        let page_size = usize::try_from(self.page_size()).unwrap();
+        let page_size = usize::try_from(self.ty().page_size()).unwrap();
 
         // The largest wasm-page-aligned region of memory is possible to
         // represent in a `usize`. This will be impossible for the system to

crates/wasmtime/src/runtime/vm/memory/shared_memory.rs

@@ -67,8 +67,8 @@ impl SharedMemory {
     }
 
     /// Return the memory type for this [`SharedMemory`].
-    pub fn ty(&self) -> wasmtime_environ::Memory {
-        self.0.ty
+    pub fn ty(&self) -> &wasmtime_environ::Memory {
+        &self.0.ty
     }
 
     /// Convert this shared memory into a [`Memory`].
@@ -172,10 +172,6 @@ impl SharedMemory {
         })
     }
 
-    pub(crate) fn page_size(&self) -> u64 {
-        self.0.ty.page_size()
-    }
-
     pub(crate) fn byte_size(&self) -> usize {
         self.0.memory.read().unwrap().byte_size()
     }

crates/wasmtime/src/runtime/vm/memory/shared_memory_disabled.rs

@@ -15,7 +15,7 @@ impl SharedMemory {
         bail!("support for shared memories was disabled at compile time");
     }
 
-    pub fn ty(&self) -> wasmtime_environ::Memory {
+    pub fn ty(&self) -> &wasmtime_environ::Memory {
         match *self {}
     }
 
@@ -53,10 +53,6 @@ impl SharedMemory {
         match *self {}
     }
 
-    pub(crate) fn page_size(&self) -> u64 {
-        match *self {}
-    }
-
     pub(crate) fn byte_size(&self) -> usize {
         match *self {}
     }

tests/all/cli_tests.rs

@@ -876,12 +876,12 @@ fn memory_growth_failure() -> Result<()> {
             "tests/all/cli_tests/memory-grow-failure.wat",
         ])
         .output()?;
-    assert!(!output.status.success());
     let stderr = String::from_utf8_lossy(&output.stderr);
     assert!(
         stderr.contains("forcing a memory growth failure to be a trap"),
         "bad stderr: {stderr}"
     );
+    assert!(!output.status.success());
     Ok(())
 }
 

tests/misc_testsuite/big-memory-behavior.wast

@@ -0,0 +1,14 @@
+;;! hogs_memory = true
+
+(module
+  (memory  0xffff)
+
+  (func (export "grow") (param i32) (result i32)
+    local.get 0
+    memory.grow)
+)
+
+(assert_return (invoke "grow" (i32.const 0)) (i32.const 0xffff))
+(assert_return (invoke "grow" (i32.const 1)) (i32.const 0xffff))
+(assert_return (invoke "grow" (i32.const 0)) (i32.const 0x10000))
+(assert_return (invoke "grow" (i32.const 1)) (i32.const -1))

tests/misc_testsuite/custom-page-sizes/max-size-invalid.wast

@@ -0,0 +1,33 @@
+;;! custom_page_sizes = true
+;;! hogs_memory = true
+
+;; FIXME(WebAssembly/custom-page-sizes#45): unclear what the semantics of this
+;; test should be. For now Wasmtime traps so this tests that traps happen, but
+;; this may change in the specification itself. Either way the problem here is
+;; that with 1-byte pages an allocation of 0xffff_ffff bytes is
+;; indistinguishable from a growth failure which returns -1. Somehow this needs
+;; reconciliation and for now it's done as a trap.
+
+(assert_trap
+  (module
+    (memory 0xffff_ffff (pagesize 1))
+  )
+  "memory minimum size of 4294967295 pages exceeds memory limits")
+
+(module $m
+  (memory (export "memory") 0xffff_fffe (pagesize 1))
+)
+
+(module
+  (import "m" "memory" (memory 0 (pagesize 1)))
+
+  (func (export "grow") (param i32) (result i32)
+    local.get 0
+    memory.grow)
+)
+
+(assert_trap (invoke "grow" (i32.const 1)) "disallowing growth to 0xffffffff bytes based on page size")
+(assert_trap (invoke "grow" (i32.const 2)) "disallowing growth to 0x100000000 bytes based on page size")
+(assert_trap (invoke "grow" (i32.const 100)) "disallowing growth to 0x100000062 bytes based on page size")
+(assert_trap (invoke "grow" (i32.const -1)) "disallowing growth to 0x1fffffffd bytes based on page size")
+(assert_return (invoke "grow" (i32.const 0)) (i32.const -2))

tests/misc_testsuite/memory-combos.wast

@@ -127,7 +127,7 @@
 (assert_trap (invoke "store_m3" (i32.const -1) (i32.const 0)) "out of bounds memory access")
 (assert_return (invoke "grow_m3" (i32.const 1)) (i32.const 1))
 (assert_return (invoke "size_m3") (i32.const 2))
-(assert_return (invoke "grow_m3" (i32.const -1)) (i32.const -1))
+(assert_trap (invoke "grow_m3" (i32.const -1)) "disallowing growth")
 (assert_return (invoke "load_m3" (i32.const 1)) (i32.const 0))
 (assert_return (invoke "store_m3" (i32.const 1) (i32.const 1)))
 (assert_return (invoke "load_m3" (i32.const 1)) (i32.const 1))
@@ -144,7 +144,7 @@
 (assert_trap (invoke "store_m4" (i32.const -1) (i32.const 0)) "out of bounds memory access")
 (assert_return (invoke "grow_m4" (i32.const 1)) (i32.const 1))
 (assert_return (invoke "size_m4") (i32.const 2))
-(assert_return (invoke "grow_m4" (i32.const -1)) (i32.const -1))
+(assert_trap (invoke "grow_m4" (i32.const -1)) "disallowing growth")
 (assert_return (invoke "load_m4" (i32.const 1)) (i32.const 0))
 (assert_return (invoke "store_m4" (i32.const 1) (i32.const 1)))
 (assert_return (invoke "load_m4" (i32.const 1)) (i32.const 1))
@@ -178,7 +178,7 @@
 (assert_trap (invoke "store_m8" (i32.const -1) (i32.const 0)) "out of bounds memory access")
 (assert_return (invoke "grow_m8" (i32.const 1)) (i32.const 1))
 (assert_return (invoke "size_m8") (i32.const 2))
-(assert_return (invoke "grow_m8" (i32.const -1)) (i32.const -1))
+(assert_trap (invoke "grow_m8" (i32.const -1)) "disallowing growth")
 (assert_return (invoke "load_m8" (i32.const 1)) (i32.const 0))
 (assert_return (invoke "store_m8" (i32.const 1) (i32.const 1)))
 (assert_return (invoke "load_m8" (i32.const 1)) (i32.const 1))