[ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber

[andriigrynenko]

Summary

This patch extends __sanitizer_finish_switch_fiber method to optionally return previous stack base and size.

This solves the problem of coroutines/fibers library not knowing the original stack context from which the library is used. It's incorrect to assume that such context is always the default stack of current thread (e.g. one such library may be used from a fiber/coroutine created by another library). Bulding a separate stack tracking mechanism would not only duplicate AsanThread, but also require each coroutines/fibers library to integrate with it.

[blastrock]

Seems fine to me, but I think you forgot to update the tests :)

[andriigrynenko]

fix the unit test

[andriigrynenko]

test/asan/TestCases/Linux/swapcontext_annotation.cc

14

The test was actually broken in trunk (not updated for the fakestack argument). Apparently it was never run (considered unsupported) because of wrong targets here.

[dvyukov]

We need a test that passes non-NULL to these arguments and shows how to use the returned values.

test/asan/TestCases/Linux/swapcontext_annotation.cc

55–57

#include <stddef.h> for NULL

[blastrock]

test/asan/TestCases/Linux/swapcontext_annotation.cc

14

Indeed, my bad, I forgot to compile and run the tests after the last modification I made to that patch. Sorry for that.

I didn't need to change this line to run them on my computer though, but if you know what you are doing with this line, it's ok. And it's strange indeed that no buildfarm caught this...

[andriigrynenko]

update unit test to use non-null values

[filcab]

Please add some printf calls to the test, to show that you have the correct stack+size, too.

Thanks for working on this.
Filipe

lib/asan/asan_thread.cc

141

Please make sure you're accounting for this. It seems the test is always passing nullptr to start_switch, which shouldn't be done unless you're finishing up a fiber execution, IIRC.

162

I think the usual style is: one statement => no braces.

test/asan/TestCases/Linux/swapcontext_annotation.cc

14

They wouldn't catch tests not being run.
Test with LLVM_LIT_ARGS=-v (instead of -sv) on your cmake config and double-check that the tests are run.

55–57

nit: nullptr?

[andriigrynenko]

Addressing comments.

[andriigrynenko]

test/asan/TestCases/Linux/swapcontext_annotation.cc

181–204

This only checks the first iteration of the loop. Can I do it better with FileCheck ?

[dvyukov]

We need a test that passes non-NULL to these arguments and shows how to use the returned values.

test/asan/TestCases/Linux/swapcontext_annotation.cc

181–204

Yes, you can add CHECKs for the second iteration as well.

[andriigrynenko]

test/asan/TestCases/Linux/swapcontext_annotation.cc

181–204

@dvyukov: What I meant is that I didn't see a way to have some kind of loop in these CHECKs. I don't want to copy-paste these same CHECKs 30 times :)

[andriigrynenko]

Do CHECKs for all iterations of the loop.

[dvyukov]

LGTM

Any other comments? Or I will submit it tomorrow.

test/asan/TestCases/Linux/swapcontext_annotation.cc

7

nice

[dvyukov]

Submitted in 282582.

include/sanitizer/common_interface_defs.h

#endif
   // argument to the start_switch function so that the fake stack is destroyed.
   // If you do not want support for stack use-after-return detection, you can
   // always pass null to these two functions.
   // Note that the fake stack mechanism is disabled during fiber switch, so if a
   // signal callback runs during the switch, it will not benefit from the stack
   // use-after-return detection.
   void __sanitizer_start_switch_fiber(void **fake_stack_save,
                                       const void *bottom, size_t size);
-  void __sanitizer_finish_switch_fiber(void *fake_stack_save);
+  void __sanitizer_finish_switch_fiber(void *fake_stack_save,
+                                       const void **bottom_old,
+                                       size_t *size_old);
 #ifdef __cplusplus
 }  // extern "C"
 #endif
 
 #endif  // SANITIZER_COMMON_INTERFACE_DEFS_H

lib/asan/asan_thread.h

void DeleteFakeStack(int tid) {
     if (!fake_stack_) return;
     FakeStack *t = fake_stack_;
     fake_stack_ = nullptr;
     SetTLSFakeStack(nullptr);
     t->Destroy(tid);
   }
 
   void StartSwitchFiber(FakeStack **fake_stack_save, uptr bottom, uptr size);
