winch: Solidify bounds check for dynamic heaps (#9156)

* winch: Solidify bounds check for dynamic heaps

This commit fixes and edge case for bounds checks for dynamic heaps.

https://github.com/bytecodealliance/wasmtime/pull/8157/files erroneously
tied the bounds check operation (more concretely the overflow check) to the size derived from from the heap
type. Even though offsets and access sizes are validated ahead-of-time
and bound to the heap type, in the case of overflow checking, we must
ensure that the operation size is tied to the target's pointer size to
avoid clamping the access size and offset addition, which would result
in missing an out-of-bounds memory access.

This commit also adds a disassembly test to avoid introducing
regressions in the future.

Additionally, this commit adds more comments around why `pointer_size`
is used for certain bounds checking operations.

* Update disassembly test

tests/disas/winch/x64/store/oob.wat

@@ -0,0 +1,48 @@
+;;! target = "x86_64"
+;;! test = "winch"
+;;! flags = " -O static-memory-maximum-size=0"
+(module
+  (memory 1)
+  (func (export "foo") (param $i i32)
+    i32.const 0
+    (local.get $i)
+    i32.store8 offset=4294967295
+  )
+)
+
+;; wasm[0]::function[0]:
+;;       pushq   %rbp
+;;       movq    %rsp, %rbp
+;;       movq    8(%rdi), %r11
+;;       movq    (%r11), %r11
+;;       addq    $0x20, %r11
+;;       cmpq    %rsp, %r11
+;;       ja      0x85
+;;   1b: movq    %rdi, %r14
+;;       subq    $0x20, %rsp
+;;       movq    %rdi, 0x18(%rsp)
+;;       movq    %rsi, 0x10(%rsp)
+;;       movl    %edx, 0xc(%rsp)
+;;       movl    0xc(%rsp), %eax
+;;       movl    $0, %ecx
+;;       movq    0x68(%r14), %rdx
+;;       movl    %ecx, %ebx
+;;       movabsq $0x100000000, %r11
+;;       addq    %r11, %rbx
+;;       jb      0x87
+;;   52: cmpq    %rdx, %rbx
+;;       ja      0x89
+;;   5b: movq    0x60(%r14), %rsi
+;;       addq    %rcx, %rsi
+;;       movabsq $0xffffffff, %r11
+;;       addq    %r11, %rsi
+;;       movq    $0, %rdi
+;;       cmpq    %rdx, %rbx
+;;       cmovaq  %rdi, %rsi
+;;       movb    %al, (%rsi)
+;;       addq    $0x20, %rsp
+;;       popq    %rbp
+;;       retq
+;;   85: ud2
+;;   87: ud2
+;;   89: ud2

tests/misc_testsuite/winch/oob.wast

@@ -0,0 +1,10 @@
+(module
+  (memory 1)
+  (func (export "foo") (param $i i32)
+    i32.const 0
+    (local.get $i)
+    i32.store8 offset=4294967295
+  )
+)
+
+(assert_trap (invoke "foo" (i32.const 0)) "out of bounds") 

winch/codegen/src/codegen/mod.rs

@@ -593,6 +593,14 @@ where
                 // Move the value of the index to the
                 // index_offset_and_access_size register to perform the overflow
                 // check to avoid clobbering the initial index value.
+                //
+                // We derive size of the operation from the heap type since:
+                //
+                // * This is the first assignment to the
+                // `index_offset_and_access_size` register
+                //
+                // * The memory64 proposal specifies that the index is bound to
+                // the heap type instead of hardcoding it to 32-bits (i32).
                 self.masm.mov(
                     index_reg.into(),
                     index_offset_and_access_size,
@@ -601,11 +609,18 @@ where
                 // Perform
                 // index = index + offset + access_size, trapping if the
                 // addition overflows.
+                //
+                // We use the target's pointer size rather than depending on the heap
+                // type since we want to check for overflow; even though the
+                // offset and access size are guaranteed to be bounded by the heap
+                // type, when added, if used with the wrong operand size, their
+                // result could be clamped, resulting in an erroneus overflow
+                // check.
                 self.masm.checked_uadd(
                     index_offset_and_access_size,
                     index_offset_and_access_size,
                     RegImm::i64(offset_with_access_size as i64),
-                    heap.ty.into(),
+                    ptr_size,
                     TrapCode::HeapOutOfBounds,
                 );
 
@@ -623,7 +638,10 @@ where
                         masm.cmp(
                             index_offset_and_access_size.into(),
                             bounds_reg.into(),
-                            heap.ty.into(),
+                            // We use the pointer size to keep the bounds
+                            // comparison consistent with the result of the
+                            // overflow check above.
+                            ptr_size,
                         );
                         IntCmpKind::GtU
                     },
@@ -707,7 +725,12 @@ where
                         masm.cmp(
                             index_reg,
                             RegImm::i64(adjusted_bounds as i64),
-                            heap.ty.into(),
+                            // Similar to the dynamic heap case, even though the
+                            // offset and access size are bound through the heap
+                            // type, when added they can overflow, resulting in
+                            // an erroneus comparison, therfore we rely on the
+                            // target pointer size.
+                            ptr_size,
                         );
                         IntCmpKind::GtU
                     },