libsanitizer merge from upstream r191666

On Fri, Nov 15, 2013 at 12:06:25PM +0400, Konstantin Serebryany wrote:
> On Thu, Nov 14, 2013 at 10:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Nov 14, 2013 at 07:08:17PM +0100, Jakub Jelinek wrote:
> >> On Thu, Nov 14, 2013 at 09:56:36PM +0400, Konstantin Serebryany wrote:
> >> > I thought about alignment but did not reflect it anywhere in the
> >> > interface/comments.
> >> > The alignment should be min(4096, N), which is enough for most purposes.
> >>
> >> You mean max(4096, N), right?
> >
> > Oops, of course min.
> >
> >> And, what exactly is N?  1 << (class_id + 6)?
> 
> Right.
> The minimal allocation unit is 64-aligned 64-bytes.
> Then 128-aligned 128-bytes and so on up to 4096.
> Then only 4096-alignment is guaranteed.

Ok.

> These are the chunks returned by  __asan_stack_malloc.
> The compiler pads them with redzones which in clang (and IIRC in gcc)
> are 32-aligned 32-bytes.

In GCC if no var actually requires 32-byte alignment, the redzones can
be say only 16-byte or 8-byte aligned.

> You raised a valid concern about 512-bit (64-byte) alignment.
> Clang asan does not support that now (will just not add a redzone to
> such variables).
> Someday we'll implement adaptable redzones for stack (we did that for
> globals and heap already) which will fix this.

GCC handles those, though I've found the patch I've posted yesterday
actually broke that when using __asan_stack_malloc.  The problem is that
in GCC what is guaranteed to be aligned properly is the end of the highest
red zone, vars are allocated (sorted by decreasing alignment I think) from
top to bottom.  So, say for:
void bar (char *);
void
foo (void)
{
  char buf[64] __attribute__((aligned (64)));
  bar (buf);
}
there would be 64-byte red zone after buf and 32-byte red zone before buf.
Similarly for
void
baz (void)
{
  char buf[96] __attribute__((aligned (64)));
  bar (buf);
}
there is 32-byte red zone after buf and 32-byte red zone before buf.
The following updated patch, in addition of forcing not using
__asan_stack_malloc_N if it is known not to have sufficient alignment,
will for foo above instead of:
base = __asan_stack_malloc_2 (160, top - 160);
do:
base = __asan_stack_malloc_2 (192, top - 192) + 32;
and to base store the magic word, descriptor string pointer etc.
Is that ok with libsanitizer (i.e. that it doesn't assume that the magic
word must be at what it returns)?  If not, I'd have to increase the
bottom red zone, which I'd like to avoid if possible.
For alignment <= 32 this isn't a problem, the distance between start of
first red zone and end of last red zone is always divisible by 32.

2013-11-15  Jakub Jelinek  <jakub@redhat.com>

	* cfgexpand.c (struct stack_vars_data): Add asan_base and asan_alignb
	fields.
	(expand_stack_vars): For -fsanitize=address, use (and set initially)
	data->asan_base as base for vars and update asan_alignb.
	(expand_used_vars): Initialize data.asan_base and data.asan_alignb.
	Pass them to asan_emit_stack_protection.
	* asan.c (asan_detect_stack_use_after_return): New variable.
	(asan_emit_stack_protection): Add pbase and alignb arguments.
	Implement use after return sanitization.
	* asan.h (asan_emit_stack_protection): Adjust prototype.
	(ASAN_STACK_MAGIC_USE_AFTER_RET, ASAN_STACK_RETIRED_MAGIC): Define.



	Jakub

