From 33d73379aad3ed7dcbb9b10f945d3bc6264b79bd Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris@google.com>
Date: Fri, 2 Jul 2021 15:46:18 -0700
Subject: [PATCH] Fix race when frees after main thread finishes.

When the main thread is exiting, the code deleted the g_debug global
pointer and destroys the disable pthread key. Unfortunately, if
malloc debug was enabled in a way that requires a header for the pointer,
any frees that occur after the main thread is torn down result in calls
to the underlying allocator with bad pointers.
To avoid this, don't delete the g_debug pointer and don't destroy the
disable pthread key.

Added a new system test that allocates a lot of pointers and frees them
after letting the main thread finish.

Also, fix one test that can fail sporadically due to a lack of unwinding
information on arm32.

Bug: 189541929

Test: Passes new system tests.
Change-Id: I1cfe868987a8f0dc880a5b65de6709f44a5f1988
---
 libc/malloc_debug/malloc_debug.cpp            |  7 +-
 .../tests/malloc_debug_system_tests.cpp       | 67 +++++++++++++++++--
 2 files changed, 65 insertions(+), 9 deletions(-)

diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp
index 609f030bf..d23ab15c3 100644
--- a/libc/malloc_debug/malloc_debug.cpp
+++ b/libc/malloc_debug/malloc_debug.cpp
@@ -362,10 +362,9 @@ void debug_finalize() {
 
   backtrace_shutdown();
 
-  delete g_debug;
-  g_debug = nullptr;
-
-  DebugDisableFinalize();
+  // In order to prevent any issues of threads freeing previous pointers
+  // after the main thread calls this code, simply leak the g_debug pointer
+  // and do not destroy the debug disable pthread key.
 }
 
 void debug_get_malloc_leak_info(uint8_t** info, size_t* overall_size, size_t* info_size,
diff --git a/libc/malloc_debug/tests/malloc_debug_system_tests.cpp b/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
index 1bfe61e77..68b3a1ec0 100644
--- a/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
+++ b/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
@@ -30,6 +30,7 @@
 #include <fcntl.h>
 #include <poll.h>
 #include <signal.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -449,12 +450,12 @@ TEST(MallocDebugSystemTest, verify_leak_allocation_limit) {
 }
 
 static constexpr int kExpectedExitCode = 30;
+static constexpr size_t kMaxThreads = sizeof(uint32_t) * 8;
 
 TEST(MallocTests, DISABLED_exit_while_threads_allocating) {
-  std::atomic_uint32_t thread_mask;
-  thread_mask = 0;
+  std::atomic_uint32_t thread_mask = {};
 
-  for (size_t i = 0; i < 32; i++) {
+  for (size_t i = 0; i < kMaxThreads; i++) {
     std::thread malloc_thread([&thread_mask, i] {
       while (true) {
         void* ptr = malloc(100);
@@ -469,7 +470,7 @@ TEST(MallocTests, DISABLED_exit_while_threads_allocating) {
   }
 
   // Wait until each thread has done at least one allocation.
-  while (thread_mask.load() != 0xffffffff)
+  while (thread_mask.load() != UINT32_MAX)
     ;
   exit(kExpectedExitCode);
 }
@@ -492,6 +493,59 @@ TEST(MallocDebugSystemTest, exit_while_threads_allocating) {
   }
 }
 
+TEST(MallocTests, DISABLED_exit_while_threads_freeing_allocs_with_header) {
+  static constexpr size_t kMaxAllocsPerThread = 1000;
+  std::atomic_uint32_t thread_mask = {};
+  std::atomic_bool run;
+
+  std::vector<std::vector<void*>> allocs(kMaxThreads);
+  // Pre-allocate a bunch of memory so that we can try to trigger
+  // the frees after the main thread finishes.
+  for (size_t i = 0; i < kMaxThreads; i++) {
+    for (size_t j = 0; j < kMaxAllocsPerThread; j++) {
+      void* ptr = malloc(8);
+      ASSERT_TRUE(ptr != nullptr);
+      allocs[i].push_back(ptr);
+    }
+  }
+
+  for (size_t i = 0; i < kMaxThreads; i++) {
+    std::thread malloc_thread([&thread_mask, &run, &allocs, i] {
+      thread_mask.fetch_or(1 << i);
+      while (!run)
+        ;
+      for (auto ptr : allocs[i]) {
+        free(ptr);
+      }
+    });
+    malloc_thread.detach();
+  }
+
+  // Wait until all threads are running.
+  while (thread_mask.load() != UINT32_MAX)
+    ;
+  run = true;
+  exit(kExpectedExitCode);
+}
+
+TEST(MallocDebugSystemTest, exit_while_threads_freeing_allocs_with_header) {
+  for (size_t i = 0; i < 50; i++) {
+    SCOPED_TRACE(::testing::Message() << "Run " << i);
+    pid_t pid;
+    // Enable guard to force the use of a header.
+    ASSERT_NO_FATAL_FAILURE(
+        Exec("MallocTests.DISABLED_exit_while_threads_freeing_allocs_with_header", "verbose guard",
+             &pid, kExpectedExitCode));
+
+    ASSERT_NO_FATAL_FAILURE(FindStrings(pid, std::vector<const char*>{"malloc debug enabled"}));
+
+    std::string log_str;
+    GetLogStr(pid, &log_str, LOG_ID_CRASH);
+    ASSERT_TRUE(log_str.find("Fatal signal") == std::string::npos)
+        << "Found crash in log.\nLog message: " << log_str;
+  }
+}
+
 TEST(MallocTests, DISABLED_write_leak_info) {
   TemporaryFile tf;
   ASSERT_TRUE(tf.fd != -1);
@@ -555,8 +609,11 @@ TEST(MallocTests, DISABLED_malloc_and_backtrace_deadlock) {
   static constexpr size_t kNumUnwinds = 1000;
   for (size_t i = 0; i < kNumUnwinds; i++) {
     std::unique_ptr<Backtrace> backtrace(Backtrace::Create(getpid(), tid));
+    // Only verify that there is at least one frame in the unwind.
+    // This is not a test of the unwinder and clang for arm seems to
+    // produces an increasing number of code that does not have unwind
+    // information.
     ASSERT_TRUE(backtrace->Unwind(0)) << "Failed on unwind " << i;
-    ASSERT_LT(1, backtrace->NumFrames()) << "Failed on unwind " << i;
   }
   running = false;
   thread.join();