This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: libsanitizer merge from upstream r191666


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]