[libasan] Remove 4Mb stack limit for swapcontext unpoisoning

[itrofimow]

Based on https://github.com/google/sanitizers/issues/1494

kMaxSaneContextStackSize was introduced 9 years ago, and with increasing popularity of coroutines in c++ world (be that std or boost coroutines) this limitation seems a bit off.
After all, is >4Mb coroutine stack actually insane in 2022?

[vitalybuka]

Could you please add a test?

compiler-rt/lib/asan/asan_interceptors.cpp

251

It should probably a CHECK
e.g. CHECK_LE(ssize, 1<<28)

[eugenis]

LGTM

[eugenis]

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

[itrofimow]

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

In boost::Coroutine2 stack size might be user-provided and i am not sure if there is a limitation at all.

In D129219#3633402, @vitalybuka wrote:

Could you please add a test?

Sure, but could you please suggest an appropriate place for that and where to look for references? - i'm not familiar with the codebase

[vitalybuka]

In D129219#3633460, @itrofimow wrote:

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

In boost::Coroutine2 stack size might be user-provided and i am not sure if there is a limitation at all.

In D129219#3633402, @vitalybuka wrote:

Could you please add a test?

Sure, but could you please suggest an appropriate place for that and where to look for references? - i'm not familiar with the codebase

Take a look at:
compiler-rt/test/asan/TestCases/Linux/swapcontext_test.cpp
compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp

You may consider to modify them, or just fork and throw away unneeded stuff.
I expect new test is just simplest piece of code which use this function and fails without patch and passes with the patch.

[eugenis]

In D129219#3633460, @itrofimow wrote:

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

In boost::Coroutine2 stack size might be user-provided and i am not sure if there is a limitation at all.

Right, same as ex. the size argument in memset. If it's wrong, we crash, but that's the user's fault. As opposed to some heuristic to detect stack limits, or that thing where we peek at glibc internals to parse the thread header which keeps changing between versions...

[itrofimow]

making sure this test breaks on trunk

[itrofimow]

now checking the test with a patch

[itrofimow]

+ clang-format

[itrofimow]

In D129219#3633478, @vitalybuka wrote:

In D129219#3633460, @itrofimow wrote:

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

In boost::Coroutine2 stack size might be user-provided and i am not sure if there is a limitation at all.

In D129219#3633402, @vitalybuka wrote:

Could you please add a test?

Sure, but could you please suggest an appropriate place for that and where to look for references? - i'm not familiar with the codebase

Take a look at:
compiler-rt/test/asan/TestCases/Linux/swapcontext_test.cpp
compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp

You may consider to modify them, or just fork and throw away unneeded stuff.
I expect new test is just simplest piece of code which use this function and fails without patch and passes with the patch.

Added a test, could you please have a look?
It's not exactly the simplest piece of code unfortunately, but i guess it's not that bad:
it resembles pretty close to what boost.Coroutine2 does, and fixing that was the motivation for the patch.

P.S. I did check that it fails without the patch, that could be seen in builds history

[vitalybuka]

rebase

[vitalybuka]

Thanks, LGTM.

compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp

68

I moved formatting into a separate patch

[vitalybuka]

rebase

[itrofimow]

In D129219#3667281, @vitalybuka wrote:

rebase

Hi! I see this got reverted, can i help with landing it once again?

[vitalybuka]

In D129219#3669035, @itrofimow wrote:

In D129219#3667281, @vitalybuka wrote:

rebase

Hi! I see this got reverted, can i help with landing it once again?

I uploaded it with slight modifications here. Hopefully it will work with D130218. Maybe you can check both, or even test in your environment.
It's not perfect, but better then we had before.

[vitalybuka]

rebase

compiler-rt/lib/asan/asan_interceptors.cpp

