From cd43bae6a49c471a1646008bbabed32f01a3e4e7 Mon Sep 17 00:00:00 2001 From: Julian Seward Date: Thu, 8 Mar 2012 14:51:01 +0000 Subject: [PATCH] Change the behaviour of --partial-loads-ok=yes to avoid false negatives, by marking the V bits that come from out of range parts of the access as undefined; and hence any use of them leads to an value error. Prior to this they were marked as defined and could be used without error. Behaviour of --partial-loads-ok=no (the default case) is unchanged. Also add some testing thereof. Fixes #294523. Modified version of a patch and testcase by Patrick J. LoPresti (lopresti@gmail.com). git-svn-id: svn://svn.valgrind.org/valgrind/trunk@12430 --- memcheck/mc_main.c | 100 ++++++++++++++------ memcheck/tests/Makefile.am | 17 ++-- memcheck/tests/test-plo-no.stderr.exp-le32 | 1 + memcheck/tests/test-plo-no.stderr.exp-le64 | 18 ++++ memcheck/tests/test-plo-no.stdout.exp | 0 memcheck/tests/test-plo-no.vgtest | 2 + memcheck/tests/test-plo-yes.stderr.exp-le32 | 1 + memcheck/tests/test-plo-yes.stderr.exp-le64 | 9 ++ memcheck/tests/test-plo-yes.stdout.exp | 0 memcheck/tests/test-plo-yes.vgtest | 2 + memcheck/tests/test-plo.c | 86 +++++++++++++++++ 11 files changed, 201 insertions(+), 35 deletions(-) create mode 100644 memcheck/tests/test-plo-no.stderr.exp-le32 create mode 100644 memcheck/tests/test-plo-no.stderr.exp-le64 create mode 100644 memcheck/tests/test-plo-no.stdout.exp create mode 100644 memcheck/tests/test-plo-no.vgtest create mode 100644 memcheck/tests/test-plo-yes.stderr.exp-le32 create mode 100644 memcheck/tests/test-plo-yes.stderr.exp-le64 create mode 100644 memcheck/tests/test-plo-yes.stdout.exp create mode 100644 memcheck/tests/test-plo-yes.vgtest create mode 100644 memcheck/tests/test-plo.c diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c index d83994f780..12f6f292ed 100644 --- a/memcheck/mc_main.c +++ b/memcheck/mc_main.c @@ -1141,19 +1141,6 @@ static __attribute__((noinline)) ULong mc_LOADVn_slow ( Addr a, SizeT nBits, Bool bigendian ) { - /* Make up a 64-bit result V word, which contains the loaded data for - valid addresses and Defined for invalid addresses. Iterate over - the bytes in the word, from the most significant down to the - least. */ - ULong vbits64 = V_BITS64_UNDEFINED; - SizeT szB = nBits / 8; - SSizeT i; // Must be signed. - SizeT n_addrs_bad = 0; - Addr ai; - Bool partial_load_exemption_applies; - UChar vbits8; - Bool ok; - PROF_EVENT(30, "mc_LOADVn_slow"); /* ------------ BEGIN semi-fast cases ------------ */ @@ -1188,37 +1175,92 @@ ULong mc_LOADVn_slow ( Addr a, SizeT nBits, Bool bigendian ) } /* ------------ END semi-fast cases ------------ */ + ULong vbits64 = V_BITS64_UNDEFINED; /* result */ + ULong pessim64 = V_BITS64_DEFINED; /* only used when p-l-ok=yes */ + SSizeT szB = nBits / 8; + SSizeT i; /* Must be signed. */ + SizeT n_addrs_bad = 0; + Addr ai; + UChar vbits8; + Bool ok; + tl_assert(nBits == 64 || nBits == 32 || nBits == 16 || nBits == 8); + /* Make up a 64-bit result V word, which contains the loaded data + for valid addresses and Defined for invalid addresses. Iterate + over the bytes in the word, from the most significant down to + the least. The vbits to return are calculated into vbits64. + Also compute the pessimising value to be used when + --partial-loads-ok=yes. n_addrs_bad is redundant (the relevant + info can be gleaned from pessim64) but is used as a + cross-check. */ for (i = szB-1; i >= 0; i--) { PROF_EVENT(31, "mc_LOADVn_slow(loop)"); ai = a + byte_offset_w(szB, bigendian, i); ok = get_vbits8(ai, &vbits8); - if (!ok) n_addrs_bad++; vbits64 <<= 8; vbits64 |= vbits8; + if (!ok) n_addrs_bad++; + pessim64 <<= 8; + pessim64 |= (ok ? V_BITS8_DEFINED : V_BITS8_UNDEFINED); } - /* This is a hack which avoids producing errors for code which - insists in stepping along byte strings in aligned word-sized - chunks, and there is a partially defined word at the end. (eg, - optimised strlen). Such code is basically broken at least WRT - semantics of ANSI C, but sometimes users don't have the option - to fix it, and so this option is provided. Note it is now - defaulted to not-engaged. + /* In the common case, all the addresses involved are valid, so we + just return the computed V bits and have done. */ + if (LIKELY(n_addrs_bad == 0)) + return vbits64; - A load from a partially-addressible place is allowed if: - - the command-line flag is set + /* If there's no possibility of getting a partial-loads-ok + exemption, report the error and quit. */ + if (!MC_(clo_partial_loads_ok)) { + MC_(record_address_error)( VG_(get_running_tid)(), a, szB, False ); + return vbits64; + } + + /* The partial-loads-ok excemption might apply. Find out if it + does. If so, don't report an addressing error, but do return + Undefined for the bytes that are out of range, so as to avoid + false negatives. If it doesn't apply, just report an addressing + error in the usual way. */ + + /* Some code steps along byte strings in aligned word-sized chunks + even when there is only a partially defined word at the end (eg, + optimised strlen). This is allowed by the memory model of + modern machines, since an aligned load cannot span two pages and + thus cannot "partially fault". Despite such behaviour being + declared undefined by ANSI C/C++. + + Therefore, a load from a partially-addressible place is allowed + if all of the following hold: + - the command-line flag is set [by default, it isn't] - it's a word-sized, word-aligned load - at least one of the addresses in the word *is* valid + + Since this suppresses the addressing error, we avoid false + negatives by marking bytes undefined when they come from an + invalid address. */ - partial_load_exemption_applies - = MC_(clo_partial_loads_ok) && szB == VG_WORDSIZE - && VG_IS_WORD_ALIGNED(a) - && n_addrs_bad < VG_WORDSIZE; - if (n_addrs_bad > 0 && !partial_load_exemption_applies) - MC_(record_address_error)( VG_(get_running_tid)(), a, szB, False ); + /* "at least one of the addresses is invalid" */ + tl_assert(pessim64 != V_BITS64_DEFINED); + + if (szB == VG_WORDSIZE && VG_IS_WORD_ALIGNED(a) + && n_addrs_bad < VG_WORDSIZE) { + /* Exemption applies. Use the previously computed pessimising + value for vbits64 and return the combined result, but don't + flag an addressing error. The pessimising value is Defined + for valid addresses and Undefined for invalid addresses. */ + /* for assumption that doing bitwise or implements UifU */ + tl_assert(V_BIT_UNDEFINED == 1 && V_BIT_DEFINED == 0); + /* (really need "UifU" here...) + vbits64 UifU= pessim64 (is pessimised by it, iow) */ + vbits64 |= pessim64; + return vbits64; + } + + /* Exemption doesn't apply. Flag an addressing error in the normal + way. */ + MC_(record_address_error)( VG_(get_running_tid)(), a, szB, False ); return vbits64; } diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am index 55ba531e99..08d44e2e83 100644 --- a/memcheck/tests/Makefile.am +++ b/memcheck/tests/Makefile.am @@ -187,15 +187,19 @@ EXTRA_DIST = \ supp2.stderr.exp supp2.vgtest \ supp.supp \ suppfree.stderr.exp suppfree.vgtest \ + test-plo-no.vgtest test-plo-no.stdout.exp \ + test-plo-no.stderr.exp-le64 test-plo-no.stderr.exp-le32 \ + test-plo-yes.vgtest test-plo-yes.stdout.exp \ + test-plo-yes.stderr.exp-le64 test-plo-yes.stderr.exp-le32 \ trivialleak.stderr.exp trivialleak.vgtest trivialleak.stderr.exp2 \ unit_libcbase.stderr.exp unit_libcbase.vgtest \ unit_oset.stderr.exp unit_oset.stdout.exp unit_oset.vgtest \ - varinfo1.vgtest varinfo1.stdout.exp varinfo1.stderr.exp varinfo1.stderr.exp-ppc64\ - varinfo2.vgtest varinfo2.stdout.exp varinfo2.stderr.exp varinfo2.stderr.exp-ppc64\ - varinfo3.vgtest varinfo3.stdout.exp varinfo3.stderr.exp varinfo3.stderr.exp-ppc64\ - varinfo4.vgtest varinfo4.stdout.exp varinfo4.stderr.exp varinfo4.stderr.exp-ppc64\ - varinfo5.vgtest varinfo5.stdout.exp varinfo5.stderr.exp varinfo5.stderr.exp-ppc64\ - varinfo6.vgtest varinfo6.stdout.exp varinfo6.stderr.exp varinfo6.stderr.exp-ppc64\ + varinfo1.vgtest varinfo1.stdout.exp varinfo1.stderr.exp varinfo1.stderr.exp-ppc64 \ + varinfo2.vgtest varinfo2.stdout.exp varinfo2.stderr.exp varinfo2.stderr.exp-ppc64 \ + varinfo3.vgtest varinfo3.stdout.exp varinfo3.stderr.exp varinfo3.stderr.exp-ppc64 \ + varinfo4.vgtest varinfo4.stdout.exp varinfo4.stderr.exp varinfo4.stderr.exp-ppc64 \ + varinfo5.vgtest varinfo5.stdout.exp varinfo5.stderr.exp varinfo5.stderr.exp-ppc64 \ + varinfo6.vgtest varinfo6.stdout.exp varinfo6.stderr.exp varinfo6.stderr.exp-ppc64 \ vcpu_bz2.stdout.exp vcpu_bz2.stderr.exp vcpu_bz2.vgtest \ vcpu_fbench.stdout.exp vcpu_fbench.stderr.exp vcpu_fbench.vgtest \ vcpu_fnfns.stdout.exp vcpu_fnfns.stdout.exp-glibc28-amd64 \ @@ -266,6 +270,7 @@ check_PROGRAMS = \ strchr \ str_tester \ supp_unknown supp1 supp2 suppfree \ + test-plo \ trivialleak \ unit_libcbase unit_oset \ varinfo1 varinfo2 varinfo3 varinfo4 \ diff --git a/memcheck/tests/test-plo-no.stderr.exp-le32 b/memcheck/tests/test-plo-no.stderr.exp-le32 new file mode 100644 index 0000000000..275b89cf02 --- /dev/null +++ b/memcheck/tests/test-plo-no.stderr.exp-le32 @@ -0,0 +1 @@ +XXX put 32 bit results in here diff --git a/memcheck/tests/test-plo-no.stderr.exp-le64 b/memcheck/tests/test-plo-no.stderr.exp-le64 new file mode 100644 index 0000000000..0099d1b340 --- /dev/null +++ b/memcheck/tests/test-plo-no.stderr.exp-le64 @@ -0,0 +1,18 @@ +Invalid read of size 8 + ... + Address 0x........ is 0 bytes inside a block of size 5 alloc'd + at 0x........: memalign (vg_replace_malloc.c:...) + ... + +Invalid read of size 8 + ... + Address 0x........ is 0 bytes inside a block of size 5 alloc'd + at 0x........: memalign (vg_replace_malloc.c:...) + ... + +Invalid read of size 8 + ... + Address 0x........ is 8 bytes inside a block of size 24 free'd + at 0x........: free (vg_replace_malloc.c:...) + ... + diff --git a/memcheck/tests/test-plo-no.stdout.exp b/memcheck/tests/test-plo-no.stdout.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/test-plo-no.vgtest b/memcheck/tests/test-plo-no.vgtest new file mode 100644 index 0000000000..247a134894 --- /dev/null +++ b/memcheck/tests/test-plo-no.vgtest @@ -0,0 +1,2 @@ +prog: test-plo +vgopts: -q diff --git a/memcheck/tests/test-plo-yes.stderr.exp-le32 b/memcheck/tests/test-plo-yes.stderr.exp-le32 new file mode 100644 index 0000000000..275b89cf02 --- /dev/null +++ b/memcheck/tests/test-plo-yes.stderr.exp-le32 @@ -0,0 +1 @@ +XXX put 32 bit results in here diff --git a/memcheck/tests/test-plo-yes.stderr.exp-le64 b/memcheck/tests/test-plo-yes.stderr.exp-le64 new file mode 100644 index 0000000000..22fc3c0edd --- /dev/null +++ b/memcheck/tests/test-plo-yes.stderr.exp-le64 @@ -0,0 +1,9 @@ +Conditional jump or move depends on uninitialised value(s) + ... + +Invalid read of size 8 + ... + Address 0x........ is 8 bytes inside a block of size 24 free'd + at 0x........: free (vg_replace_malloc.c:...) + ... + diff --git a/memcheck/tests/test-plo-yes.stdout.exp b/memcheck/tests/test-plo-yes.stdout.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/test-plo-yes.vgtest b/memcheck/tests/test-plo-yes.vgtest new file mode 100644 index 0000000000..79119ea46c --- /dev/null +++ b/memcheck/tests/test-plo-yes.vgtest @@ -0,0 +1,2 @@ +prog: test-plo +vgopts: -q --partial-loads-ok=yes diff --git a/memcheck/tests/test-plo.c b/memcheck/tests/test-plo.c new file mode 100644 index 0000000000..9ce667a2e3 --- /dev/null +++ b/memcheck/tests/test-plo.c @@ -0,0 +1,86 @@ +#include +#include +#include + +typedef unsigned long long int ULong; +typedef unsigned long int UWord; + +__attribute__((noinline)) +static int my_ffsll ( ULong x ) +{ + int i; + for (i = 0; i < 64; i++) { + if ((x & 1ULL) == 1ULL) + break; + x >>= 1; + } + return i+1; +} + +/* Find length of string, assuming it is aligned and shorter than 8 + characters. Little-endian only. */ +__attribute__((noinline)) +static int aligned_strlen(char *s) +{ + /* This is for 64-bit platforms */ + assert(sizeof(ULong) == 8); + /* ..and only works for aligned input */ + assert(((unsigned long)s & 0x7) == 0); + + /* read 8 bytes */ + ULong val = *(ULong*)s; + /* Subtract one from each byte */ + ULong val2 = val - 0x0101010101010101ULL; + /* Find lowest byte whose high bit changed */ + val2 ^= val; + val2 &= 0x8080808080808080ULL; + + return (my_ffsll(val2) / 8) - 1; +} + +__attribute__((noinline)) void foo ( int x ) +{ + __asm__ __volatile__("":::"memory"); +} + +int +main(int argc, char *argv[]) +{ + char *buf = memalign(8, 5); + buf[0] = 'a'; + buf[1] = 'b'; + buf[2] = 'c'; + buf[3] = 'd'; + buf[4] = '\0'; + + /* --partial-loads-ok=no: expect addr error (here) */ + /* --partial-loads-ok=yes: expect no error */ + if (aligned_strlen(buf) == 4) + foo(44); + + /* --partial-loads-ok=no: expect addr error (here) */ + /* --partial-loads-ok=yes: expect value error (in my_ffsll) */ + buf[4] = 'x'; + if (aligned_strlen(buf) == 0) + foo(37); + + free(buf); + + /* Also, we need to check that a completely out-of-range, + word-sized load gives an addressing error regardless of the + start of --partial-loads-ok=. *And* that the resulting + value is completely defined. */ + UWord* words = malloc(3 * sizeof(UWord)); + free(words); + + /* Should ALWAYS give an addr error. */ + UWord w = words[1]; + + /* Should NEVER give an error (you might expect a value one, but no.) */ + if (w == 0x31415927) { + fprintf(stderr, + "Elvis is alive and well and living in Milton Keynes.\n"); + } + + return 0; +} -- 2.47.2