On Fri, Nov 15, 2013 at 2:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Nov 15, 2013 at 12:06:25PM +0400, Konstantin Serebryany wrote:
>> On Thu, Nov 14, 2013 at 10:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Thu, Nov 14, 2013 at 07:08:17PM +0100, Jakub Jelinek wrote:
>> >> On Thu, Nov 14, 2013 at 09:56:36PM +0400, Konstantin Serebryany wrote:
>> >> > I thought about alignment but did not reflect it anywhere in the
>> >> > interface/comments.
>> >> > The alignment should be min(4096, N), which is enough for most purposes.
>> >>
>> >> You mean max(4096, N), right?
>> >
>> > Oops, of course min.
>> >
>> >> And, what exactly is N?  1 << (class_id + 6)?
>>
>> Right.
>> The minimal allocation unit is 64-aligned 64-bytes.
>> Then 128-aligned 128-bytes and so on up to 4096.
>> Then only 4096-alignment is guaranteed.
>
> Ok.
>
>> These are the chunks returned by  __asan_stack_malloc.
>> The compiler pads them with redzones which in clang (and IIRC in gcc)
>> are 32-aligned 32-bytes.
>
> In GCC if no var actually requires 32-byte alignment, the redzones can
> be say only 16-byte or 8-byte aligned.
>
>> You raised a valid concern about 512-bit (64-byte) alignment.
>> Clang asan does not support that now (will just not add a redzone to
>> such variables).
>> Someday we'll implement adaptable redzones for stack (we did that for
>> globals and heap already) which will fix this.
>
> GCC handles those, though I've found the patch I've posted yesterday
> actually broke that when using __asan_stack_malloc.  The problem is that
> in GCC what is guaranteed to be aligned properly is the end of the highest
> red zone, vars are allocated (sorted by decreasing alignment I think) from
> top to bottom.  So, say for:
> void bar (char *);
> void
> foo (void)
> {
>   char buf[64] __attribute__((aligned (64)));
>   bar (buf);
> }
> there would be 64-byte red zone after buf and 32-byte red zone before buf.
> Similarly for
> void
> baz (void)
> {
>   char buf[96] __attribute__((aligned (64)));
>   bar (buf);
> }
> there is 32-byte red zone after buf and 32-byte red zone before buf.
> The following updated patch, in addition of forcing not using
> __asan_stack_malloc_N if it is known not to have sufficient alignment,
> will for foo above instead of:
> base = __asan_stack_malloc_2 (160, top - 160);
> do:
> base = __asan_stack_malloc_2 (192, top - 192) + 32;

I afraid it actually wants the header (magic, descr, pc) to be in the
first 3 words in the
memory returned by __asan_stack_malloc_*
FakeStack::AddrIsInFakeStack(addr) returns the beginning of the allocated chunk
and then AsanThread::GetFrameNameByAddr expects the header to be there.

--kcc


