From 79d7d2169789377656c0444c71d470a9f5081413 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Tue, 19 Oct 2021 09:51:37 -0700
Subject: [PATCH] Fix an off-by-two condition in heap legalization

This commit fixes an issue in Cranelift where legalization of
`heap_addr` instructions (used by wasm to represent heap accesses) could
be off-by-two where loads that should be valid were actually treated as
invalid. The bug here happened in an optimization where tests against
odd constants were being altered to tests against even constants by
subtracting one from the limit instead of adding one to the limit. The
comment around this area has been updated in accordance with a little
more math-stuff as well to help future readers.
---
 cranelift/codegen/src/legalizer/heap.rs | 16 +++++++++--
 tests/all/memory.rs                     | 37 +++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/cranelift/codegen/src/legalizer/heap.rs b/cranelift/codegen/src/legalizer/heap.rs
index 5239c67daf48..f423632a6a6a 100644
--- a/cranelift/codegen/src/legalizer/heap.rs
+++ b/cranelift/codegen/src/legalizer/heap.rs
@@ -157,10 +157,20 @@ fn static_addr(
     let mut spectre_oob_comparison = None;
     offset = cast_offset_to_pointer_ty(offset, offset_ty, addr_ty, &mut pos);
     if offset_ty != ir::types::I32 || limit < 0xffff_ffff {
+        // Here we want to test the condition `offset > limit` and if that's
+        // true then this is an out-of-bounds access and needs to trap. For ARM
+        // and other RISC architectures it's easier to test against an immediate
+        // that's even instead of odd, so if `limit` is odd then we instead test
+        // for `offset >= limit + 1`.
+        //
+        // The thinking behind this is that:
+        //
+        //      A >= B + 1  =>  A - 1 >= B  =>  A > B
+        //
+        // where the last step here is true because A/B are integers, which
+        // should mean that `A >= B + 1` is an equivalent check for `A > B`
         let (cc, lhs, limit_imm) = if limit & 1 == 1 {
-            // Prefer testing `offset >= limit - 1` when limit is odd because an even number is
-            // likely to be a convenient constant on ARM and other RISC architectures.
-            let limit = limit as i64 - 1;
+            let limit = limit as i64 + 1;
             (IntCC::UnsignedGreaterThanOrEqual, offset, limit)
         } else {
             let limit = limit as i64;
diff --git a/tests/all/memory.rs b/tests/all/memory.rs
index bf8f564aa37d..e2d823508b3a 100644
--- a/tests/all/memory.rs
+++ b/tests/all/memory.rs
@@ -321,3 +321,40 @@ fn massive_64_bit_still_limited() -> Result<()> {
         }
     }
 }
+
+#[test]
+fn tiny_static_heap() -> Result<()> {
+    // The size of the memory in the module below is the exact same size as
+    // the static memory size limit in the configuration. This is intended to
+    // specifically test that a load of all the valid addresses of the memory
+    // all pass bounds-checks in cranelift to help weed out any off-by-one bugs.
+    let mut config = Config::new();
+    config.static_memory_maximum_size(65536);
+    let engine = Engine::new(&config)?;
+    let mut store = Store::new(&engine, ());
+
+    let module = Module::new(
+        &engine,
+        r#"
+            (module
+                (memory 1 1)
+                (func (export "run")
+                    (local $i i32)
+
+                    (loop
+                        (if (i32.eq (local.get $i) (i32.const 65536))
+                            (return))
+                        (drop (i32.load8_u (local.get $i)))
+                        (local.set $i (i32.add (local.get $i) (i32.const 1)))
+                        br 0
+                    )
+                )
+            )
+        "#,
+    )?;
+
+    let i = Instance::new(&mut store, &module, &[])?;
+    let f = i.get_typed_func::<(), (), _>(&mut store, "run")?;
+    f.call(&mut store, ())?;
+    Ok(())
+}