From c9a81f008d5a5e3c63fa3c6162c6f1fbb5bc6f99 Mon Sep 17 00:00:00 2001
From: Chris Fallin <chris@cfallin.org>
Date: Tue, 1 Dec 2020 23:32:44 -0800
Subject: [PATCH] x64 backend: fix condition-code used for part of explicit
 heap check.

A dynamic heap address computation may create up to two conditional
branches: the usual bounds-check, but also (in some cases) an
offset-addition overflow check.

The x64 backend had reversed the condition code for this check,
resulting in an always-trapping execution for a valid offset. I'm
somewhat surprised this has existed so long, but I suppose the
particular conditions (large offset, small offset guard, dynamic heap)
have been somewhat rare in our testing so far.

Found via fuzzing in #2453.
---
 cranelift/codegen/src/isa/x64/mod.rs          | 12 +++++-----
 .../filetests/filetests/isa/x64/heap.clif     | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 cranelift/filetests/filetests/isa/x64/heap.clif

diff --git a/cranelift/codegen/src/isa/x64/mod.rs b/cranelift/codegen/src/isa/x64/mod.rs
index fd4444498d..73183f79e8 100644
--- a/cranelift/codegen/src/isa/x64/mod.rs
+++ b/cranelift/codegen/src/isa/x64/mod.rs
@@ -92,15 +92,15 @@ impl MachBackend for X64Backend {
     }
 
     fn unsigned_add_overflow_condition(&self) -> IntCC {
-        // Unsigned `>=`; this corresponds to the carry flag set on x86, which happens on
-        // overflow of an add.
-        IntCC::UnsignedGreaterThanOrEqual
+        // Unsigned `<`; this corresponds to the carry flag set on x86, which
+        // indicates an add has overflowed.
+        IntCC::UnsignedLessThan
     }
 
     fn unsigned_sub_overflow_condition(&self) -> IntCC {
-        // unsigned `>=`; this corresponds to the carry flag set on x86, which happens on
-        // underflow of a subtract (carry is borrow for subtract).
-        IntCC::UnsignedGreaterThanOrEqual
+        // unsigned `<`; this corresponds to the carry flag set on x86, which
+        // indicates a sub has underflowed (carry is borrow for subtract).
+        IntCC::UnsignedLessThan
     }
 
     #[cfg(feature = "unwind")]
diff --git a/cranelift/filetests/filetests/isa/x64/heap.clif b/cranelift/filetests/filetests/isa/x64/heap.clif
new file mode 100644
index 0000000000..d9efb083c6
--- /dev/null
+++ b/cranelift/filetests/filetests/isa/x64/heap.clif
@@ -0,0 +1,23 @@
+test compile
+target x86_64
+feature "experimental_x64"
+
+function %f(i32, i64 vmctx) -> i64 {
+    gv0 = vmctx
+    gv1 = load.i64 notrap aligned gv0+0
+    gv2 = load.i32 notrap aligned gv0+8
+    heap0 = dynamic gv1, bound gv2, offset_guard 0x1000, index_type i32
+
+block0(v0: i32, v1: i64):
+
+    v2 = heap_addr.i64 heap0, v0, 0x8000
+    ; check:  movl    8(%rsi), %r12d
+    ; nextln: movq    %rdi, %r13
+    ; nextln: addl    $$32768, %r13d
+    ; nextln: jnb ; ud2 heap_oob ;
+    ; nextln: cmpl    %r12d, %r13d
+    ; nextln: jbe     label1; j label2
+    ; check:  Block 1:
+ 
+    return v2
+}