[libc-commits] [libc] 916e9be - Cleanup code in `thread_exit`

[Noah Goldstein via libc-commits]

Author: Noah Goldstein
Date: 2023-05-22T15:54:19-05:00
New Revision: 916e9be4aa0aa4bfa3ab0f1b3f91e59b81dc10b2

URL: https://github.com/llvm/llvm-project/commit/916e9be4aa0aa4bfa3ab0f1b3f91e59b81dc10b2
DIFF: https://github.com/llvm/llvm-project/commit/916e9be4aa0aa4bfa3ab0f1b3f91e59b81dc10b2.diff

LOG: Cleanup code in `thread_exit`

1) Avoid proper function calls and referencing local variables after
the stack has been deallocated. A proper function call/return or local
variable reference that may have spilled will cause invalid memory
reads after the stack has been deallocated.

2) Mark the function as [[noreturn]] and place
`__builtin_unreachable()` after the `SYS_exit` syscalls.

Reviewed By: sivachandra

Differential Revision: https://reviews.llvm.org/D151142

Added: 
    

Modified: 
    libc/src/__support/threads/linux/thread.cpp
    libc/src/__support/threads/thread.h

Removed: 
    


################################################################################
diff  --git a/libc/src/__support/threads/linux/thread.cpp b/libc/src/__support/threads/linux/thread.cpp
index a2bd449a24db..60baff5e8998 100644
--- a/libc/src/__support/threads/linux/thread.cpp
+++ b/libc/src/__support/threads/linux/thread.cpp
@@ -117,7 +117,11 @@ LIBC_INLINE ErrorOr<void *> alloc_stack(size_t stacksize, size_t guardsize) {
   return reinterpret_cast<void *>(mmap_result);
 }
 
-LIBC_INLINE void free_stack(void *stack, size_t stacksize, size_t guardsize) {
+// This must always be inlined as we may be freeing the calling threads stack in
+// which case a normal return from the top the stack would cause an invalid
+// memory read.
+static __attribute__((always_inline)) inline void
+free_stack(void *stack, size_t stacksize, size_t guardsize) {
   uintptr_t stackaddr = reinterpret_cast<uintptr_t>(stack);
   stackaddr -= guardsize;
   stack = reinterpret_cast<void *>(stackaddr);
@@ -137,7 +141,11 @@ struct alignas(STACK_ALIGNMENT) StartArgs {
   void *arg;
 };
 
-static void cleanup_thread_resources(ThreadAttributes *attrib) {
+// This must always be inlined as we may be freeing the calling threads stack in
+// which case a normal return from the top the stack would cause an invalid
+// memory read.
+static __attribute__((always_inline)) inline void
+cleanup_thread_resources(ThreadAttributes *attrib) {
   // Cleanup the TLS before the stack as the TLS information is stored on
   // the stack.
   cleanup_tls(attrib->tls, attrib->tls_size);
@@ -496,14 +504,18 @@ void thread_exit(ThreadReturnValue retval, ThreadStyle style) {
     // Set the CLEAR_TID address to nullptr to prevent the kernel
     // from signalling at a non-existent futex location.
     __llvm_libc::syscall_impl(SYS_set_tid_address, 0);
+    // Return value for detached thread should be unused. We need to avoid
+    // referencing `style` or `retval.*` because they may be stored on the stack
+    // and we have deallocated our stack!
+    __llvm_libc::syscall_impl(SYS_exit, 0);
+    __builtin_unreachable();
   }
 
-  // TODO: We may have deallocated the stack. Can we reference local variables
-  // that may have spilled?
   if (style == ThreadStyle::POSIX)
     __llvm_libc::syscall_impl(SYS_exit, retval.posix_retval);
   else
     __llvm_libc::syscall_impl(SYS_exit, retval.stdc_retval);
+  __builtin_unreachable();
 }
 
 } // namespace __llvm_libc

diff  --git a/libc/src/__support/threads/thread.h b/libc/src/__support/threads/thread.h
index 5843d9196a2c..491816348670 100644
--- a/libc/src/__support/threads/thread.h
+++ b/libc/src/__support/threads/thread.h
@@ -228,7 +228,7 @@ struct Thread {
 extern thread_local Thread self;
 
 // Platforms should implement this function.
-void thread_exit(ThreadReturnValue retval, ThreadStyle style);
+[[noreturn]] void thread_exit(ThreadReturnValue retval, ThreadStyle style);
 
 namespace internal {
 // Internal namespace containing utilities which are to be used by platform