> and to base store the magic word, descriptor string pointer etc.
> Is that ok with libsanitizer (i.e. that it doesn't assume that the magic
> word must be at what it returns)?  If not, I'd have to increase the
> bottom red zone, which I'd like to avoid if possible.
> For alignment <= 32 this isn't a problem, the distance between start of
> first red zone and end of last red zone is always divisible by 32.
>
> 2013-11-15  Jakub Jelinek  <jakub@redhat.com>
>
>         * cfgexpand.c (struct stack_vars_data): Add asan_base and asan_alignb
>         fields.
>         (expand_stack_vars): For -fsanitize=address, use (and set initially)
>         data->asan_base as base for vars and update asan_alignb.
>         (expand_used_vars): Initialize data.asan_base and data.asan_alignb.
>         Pass them to asan_emit_stack_protection.
>         * asan.c (asan_detect_stack_use_after_return): New variable.
>         (asan_emit_stack_protection): Add pbase and alignb arguments.
>         Implement use after return sanitization.
>         * asan.h (asan_emit_stack_protection): Adjust prototype.
>         (ASAN_STACK_MAGIC_USE_AFTER_RET, ASAN_STACK_RETIRED_MAGIC): Define.
>
> --- gcc/cfgexpand.c.jj  2013-11-15 09:39:40.723074494 +0100
> +++ gcc/cfgexpand.c     2013-11-15 10:04:17.229465817 +0100
> @@ -879,6 +879,12 @@ struct stack_vars_data
>
>    /* Vector of partition representative decls in between the paddings.  */
>    vec<tree> asan_decl_vec;
> +
> +  /* Base pseudo register for Address Sanitizer protected automatic vars.  */
> +  rtx asan_base;
> +
> +  /* Alignment needed for the Address Sanitizer protected automatic vars.  */
> +  unsigned int asan_alignb;
>  };
>
>  /* A subroutine of expand_used_vars.  Give each partition representative
> @@ -963,6 +969,7 @@ expand_stack_vars (bool (*pred) (size_t)
>        alignb = stack_vars[i].alignb;
>        if (alignb * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT)
>         {
> +         base = virtual_stack_vars_rtx;
>           if ((flag_sanitize & SANITIZE_ADDRESS) && pred)
>             {
>               HOST_WIDE_INT prev_offset = frame_offset;
> @@ -991,10 +998,13 @@ expand_stack_vars (bool (*pred) (size_t)
>               if (repr_decl == NULL_TREE)
>                 repr_decl = stack_vars[i].decl;
>               data->asan_decl_vec.safe_push (repr_decl);
> +             data->asan_alignb = MAX (data->asan_alignb, alignb);
> +             if (data->asan_base == NULL)
> +               data->asan_base = gen_reg_rtx (Pmode);
> +             base = data->asan_base;
>             }
>           else
>             offset = alloc_stack_frame_space (stack_vars[i].size, alignb);
> -         base = virtual_stack_vars_rtx;
>           base_align = crtl->max_used_stack_slot_alignment;
>         }
>        else
> @@ -1781,6 +1791,8 @@ expand_used_vars (void)
>
>        data.asan_vec = vNULL;
>        data.asan_decl_vec = vNULL;
> +      data.asan_base = NULL_RTX;
> +      data.asan_alignb = 0;
>
>        /* Reorder decls to be protected by iterating over the variables
>          array multiple times, and allocating out of each phase in turn.  */
> @@ -1813,8 +1825,10 @@ expand_used_vars (void)
>
>           var_end_seq
>             = asan_emit_stack_protection (virtual_stack_vars_rtx,
> +                                         data.asan_base,
> +                                         data.asan_alignb,
>                                           data.asan_vec.address (),
> -                                         data.asan_decl_vec. address (),
> +                                         data.asan_decl_vec.address (),
>                                           data.asan_vec.length ());
>         }
>
> --- gcc/asan.c.jj       2013-11-15 09:39:35.815099787 +0100
> +++ gcc/asan.c  2013-11-15 10:43:59.823217539 +0100
> @@ -227,6 +227,9 @@ alias_set_type asan_shadow_set = -1;
>     alias set is used for all shadow memory accesses.  */
>  static GTY(()) tree shadow_ptr_types[2];
>
> +/* Decl for __asan_option_detect_stack_use_after_return.  */
> +static GTY(()) tree asan_detect_stack_use_after_return;
> +
>  /* Hashtable support for memory references used by gimple
>     statements.  */
>
> @@ -940,20 +943,26 @@ asan_function_start (void)
>     and DECLS is an array of representative decls for each var partition.
>     LENGTH is the length of the OFFSETS array, DECLS array is LENGTH / 2 - 1
>     elements long (OFFSETS include gap before the first variable as well
> -   as gaps after each stack variable).  */
> +   as gaps after each stack variable).  PBASE is, if non-NULL, some pseudo
> +   register which stack vars DECL_RTLs are based on.  Either BASE should be
> +   assigned to PBASE, when not doing use after return protection, or
> +   corresponding address based on __asan_stack_malloc* return value.  */
>
>  rtx
> -asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
> -                           int length)
> +asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
> +                           HOST_WIDE_INT *offsets, tree *decls, int length)
>  {
> -  rtx shadow_base, shadow_mem, ret, mem;
> +  rtx shadow_base, shadow_mem, ret, mem, orig_base, lab;
>    char buf[30];
>    unsigned char shadow_bytes[4];
> -  HOST_WIDE_INT base_offset = offsets[length - 1], offset, prev_offset;
> +  HOST_WIDE_INT base_offset = offsets[length - 1];
> +  HOST_WIDE_INT base_align_bias = 0, offset, prev_offset;
> +  HOST_WIDE_INT asan_frame_size = offsets[0] - base_offset;
>    HOST_WIDE_INT last_offset, last_size;
>    int l;
>    unsigned char cur_shadow_byte = ASAN_STACK_MAGIC_LEFT;
>    tree str_cst, decl, id;
> +  int use_after_return_class = -1;
>
>    if (shadow_ptr_types[0] == NULL_TREE)
>      asan_init_shadow_ptr_types ();
> @@ -983,10 +992,67 @@ asan_emit_stack_protection (rtx base, HO
>    str_cst = asan_pp_string (&asan_pp);
>
>    /* Emit the prologue sequence.  */
> +  if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase)
> +    {
> +      use_after_return_class = floor_log2 (asan_frame_size - 1) - 5;
> +      /* __asan_stack_malloc_N guarantees alignment
> +         N < 6 ? (64 << N) : 4096 bytes.  */
> +      if (alignb > (use_after_return_class < 6
> +                   ? (64U << use_after_return_class) : 4096U))
> +       use_after_return_class = -1;
> +      else if (alignb > ASAN_RED_ZONE_SIZE && (asan_frame_size & (alignb - 1)))
> +       base_align_bias = ((asan_frame_size + alignb - 1)
> +                          & ~(alignb - HOST_WIDE_INT_1)) - asan_frame_size;
> +    }
> +  if (use_after_return_class == -1 && pbase)
> +    emit_move_insn (pbase, base);
>    base = expand_binop (Pmode, add_optab, base,
> -                      gen_int_mode (base_offset, Pmode),
> +                      gen_int_mode (base_offset - base_align_bias, Pmode),
>                        NULL_RTX, 1, OPTAB_DIRECT);
> +  orig_base = NULL_RTX;
> +  if (use_after_return_class != -1)
> +    {
> +      if (asan_detect_stack_use_after_return == NULL_TREE)
> +       {
> +         id = get_identifier ("__asan_option_detect_stack_use_after_return");
> +         decl = build_decl (BUILTINS_LOCATION, VAR_DECL, id,
> +                            integer_type_node);
> +         SET_DECL_ASSEMBLER_NAME (decl, id);
> +         TREE_ADDRESSABLE (decl) = 1;
> +         DECL_ARTIFICIAL (decl) = 1;
> +         DECL_IGNORED_P (decl) = 1;
> +         DECL_EXTERNAL (decl) = 1;
> +         TREE_STATIC (decl) = 1;
> +         TREE_PUBLIC (decl) = 1;
> +         TREE_USED (decl) = 1;
> +         asan_detect_stack_use_after_return = decl;
> +       }
> +      orig_base = gen_reg_rtx (Pmode);
> +      emit_move_insn (orig_base, base);
> +      ret = expand_normal (asan_detect_stack_use_after_return);
> +      lab = gen_label_rtx ();
> +      int very_likely = REG_BR_PROB_BASE - (REG_BR_PROB_BASE / 2000 - 1);
> +      emit_cmp_and_jump_insns (ret, const0_rtx, EQ, NULL_RTX,
> +                              VOIDmode, 0, lab, very_likely);
> +      snprintf (buf, sizeof buf, "__asan_stack_malloc_%d",
> +               use_after_return_class);
> +      ret = init_one_libfunc (buf);
> +      rtx addr = convert_memory_address (ptr_mode, base);
> +      ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, 2,
> +                                    GEN_INT (asan_frame_size
> +                                             + base_align_bias),
> +                                    TYPE_MODE (pointer_sized_int_node),
> +                                    addr, ptr_mode);
> +      ret = convert_memory_address (Pmode, ret);
> +      emit_move_insn (base, ret);
> +      emit_label (lab);
> +      emit_move_insn (pbase, expand_binop (Pmode, add_optab, base,
> +                                          gen_int_mode (base_align_bias
> +                                                        - base_offset, Pmode),
> +                                          NULL_RTX, 1, OPTAB_DIRECT));
> +    }
>    mem = gen_rtx_MEM (ptr_mode, base);
> +  mem = adjust_address (mem, VOIDmode, base_align_bias);
>    emit_move_insn (mem, gen_int_mode (ASAN_STACK_FRAME_MAGIC, ptr_mode));
>    mem = adjust_address (mem, VOIDmode, GET_MODE_SIZE (ptr_mode));
>    emit_move_insn (mem, expand_normal (str_cst));
> @@ -1010,10 +1076,10 @@ asan_emit_stack_protection (rtx base, HO
>    shadow_base = expand_binop (Pmode, lshr_optab, base,
>                               GEN_INT (ASAN_SHADOW_SHIFT),
>                               NULL_RTX, 1, OPTAB_DIRECT);
> -  shadow_base = expand_binop (Pmode, add_optab, shadow_base,
> -                             gen_int_mode (targetm.asan_shadow_offset (),
> -                                           Pmode),
> -                             NULL_RTX, 1, OPTAB_DIRECT);
> +  shadow_base
> +    = plus_constant (Pmode, shadow_base,
> +                    targetm.asan_shadow_offset ()
> +                    + (base_align_bias >> ASAN_SHADOW_SHIFT));
>    gcc_assert (asan_shadow_set != -1
>               && (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
>    shadow_mem = gen_rtx_MEM (SImode, shadow_base);
> @@ -1064,6 +1130,48 @@ asan_emit_stack_protection (rtx base, HO
>    /* Construct epilogue sequence.  */
>    start_sequence ();
>
> +  lab = NULL_RTX;
> +  if (use_after_return_class != -1)
> +    {
> +      rtx lab2 = gen_label_rtx ();
> +      char c = (char) ASAN_STACK_MAGIC_USE_AFTER_RET;
> +      int very_likely = REG_BR_PROB_BASE - (REG_BR_PROB_BASE / 2000 - 1);
> +      emit_cmp_and_jump_insns (orig_base, base, EQ, NULL_RTX,
> +                              VOIDmode, 0, lab2, very_likely);
> +      shadow_mem = gen_rtx_MEM (BLKmode, shadow_base);
> +      set_mem_alias_set (shadow_mem, asan_shadow_set);
> +      mem = gen_rtx_MEM (ptr_mode, base);
> +      mem = adjust_address (mem, VOIDmode, base_align_bias);
> +      emit_move_insn (mem, gen_int_mode (ASAN_STACK_RETIRED_MAGIC, ptr_mode));
> +      if (use_after_return_class < 5
> +         && can_store_by_pieces (asan_frame_size >> ASAN_SHADOW_SHIFT,
> +                                 builtin_memset_read_str, &c, BITS_PER_UNIT,
> +                                 true))
> +       store_by_pieces (shadow_mem, asan_frame_size >> ASAN_SHADOW_SHIFT,
> +                        builtin_memset_read_str, &c, BITS_PER_UNIT, true, 0);
> +      else if (use_after_return_class >= 5
> +              || !set_storage_via_setmem (shadow_mem,
> +                                          GEN_INT (asan_frame_size
> +                                                   >> ASAN_SHADOW_SHIFT),
> +                                          gen_int_mode (c, QImode),
> +                                          BITS_PER_UNIT, BITS_PER_UNIT,
> +                                          -1))
> +       {
> +         snprintf (buf, sizeof buf, "__asan_stack_free_%d",
> +                   use_after_return_class);
> +         ret = init_one_libfunc (buf);
> +         rtx addr = convert_memory_address (ptr_mode, base);
> +         rtx orig_addr = convert_memory_address (ptr_mode, orig_base);
> +         emit_library_call (ret, LCT_NORMAL, ptr_mode, 3, addr, ptr_mode,
> +                            GEN_INT (asan_frame_size + base_align_bias),
> +                            TYPE_MODE (pointer_sized_int_node),
> +                            orig_addr, ptr_mode);
> +       }
> +      lab = gen_label_rtx ();
> +      emit_jump (lab);
> +      emit_label (lab2);
> +    }
> +
>    shadow_mem = gen_rtx_MEM (BLKmode, shadow_base);
>    set_mem_alias_set (shadow_mem, asan_shadow_set);
>    prev_offset = base_offset;
> @@ -1096,6 +1204,8 @@ asan_emit_stack_protection (rtx base, HO
>      }
>
>    do_pending_stack_adjust ();
> +  if (lab)
> +    emit_label (lab);
>
>    ret = get_insns ();
>    end_sequence ();
> --- gcc/asan.h.jj       2013-11-14 18:25:44.435248167 +0100
> +++ gcc/asan.h  2013-11-15 09:48:09.232452574 +0100
> @@ -23,7 +23,8 @@ along with GCC; see the file COPYING3.
>
>  extern void asan_function_start (void);
>  extern void asan_finish_file (void);
> -extern rtx asan_emit_stack_protection (rtx, HOST_WIDE_INT *, tree *, int);
> +extern rtx asan_emit_stack_protection (rtx, rtx, unsigned int, HOST_WIDE_INT *,
> +                                      tree *, int);
>  extern bool asan_protect_global (tree);
>  extern void initialize_sanitizer_builtins (void);
>
> @@ -48,8 +49,10 @@ extern alias_set_type asan_shadow_set;
>  #define ASAN_STACK_MAGIC_MIDDLE                0xf2
>  #define ASAN_STACK_MAGIC_RIGHT         0xf3
>  #define ASAN_STACK_MAGIC_PARTIAL       0xf4
> +#define ASAN_STACK_MAGIC_USE_AFTER_RET 0xf5
>
> -#define ASAN_STACK_FRAME_MAGIC 0x41b58ab3
> +#define ASAN_STACK_FRAME_MAGIC         0x41b58ab3
> +#define ASAN_STACK_RETIRED_MAGIC       0x45e0360e
>
>  /* Return true if DECL should be guarded on the stack.  */
>
>
>
>         Jakub

On Fri, Nov 15, 2013 at 06:12:07PM +0400, Konstantin Serebryany wrote:
> I afraid it actually wants the header (magic, descr, pc) to be in the
> first 3 words in the
> memory returned by __asan_stack_malloc_*
> FakeStack::AddrIsInFakeStack(addr) returns the beginning of the allocated chunk
> and then AsanThread::GetFrameNameByAddr expects the header to be there.

Can it be changed?  I mean, adding potentially very large first red zone
would be quite expensive, and would have to be done unconditionally, even
when not using fake stacks.

I mean, in AsanThread::GetFrameNameByAddr do (pseudo patch):
+  u8 *shadow_bottom;
   if (AddrIsInStack(addr)) {
     bottom = stack_bottom();
+    shadow_bottom = (u8*)MemToShadow(bottom);
   } else if (has_fake_stack()) {
     bottom = fake_stack()->AddrIsInFakeStack(addr);
     CHECK(bottom);
-    *offset = addr - bottom;
-    *frame_pc = ((uptr*)bottom)[2];
-    return  (const char *)((uptr*)bottom)[1];
+    shadow_bottom = (u8*)MemToShadow(bottom);
+    if (*shadow_bottom == kAsanStackLeftRedzoneMagic) {
+      *offset = addr - bottom;
+      *frame_pc = ((uptr*)bottom)[2];
+      return  (const char *)((uptr*)bottom)[1];
+    }
   }
   uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1);  // align addr.
   u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
-  u8 *shadow_bottom = (u8*)MemToShadow(bottom);


	Jakub

On Fri, Nov 15, 2013 at 6:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Nov 15, 2013 at 06:12:07PM +0400, Konstantin Serebryany wrote:
>> I afraid it actually wants the header (magic, descr, pc) to be in the
>> first 3 words in the
>> memory returned by __asan_stack_malloc_*
>> FakeStack::AddrIsInFakeStack(addr) returns the beginning of the allocated chunk
>> and then AsanThread::GetFrameNameByAddr expects the header to be there.
>
> Can it be changed?

Maybe, but not like this.
For fake stack, when the frame is already dead, the shadow will have
different value
(kAsanStackAfterReturnMagic) and the checks (*shadow_ptr ==
kAsanStackLeftRedzoneMagic) will not work

If we do this, we'll need to test very very carefully.
This thing is too subtle -- it required a few attempts to get right.
So I'd prefer not to.

Why can't we create the redzone of max(32, alignment) bytes?


> I mean, adding potentially very large first red zone
> would be quite expensive, and would have to be done unconditionally, even
> when not using fake stacks.
>
> I mean, in AsanThread::GetFrameNameByAddr do (pseudo patch):
> +  u8 *shadow_bottom;
>    if (AddrIsInStack(addr)) {
>      bottom = stack_bottom();
> +    shadow_bottom = (u8*)MemToShadow(bottom);
>    } else if (has_fake_stack()) {
>      bottom = fake_stack()->AddrIsInFakeStack(addr);
>      CHECK(bottom);
> -    *offset = addr - bottom;
> -    *frame_pc = ((uptr*)bottom)[2];
> -    return  (const char *)((uptr*)bottom)[1];
> +    shadow_bottom = (u8*)MemToShadow(bottom);
> +    if (*shadow_bottom == kAsanStackLeftRedzoneMagic) {
> +      *offset = addr - bottom;
> +      *frame_pc = ((uptr*)bottom)[2];
> +      return  (const char *)((uptr*)bottom)[1];
> +    }
>    }
>    uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1);  // align addr.
>    u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
> -  u8 *shadow_bottom = (u8*)MemToShadow(bottom);
>
>
>         Jakub

On Fri, Nov 15, 2013 at 06:46:25PM +0400, Konstantin Serebryany wrote:
> On Fri, Nov 15, 2013 at 6:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Nov 15, 2013 at 06:12:07PM +0400, Konstantin Serebryany wrote:
> >> I afraid it actually wants the header (magic, descr, pc) to be in the
> >> first 3 words in the
> >> memory returned by __asan_stack_malloc_*
> >> FakeStack::AddrIsInFakeStack(addr) returns the beginning of the allocated chunk
> >> and then AsanThread::GetFrameNameByAddr expects the header to be there.
> >
> > Can it be changed?
> 
> Maybe, but not like this.
> For fake stack, when the frame is already dead, the shadow will have
> different value
> (kAsanStackAfterReturnMagic) and the checks (*shadow_ptr ==
> kAsanStackLeftRedzoneMagic) will not work
> 
> If we do this, we'll need to test very very carefully.
> This thing is too subtle -- it required a few attempts to get right.
> So I'd prefer not to.
> 
> Why can't we create the redzone of max(32, alignment) bytes?

Because it is it is expensive, consider say a 2048 byte aligned variable,
that would mean another uselessly up to 2048-32 bytes of red zone, and it
would have to be done always, not just when the __asan_option_* is non-zero.

So, I'd strongly prefer not to do that.

> > I mean, adding potentially very large first red zone
> > would be quite expensive, and would have to be done unconditionally, even
> > when not using fake stacks.
> >
> > I mean, in AsanThread::GetFrameNameByAddr do (pseudo patch):
> > +  u8 *shadow_bottom;
> >    if (AddrIsInStack(addr)) {
> >      bottom = stack_bottom();
> > +    shadow_bottom = (u8*)MemToShadow(bottom);
> >    } else if (has_fake_stack()) {
> >      bottom = fake_stack()->AddrIsInFakeStack(addr);
> >      CHECK(bottom);
> > -    *offset = addr - bottom;
> > -    *frame_pc = ((uptr*)bottom)[2];
> > -    return  (const char *)((uptr*)bottom)[1];
> > +    shadow_bottom = (u8*)MemToShadow(bottom);
> > +    if (*shadow_bottom == kAsanStackLeftRedzoneMagic) {

So, just do instead:
  if (*shadow_bottom == 0) {
    while (*reinterpret_cast<u64*>(shadow_bottom) == 0)
      shadow_bottom += sizeof(u64);
    while (*shadow_bottom == 0) shadow_bottom++;
    bottom = SHADOW_TO_MEM (shadow_bottom);
  }
?

Plus make sure that in OnFree called SetShadow you don't overwrite
initial 0 shadow bytes.  Just look if *MemToShadow (ptr) is 0,
and if no, do SetShadow as now, if yes, first skip all 0 bytes and then
do PoisonShadow on the rest.

  FakeStack::Deallocate(ptr, class_id);
  u8 *shadow = MemToShadow(ptr);
  if (__builtin_expect (*shadow != 0))
    SetShadow(ptr, size, class_id, kMagic8);
  else
    {
      while (*reinterpret_cast<u64*>(shadow) == 0)
	shadow += sizeof(u64);
      while (*shadow == 0)
	shadow++;
      uptr bptr = SHADOW_TO_MEM (shadow);
      PoisonShadow(bptr, size - (bptr - ptr), kMagic1);
    }

Then there is no extra overhead because of this when not doing use after
return instrumentation, and just one byte memory read
cost (plus for the first function also MemToShadow cost) for the cases where
the fake stack starts at the beginning, and just small additional cost
otherwise.

> > +      *offset = addr - bottom;
> > +      *frame_pc = ((uptr*)bottom)[2];
> > +      return  (const char *)((uptr*)bottom)[1];
> > +    }
> >    }
> >    uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1);  // align addr.
> >    u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
> > -  u8 *shadow_bottom = (u8*)MemToShadow(bottom);
> >
> >
> >         Jakub

	Jakub

On Fri, Nov 15, 2013 at 7:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Nov 15, 2013 at 06:46:25PM +0400, Konstantin Serebryany wrote:
>> On Fri, Nov 15, 2013 at 6:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Fri, Nov 15, 2013 at 06:12:07PM +0400, Konstantin Serebryany wrote:
>> >> I afraid it actually wants the header (magic, descr, pc) to be in the
>> >> first 3 words in the
>> >> memory returned by __asan_stack_malloc_*
>> >> FakeStack::AddrIsInFakeStack(addr) returns the beginning of the allocated chunk
>> >> and then AsanThread::GetFrameNameByAddr expects the header to be there.
>> >
>> > Can it be changed?
>>
>> Maybe, but not like this.
>> For fake stack, when the frame is already dead, the shadow will have
>> different value
>> (kAsanStackAfterReturnMagic) and the checks (*shadow_ptr ==
>> kAsanStackLeftRedzoneMagic) will not work
>>
>> If we do this, we'll need to test very very carefully.
>> This thing is too subtle -- it required a few attempts to get right.
>> So I'd prefer not to.
>>
>> Why can't we create the redzone of max(32, alignment) bytes?
>
> Because it is it is expensive, consider say a 2048 byte aligned variable,
Do these happen?
My understanding is that 32-byte aligned things are common and 64-bit
aligned things
will soon become common too with avx-512.
But why would anyone use more?

And even if we have 2048-byte aligned vars the overhead is not that
big -- it's just 2x.
We have much larger overhead on regular functions, e.g. for "char a,
b, c, d, e, f, g, h".


> that would mean another uselessly up to 2048-32 bytes of red zone, and it
> would have to be done always, not just when the __asan_option_* is non-zero.
>
> So, I'd strongly prefer not to do that.
>
>> > I mean, adding potentially very large first red zone
>> > would be quite expensive, and would have to be done unconditionally, even
>> > when not using fake stacks.
>> >
>> > I mean, in AsanThread::GetFrameNameByAddr do (pseudo patch):
>> > +  u8 *shadow_bottom;
>> >    if (AddrIsInStack(addr)) {
>> >      bottom = stack_bottom();
>> > +    shadow_bottom = (u8*)MemToShadow(bottom);
>> >    } else if (has_fake_stack()) {
>> >      bottom = fake_stack()->AddrIsInFakeStack(addr);
>> >      CHECK(bottom);
>> > -    *offset = addr - bottom;
>> > -    *frame_pc = ((uptr*)bottom)[2];
>> > -    return  (const char *)((uptr*)bottom)[1];
>> > +    shadow_bottom = (u8*)MemToShadow(bottom);
>> > +    if (*shadow_bottom == kAsanStackLeftRedzoneMagic) {
>
> So, just do instead:
>   if (*shadow_bottom == 0) {
>     while (*reinterpret_cast<u64*>(shadow_bottom) == 0)
>       shadow_bottom += sizeof(u64);
>     while (*shadow_bottom == 0) shadow_bottom++;
>     bottom = SHADOW_TO_MEM (shadow_bottom);
>   }
> ?

You suggest to keep some of the shadow, that corresponds to
unaddressable memory, zeroed (un-poisoned).
That's an invitation to a false negative.
Asan does not guarantee catching a bug if the access goes way out of
bounds (more then the redzone size)
but we don't want to artificially reduce the probability of catching such bugs.



--kcc

>
> Plus make sure that in OnFree called SetShadow you don't overwrite
> initial 0 shadow bytes.  Just look if *MemToShadow (ptr) is 0,
> and if no, do SetShadow as now, if yes, first skip all 0 bytes and then
> do PoisonShadow on the rest.
>
>   FakeStack::Deallocate(ptr, class_id);
>   u8 *shadow = MemToShadow(ptr);
>   if (__builtin_expect (*shadow != 0))
>     SetShadow(ptr, size, class_id, kMagic8);
>   else
>     {
>       while (*reinterpret_cast<u64*>(shadow) == 0)
>         shadow += sizeof(u64);
>       while (*shadow == 0)
>         shadow++;
>       uptr bptr = SHADOW_TO_MEM (shadow);
>       PoisonShadow(bptr, size - (bptr - ptr), kMagic1);
>     }
>
> Then there is no extra overhead because of this when not doing use after
> return instrumentation, and just one byte memory read
> cost (plus for the first function also MemToShadow cost) for the cases where
> the fake stack starts at the beginning, and just small additional cost
> otherwise.
>
>> > +      *offset = addr - bottom;
>> > +      *frame_pc = ((uptr*)bottom)[2];
>> > +      return  (const char *)((uptr*)bottom)[1];
>> > +    }
>> >    }
>> >    uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1);  // align addr.
>> >    u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
>> > -  u8 *shadow_bottom = (u8*)MemToShadow(bottom);
>> >
>> >
>> >         Jakub
>
>         Jakub

