On Fri, Nov 15, 2013 at 7:18 PM, Jakub Jelinek <ja...@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 <ja...@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
