Fix an off-by-two condition in heap legalization (#3462)

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

@@ -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
+            let limit = limit as i64 + 1;
-            // likely to be a convenient constant on ARM and other RISC architectures.
-            let limit = limit as i64 - 1;
             (IntCC::UnsignedGreaterThanOrEqual, offset, limit)
         } else {
             let limit = limit as i64;

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(())
+}