Merge branch 'tiago/wasmer-read-only-mem' (#3315)

* origin/tiago/wasmer-read-only-mem:
  Changelog for #3315
  Test that RO wasm memory cannot grow
  Avoid growing wasm memory when performing reads

.changelog/unreleased/improvements/3315-wasmer-read-only-mem.md

@@ -0,0 +1,2 @@
+- Avoid growing wasm memory when performing read-only accesses.
+  ([\#3315](https://github.com/anoma/namada/pull/3315))

crates/namada/src/vm/wasm/memory.rs

@@ -26,6 +26,8 @@ use crate::vm::types::VpInput;
 #[allow(missing_docs)]
 #[derive(Error, Debug)]
 pub enum Error {
+    #[error("Attempted to modify read-only memory")]
+    ReadOnly,
     #[error("Offset {0}+{1} overflows 32 bits storage")]
     OverflowingOffset(u64, usize),
     #[error("Failed initializing the memory: {0}")]
@@ -186,14 +188,17 @@ pub fn write_vp_inputs(
 
 /// Check that the given offset and length fits into the memory bounds. If not,
 /// it will try to grow the memory.
-// TODO(namada#3314): avoid growing memory if we're only performing reads;
-// return an Err instead
-fn check_bounds(
-    store: &mut impl wasmer::AsStoreMut,
+fn check_bounds<F, S>(
+    store: &mut S,
     memory: &Memory,
     base_addr: u64,
     offset: usize,
-) -> Result<()> {
+    grow_callback: F,
+) -> Result<()>
+where
+    S: wasmer::AsStoreMut,
+    F: Fn(u64, &Memory, &mut S) -> Result<()>,
+{
     let store_mut = store.as_store_mut();
     let memview = memory.view(&store_mut);
 
@@ -216,26 +221,7 @@ fn check_bounds(
         })
         .ok_or(Error::OverflowingOffset(base_addr, offset))?;
     if memview.data_size() < desired_offset {
-        let cur_pages = memview.size().0 as usize;
-        let capacity = checked!(cur_pages * WASM_PAGE_SIZE)?;
-        // usizes should be at least 32 bits wide on most architectures,
-        // so this cast shouldn't cause panics, given the invariant that
-        // `desired_offset` is at most a 32 bit wide value. moreover,
-        // `capacity` should not be larger than `memory.data_size()`,
-        // so this subtraction should never fail
-        let desired_offset = usize::try_from(desired_offset)?;
-        let missing = checked!(desired_offset - capacity)?;
-        // extrapolate the number of pages missing to allow addressing
-        // the desired memory offset
-        let req_pages =
-            checked!((missing + WASM_PAGE_SIZE - 1) / WASM_PAGE_SIZE)?;
-        let req_pages: u32 = u32::try_from(req_pages)?;
-        tracing::debug!(req_pages, "Attempting to grow wasm memory");
-        memory.grow(store, req_pages).map_err(Error::Grow)?;
-        tracing::debug!(
-            mem_size = memory.view(&store.as_store_mut()).data_size(),
-            "Wasm memory size has been successfully extended"
-        );
+        grow_callback(desired_offset, memory, store)?;
     }
     Ok(())
 }
@@ -247,7 +233,13 @@ fn read_memory_bytes(
     offset: u64,
     len: usize,
 ) -> Result<Vec<u8>> {
-    check_bounds(store, memory, offset, len)?;
+    check_bounds(
+        store,
+        memory,
+        offset,
+        len,
+        |_desired_offset, _memory, _store| Err(Error::ReadOnly),
+    )?;
     let mut buf = vec![0; len];
     memory.view(&store.as_store_mut()).read(offset, &mut buf)?;
     Ok(buf)
@@ -261,7 +253,43 @@ fn write_memory_bytes(
     bytes: impl AsRef<[u8]>,
 ) -> Result<()> {
     let buf = bytes.as_ref();
-    check_bounds(store, memory, offset, buf.len() as _)?;
+    check_bounds(
+        store,
+        memory,
+        offset,
+        buf.len() as _,
+        |desired_offset, memory, store| {
+            let store_mut = store.as_store_mut();
+            let memview = memory.view(&store_mut);
+
+            let cur_pages = memview.size().0 as usize;
+            let capacity = checked!(cur_pages * WASM_PAGE_SIZE)?;
+
+            // usizes should be at least 32 bits wide on most architectures,
+            // so this cast shouldn't cause panics, given the invariant that
+            // `desired_offset` is at most a 32 bit wide value. moreover,
+            // `capacity` should not be larger than `memory.data_size()`,
+            // so this subtraction should never fail
+            let desired_offset = usize::try_from(desired_offset)?;
+            let missing = checked!(desired_offset - capacity)?;
+
+            // extrapolate the number of pages missing to allow addressing
+            // the desired memory offset
+            let req_pages =
+                checked!((missing + WASM_PAGE_SIZE - 1) / WASM_PAGE_SIZE)?;
+            let req_pages: u32 = u32::try_from(req_pages)?;
+
+            tracing::debug!(req_pages, "Attempting to grow wasm memory");
+
+            memory.grow(store, req_pages).map_err(Error::Grow)?;
+            tracing::debug!(
+                mem_size = memory.view(&store.as_store_mut()).data_size(),
+                "Wasm memory size has been successfully extended"
+            );
+
+            Ok(())
+        },
+    )?;
     memory.view(&store.as_store_mut()).write(offset, buf)?;
     Ok(())
 }

crates/namada/src/vm/wasm/run.rs

@@ -940,14 +940,17 @@ mod tests {
 
     use borsh_ext::BorshSerializeExt;
     use itertools::Either;
+    use namada_sdk::arith::checked;
     use namada_state::StorageWrite;
     use namada_test_utils::TestWasms;
     use namada_token::DenominatedAmount;
     use namada_tx::data::{Fee, TxType};
     use namada_tx::{Code, Data};
     use test_log::test;
+    use wasmer::WASM_PAGE_SIZE;
     use wasmer_vm::TrapCode;
 
+    use super::memory::{TX_MEMORY_INIT_PAGES, VP_MEMORY_INIT_PAGES};
     use super::*;
     use crate::state::testing::TestState;
     use crate::tx::data::eval_vp::EvalVp;
@@ -985,15 +988,15 @@ mod tests {
         let error = execute_tx_with_code(tx_code).expect_err(PANIC_MSG);
         assert!(
             matches!(
-                assert_rt_mem_error(&error, PANIC_MSG),
+                assert_tx_rt_mem_error(&error, PANIC_MSG),
                 memory::Error::OverflowingOffset(18446744073709551615, 1),
             ),
             "{PANIC_MSG}"
         );
     }
 
-    /// Extract a wasm runtime memory error from some [`Error`].
-    fn assert_rt_mem_error<'err>(
+    /// Extract a tx wasm runtime memory error from some [`Error`].
+    fn assert_tx_rt_mem_error<'err>(
         error: &'err Error,
         assert_msg: &str,
     ) -> &'err memory::Error {
@@ -1014,6 +1017,29 @@ mod tests {
             .unwrap_or_else(|| panic!("{assert_msg}: {tx_mem_err}"))
     }
 
+    /// Extract a vp wasm runtime memory error from some [`Error`].
+    fn assert_vp_rt_mem_error<'err>(
+        error: &'err Error,
+        assert_msg: &str,
+    ) -> &'err memory::Error {
+        let Error::RuntimeError(rt_error) = error else {
+            panic!("{assert_msg}: {error}");
+        };
+        let source_err =
+            rt_error.source().expect("No runtime error source found");
+        let downcasted_tx_rt_err: &vp_host_fns::RuntimeError = source_err
+            .downcast_ref()
+            .unwrap_or_else(|| panic!("{assert_msg}: {source_err}"));
+        let vp_host_fns::RuntimeError::MemoryError(vp_mem_err) =
+            downcasted_tx_rt_err
+        else {
+            panic!("{assert_msg}: {downcasted_tx_rt_err}");
+        };
+        vp_mem_err
+            .downcast_ref()
+            .unwrap_or_else(|| panic!("{assert_msg}: {vp_mem_err}"))
+    }
+
     /// Test that when a transaction wasm goes over the stack-height limit, the
     /// execution is aborted.
     #[test]
@@ -1851,6 +1877,116 @@ mod tests {
         assert!(matches!(result.unwrap_err(), Error::GasError(_)));
     }
 
+    #[test]
+    fn test_tx_ro_memory_wont_grow() {
+        // a transaction that accesses memory out of bounds
+        let out_of_bounds_index =
+            checked!(2usize * TX_MEMORY_INIT_PAGES as usize * WASM_PAGE_SIZE)
+                .unwrap();
+        let tx_code = wasmer::wat2wasm(format!(
+            r#"
+            (module
+                (import "env" "namada_tx_read" (func (param i64 i64) (result i64)))
+                (func (param i64 i64) (result i64)
+                    i64.const {out_of_bounds_index}
+                    i64.const 1
+                    (call 0)
+                )
+                (memory 16)
+                (export "memory" (memory 0))
+                (export "_apply_tx" (func 1))
+            )
+            "#
+        ).as_bytes())
+        .expect("unexpected error converting wat2wasm")
+        .into_owned();
+
+        const PANIC_MSG: &str =
+            "Test should have failed with a wasm runtime memory error";
+
+        let error = execute_tx_with_code(tx_code).expect_err(PANIC_MSG);
+        assert!(
+            matches!(
+                assert_tx_rt_mem_error(&error, PANIC_MSG),
+                memory::Error::ReadOnly,
+            ),
+            "{PANIC_MSG}"
+        );
+    }
+
+    #[test]
+    fn test_vp_ro_memory_wont_grow() {
+        // vp code that accesses memory out of bounds
+        let out_of_bounds_index =
+            checked!(2usize * VP_MEMORY_INIT_PAGES as usize * WASM_PAGE_SIZE)
+                .unwrap();
+        let vp_code = wasmer::wat2wasm(format!(
+            r#"
+            (module
+                (type (;0;) (func (param i64 i64 i64 i64 i64 i64 i64 i64) (result i64)))
+                (import "env" "namada_vp_read_pre" (func (param i64 i64) (result i64)))
+
+                (func $_validate_tx (type 0) (param i64 i64 i64 i64 i64 i64 i64 i64) (result i64)
+                    i64.const {out_of_bounds_index}
+                    i64.const 1
+                    (call 0)
+                )
+
+                (table (;0;) 1 1 funcref)
+                (memory (;0;) 16)
+                (global (;0;) (mut i32) (i32.const 1048576))
+                (export "memory" (memory 0))
+                (export "_validate_tx" (func $_validate_tx)))
+            "#).as_bytes(),
+        )
+        .expect("unexpected error converting wat2wasm").into_owned();
+
+        const PANIC_MSG: &str =
+            "Test should have failed with a wasm runtime memory error";
+
+        let error = execute_vp_with_code(vp_code).expect_err(PANIC_MSG);
+        assert!(
+            matches!(
+                assert_vp_rt_mem_error(&error, PANIC_MSG),
+                memory::Error::ReadOnly,
+            ),
+            "{PANIC_MSG}"
+        );
+    }
+
+    fn execute_vp_with_code(vp_code: Vec<u8>) -> Result<()> {
+        let mut outer_tx = Tx::from_type(TxType::Raw);
+        outer_tx.push_default_inner_tx();
+        let tx_index = TxIndex::default();
+        let mut state = TestState::default();
+        let addr = state.in_mem_mut().address_gen.generate_address("rng seed");
+        let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(
+            &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()),
+        ));
+        let keys_changed = BTreeSet::new();
+        let verifiers = BTreeSet::new();
+        let (vp_cache, _) = wasm::compilation_cache::common::testing::cache();
+        // store the vp code
+        let code_hash = Hash::sha256(&vp_code);
+        let code_len = vp_code.len() as u64;
+        let key = Key::wasm_code(&code_hash);
+        let len_key = Key::wasm_code_len(&code_hash);
+        state.write(&key, vp_code).unwrap();
+        state.write(&len_key, code_len).unwrap();
+
+        vp(
+            code_hash,
+            &outer_tx.batch_ref_first_tx(),
+            &tx_index,
+            &addr,
+            &state,
+            &gas_meter,
+            &keys_changed,
+            &verifiers,
+            vp_cache,
+        )
+    }
+
     fn execute_tx_with_code(tx_code: Vec<u8>) -> Result<BTreeSet<Address>> {
         let tx_data = vec![];
         let tx_index = TxIndex::default();
@@ -1954,36 +2090,7 @@ mod tests {
         )
             .expect("unexpected error converting wat2wasm").into_owned();
 
-        let mut outer_tx = Tx::from_type(TxType::Raw);
-        outer_tx.push_default_inner_tx();
-        let tx_index = TxIndex::default();
-        let mut state = TestState::default();
-        let addr = state.in_mem_mut().address_gen.generate_address("rng seed");
-        let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(
-            &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()),
-        ));
-        let keys_changed = BTreeSet::new();
-        let verifiers = BTreeSet::new();
-        let (vp_cache, _) = wasm::compilation_cache::common::testing::cache();
-        // store the vp code
-        let code_hash = Hash::sha256(&vp_code);
-        let code_len = vp_code.len() as u64;
-        let key = Key::wasm_code(&code_hash);
-        let len_key = Key::wasm_code_len(&code_hash);
-        state.write(&key, vp_code).unwrap();
-        state.write(&len_key, code_len).unwrap();
-
-        vp(
-            code_hash,
-            &outer_tx.batch_ref_first_tx(),
-            &tx_index,
-            &addr,
-            &state,
-            &gas_meter,
-            &keys_changed,
-            &verifiers,
-            vp_cache,
-        )
+        execute_vp_with_code(vp_code)
     }
 
     fn get_trap_code(error: &Error) -> Either<TrapCode, String> {