- Unpoison stack for ASan when tearing down stack frames

- Unpoison stack for ASan when tearing down stack frames
  during exception handling.

R=kcc@google.com

Committed: https://code.google.com/p/dart/source/detail?r=23746

[Anton Muhin, 2013-06-03 12:55:07]

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
nit: as per ASAN source code, one is supposed to use macros instead function
calls, up to you if you'd like to follow this advice or not.

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
one hypothetical scenario I am concerned with:

1) imagine stacks of two threads allocated side by side;
2) the thread with bigger stack address almost exhausts its stack and throws an
exception, and your overestimate of the region to unpoison intersects with part
of lower address stack and this region is being poisoned/unpoised right now
3) race condition: according to the spec one shouldn't poison/unpoison the same
memory address concurrently.

I am not sure if this scenario is real: ASAN's runtime itself employes pretty
similar trick.

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
who will poison this region back?

I suspect ASAN will poison some part of stack on function invocation, but
probably only the frame needed, so it looks like low addresses may stay
unpoisned for a long time.

Also, I don't know details, but ASAM runtime may assume that unused part of
stack is poisoned.  For example, when allocating new frame, it may only unpoison
allocated locals and not poison guards.

[Ivan Posva, 2013-06-06 17:17:05]

Adding Kostya to make sure we are still doing the right thing here.

Thanks,
-Ivan

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/03 12:55:07, Anton Muhin wrote:
> one hypothetical scenario I am concerned with:
> 
> 1) imagine stacks of two threads allocated side by side;
> 2) the thread with bigger stack address almost exhausts its stack and throws
an
> exception, and your overestimate of the region to unpoison intersects with
part
> of lower address stack and this region is being poisoned/unpoised right now
> 3) race condition: according to the spec one shouldn't poison/unpoison the
same
> memory address concurrently.
> 
> I am not sure if this scenario is real: ASAN's runtime itself employes pretty
> similar trick.

Since there are guard pages between stacks, and the slack being added is less
than a page, this code will not touch a different stack's memory.

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/03 12:55:07, Anton Muhin wrote:
> who will poison this region back?
> 
> I suspect ASAN will poison some part of stack on function invocation, but
> probably only the frame needed, so it looks like low addresses may stay
> unpoisned for a long time.
> 
> Also, I don't know details, but ASAM runtime may assume that unused part of
> stack is poisoned.  For example, when allocating new frame, it may only
unpoison
> allocated locals and not poison guards.

As far as I understand, function entries that are called after we unwind the
stack will poison the area they need poisoned. What we are doing here is just
accounting for all the missed function exits which we are skipping due to the
fact that we are effectively executing a long jump.

[kcc1, 2013-06-07 06:38:45]

LGTM

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/03 12:55:07, Anton Muhin wrote:
> nit: as per ASAN source code, one is supposed to use macros instead function
> calls, up to you if you'd like to follow this advice or not.

If you include the asan's header, the macro makes your life simpler.
But in dart I guess you don't want to depend on asan headers.

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/03 12:55:07, Anton Muhin wrote:
> one hypothetical scenario I am concerned with:
> 
> 1) imagine stacks of two threads allocated side by side;
> 2) the thread with bigger stack address almost exhausts its stack and throws
an
> exception, and your overestimate of the region to unpoison intersects with
part
> of lower address stack and this region is being poisoned/unpoised right now
> 3) race condition: according to the spec one shouldn't poison/unpoison the
same
> memory address concurrently.
> 
> I am not sure if this scenario is real: ASAN's runtime itself employes pretty
> similar trick.

In theory this may happen, but given the relatively small constant
(1024) and the implementation details of glibc's threads' stacks
this will not happen.

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/03 12:55:07, Anton Muhin wrote:
> who will poison this region back?

Once we enter another function it will poison its portion of stack. 

> 
> I suspect ASAN will poison some part of stack on function invocation, but
> probably only the frame needed, so it looks like low addresses may stay
> unpoisned for a long time.

I assume that "stack_pointer" is the place to which we unwind, and not the
beginning of the entire thread's stack.
If so, the code is just fine. 


> 
> Also, I don't know details, but ASAM runtime may assume that unused part of
> stack is poisoned.  For example, when allocating new frame, it may only
unpoison
> allocated locals and not poison guards.

No. The asan's protocol is that all memory is unpoisoned unless we poisoned it
on purpose 
(e.g. on function entry).

[Ivan Posva, 2013-06-07 15:23:12]

Kostya,

Thanks so much for verifying.

-Ivan

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/15836008/diff/1/runtime/vm/exceptions.cc#newc...
runtime/vm/exceptions.cc:262:
__asan_unpoison_memory_region(reinterpret_cast<void*>(current_sp),
On 2013/06/07 06:38:45, kcc1 wrote:
> On 2013/06/03 12:55:07, Anton Muhin wrote:
> > who will poison this region back?
> 
> Once we enter another function it will poison its portion of stack. 
> 
> > 
> > I suspect ASAN will poison some part of stack on function invocation, but
> > probably only the frame needed, so it looks like low addresses may stay
> > unpoisned for a long time.
> 
> I assume that "stack_pointer" is the place to which we unwind, and not the
> beginning of the entire thread's stack.
> If so, the code is just fine. 

That is correct. stack_pointer is the value that will be set as for example the
ESP on ia32. It is the stack pointer when we resume execution at the exception
handler.

> > 
> > Also, I don't know details, but ASAM runtime may assume that unused part of
> > stack is poisoned.  For example, when allocating new frame, it may only
> unpoison
> > allocated locals and not poison guards.
> 
> No. The asan's protocol is that all memory is unpoisoned unless we poisoned it
> on purpose 
> (e.g. on function entry).
> 
>

[Ivan Posva, 2013-06-07 15:31:23]

Committed patchset #1 manually as r23746 (presubmit successful).