[asan] Fix GC of FakeFrames

[vitalybuka]

Summary

When FakeStack GC from altstack, it may see default stack on lower
addressed and incorectly disard all frames.

Fixes bug exposed by D153536.

[fmayer]

compiler-rt/lib/asan/asan_fake_stack.cpp

138

just confirming (not that familiar with ASan) that this leads to false negatives, correct?

[vitalybuka]

compiler-rt/lib/asan/asan_fake_stack.cpp

138

No, nested stacks may still cause false positives, if we discard frame, which is actually alive.

[vitalybuka]

update comment

[vitalybuka]

comment

[vitalybuka]

-pthread

[kstoimenov]

compiler-rt/lib/asan/asan_fake_stack.cpp

167–168

Add a comment what is the intent of this code.

[vitalybuka]

comment

[ro]

The new test SEGVs on Solaris/amd64. I haven't started investigating yet.

[ro]

In D157552#4575681, @ro wrote:

The new test SEGVs on Solaris/amd64. I haven't started investigating yet.

I've digged some more: here's the stacktrace from the SEGV:

Thread 3 received signal SIGSEGV, Segmentation fault.
0x08158265 in Fn<715u> ()
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:23
23	template <size_t N> void Fn() {
1: x/i $pc
=> 0x8158265 <_Z2FnILj715EEvv+101>:	movl   $0x41b58ab3,(%eax)
(gdb) bt
#0  0x08158265 in Fn<715u> ()
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:23
#1  0x08158187 in Fn<716u> ()
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:27
[...]
#285 0x0813e067 in Fn<1000u> ()
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:27
#286 0x0813d848 in Handler (signo=6)
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:36
#287 <signal handler called>
#288 0xfe15e145 in __lwp_sigqueue () from /lib/libc.so.1
#289 0xfe155eaf in thr_kill () from /lib/libc.so.1
#290 0xfe08d9c2 in raise () from /lib/libc.so.1
#291 0xfe05b094 in abort () from /lib/libc.so.1
#292 0x0813d7f8 in Thread (arg=0xfdafe800)
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp:64
#293 0x0810315c in asan_thread_start ()
    at /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/compiler-rt/lib/asan/asan_interceptors.cpp:234
#294 0xfe158a9b in _thrp_setup () from /lib/libc.so.1
#295 0xfe158dd0 in ?? () from /lib/libc.so.1
#296 0x00000000 in ?? ()

At that point, %eax is 0xfdafc6c0. If you look at the process mappings with pmap -x, you get:

4093:	/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/stage1/p
 Address Kbytes  RSS Anon Lock Mode     Mapped File
08050000   1332  836    -    - r-x----  [ text ] fake_stack_gc.cpp.tmp
081AC000    188  188   16    - rwx----  [ data ] fake_stack_gc.cpp.tmp
081DB000   9160  232  232    - rwx----  [ data ] fake_stack_gc.cpp.tmp
08ACD000      4    4    4    - rwx----  [ heap ]
21010000      4    4    4    - rw--R-E  [ anon ]
[...]
FD9FA000   1032   20   20    - rw-----  [ anon ]
FDAFD000   1032 1032 1032    - rw-----  [ anon ]
FDC00000   2184    -    -    - rw-----  [ anon ]

The mapping at 0xFD9FA000 is from FD9FA000 to FDAFC000. So %eax points to unmapped memory, thus a stack overflow. It seems the test assumes larger stacks somehow.

[steven_wu]

This breaks macOS bot: https://green.lab.llvm.org/green/job/clang-stage1-RA/35236/

[arphaman]

@vitalybuka could you please take a look at the failure on the macOS bot that Steven highlighted above?

[arphaman]

I marked it as Unsupported on Darwin for now while it's being investigated. Please let me know if you can resolve it.

[vitalybuka]

Thanks for feedback. Probably fixed and reenabled on Darwin.
Recursion stack usage was expected to be least 2Mb (more with redzones), but I allocated only 1Mb. Strange that it consistently passed on Linux x86_64, aarch64 bots.

[ro]

In D157552#4581365, @vitalybuka wrote:

Recursion stack usage was expected to be least 2Mb (more with redzones), but I allocated only 1Mb. Strange that it consistently passed on Linux x86_64, aarch64 bots.

That patch also fixed the Solaris/amd64 buildbot. Thanks.

[arphaman]

Thanks for feedback. Probably fixed and reenabled on Darwin.

Unfortunately it looks like it still fails on Darwin even after that change:

<stdin>:1:1: note: scanning from here
Assertion failed: (pthread_attr_setstack(&attr, main_stack, kStackSize) == 0), function main, file /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp, line 79.
^

https://green.lab.llvm.org/green/job/clang-stage1-RA/35285/testReport/junit/AddressSanitizer-x86_64-darwin/TestCases_Posix/fake_stack_gc_cpp/

Could you please take another look?

compiler-rt/lib/asan/asan_fake_stack.cpp

uptr FakeStack::AddrIsInFakeStack(uptr ptr, uptr *frame_beg, uptr *frame_end) {
   *frame_beg = res + sizeof(FakeFrame);
   return res;
 }
 
 void FakeStack::HandleNoReturn() {
   needs_gc_ = true;
 }
 
+// Hack: The statement below is not true if we take into account sigaltstack or
+// makecontext. It should be possible to make GC to discard wrong stack frame if
+// we use these tools. For now, let's support the simplest case and allow GC to
+// discard only frames from the default stack, assuming there is no buffer on
+// the stack which is used for makecontext or sigaltstack.
+//
 // When throw, longjmp or some such happens we don't call OnFree() and
 // as the result may leak one or more fake frames, but the good news is that
 // we are notified about all such events by HandleNoReturn().
 // If we recently had such no-return event we need to collect garbage frames.
 // We do it based on their 'real_stack' values -- everything that is lower
 // than the current real_stack is garbage.
 NOINLINE void FakeStack::GC(uptr real_stack) {
+  AsanThread *curr_thread = GetCurrentThread();
+  if (!curr_thread)
+    return;  // Try again when we have a thread.
+  auto top = curr_thread->stack_top();
+  auto bottom = curr_thread->stack_bottom();
+  if (real_stack < top || real_stack > bottom)
+    return;  // Not the default stack.
+
   for (uptr class_id = 0; class_id < kNumberOfSizeClasses; class_id++) {
     u8 *flags = GetFlags(stack_size_log(), class_id);
     for (uptr i = 0, n = NumberOfFrames(stack_size_log(), class_id); i < n;
          i++) {
       if (flags[i] == 0) continue;  // not allocated.
       FakeFrame *ff = reinterpret_cast<FakeFrame *>(
           GetFrame(stack_size_log(), class_id, i));
-      if (ff->real_stack < real_stack) {
+      // GC only on the default stack.
+      if (ff->real_stack < real_stack && ff->real_stack >= top) {
         flags[i] = 0;
+        SetShadow(reinterpret_cast<uptr>(ff), BytesInSizeClass(class_id),
+                  class_id, kMagic8);
       }
     }
   }
   needs_gc_ = false;
 }
 
 void FakeStack::ForEachFakeFrame(RangeIteratorCallback callback, void *arg) {
   for (uptr class_id = 0; class_id < kNumberOfSizeClasses; class_id++) {

compiler-rt/test/asan/TestCases/Posix/fake_stack_gc.cpp

+// RUN: %clangxx_asan -O0 -g %s -o %t && %env_asan_opts=use_sigaltstack=0 %run not --crash %t 2>&1 | FileCheck %s
+
+// Check that fake stack does not discard frames on the main stack, when GC is
+// triggered from high alt stack.
+
+#include <algorithm>
+#include <assert.h>
+#include <csignal>
+#include <cstdint>
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+const size_t kStackSize = 0x100000;
+
+int *on_thread;
+int *p;
+
+template <size_t N> void Fn() {
+  int t[N];
+  p = t;
+  if constexpr (N > 1)
+    Fn<N - 1>();
+}
+
+static void Handler(int signo) {
+  fprintf(stderr, "Handler Frame:%p\n", __builtin_frame_address(0));
+
+  // Trigger GC and create a lot of frame to reuse "Thread" frame if it was
+  // discarded.
+  for (int i = 0; i < 1000; ++i)
+    Fn<1000>();
+  // If we discarder and reused "Thread" frame, the next line will crash with
+  // false report.
+  *on_thread = 10;
+  fprintf(stderr, "SUCCESS\n");
+  // CHECK: SUCCESS
+}
+
+void *Thread(void *arg) {
+  fprintf(stderr, "Thread Frame:%p\n", __builtin_frame_address(0));
+  stack_t stack = {
+      .ss_sp = arg,
+      .ss_flags = 0,
+      .ss_size = kStackSize,
+  };
+  assert(sigaltstack(&stack, nullptr) == 0);
+
+  struct sigaction sa = {};
+  sa.sa_handler = Handler;
+  sa.sa_flags = SA_ONSTACK;
+  sigaction(SIGABRT, &sa, nullptr);
+
+  // Store pointer to the local var, so we can access this frame from the signal
+  // handler when the frame is still alive.
+  int n;
+  on_thread = &n;
+
+  // Abort should schedule FakeStack GC and call handler on alt stack.
+  abort();
+}
+
+int main(void) {
+  // Allocate main and alt stack for future thread.
+  void *main_stack = malloc(kStackSize);
+  void *alt_stack = malloc(kStackSize);
+
+  // Pick the lower stack as the main stack, as we want to trigger GC in
+  // FakeStack from alt stack in a such way that main stack is allocated below.
+  if ((uintptr_t)main_stack > (uintptr_t)alt_stack)
+    std::swap(alt_stack, main_stack);
+
+  pthread_attr_t attr;
+  assert(pthread_attr_init(&attr) == 0);
+  assert(pthread_attr_setstack(&attr, main_stack, kStackSize) == 0);
+
+  fprintf(stderr, "main_stack: %p-%p\n", main_stack,
+          (char *)main_stack + kStackSize);
+  fprintf(stderr, "alt_stack: %p-%p\n", alt_stack,
+          (char *)alt_stack + kStackSize);
+
+  pthread_t tid;
+  assert(pthread_create(&tid, &attr, Thread, alt_stack) == 0);
+
+  pthread_join(tid, nullptr);
+
+  free(main_stack);
+  free(alt_stack);
+
+  return 0;
+}