Re: libsanitizer merge from upstream r191666

[Jakub Jelinek <jakub at redhat dot com>]

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