INTERCEPTOR(int, pthread_join, void *t, void **arg) {
   return real_pthread_join(t, arg);
 }
 
 DEFINE_REAL_PTHREAD_FUNCTIONS
 #endif  // ASAN_INTERCEPT_PTHREAD_CREATE
 
 #if ASAN_INTERCEPT_SWAPCONTEXT
 static void ClearShadowMemoryForContextStack(uptr stack, uptr ssize) {
+  // Only clear if we know the stack. This should be true only for contexts
+  // created with makecontext().
+  if (!ssize)
+    return;
   // Align to page size.
   uptr PageSize = GetPageSizeCached();
   uptr bottom = RoundDownTo(stack, PageSize);
+  if (!AddrIsInMem(bottom))
+    return;
   ssize += stack - bottom;
   ssize = RoundUpTo(ssize, PageSize);
-  static const uptr kMaxSaneContextStackSize = 1 << 22;  // 4 Mb
-  if (AddrIsInMem(bottom) && ssize && ssize <= kMaxSaneContextStackSize)
-    PoisonShadow(bottom, ssize, 0);
 }
 
 INTERCEPTOR(int, getcontext, struct ucontext_t *ucp) {
   // API does not requires to have ucp clean, and sets only part of fields. We
   // use ucp->uc_stack to unpoison new stack. We prefer to have zeroes then
   // uninitialized bytes.
   ResetContextStack(ucp);
   return REAL(getcontext)(ucp);

compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp

void NextChild() {
 
   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(nullptr, main_thread_stack,
                                  main_thread_stacksize);
   CallNoReturn();
   if (swapcontext(&next_child_context, &orig_context) < 0) {
     perror("swapcontext");
     _exit(1);
   }
 }
 
int Run(int arg, int mode, char *child_stack) {
 
   // 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];
 }
 
+ucontext_t orig_huge_stack_context;
+ucontext_t child_huge_stack_context;
+
+// There used to be a limitation for stack unpoisoning (size <= 4Mb), check that it's gone.
+const int kHugeStackSize = 1 << 23;
+
+void ChildHugeStack() {
+  __sanitizer_finish_switch_fiber(nullptr, &main_thread_stack,
+                                  &main_thread_stacksize);
+  char x[32] = {0}; // Stack gets poisoned.
+  __sanitizer_start_switch_fiber(nullptr, main_thread_stack,
+                                 main_thread_stacksize);
+  if (swapcontext(&child_huge_stack_context, &orig_huge_stack_context) < 0) {
+    perror("swapcontext");
+    _exit(1);
+  }
+}
+
+void DoRunHugeStack(char *child_stack) {
+  getcontext(&child_huge_stack_context);
+  child_huge_stack_context.uc_stack.ss_sp = child_stack;
+  child_huge_stack_context.uc_stack.ss_size = kHugeStackSize;
+  makecontext(&child_huge_stack_context, (void (*)())ChildHugeStack, 0);
+  void *fake_stack_save;
+  __sanitizer_start_switch_fiber(&fake_stack_save,
+                                 child_huge_stack_context.uc_stack.ss_sp,
+                                 child_huge_stack_context.uc_stack.ss_size);
+  if (swapcontext(&orig_huge_stack_context, &child_huge_stack_context) < 0) {
+    perror("swapcontext");
+    _exit(1);
+  }
+  __sanitizer_finish_switch_fiber(
+      fake_stack_save, (const void **)&child_huge_stack_context.uc_stack.ss_sp,
+      &child_huge_stack_context.uc_stack.ss_size);
+  for (int i = 0; i < kHugeStackSize; ++i) {
+    child_stack[i] = i;
+  }
+}
+
+void RunHugeStack() {
+  const int run_offset = 1 << 14;
+  char *heap = new char[kHugeStackSize + run_offset + 1];
+  DoRunHugeStack(heap);
+  DoRunHugeStack(heap + run_offset);
+  DoRunHugeStack(heap);
+  delete[] heap;
+}
+
 void handler(int sig) { CallNoReturn(); }
 
 int main(int argc, char **argv) {
+  // CHECK: WARNING: ASan doesn't fully support makecontext/swapcontext
+  // CHECK-NOT: ASan is ignoring requested __asan_handle_no_return
+  RunHugeStack();
+
   // 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);
   }
 
   itimerval t;
   t.it_interval.tv_sec = 0;
   t.it_interval.tv_usec = 10;
   t.it_value = t.it_interval;
   if (setitimer(ITIMER_PROF, &t, 0)) {
     perror("setitimer");
     _exit(1);
   }
 
   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]*]]