[lsan] Fix stack buffer overwrite in SuspendedThreadsListMac::GetRegistersAndSP

[kubamracek]

Summary

The call to the thread_get_state syscall (that fetches the register values for a thread) on arm64 is mistakenly claiming that the buffer to receive the register state is larger that its actual size on the stack -- the struct on the stack is arm_thread_state64_t, but the MACHINE_THREAD_STATE + MACHINE_THREAD_STATE_COUNT refer to the "unified arm state" struct (which is larger).

Fixes https://github.com/llvm/llvm-project/issues/58503.

[vitalybuka]

BTW. Repro is very trivial, would you like to add a regression test?

[kubamracek]

BTW. Repro is very trivial, would you like to add a regression test?

Do you have a specific test/approach in mind? I think this is well covered by existing tests, basically all LSan tests exercise this codepath. But the bug (the hang) only triggers on arm64 and I bet the reason why CI and buildbots didn't discover this is that they don't actually run on Apple Silicon Mac hardware.

[vitalybuka]

In D137292#3903767, @kubamracek wrote:

BTW. Repro is very trivial, would you like to add a regression test?

Do you have a specific test/approach in mind? I think this is well covered by existing tests, basically all LSan tests exercise this codepath. But the bug (the hang) only triggers on arm64 and I bet the reason why CI and buildbots didn't discover this is that they don't actually run on Apple Silicon Mac hardware.

You are right. LGTM without the test.

[usama54321]

Added a minor comment below. Apart from that looks good to me

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp

152–153

I will just suggest using the corresponding macros for reg count, e.g. ARM_THREAD_STATE64_COUNT etc. from the headers rather the inlining the calculation. Apart from that looks good to me.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp

152–153

I think I'm slightly against that (I considered it initially) because this is exactly what caused the bug in the first place (!) and triggered a stack smasher (!!!) with very non-obvious consequences. The reg_count variable is telling thread_get_state how large the buffer is that it can write to... and I'd say that the most correct and most obvious way of getting that right is by actually using sizeof on the actual stack variable that holds the buffer.

[usama54321]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp

152–153

In that case, it is fine. I suggested that because I have been recently fixing bugs in ubsan where some definitions were out of sync from the headers. However, that seems less likely here.

[usama54321]

LGTM

[thetruestblue]

LGTM -- fixes the arm64 hang I've been running into.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp

 void StopTheWorld(StopTheWorldCallback callback, void *argument) {
   struct RunThreadArgs arg = {callback, argument};
   pthread_t run_thread = (pthread_t)internal_start_thread(RunThread, &arg);
   internal_join_thread(run_thread);
 }
 
 #if defined(__x86_64__)
 typedef x86_thread_state64_t regs_struct;
+#define regs_flavor x86_THREAD_STATE64
 
 #define SP_REG __rsp
 
 #elif defined(__aarch64__)
 typedef arm_thread_state64_t regs_struct;
+#define regs_flavor ARM_THREAD_STATE64
 
 # if __DARWIN_UNIX03
 #  define SP_REG __sp
 # else
 #  define SP_REG sp
 # endif
 
 #elif defined(__i386)
 typedef x86_thread_state32_t regs_struct;
+#define regs_flavor x86_THREAD_STATE32
 
 #define SP_REG __esp
 
 #else
 #error "Unsupported architecture"
 #endif
 
 tid_t SuspendedThreadsListMac::GetThreadID(uptr index) const {
void SuspendedThreadsListMac::Append(thread_t thread) {
   threads_.push_back({info.thread_id, thread});
 }
 
 PtraceRegistersStatus SuspendedThreadsListMac::GetRegistersAndSP(
     uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
   thread_t thread = GetThread(index);
   regs_struct regs;
   int err;
-  mach_msg_type_number_t reg_count = MACHINE_THREAD_STATE_COUNT;
-  err = thread_get_state(thread, MACHINE_THREAD_STATE, (thread_state_t)&regs,
+  mach_msg_type_number_t reg_count = sizeof(regs) / sizeof(natural_t);
+  err = thread_get_state(thread, regs_flavor, (thread_state_t)&regs,
                          &reg_count);
   if (err != KERN_SUCCESS) {
     VReport(1, "Error - unable to get registers for a thread\n");
     // KERN_INVALID_ARGUMENT indicates that either the flavor is invalid,
     // or the thread does not exist. The other possible error case,
     // MIG_ARRAY_TOO_LARGE, means that the state is too large, but it's
     // still safe to proceed.
     return err == KERN_INVALID_ARGUMENT ? REGISTERS_UNAVAILABLE_FATAL