This is an archive of the discontinued LLVM Phabricator instance.

[asan] Improved support for swapcontext()
Needs ReviewPublic

Authored by pdziepak on Feb 3 2016, 12:55 AM.

Details

Reviewers
kcc
Summary

These changess improve ASan support for swapcontext() and friends mainly
focusing on eliminating false positives caused by throwing and catching
exceptions when on a custom user stack.

This is achieved by:

  1. Making swapcontext() and getcontext() fill ucontext_t::uc_stack with proper

values.

  1. Adding ability to AsanThread to change current stack (in an async-safe way).

Some of the things that are still not fully supported:

  • swapcontext() and fake stacks
  • changing context from signal handlers
  • changing context using longjmp()
  • final context change via ucontext_t::uc_link

Diff Detail

Event Timeline

pdziepak updated this revision to Diff 46753.Feb 3 2016, 12:55 AM
pdziepak retitled this revision from to [asan] Improved support for swapcontext().
pdziepak updated this object.
pdziepak added a reviewer: kcc.
pdziepak added a subscriber: llvm-commits.
kcc edited edge metadata.Feb 3 2016, 6:30 PM

Thanks for this work, but, just curious, what is your motivation?
What kind of software are you trying to apply asan to?
I haven't heard about swapcontext for a loooong time.

The change in AsanThread is quite intrusive and potentially risky.
While I don't see what may go wrong, I'd like to have more tests, e.g. with threads, leaks, signals, etc.

Besides, I'd like to have another pair of eyes here. Anyone?

lib/asan/asan_interceptors.cc
350

initialize these just in case (with =0); same below.

351

do you know a situation where curr_thread is nullptr? same below

test/asan/TestCases/Linux/swapcontext_test.cc
32

shouldn't we call Func from Throw() or some such?

test/lsan/TestCases/swapcontext.cc
5

don't we still want to test the case with heap memory?

samsonov added inline comments.
lib/asan/asan_interceptors.cc
351

Yeah, I believe this happens during thread destruction.

354

Won't these be overwritten anyway by real swapcontext(), when it will be copying ucp to oucp?

392

Why do you need this?

lib/asan/asan_thread.cc
204

Maybe, just add a constructor for StackDescriptor that would fill everything with zeroes?

lib/asan/asan_thread.h
170

Note: you can replace three pointers with a single "int current_stack_;", where
next_stack_ = &stacks_[current_stack_];
previous_stack_ = &stacks_[(current_stack_ + 1) % 3];
temp_stack_ = &stacks_[(current_stack_ + 2) % 3];

test/lsan/TestCases/swapcontext.cc
5

We definitely still want to run it. Can we predict if LSan will or won't report leak in this case?

The main motivation is to avoid false positives we have been getting in SeaStar ( http://www.seastar-project.org/ ). It is a future/promise/continuation based framework that also implements coroutines using swapcontext() and friends, so that we get something similar to resumable functions (actually, it's using mostly longjmp() but that's more difficult to support properly in ASan). With these changes the false positives we've been seeing so far are gone.

lib/asan/asan_interceptors.cc
351

Every use of GetCurrentThread() I've seen in ASan code has guard against the function returning nullptr. I admit I haven't dug deeper but just wanted to be safe and followed what is done in the other places.

354

swapcontext() doesn't copy ucp to oucp. It saves the current context to oucp (so that when it is restored it behaves as if swapcontext() has returned 0) and switches to ucp.
However, glibc implementations of swapcontext() and getcontext() (at least their x86 versions) don't write anything to ucontext_t::uc_stack and that's why that WriteContextStack() is needed – to preserve stack information across setcontext/getcontext/swapcontext calls. If swapcontext() actually writes something to uc_stack it is ok as long it is valid data and not zeroes or some garbage.

392

Like I mentioned in earlier reply, getcontext() (or at least some its implementations) doesn't write anything to ucontext_t::uc_stack. Relying in setcontext() and swapcontext() on the fact that uc_stack contains valid information makes thing much easier so I decided to "fix" getcontext() and swapcontext().

lib/asan/asan_thread.h
170

Is it worth it? We save no more than 24 bytes per thread which usually will be much less than 1% percent of memory used by the thread stack, but make code both harder to read (another layer of indirection since logic needed to get stack pointer will have to be hidden in some function) and longer to execute (mod non-power-of-two).

test/asan/TestCases/Linux/swapcontext_test.cc
32

What these changes are trying to fix is mainly the false positive like this:

  1. An exception is thrown on custom stack.
  2. __asan_handle_no_return() is called but doesn't have correct stack information so just prints a warning and bails out, as a result stack shadow memory is not cleared
  3. Exception is handled, stack is unwind. Execution continues.
  4. Function called from exception handler or after exception was handled touches stack, but the shadow memory contains stale information (that was supposed to be cleared by __asan_handle_no_return()). Result: false positive.

So that's what I am testing here. Throw() has some data on stack, exception is thrown and handled and then another function touches the stack in places that Throw() has used before.

test/lsan/TestCases/swapcontext.cc
5

LSan will retain its original behaviour (i.e. not reporting leaks on custom stack). The solution would be to either add similar changes to LSan (or perhaps reduce code duplication between LSan and ASan) or write these testing directives so that different behaviour is expected from LSan than from ASan. I am not sure if the latter is possible.