SafeStack: Delay thread stack clean-up

[vlad.tsyrklevich]

Summary

glibc can call SafeStack instrumented code even after the last pthread
data destructor has run. Delay cleaning-up unsafe stacks for threads
until the thread is dead by having future threads clean-up prior threads
stacks.

[vlad.tsyrklevich]

lib/safestack/safestack.cc

162

Wasn't sure if I was allowed to use libpthread from a runtime. I think I could replace the mutex with the __atomic_exchange() built-in if I need to.

[eugenis]

lib/safestack/safestack.cc

162

We are using it already, so it should be fine.
I think it would only matter if we instrument libc... let's not worry about that for now.

202

You are trying to append to the end of temp_stacks list.

But if we managed to destroy all pending stacks, then stackp points to temp_stacks itself, which is a local variable, and this store is dead.

[vlad.tsyrklevich]

• Fix logic bug found by eugenis

[cryptoad]

Random thought: isn't the introduction of malloc here (as opposed to an OS backed alternative like mmap) gonna mess compatibility with other Sanitizers that intercept it? (thinking of Scudo which is currently compatible with SafeStack but I haven't tested).

[vlad.tsyrklevich]

In D50406#1194152, @cryptoad wrote:

Random thought: isn't the introduction of malloc here (as opposed to an OS backed alternative like mmap) gonna mess compatibility with other Sanitizers that intercept it? (thinking of Scudo which is currently compatible with SafeStack but I haven't tested).

malloc() would only be called during thread destruction, so it doesn't seem like there should be an issue unless malloc() causes a thread to destruct and even then this function shouldn't re-enter because of the order of when pthread_setspecific() is called. Perhaps I'm just failing to imagine a situation under which this could be an issue?

[cryptoad]

In D50406#1194173, @vlad.tsyrklevich wrote:

malloc() would only be called during thread destruction, so it doesn't seem like there should be an issue unless malloc() causes a thread to destruct and even then this function shouldn't re-enter because of the order of when pthread_setspecific() is called. Perhaps I'm just failing to imagine a situation under which this could be an issue?

Ack. Tried out a few things, as far as I can tell it works as intended.

lib/safestack/safestack.cc

 //===-- safestack.cc ------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the runtime support for the safe stack protection
 // mechanism. The runtime manages allocation/deallocation of the unsafe stack
 // for the main thread, as well as all pthreads that are created/destroyed
 // during program execution.
 //
 //===----------------------------------------------------------------------===//
 
+#include <errno.h>
 #include <limits.h>
 #include <pthread.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <unistd.h>
+#include <stdlib.h>
 #include <sys/resource.h>
 #include <sys/types.h>
 #if !defined(__NetBSD__)
 #include <sys/user.h>
 #endif
 
 #include "interception/interception.h"
 #include "sanitizer_common/sanitizer_common.h"
static inline void unsafe_stack_setup(void *start, size_t size, size_t guard) {
   CHECK_EQ((((size_t)stack_ptr) & (kStackAlign - 1)), 0);
 
   __safestack_unsafe_stack_ptr = stack_ptr;
   unsafe_stack_start = start;
   unsafe_stack_size = size;
   unsafe_stack_guard = guard;
 }
 
