On Fri, May 23, 2014 at 8:45 PM, Jakub Jelinek <ja...@redhat.com> wrote: > 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
With clang I get this nice report: ==20989==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffffffdf02 at pc 0x4b20db bp 0x7fffffffdd70 sp 0x7fffffffdd68 READ of size 4 at 0x7fffffffdf02 thread T0 #0 0x4b20da in foo(S*) /home/kcc/tmp/jakub-unaligned.c:6 #1 0x4b2744 in main /home/kcc/tmp/jakub-unaligned.c:30 #2 0x7ffff6bfd76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #3 0x4b1e6c in _start (/usr/local/google/kcc/llvm_cmake/a.out+0x4b1e6c) Address 0x7fffffffdf02 is located in stack of thread T0 at offset 66 in frame #0 0x4b24ef in main /home/kcc/tmp/jakub-unaligned.c:23 This frame has 5 object(s): [32, 36) 'retval' [48, 64) 't' <== Memory access at offset 66 overflows this variable [80, 84) 'v' [96, 104) 'p' [128, 132) 'w' 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: stack-buffer-overflow /home/kcc/tmp/jakub-unaligned.c:6 foo(S*) Shadow bytes around the buggy address: 0x10007fff7b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7bb0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 0x10007fff7bc0: 00 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7bd0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 04 f2 00 00 =>0x10007fff7be0:[f2]f2 04 f2 00 f2 f2 f2 04 f3 f3 f3 00 00 00 00 0x10007fff7bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10007fff7c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 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