On Fri, Nov 15, 2013 at 10:17:49PM +0400, Konstantin Serebryany wrote:
> >> Why can't we create the redzone of max(32, alignment) bytes?
> >
> > Because it is it is expensive, consider say a 2048 byte aligned variable,
> Do these happen?

They are supported and some programs do use them, perhaps less often than
32-byte and 64-byte aligned vars, but still.

> > So, just do instead:
> >   if (*shadow_bottom == 0) {
> >     while (*reinterpret_cast<u64*>(shadow_bottom) == 0)
> >       shadow_bottom += sizeof(u64);
> >     while (*shadow_bottom == 0) shadow_bottom++;
> >     bottom = SHADOW_TO_MEM (shadow_bottom);
> >   }
> > ?
> 
> You suggest to keep some of the shadow, that corresponds to
> unaddressable memory, zeroed (un-poisoned).

Well, that is what you get if you just use it, __asan_stack_malloc_N gives
you the whole area cleared, and if you put base not at the very beginning
of the area, but say 32 bytes from the start, then there will be red zone
after it.  You aren't poisioning shadow from size till end of the stack
frame upon __asan_stack_malloc_N either.

	Jakub

On Fri, Nov 15, 2013 at 10:36 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Nov 15, 2013 at 10:17:49PM +0400, Konstantin Serebryany wrote:
>> >> Why can't we create the redzone of max(32, alignment) bytes?
>> >
>> > Because it is it is expensive, consider say a 2048 byte aligned variable,
>> Do these happen?
>
> They are supported and some programs do use them, perhaps less often than
> 32-byte and 64-byte aligned vars, but still.
>
>> > So, just do instead:
>> >   if (*shadow_bottom == 0) {
>> >     while (*reinterpret_cast<u64*>(shadow_bottom) == 0)
>> >       shadow_bottom += sizeof(u64);
>> >     while (*shadow_bottom == 0) shadow_bottom++;
>> >     bottom = SHADOW_TO_MEM (shadow_bottom);
>> >   }
>> > ?
>>
>> You suggest to keep some of the shadow, that corresponds to
>> unaddressable memory, zeroed (un-poisoned).
>
> Well, that is what you get if you just use it, __asan_stack_malloc_N gives
> you the whole area cleared, and if you put base not at the very beginning
> of the area, but say 32 bytes from the start, then there will be red zone
> after it.  You aren't poisioning shadow from size till end of the stack
> frame upon __asan_stack_malloc_N either.

Well, yes. And we need to fix it, not canonicalize it. Please, let's not rush.
There are many more problems with asan's stack frame layout that I want to fix.
Let me describe them in detail in a bug next week.
For one, the frames are sometimes too large -- we can not deploy
use-after-return widely because of this.

--kcc

>
>         Jakub