On Fri, May 23, 2014 at 04:11:48PM +0400, Konstantin Serebryany wrote: > >> 2) it doesn't still deal with unaligned power of two accesses properly, > >> but neither does llvm (at least not 3.4). Am not talking about > >> undefined behavior cases where the compiler isn't told the access > >> is misaligned, but e.g. when accessing struct S { int x; } > >> __attribute__((packed)) and similar (or aligned(1)). Supposedly > >> we could force __asan_report_*_n for that case too, because > >> normal wider check assumes it is aligned > > > > Yep, we don't do it. > Now we do: http://llvm.org/viewvc/llvm-project?rev=209508&view=rev
Here is the GCC side of the thing, ok for trunk if it bootstraps/regtests? (on top of the earlier posted two patches). Note, I think some work is needed on the library side, ERROR: AddressSanitizer: unknown-crash on address 0xffc439cf at pc 0x804898e bp 0xffc438d8 sp 0xffc438cc READ of size 4 at 0xffc439cf thread T0 #0 0x804898d in foo /usr/src/gcc/gcc/testsuite/c-c++-common/asan/misalign-1.c:10 #1 0x8048746 in main /usr/src/gcc/gcc/testsuite/c-c++-common/asan/misalign-1.c:34 #2 0x49e39b72 in __libc_start_main (/lib/libc.so.6+0x49e39b72) #3 0x8048848 (/usr/src/gcc/obj2/gcc/testsuite/gcc/misalign-1.exe+0x8048848) Address 0xffc439cf is located in stack of thread T0 at offset 175 in frame #0 0x804868f in main /usr/src/gcc/gcc/testsuite/c-c++-common/asan/misalign-1.c:27 This frame has 3 object(s): [32, 36) 'v' [96, 100) 'w' [160, 176) 't' <== Memory access at offset 175 partially overflows this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: unknown-crash /usr/src/gcc/gcc/testsuite/c-c++-common/asan/misalign-1.c:10 foo Shadow bytes around the buggy address: 0x3ff886e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff886f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88720: 00 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2 =>0x3ff88730: 04 f4 f4 f4 f2 f2 f2 f2 00[00]f4 f4 f3 f3 f3 f3 0x3ff88740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff88780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc ASan internal: fe is just too ugly (I mean, it shouldn't print unknown-crash). It is true that the first byte of the __asan_report_load_n range corresponds to shadow byte 0, but for _n you should either check it for all bytes in that range, or at least the first and last byte (which would correspond to what the caller of __asan_report_*_n actually does right now). 2014-05-23 Jakub Jelinek <ja...@redhat.com> * asan.c (report_error_func): Add SLOW_P argument, use BUILT_IN_ASAN_*_N if set. (build_check_stmt): Likewise. (instrument_derefs): If T has insufficient alignment, force same handling as for odd sizes. * c-c++-common/asan/misalign-1.c: New test. * c-c++-common/asan/misalign-2.c: New test. --- gcc/asan.c.jj 2014-05-23 17:17:46.000000000 +0200 +++ gcc/asan.c 2014-05-23 18:14:08.630807014 +0200 @@ -1319,7 +1319,7 @@ asan_protect_global (tree decl) IS_STORE is either 1 (for a store) or 0 (for a load). */ static tree -report_error_func (bool is_store, HOST_WIDE_INT size_in_bytes) +report_error_func (bool is_store, HOST_WIDE_INT size_in_bytes, bool slow_p) { static enum built_in_function report[2][6] = { { BUILT_IN_ASAN_REPORT_LOAD1, BUILT_IN_ASAN_REPORT_LOAD2, @@ -1329,7 +1329,8 @@ report_error_func (bool is_store, HOST_W BUILT_IN_ASAN_REPORT_STORE4, BUILT_IN_ASAN_REPORT_STORE8, BUILT_IN_ASAN_REPORT_STORE16, BUILT_IN_ASAN_REPORT_STORE_N } }; if ((size_in_bytes & (size_in_bytes - 1)) != 0 - || size_in_bytes > 16) + || size_in_bytes > 16 + || slow_p) return builtin_decl_implicit (report[is_store][5]); return builtin_decl_implicit (report[is_store][exact_log2 (size_in_bytes)]); } @@ -1508,7 +1509,8 @@ build_shadow_mem_access (gimple_stmt_ite static void build_check_stmt (location_t location, tree base, gimple_stmt_iterator *iter, - bool before_p, bool is_store, HOST_WIDE_INT size_in_bytes) + bool before_p, bool is_store, HOST_WIDE_INT size_in_bytes, + bool slow_p = false) { gimple_stmt_iterator gsi; basic_block then_bb, else_bb; @@ -1522,9 +1524,15 @@ build_check_stmt (location_t location, t HOST_WIDE_INT real_size_in_bytes = size_in_bytes; tree sz_arg = NULL_TREE; - if ((size_in_bytes & (size_in_bytes - 1)) != 0 - || size_in_bytes > 16) - real_size_in_bytes = 1; + if (size_in_bytes == 1) + slow_p = false; + else if ((size_in_bytes & (size_in_bytes - 1)) != 0 + || size_in_bytes > 16 + || slow_p) + { + real_size_in_bytes = 1; + slow_p = true; + } /* Get an iterator on the point where we can add the condition statement for the instrumentation. */ @@ -1582,8 +1590,8 @@ build_check_stmt (location_t location, t t = gimple_assign_lhs (gimple_seq_last (seq)); gimple_seq_set_location (seq, location); gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); - /* For weird access sizes, check first and last byte. */ - if (real_size_in_bytes != size_in_bytes) + /* For weird access sizes or misaligned, check first and last byte. */ + if (slow_p) { g = gimple_build_assign_with_ops (PLUS_EXPR, make_ssa_name (uintptr_type, NULL), @@ -1626,7 +1634,7 @@ build_check_stmt (location_t location, t /* Generate call to the run-time library (e.g. __asan_report_load8). */ gsi = gsi_start_bb (then_bb); - g = gimple_build_call (report_error_func (is_store, size_in_bytes), + g = gimple_build_call (report_error_func (is_store, size_in_bytes, slow_p), sz_arg ? 2 : 1, base_addr, sz_arg); gimple_set_location (g, location); gsi_insert_after (&gsi, g, GSI_NEW_STMT); @@ -1722,8 +1730,31 @@ instrument_derefs (gimple_stmt_iterator base = build_fold_addr_expr (t); if (!has_mem_ref_been_instrumented (base, size_in_bytes)) { + bool slow_p = false; + if (size_in_bytes > 1) + { + if ((size_in_bytes & (size_in_bytes - 1)) != 0 + || size_in_bytes > 16) + slow_p = true; + else + { + unsigned int align = get_object_alignment (t); + if (align < size_in_bytes * BITS_PER_UNIT) + { + /* On non-strict alignment targets, if + 16-byte access is just 8-byte aligned, + this will result in misaligned shadow + memory 2 byte load, but otherwise can + be handled using one read. */ + if (size_in_bytes != 16 + || STRICT_ALIGNMENT + || align < 8 * BITS_PER_UNIT) + slow_p = true; + } + } + } build_check_stmt (location, base, iter, /*before_p=*/true, - is_store, size_in_bytes); + is_store, size_in_bytes, slow_p); update_mem_ref_hash_table (base, size_in_bytes); update_mem_ref_hash_table (t, size_in_bytes); } --- gcc/testsuite/c-c++-common/asan/misalign-1.c.jj 2014-05-23 18:03:40.400842565 +0200 +++ gcc/testsuite/c-c++-common/asan/misalign-1.c 2014-05-23 18:29:42.899264340 +0200 @@ -0,0 +1,42 @@ +/* { dg-do run { target { ilp32 || lp64 } } } */ +/* { dg-options "-O2" } */ +/* { dg-shouldfail "asan" } */ + +struct S { int i; } __attribute__ ((packed)); + +__attribute__((noinline, noclone)) int +foo (struct S *s) +{ + return s->i; +} + +__attribute__((noinline, noclone)) int +bar (int *s) +{ + return *s; +} + +__attribute__((noinline, noclone)) struct S +baz (struct S *s) +{ + return *s; +} + +int +main () +{ + struct T { char a[3]; struct S b[3]; char c; } t; + int v = 5; + struct S *p = t.b; + asm volatile ("" : "+rm" (p)); + p += 3; + if (bar (&v) != 5) __builtin_abort (); + volatile int w = foo (p); + return 0; +} + +/* { dg-output "ERROR: AddressSanitizer:\[^\n\r]*on address\[^\n\r]*" } */ +/* { dg-output "0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output " #0 0x\[0-9a-f\]+ (in _*foo(\[^\n\r]*misalign-1.c:10|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output " #1 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*misalign-1.c:34|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ --- gcc/testsuite/c-c++-common/asan/misalign-2.c.jj 2014-05-23 18:07:59.820594400 +0200 +++ gcc/testsuite/c-c++-common/asan/misalign-2.c 2014-05-23 18:29:51.808220759 +0200 @@ -0,0 +1,42 @@ +/* { dg-do run { target { ilp32 || lp64 } } } */ +/* { dg-options "-O2" } */ +/* { dg-shouldfail "asan" } */ + +struct S { int i; } __attribute__ ((packed)); + +__attribute__((noinline, noclone)) int +foo (struct S *s) +{ + return s->i; +} + +__attribute__((noinline, noclone)) int +bar (int *s) +{ + return *s; +} + +__attribute__((noinline, noclone)) struct S +baz (struct S *s) +{ + return *s; +} + +int +main () +{ + struct T { char a[3]; struct S b[3]; char c; } t; + int v = 5; + struct S *p = t.b; + asm volatile ("" : "+rm" (p)); + p += 3; + if (bar (&v) != 5) __builtin_abort (); + volatile struct S w = baz (p); + return 0; +} + +/* { dg-output "ERROR: AddressSanitizer:\[^\n\r]*on address\[^\n\r]*" } */ +/* { dg-output "0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output " #0 0x\[0-9a-f\]+ (in _*baz(\[^\n\r]*misalign-2.c:22|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output " #1 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*misalign-2.c:34|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ Jakub