-  void FinishSwitchFiber(FakeStack *fake_stack_save);
+  void FinishSwitchFiber(FakeStack *fake_stack_save, uptr *bottom_old,
+                         uptr *size_old);
 
   bool has_fake_stack() {
     return !atomic_load(&stack_switching_, memory_order_relaxed) &&
            (reinterpret_cast<uptr>(fake_stack_) > 1);
   }
 
   FakeStack *fake_stack() {
     if (!__asan_option_detect_stack_use_after_return)

lib/asan/asan_thread.cc

void AsanThread::StartSwitchFiber(FakeStack **fake_stack_save, uptr bottom,
 
   FakeStack *current_fake_stack = fake_stack_;
   if (fake_stack_save)
     *fake_stack_save = fake_stack_;
   fake_stack_ = nullptr;
   SetTLSFakeStack(nullptr);
   // if fake_stack_save is null, the fiber will die, delete the fakestack
   if (!fake_stack_save && current_fake_stack)
     current_fake_stack->Destroy(this->tid());
 }
 
-void AsanThread::FinishSwitchFiber(FakeStack *fake_stack_save) {
+void AsanThread::FinishSwitchFiber(FakeStack *fake_stack_save,
+                                   uptr *bottom_old,
+                                   uptr *size_old) {
   if (!atomic_load(&stack_switching_, memory_order_relaxed)) {
     Report("ERROR: finishing a fiber switch that has not started\n");
     Die();
   }
 
   if (fake_stack_save) {
     SetTLSFakeStack(fake_stack_save);
     fake_stack_ = fake_stack_save;
   }
 
+  if (bottom_old)
+    *bottom_old = stack_bottom_;
+  if (size_old)
+    *size_old = stack_top_ - stack_bottom_;
   stack_bottom_ = next_stack_bottom_;
   stack_top_ = next_stack_top_;
   atomic_store(&stack_switching_, 0, memory_order_release);
   next_stack_top_ = 0;
   next_stack_bottom_ = 0;
 }
 
 inline AsanThread::StackBounds AsanThread::GetStackBounds() const {
   if (!atomic_load(&stack_switching_, memory_order_acquire))
     return StackBounds{stack_bottom_, stack_top_};  // NOLINT
void __sanitizer_start_switch_fiber(void **fakestacksave, const void *bottom,
   if (!t) {
     VReport(1, "__asan_start_switch_fiber called from unknown thread\n");
     return;
   }
   t->StartSwitchFiber((FakeStack**)fakestacksave, (uptr)bottom, size);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_finish_switch_fiber(void* fakestack) {
+void __sanitizer_finish_switch_fiber(void* fakestack,
+                                     const void **bottom_old,
+                                     uptr *size_old) {
   AsanThread *t = GetCurrentThread();
   if (!t) {
     VReport(1, "__asan_finish_switch_fiber called from unknown thread\n");
     return;
   }
-  t->FinishSwitchFiber((FakeStack*)fakestack);
+  t->FinishSwitchFiber((FakeStack*)fakestack,
+                       (uptr*)bottom_old,
+                       (uptr*)size_old);
 }
 }

test/asan/TestCases/Linux/swapcontext_annotation.cc

 // Check that ASan plays well with annotated makecontext/swapcontext.
 
-// RUN: %clangxx_asan -lpthread -O0 %s -o %t && %run %t 2>&1 | FileCheck %s
-// RUN: %clangxx_asan -lpthread -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
-// RUN: %clangxx_asan -lpthread -O2 %s -o %t && %run %t 2>&1 | FileCheck %s
-// RUN: %clangxx_asan -lpthread -O3 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -std=c++11 -lpthread -O0 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -std=c++11 -lpthread -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -std=c++11 -lpthread -O2 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -std=c++11 -lpthread -O3 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -std=c++11 -lpthread -O0 %s -o %t && %run %t 2>&1 | FileCheck <( seq 60 | xargs -i -- grep LOOPCHECK %s ) --check-prefix LOOPCHECK
+// RUN: %clangxx_asan -std=c++11 -lpthread -O1 %s -o %t && %run %t 2>&1 | FileCheck <( seq 60 | xargs -i -- grep LOOPCHECK %s ) --check-prefix LOOPCHECK
+// RUN: %clangxx_asan -std=c++11 -lpthread -O2 %s -o %t && %run %t 2>&1 | FileCheck <( seq 60 | xargs -i -- grep LOOPCHECK %s ) --check-prefix LOOPCHECK
+// RUN: %clangxx_asan -std=c++11 -lpthread -O3 %s -o %t && %run %t 2>&1 | FileCheck <( seq 60 | xargs -i -- grep LOOPCHECK %s ) --check-prefix LOOPCHECK
+
 //
 // This test is too subtle to try on non-x86 arch for now.
-// REQUIRES: x86_64-supported-target,i386-supported-target
+// REQUIRES: x86-target-arch
 
 #include <pthread.h>
 #include <setjmp.h>
 #include <stdio.h>
 #include <sys/time.h>
 #include <ucontext.h>
 #include <unistd.h>
 
 #include <sanitizer/common_interface_defs.h>
 
 ucontext_t orig_context;
 ucontext_t child_context;
 ucontext_t next_child_context;
 
 char *next_child_stack;
 
 const int kStackSize = 1 << 20;
 
-void *main_thread_stack;
+const void *main_thread_stack;
 size_t main_thread_stacksize;
 
+const void *from_stack;
+size_t from_stacksize;
+
 __attribute__((noinline, noreturn)) void LongJump(jmp_buf env) {
   longjmp(env, 1);
   _exit(1);
 }
 
 // Simulate __asan_handle_no_return().
 __attribute__((noinline)) void CallNoReturn() {
   jmp_buf env;
   if (setjmp(env) != 0) return;
 
   LongJump(env);
   _exit(1);
 }
 
 void NextChild() {
   CallNoReturn();
-  __sanitizer_finish_switch_fiber();
+  __sanitizer_finish_switch_fiber(nullptr, &from_stack, &from_stacksize);
+
+  printf("NextChild from: %p %zu\n", from_stack, from_stacksize);
 
   char x[32] = {0};  // Stack gets poisoned.
   printf("NextChild: %p\n", x);
 
   CallNoReturn();
 
-  __sanitizer_start_switch_fiber(main_thread_stack, main_thread_stacksize);
+  __sanitizer_start_switch_fiber(nullptr,
+                                 main_thread_stack,
+                                 main_thread_stacksize);
   CallNoReturn();
   if (swapcontext(&next_child_context, &orig_context) < 0) {
     perror("swapcontext");
     _exit(1);
   }
 }
 
 void Child(int mode) {
   CallNoReturn();
-  __sanitizer_finish_switch_fiber();
+  __sanitizer_finish_switch_fiber(nullptr,
+                                  &main_thread_stack,
+                                  &main_thread_stacksize);
   char x[32] = {0};  // Stack gets poisoned.
   printf("Child: %p\n", x);
   CallNoReturn();
   // (a) Do nothing, just return to parent function.
   // (b) Jump into the original function. Stack remains poisoned unless we do
   //     something.
   // (c) Jump to another function which will then jump back to the main function
   if (mode == 0) {
-    __sanitizer_start_switch_fiber(main_thread_stack, main_thread_stacksize);
+    __sanitizer_start_switch_fiber(nullptr,
+                                   main_thread_stack,
+                                   main_thread_stacksize);
     CallNoReturn();
   } else if (mode == 1) {
-    __sanitizer_start_switch_fiber(main_thread_stack, main_thread_stacksize);
+    __sanitizer_start_switch_fiber(nullptr,
+                                   main_thread_stack,
+                                   main_thread_stacksize);
     CallNoReturn();
     if (swapcontext(&child_context, &orig_context) < 0) {
       perror("swapcontext");
       _exit(1);
     }
   } else if (mode == 2) {
+    printf("NextChild stack: %p\n", next_child_stack);
+
     getcontext(&next_child_context);
     next_child_context.uc_stack.ss_sp = next_child_stack;
     next_child_context.uc_stack.ss_size = kStackSize / 2;
     makecontext(&next_child_context, (void (*)())NextChild, 0);
-    __sanitizer_start_switch_fiber(next_child_context.uc_stack.ss_sp,
+    __sanitizer_start_switch_fiber(nullptr,
+                                   next_child_context.uc_stack.ss_sp,
                                    next_child_context.uc_stack.ss_size);
     CallNoReturn();
     if (swapcontext(&child_context, &next_child_context) < 0) {
       perror("swapcontext");
       _exit(1);
     }
   }
 }
 
 int Run(int arg, int mode, char *child_stack) {
   printf("Child stack: %p\n", child_stack);
   // Setup child context.
   getcontext(&child_context);
   child_context.uc_stack.ss_sp = child_stack;
   child_context.uc_stack.ss_size = kStackSize / 2;
   if (mode == 0) {
     child_context.uc_link = &orig_context;
   }
   makecontext(&child_context, (void (*)())Child, 1, mode);
   CallNoReturn();
-  __sanitizer_start_switch_fiber(child_context.uc_stack.ss_sp,
+  void* fake_stack_save;
+  __sanitizer_start_switch_fiber(&fake_stack_save,
+                                 child_context.uc_stack.ss_sp,
                                  child_context.uc_stack.ss_size);
   CallNoReturn();
   if (swapcontext(&orig_context, &child_context) < 0) {
     perror("swapcontext");
     _exit(1);
   }
   CallNoReturn();
-  __sanitizer_finish_switch_fiber();
+  __sanitizer_finish_switch_fiber(fake_stack_save,
+                                  &from_stack,
+                                  &from_stacksize);
   CallNoReturn();
+  printf("Main context from: %p %zu\n", from_stack, from_stacksize);
 
   // Touch childs's stack to make sure it's unpoisoned.
   for (int i = 0; i < kStackSize; i++) {
     child_stack[i] = i;
   }
   return child_stack[arg];
 }
 
 void handler(int sig) { CallNoReturn(); }
 
-void InitStackBounds() {
-  pthread_attr_t attr;
-  pthread_attr_init(&attr);
-  pthread_getattr_np(pthread_self(), &attr);
-  pthread_attr_getstack(&attr, &main_thread_stack, &main_thread_stacksize);
-  pthread_attr_destroy(&attr);
-}
-
 int main(int argc, char **argv) {
-  InitStackBounds();
-
   // set up a signal that will spam and trigger __asan_handle_no_return at
   // tricky moments
   struct sigaction act = {};
   act.sa_handler = &handler;
   if (sigaction(SIGPROF, &act, 0)) {
     perror("sigaction");
     _exit(1);
   }
int main(int argc, char **argv) {
   char *heap = new char[kStackSize + 1];
   next_child_stack = new char[kStackSize + 1];
   char stack[kStackSize + 1];
   // CHECK: WARNING: ASan doesn't fully support makecontext/swapcontext
   int ret = 0;
   // CHECK-NOT: ASan is ignoring requested __asan_handle_no_return
   for (unsigned int i = 0; i < 30; ++i) {
     ret += Run(argc - 1, 0, stack);
+    // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+    // LOOPCHECK: Main context from: [[CHILD_STACK]] 524288
     ret += Run(argc - 1, 1, stack);
+    // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+    // LOOPCHECK: Main context from: [[CHILD_STACK]] 524288
     ret += Run(argc - 1, 2, stack);
+    // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+    // LOOPCHECK: NextChild stack: [[NEXT_CHILD_STACK:0x[0-9a-f]*]]
+    // LOOPCHECK: NextChild from: [[CHILD_STACK]] 524288
+    // LOOPCHECK: Main context from: [[NEXT_CHILD_STACK]] 524288
     ret += Run(argc - 1, 0, heap);
     ret += Run(argc - 1, 1, heap);
     ret += Run(argc - 1, 2, heap);
+    printf("Iteration %d passed\n", i);
   }
+
   // CHECK: Test passed
   printf("Test passed\n");
 
   delete[] heap;
   delete[] next_child_stack;
 
   return ret;
 }