[PATCH] D16844: [asan] Improved support for swapcontext()

[Paweł Dziepak via llvm-commits]

pdziepak added a comment.

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.


================
Comment at: lib/asan/asan_interceptors.cc:351
@@ +350,3 @@
+  uptr old_stack, old_ssize;
+  if (curr_thread) {
+    old_stack = curr_thread->stack_bottom();
----------------
samsonov wrote:
> kcc wrote:
> > do you know a situation where curr_thread is nullptr? same below
> Yeah, I believe this happens during thread destruction.
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.

================
Comment at: lib/asan/asan_interceptors.cc:354
@@ +353,3 @@
+    old_ssize = curr_thread->stack_size();
+    WriteContextStack(oucp, old_stack, old_ssize);
+    curr_thread->SetUserStack(stack, ssize);
----------------
samsonov wrote:
> Won't these be overwritten anyway by real swapcontext(), when it will be copying ucp to oucp?
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.

================
Comment at: lib/asan/asan_interceptors.cc:392
@@ +391,3 @@
+  if (!res && curr_thread) {
+    WriteContextStack(ucp, curr_thread->stack_bottom(),
+                      curr_thread->stack_size());
----------------
samsonov wrote:
> Why do you need this?
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().

================
Comment at: lib/asan/asan_thread.h:170
@@ +169,3 @@
+  StackDescriptor stacks_[3];
+  StackDescriptor* temp_stack_;
+  StackDescriptor* next_stack_;
----------------
samsonov wrote:
> 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];
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).

================
Comment at: test/asan/TestCases/Linux/swapcontext_test.cc:32
@@ -25,1 +31,3 @@
 __attribute__((noinline))
+void Func() {
+  int buf[4 * 1024];
----------------
kcc wrote:
> shouldn't we call Func from Throw() or some such? 
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.

================
Comment at: test/lsan/TestCases/swapcontext.cc:5
@@ -4,3 +1,2 @@
 // RUN: %clangxx_lsan %s -o %t
-// RUN: %run %t 2>&1
 // RUN: not %run %t foo 2>&1 | FileCheck %s
----------------
samsonov wrote:
> kcc wrote:
> > don't we still want to test the case with heap memory? 
> We definitely still want to run it. Can we predict if LSan will or won't report leak in this case?
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.


http://reviews.llvm.org/D16844