From 7e78a9a990c990631adb3f7c7edfd8cda418547f Mon Sep 17 00:00:00 2001 From: Paul Floyd Date: Sun, 15 Sep 2024 09:52:56 +0200 Subject: [PATCH] Bug 492210 - False positive on x86/amd64 with ZF taken directly from addition Also adds similar checks for short and char equivalents to the original int reproducer. Initial fix provided by Alexander Monakov Two versions of the testcase, one with default options and one with -expensive-definedness-checks=yes because the byte operations subb and addb need the flag turned on explicitly. --- .gitignore | 1 + NEWS | 1 + VEX/priv/guest_amd64_helpers.c | 37 ++++++- memcheck/tests/amd64/Makefile.am | 3 + memcheck/tests/amd64/bug492210.c | 106 ++++++++++++++++++++ memcheck/tests/amd64/bug492210_1.stderr.exp | 12 +++ memcheck/tests/amd64/bug492210_1.vgtest | 4 + memcheck/tests/amd64/bug492210_2.stderr.exp | 0 memcheck/tests/amd64/bug492210_2.vgtest | 4 + 9 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 memcheck/tests/amd64/bug492210.c create mode 100644 memcheck/tests/amd64/bug492210_1.stderr.exp create mode 100644 memcheck/tests/amd64/bug492210_1.vgtest create mode 100644 memcheck/tests/amd64/bug492210_2.stderr.exp create mode 100644 memcheck/tests/amd64/bug492210_2.vgtest diff --git a/.gitignore b/.gitignore index bf477a4fa6..c15cf93e00 100644 --- a/.gitignore +++ b/.gitignore @@ -1070,6 +1070,7 @@ /memcheck/tests/amd64/bt_everything /memcheck/tests/amd64/bug132146 /memcheck/tests/amd64/bug279698 +/memcheck/tests/amd64/bug492210 /memcheck/tests/amd64/defcfaexpr /memcheck/tests/amd64/fxsave-amd64 /memcheck/tests/amd64/int3-amd64 diff --git a/NEWS b/NEWS index 3906beedf9..b9cb1d5dc2 100644 --- a/NEWS +++ b/NEWS @@ -62,6 +62,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 490651 Stop using -flto-partition=one 491394 (vgModuleLocal_addDiCfSI): Assertion 'di->fsm.have_rx_map && di->fsm.rw_map_count' failed +492210 False positive on x86/amd64 with ZF taken directly from addition 492214 statx(fd, NULL, AT_EMPTY_PATH) is supported since Linux 6.11 but not supported in valgrind 492663 Valgrind ignores debug info for some binaries diff --git a/VEX/priv/guest_amd64_helpers.c b/VEX/priv/guest_amd64_helpers.c index da1cabc3cb..9997b42669 100644 --- a/VEX/priv/guest_amd64_helpers.c +++ b/VEX/priv/guest_amd64_helpers.c @@ -1062,6 +1062,7 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name, # define binop(_op,_a1,_a2) IRExpr_Binop((_op),(_a1),(_a2)) # define mkU64(_n) IRExpr_Const(IRConst_U64(_n)) # define mkU32(_n) IRExpr_Const(IRConst_U32(_n)) +# define mkU16(_n) IRExpr_Const(IRConst_U16(_n)) # define mkU8(_n) IRExpr_Const(IRConst_U8(_n)) Int i, arity = 0; @@ -1140,6 +1141,15 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name, } + /* 4, */ + if (isU64(cc_op, AMD64G_CC_OP_ADDL) && isU64(cond, AMD64CondZ)) { + /* long long add, then Z --> test ((int)(dst+src) == 0) */ + return unop(Iop_1Uto64, + binop(Iop_CmpEQ32, + unop(Iop_64to32, binop(Iop_Add64, cc_dep1, cc_dep2)), + mkU32(0))); + } + /* 8, 9 */ if (isU64(cc_op, AMD64G_CC_OP_ADDL) && isU64(cond, AMD64CondS)) { /* long add, then S (negative) @@ -1166,6 +1176,29 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name, mkU64(1)); } + /*---------------- ADDW ----------------*/ + + /* 4, */ + if (isU64(cc_op, AMD64G_CC_OP_ADDW) && isU64(cond, AMD64CondZ)) { + + /* long long add, then Z --> test ((short)(dst+src) == 0) */ + return unop(Iop_1Uto64, + binop(Iop_CmpEQ16, + unop(Iop_64to16, binop(Iop_Add64, cc_dep1, cc_dep2)), + mkU16(0))); + } + + /*---------------- ADDB ----------------*/ + + /* 4, */ + if (isU64(cc_op, AMD64G_CC_OP_ADDB) && isU64(cond, AMD64CondZ)) { + /* long long add, then Z --> test ((char)(dst+src) == 0) */ + return unop(Iop_1Uto64, + binop(Iop_CmpEQ8, + unop(Iop_64to8, binop(Iop_Add64, cc_dep1, cc_dep2)), + mkU8(0))); + } + /*---------------- SUBQ ----------------*/ /* 0, */ @@ -1307,6 +1340,8 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name, mkU8(31)), mkU64(1)); } + + /* 1, */ if (isU64(cc_op, AMD64G_CC_OP_SUBL) && isU64(cond, AMD64CondNO)) { /* No action. Never yet found a test case. */ } @@ -1578,7 +1613,7 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name, if (isU64(cc_op, AMD64G_CC_OP_SUBB) && isU64(cond, AMD64CondZ)) { /* byte sub/cmp, then Z --> test dst==src */ return unop(Iop_1Uto64, - binop(Iop_CmpEQ8, + binop(Iop_CmpEQ8, unop(Iop_64to8,cc_dep1), unop(Iop_64to8,cc_dep2))); } diff --git a/memcheck/tests/amd64/Makefile.am b/memcheck/tests/amd64/Makefile.am index 906b8f393b..c0139bdb20 100644 --- a/memcheck/tests/amd64/Makefile.am +++ b/memcheck/tests/amd64/Makefile.am @@ -13,6 +13,8 @@ EXTRA_DIST = \ bt_everything.vgtest \ bug132146.vgtest bug132146.stderr.exp bug132146.stdout.exp \ bug279698.vgtest bug279698.stderr.exp bug279698.stdout.exp \ + bug492210_1.vgtest bug492210_1.stderr.exp \ + bug492210_2.vgtest bug492210_2.stderr.exp \ fxsave-amd64.vgtest fxsave-amd64.stdout.exp fxsave-amd64.stderr.exp \ insn-bsfl.vgtest insn-bsfl.stdout.exp insn-bsfl.stderr.exp \ insn-pcmpistri.vgtest insn-pcmpistri.stdout.exp insn-pcmpistri.stderr.exp \ @@ -46,6 +48,7 @@ check_PROGRAMS = \ bt_everything \ bug132146 \ bug279698 \ + bug492210 \ fxsave-amd64 \ insn-bsfl \ insn-pmovmskb \ diff --git a/memcheck/tests/amd64/bug492210.c b/memcheck/tests/amd64/bug492210.c new file mode 100644 index 0000000000..27e1b17a7c --- /dev/null +++ b/memcheck/tests/amd64/bug492210.c @@ -0,0 +1,106 @@ +/* + * Bug 492210 False positive on x86/amd64 with ZF taken directly from addition + * + * The problem is that the Z flag wasn't being calculated for addl instructions. + * + * The same problem exists for addw and addb. + */ + + +unsigned char b; +unsigned short w; +unsigned long l; +unsigned long long q; + +extern void test(void); + +asm("\n" +".text\n" +"test:\n" + +"\tmovb b, %al\n" +"\tmovb $1, %ah\n" +"\taddb %al, %ah\n" +"\tje label1\n" +"\tlabel1:\n" + +"\taddb %al, %ah\n" +"\tjne label2\n" +"\tlabel2:\n" + +"\tsubb %al, %ah\n" +"\tje label3\n" +"\tlabel3:\n" + +"\tsubb %al, %ah\n" +"\tjne label4\n" +"\tlabel4:\n" + +"\tmov w, %ax\n" +"\tmovw $1, %bx\n" +"\taddw %ax, %bx\n" +"\tje label5\n" +"\tlabel5:\n" + +"\taddw %ax, %bx\n" +"\tjne label6\n" +"\tlabel6:\n" + +"\tsubw %ax, %bx\n" +"\tje label7\n" +"\tlabel7:\n" + +"\tsubw %ax, %bx\n" +"\tjne label8\n" +"\tlabel8:\n" + +"\tmov l, %eax\n" +"\tmov $1, %ebx\n" +"\tadd %eax, %ebx\n" +"\tje label9\n" +"\tlabel9:\n" + +"\tadd %eax, %ebx\n" +"\tjne label10\n" +"\tlabel10:\n" + +"\tsub %eax, %ebx\n" +"\tje label11\n" +"\tlabel11:\n" + +"\tsub %eax, %ebx\n" +"\tjne label12\n" +"\tlabel12:\n" + +"\tmovq q, %rax\n" +"\tmovq $1, %rbx\n" +"\taddq %rax, %rbx\n" +"\tje label13\n" +"\tlabel13:\n" + +"\taddq %rax, %rbx\n" +"\tjne label14\n" +"\tlabel14:\n" + +"\tsubq %rax, %rbx\n" +"\tje label15\n" +"\tlabel15:\n" + +"\tsubq %rax, %rbx\n" +"\tjne label16\n" +"\tlabel16:\n" + +"\tret\n" +".previous\n" +); + +int main() +{ + unsigned long long uninit; + uninit &= 0xfffffffffffffffe; + b = uninit; + w = uninit; + l = uninit; + q = uninit; + test(); +} diff --git a/memcheck/tests/amd64/bug492210_1.stderr.exp b/memcheck/tests/amd64/bug492210_1.stderr.exp new file mode 100644 index 0000000000..afaaad650e --- /dev/null +++ b/memcheck/tests/amd64/bug492210_1.stderr.exp @@ -0,0 +1,12 @@ +Conditional jump or move depends on uninitialised value(s) + ... + +Conditional jump or move depends on uninitialised value(s) + ... + +Conditional jump or move depends on uninitialised value(s) + ... + +Conditional jump or move depends on uninitialised value(s) + ... + diff --git a/memcheck/tests/amd64/bug492210_1.vgtest b/memcheck/tests/amd64/bug492210_1.vgtest new file mode 100644 index 0000000000..e80be84ec8 --- /dev/null +++ b/memcheck/tests/amd64/bug492210_1.vgtest @@ -0,0 +1,4 @@ +prog: bug492210 +vgopts: -q + + diff --git a/memcheck/tests/amd64/bug492210_2.stderr.exp b/memcheck/tests/amd64/bug492210_2.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/amd64/bug492210_2.vgtest b/memcheck/tests/amd64/bug492210_2.vgtest new file mode 100644 index 0000000000..1c4e3614f8 --- /dev/null +++ b/memcheck/tests/amd64/bug492210_2.vgtest @@ -0,0 +1,4 @@ +prog: bug492210 +vgopts: -q --expensive-definedness-checks=yes + + -- 2.47.2