Re: libsanitizer merge from upstream r208536

[Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>]

On Fri, May 23, 2014 at 8:45 PM, Jakub Jelinek <jakub@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  <jakub@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