[lsan] Fix deadlock in dl_iterate_phdr.

[eugenis]

Summary

Do not grab the allocator lock before calling dl_iterate_phdr. This may
cause a lock order inversion with (valid) user code that uses malloc
inside a dl_iterate_phdr callback.

[hctim]

compiler-rt/test/lsan/TestCases/Linux/libdl_deadlock.cpp

5 ↗

(On Diff #220766)

Nit: order includes

13 ↗

(On Diff #220766)

nit: int Callback

20 ↗

(On Diff #220766)

nit: replace with if (step == 0)

32 ↗

(On Diff #220766)

Nit: 80chars

[eugenis]

addressed review comments

compiler-rt/trunk/lib/lsan/lsan_common.h

 struct RootRegion {
   uptr begin;
   uptr size;
 };
 
 InternalMmapVector<RootRegion> const *GetRootRegions();
 void ScanRootRegion(Frontier *frontier, RootRegion const &region,
                     uptr region_begin, uptr region_end, bool is_readable);
-// Run stoptheworld while holding any platform-specific locks.
-void DoStopTheWorld(StopTheWorldCallback callback, void* argument);
+// Run stoptheworld while holding any platform-specific locks, as well as the
+// allocator and thread registry locks.
+void LockStuffAndStopTheWorld(StopTheWorldCallback callback, void* argument);
 
 void ScanRangeForPointers(uptr begin, uptr end,
                           Frontier *frontier,
                           const char *region_type, ChunkTag tag);
 void ScanGlobalRange(uptr begin, uptr end, Frontier *frontier);
 
 enum IgnoreObjectResult {
   kIgnoreObjectSuccess,

compiler-rt/trunk/lib/lsan/lsan_common.cpp

 }
 
 static bool CheckForLeaks() {
   if (&__lsan_is_turned_off && __lsan_is_turned_off())
       return false;
   EnsureMainThreadIDIsCorrect();
   CheckForLeaksParam param;
   param.success = false;
-  LockThreadRegistry();
-  LockAllocator();
-  DoStopTheWorld(CheckForLeaksCallback, &param);
-  UnlockAllocator();
-  UnlockThreadRegistry();
+  LockStuffAndStopTheWorld(CheckForLeaksCallback, &param);
 
   if (!param.success) {
     Report("LeakSanitizer has encountered a fatal error.\n");
     Report(
         "HINT: For debugging, try setting environment variable "
         "LSAN_OPTIONS=verbosity=1:log_threads=1\n");
     Report(
         "HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc)\n");

compiler-rt/trunk/lib/lsan/lsan_common_linux.cpp

 
 // While calling Die() here is undefined behavior and can potentially
 // cause race conditions, it isn't possible to intercept exit on linux,
 // so we have no choice but to call Die() from the atexit handler.
 void HandleLeaks() {
   if (common_flags()->exitcode) Die();
 }
 
-static int DoStopTheWorldCallback(struct dl_phdr_info *info, size_t size,
-                                  void *data) {
+static int LockStuffAndStopTheWorldCallback(struct dl_phdr_info *info,
+                                            size_t size, void *data) {
+  LockThreadRegistry();
+  LockAllocator();
   DoStopTheWorldParam *param = reinterpret_cast<DoStopTheWorldParam *>(data);
   StopTheWorld(param->callback, param->argument);
+  UnlockAllocator();
+  UnlockThreadRegistry();
   return 1;
 }
 
 // LSan calls dl_iterate_phdr() from the tracer task. This may deadlock: if one
 // of the threads is frozen while holding the libdl lock, the tracer will hang
 // in dl_iterate_phdr() forever.
 // Luckily, (a) the lock is reentrant and (b) libc can't distinguish between the
 // tracer task and the thread that spawned it. Thus, if we run the tracer task
 // while holding the libdl lock in the parent thread, we can safely reenter it
 // in the tracer. The solution is to run stoptheworld from a dl_iterate_phdr()
 // callback in the parent thread.
-void DoStopTheWorld(StopTheWorldCallback callback, void *argument) {
+void LockStuffAndStopTheWorld(StopTheWorldCallback callback, void *argument) {
   DoStopTheWorldParam param = {callback, argument};
-  dl_iterate_phdr(DoStopTheWorldCallback, &param);
+  dl_iterate_phdr(LockStuffAndStopTheWorldCallback, &param);
 }
 
 } // namespace __lsan
 
 #endif

compiler-rt/trunk/lib/lsan/lsan_common_mac.cpp

void ProcessPlatformSpecificAllocations(Frontier *frontier) {
   }
 }
 
 // On darwin, we can intercept _exit gracefully, and return a failing exit code
 // if required at that point. Calling Die() here is undefined behavior and
 // causes rare race conditions.
 void HandleLeaks() {}
 
-void DoStopTheWorld(StopTheWorldCallback callback, void *argument) {
+void LockStuffAndStopTheWorld(StopTheWorldCallback callback, void *argument) {
+  LockThreadRegistry();
+  LockAllocator();
   StopTheWorld(callback, argument);
+  UnlockAllocator();
+  UnlockThreadRegistry();
 }
 
 } // namespace __lsan
 
 #endif // CAN_SANITIZE_LEAKS && SANITIZER_MAC

compiler-rt/trunk/test/lsan/TestCases/Linux/libdl_deadlock.cpp

+// Regression test for a deadlock in leak detection,
+// where lsan would call dl_iterate_phdr while holding the allocator lock.
+// RUN: %clangxx_lsan %s -o %t && %run %t
+
+#include <link.h>
+#include <mutex>
+#include <stdlib.h>
+#include <thread>
+#include <unistd.h>
+
+std::mutex in, out;
+
+int Callback(struct dl_phdr_info *info, size_t size, void *data) {
+  for (int step = 0; step < 50; ++step) {
+    void *p[1000];
+    for (int i = 0; i < 1000; ++i)
+      p[i] = malloc(10 * i);
+
+    if (step == 0)
+      in.unlock();
+
+    for (int i = 0; i < 1000; ++i)
+      free(p[i]);
+  }
+  out.unlock();
+  return 1; // just once
+}
+
+void Watchdog() {
+  // This is just a fail-safe to turn a deadlock (in case the bug reappears)
+  // into a (slow) test failure.
+  usleep(10000000);
+  if (!out.try_lock()) {
+    write(2, "DEADLOCK\n", 9);
+    exit(1);
+  }
+}
+
+int main() {
+  in.lock();
+  out.lock();
+
+  std::thread t([] { dl_iterate_phdr(Callback, nullptr); });
+  t.detach();
+
+  std::thread w(Watchdog);
+  w.detach();
+
+  // Wait for the malloc thread to preheat, then start leak detection (on exit)
+  in.lock();
+  return 0;
+}