-static void unsafe_stack_free() {
-  if (unsafe_stack_start) {
-    UnmapOrDie((char *)unsafe_stack_start - unsafe_stack_guard,
-               unsafe_stack_size + unsafe_stack_guard);
-  }
-  unsafe_stack_start = nullptr;
-}
-
 /// Thread data for the cleanup handler
 static pthread_key_t thread_cleanup_key;
 
 /// Safe stack per-thread information passed to the thread_start function
 struct tinfo {
   void *(*start_routine)(void *);
   void *start_routine_arg;
 
static void *thread_start(void *arg) {
   void *(*start_routine)(void *) = tinfo->start_routine;
   void *start_routine_arg = tinfo->start_routine_arg;
 
   // Setup the unsafe stack; this will destroy tinfo content
   unsafe_stack_setup(tinfo->unsafe_stack_start, tinfo->unsafe_stack_size,
                      tinfo->unsafe_stack_guard);
 
   // Make sure out thread-specific destructor will be called
-  // FIXME: we can do this only any other specific key is set by
-  // intercepting the pthread_setspecific function itself
   pthread_setspecific(thread_cleanup_key, (void *)1);
 
   return start_routine(start_routine_arg);
 }
 
-/// Thread-specific data destructor
+/// Linked list used to store exiting threads stack/thread information.
+struct thread_stack_ll {
+  struct thread_stack_ll *next;
+  void *stack_base;
+  pid_t pid;
+  tid_t tid;
+};
+
+/// Linked list of unsafe stacks for threads that are exiting. We delay
+/// unmapping them until the thread exits.
+static thread_stack_ll *thread_stacks = nullptr;
+static pthread_mutex_t thread_stacks_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+/// Thread-specific data destructor. We want to free the unsafe stack only after
+/// this thread is terminated. libc can call functions in safestack-instrumented
+/// code (like free) after thread-specific data destructors have run.
 static void thread_cleanup_handler(void *_iter) {
-  // We want to free the unsafe stack only after all other destructors
-  // have already run. We force this function to be called multiple times.
-  // User destructors that might run more then PTHREAD_DESTRUCTOR_ITERATIONS-1
-  // times might still end up executing after the unsafe stack is deallocated.
-  size_t iter = (size_t)_iter;
-  if (iter < PTHREAD_DESTRUCTOR_ITERATIONS) {
-    pthread_setspecific(thread_cleanup_key, (void *)(iter + 1));
-  } else {
-    // This is the last iteration
-    unsafe_stack_free();
-  }
+  CHECK_NE(unsafe_stack_start, nullptr);
+  pthread_setspecific(thread_cleanup_key, NULL);
+
+  pthread_mutex_lock(&thread_stacks_mutex);
+  // Temporary list to hold the previous threads stacks so we don't hold the
+  // thread_stacks_mutex for long.
+  thread_stack_ll *temp_stacks = thread_stacks;
+  thread_stacks = nullptr;
+  pthread_mutex_unlock(&thread_stacks_mutex);
+
+  pid_t pid = getpid();
+  tid_t tid = GetTid();
+
+  // Free stacks for dead threads
+  thread_stack_ll **stackp = &temp_stacks;
+  while (*stackp) {
+    thread_stack_ll *stack = *stackp;
+    if (stack->pid != pid || TgKill(stack->pid, stack->tid, 0) == -ESRCH) {
+      UnmapOrDie(stack->stack_base, unsafe_stack_size + unsafe_stack_guard);
+      *stackp = stack->next;
+      free(stack);
+    } else
+      stackp = &stack->next;
+  }
+
+  thread_stack_ll *cur_stack =
+      (thread_stack_ll *)malloc(sizeof(thread_stack_ll));
+  cur_stack->stack_base = (char *)unsafe_stack_start - unsafe_stack_guard;
+  cur_stack->pid = pid;
+  cur_stack->tid = tid;
+
+  pthread_mutex_lock(&thread_stacks_mutex);
+  // Merge thread_stacks with the current thread's stack and any remaining
+  // temp_stacks
+  *stackp = thread_stacks;
+  cur_stack->next = temp_stacks;
+  thread_stacks = cur_stack;
+  pthread_mutex_unlock(&thread_stacks_mutex);
+
+  unsafe_stack_start = nullptr;
 }
 
 static void EnsureInterceptorsInitialized();
 
 /// Intercept thread creation operation to allocate and setup the unsafe stack
 INTERCEPTOR(int, pthread_create, pthread_t *thread,
             const pthread_attr_t *attr,
             void *(*start_routine)(void*), void *arg) {

lib/sanitizer_common/sanitizer_common.h

if (!PageSizeCached)
     PageSizeCached = GetPageSize();
   return PageSizeCached;
 }
 uptr GetMmapGranularity();
 uptr GetMaxVirtualAddress();
 uptr GetMaxUserVirtualAddress();
 // Threads
 tid_t GetTid();
+int TgKill(pid_t pid, tid_t tid, int sig);
 uptr GetThreadSelf();
 void GetThreadStackTopAndBottom(bool at_initialization, uptr *stack_top,
                                 uptr *stack_bottom);
 void GetThreadStackAndTls(bool main, uptr *stk_addr, uptr *stk_size,
                           uptr *tls_addr, uptr *tls_size);
 
 // Memory management
 void *MmapOrDie(uptr size, const char *mem_type, bool raw_report = false);

lib/sanitizer_common/sanitizer_linux.cc

#elif SANITIZER_NETBSD
   return _lwp_self();
 #elif SANITIZER_SOLARIS
   return thr_self();
 #else
   return internal_syscall(SYSCALL(gettid));
 #endif
 }
 
+int TgKill(pid_t pid, tid_t tid, int sig) {
+#if SANITIZER_LINUX
+  return internal_syscall(SYSCALL(tgkill), pid, tid, sig);
+#else
+  return internal_syscall(SYSCALL(thr_kill2), pid, tid, sig);
+#endif
+}
+
 #if !SANITIZER_SOLARIS
 u64 NanoTime() {
 #if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_OPENBSD
   timeval tv;
 #else
   kernel_timeval tv;
 #endif
   internal_memset(&tv, 0, sizeof(tv));

test/safestack/pthread-cleanup.c

 // RUN: %clang_safestack %s -pthread -o %t
-// RUN: not --crash %run %t
+// RUN: %run %t 0
+// RUN: not --crash %run %t 1
 
-// Test that unsafe stacks are deallocated correctly on thread exit.
+// Test unsafe stack deallocation. Unsafe stacks are not deallocated immediately
+// at thread exit. They are deallocated by following exiting threads.
 
 #include <stdlib.h>
 #include <string.h>
 #include <pthread.h>
 
 enum { kBufferSize = (1 << 15) };
 
-void *t1_start(void *ptr)
+void *start(void *ptr)
 {
   char buffer[kBufferSize];
   return buffer;
 }
 
 int main(int argc, char **argv)
 {
-  pthread_t t1;
-  char *buffer = NULL;
+  int arg = atoi(argv[1]);
 
-  if (pthread_create(&t1, NULL, t1_start, NULL))
+  pthread_t t1, t2;
+  char *t1_buffer = NULL;
+
+  if (pthread_create(&t1, NULL, start, NULL))
+    abort();
+  if (pthread_join(t1, &t1_buffer))
+    abort();
+
+  memset(t1_buffer, 0, kBufferSize);
+
+  if (arg == 0)
+    return 0;
+
+  if (pthread_create(&t2, NULL, start, NULL))
     abort();
-  if (pthread_join(t1, &buffer))
+  // Second thread destructor cleans up the first thread's stack.
+  if (pthread_join(t2, NULL))
     abort();
 
   // should segfault here
-  memset(buffer, 0, kBufferSize);
+  memset(t1_buffer, 0, kBufferSize);
   return 0;
 }