Don't call exit() from atexit handlers on Darwin

[fjricci]

Summary

Calling exit() from an atexit handler is undefined behavior.
On Linux, it's unavoidable, since we cannot intercept exit (_exit isn't called
if a user program uses return instead of exit()), and I haven't
seen it cause issues regardless.

However, on Darwin, I have a fairly complex internal test that hangs roughly
once in every 300 runs after leak reporting finishes, which is resolved with
this patch, and is presumably due to the undefined behavior (since the Die() is
the only thing that happens after the end of leak reporting).

In addition, this is the way TSan works as well, where an atexit handler+Die()
is used on Linux, and an _exit() interceptor is used on Darwin. I'm not sure if it's
intentionally structured that way in TSan, since TSan sets up the atexit handler and the
_exit() interceptor on both platforms, but I have observed that on Darwin, only the
_exit() interceptor is used, and on Linux the atexit handler is used.

There is some additional related discussion here: https://reviews.llvm.org/D35085

[vsk]

I haven't reviewed an lsan patch before, so I can't say whether there's anything missing (e.g I'm not sure how lsan is tested), but the approach and rationale here sgtm.

[alekseyshl]

lib/lsan/lsan_common.cc

579 ↗

(On Diff #106964)

has_reported_leaks?

589 ↗

(On Diff #106964)

Why not SetHasReportedLeaks() here, for all platforms? Linux does not care. In this case we can remove SetHasReportedLeaks() altogether and the logic is a bit easier to reason about.

lib/lsan/lsan_common.h

232 ↗

(On Diff #106964)

HandleLeaks(), maybe?

Please separate it from other two and add a comment to it.

[fjricci]

Address comments

compiler-rt/trunk/lib/asan/asan_interceptors.cc

 
 void SetThreadName(const char *name) {
   AsanThread *t = GetCurrentThread();
   if (t)
     asanThreadRegistry().SetThreadName(t->tid(), name);
 }
 
 int OnExit() {
+  if (CAN_SANITIZE_LEAKS && common_flags()->detect_leaks &&
+      __lsan::HasReportedLeaks()) {
+    return common_flags()->exitcode;
+  }
   // FIXME: ask frontend whether we need to return failure.
   return 0;
 }
 
 } // namespace __asan
 
 // ---------------------- Wrappers ---------------- {{{1
 using namespace __asan;  // NOLINT

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

 // Returns address of user-visible chunk contained in this allocator chunk.
 uptr GetUserBegin(uptr chunk);
 // Helper for __lsan_ignore_object().
 IgnoreObjectResult IgnoreObjectLocked(const void *p);
 
 // Return the linker module, if valid for the platform.
 LoadedModule *GetLinker();
 
+// Return true if LSan has finished leak checking and reported leaks.
+bool HasReportedLeaks();
+
+// Run platform-specific leak handlers.
+void HandleLeaks();
+
 // Wrapper for chunk metadata operations.
 class LsanMetadata {
  public:
   // Constructor accepts address of user-visible chunk.
   explicit LsanMetadata(uptr chunk);
   bool allocated() const;
   ChunkTag tag() const;
   void set_tag(ChunkTag value);

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

if (common_flags()->print_suppressions)
     PrintMatchedSuppressions();
   if (unsuppressed_count > 0) {
     param.leak_report.PrintSummary();
     return true;
   }
   return false;
 }
 
+static bool has_reported_leaks = false;
+bool HasReportedLeaks() { return has_reported_leaks; }
+
 void DoLeakCheck() {
   BlockingMutexLock l(&global_mutex);
   static bool already_done;
   if (already_done) return;
   already_done = true;
-  bool have_leaks = CheckForLeaks();
-  if (!have_leaks) {
-    return;
-  }
-  if (common_flags()->exitcode) {
-    Die();
-  }
+  has_reported_leaks = CheckForLeaks();
+  if (has_reported_leaks) HandleLeaks();
 }
 
 static int DoRecoverableLeakCheck() {
   BlockingMutexLock l(&global_mutex);
   bool have_leaks = CheckForLeaks();
   return have_leaks ? 1 : 0;
 }
 

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

 
 void ProcessPlatformSpecificAllocations(Frontier *frontier) {}
 
 struct DoStopTheWorldParam {
   StopTheWorldCallback callback;
   void *argument;
 };
 
+// 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) {
   DoStopTheWorldParam *param = reinterpret_cast<DoStopTheWorldParam *>(data);
   StopTheWorld(param->callback, param->argument);
   return 1;
 }
 
 // LSan calls dl_iterate_phdr() from the tracer task. This may deadlock: if one

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

if (flags()->use_root_regions) {
                        info.protection & kProtectionRead);
       }
     }
 
     address = end_address;
   }
 }
 
+// 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) {
   StopTheWorld(callback, argument);
 }
 
 } // namespace __lsan
 
 #endif // CAN_SANITIZE_LEAKS && SANITIZER_MAC

compiler-rt/trunk/lib/lsan/lsan_interceptors.cc

INTERCEPTOR(int, pthread_join, void *th, void **ret) {
   ENSURE_LSAN_INITED;
   int tid = ThreadTid((uptr)th);
   int res = REAL(pthread_join)(th, ret);
   if (res == 0)
     ThreadJoin(tid);
   return res;
 }
 
+INTERCEPTOR(void, _exit, int status) {
+  if (status == 0 && HasReportedLeaks()) status = common_flags()->exitcode;
+  REAL(_exit)(status);
+}
+
 namespace __lsan {
 
 void InitializeInterceptors() {
   INTERCEPT_FUNCTION(malloc);
   INTERCEPT_FUNCTION(free);
   LSAN_MAYBE_INTERCEPT_CFREE;
   INTERCEPT_FUNCTION(calloc);
   INTERCEPT_FUNCTION(realloc);
   LSAN_MAYBE_INTERCEPT_MEMALIGN;
   LSAN_MAYBE_INTERCEPT___LIBC_MEMALIGN;
   LSAN_MAYBE_INTERCEPT_ALIGNED_ALLOC;
   INTERCEPT_FUNCTION(posix_memalign);
   INTERCEPT_FUNCTION(valloc);
   LSAN_MAYBE_INTERCEPT_PVALLOC;
   LSAN_MAYBE_INTERCEPT_MALLOC_USABLE_SIZE;
   LSAN_MAYBE_INTERCEPT_MALLINFO;
   LSAN_MAYBE_INTERCEPT_MALLOPT;
   INTERCEPT_FUNCTION(pthread_create);
   INTERCEPT_FUNCTION(pthread_join);
+  INTERCEPT_FUNCTION(_exit);
 
   if (pthread_key_create(&g_thread_finalize_key, &thread_finalize)) {
     Report("LeakSanitizer: failed to create thread key.\n");
     Die();
   }
 }
 
 } // namespace __lsan