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

[Usama Hameed via Phabricator via llvm-commits]

usama54321 added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp:152
   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,
----------------
kubamracek wrote:
> usama54321 wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137292/new/

https://reviews.llvm.org/D137292