From 19e246dda6772ffc532b1762cd7870d6c3b01c12 Mon Sep 17 00:00:00 2001
From: Yabin Cui <yabinc@google.com>
Date: Thu, 18 Dec 2014 14:22:09 -0800
Subject: [PATCH] Fix possible leak in pthread_detach.

If pthread_detach() is called while the thread is in pthread_exit(),
it takes the risk that no one can free the pthread_internal_t.
So I add PTHREAD_ATTR_FLAG_ZOMBIE to detect this, maybe very rare, but
both glibc and netbsd libpthread have similar function.

Change-Id: Iaa15f651903b8ca07aaa7bd4de46ff14a2f93835
---
 libc/bionic/pthread_detach.cpp | 39 ++++++++++++++++++----------------
 libc/bionic/pthread_exit.cpp   |  3 +++
 libc/bionic/pthread_internal.h |  3 +++
 tests/pthread_test.cpp         |  6 ++----
 4 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp
index 715acf13a..c80066008 100644
--- a/libc/bionic/pthread_detach.cpp
+++ b/libc/bionic/pthread_detach.cpp
@@ -27,29 +27,32 @@
  */
 
 #include <errno.h>
+#include <pthread.h>
 
 #include "pthread_accessor.h"
 
 int pthread_detach(pthread_t t) {
-  pthread_accessor thread(t);
-  if (thread.get() == NULL) {
+  {
+    pthread_accessor thread(t);
+    if (thread.get() == NULL) {
       return ESRCH;
+    }
+
+    if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) {
+      return EINVAL; // Already detached.
+    }
+
+    if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+      return 0; // Already being joined; silently do nothing, like glibc.
+    }
+
+    // If the thread has not exited, we can detach it safely.
+    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) {
+      thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
+      return 0;
+    }
   }
 
-  if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) {
-    return EINVAL; // Already detached.
-  }
-
-  if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
-    return 0; // Already being joined; silently do nothing, like glibc.
-  }
-
-  if (thread->tid == 0) {
-    // Already exited; clean up.
-    _pthread_internal_remove_locked(thread.get(), true);
-    return 0;
-  }
-
-  thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
-  return 0;
+  // The thread is in zombie state, use pthread_join to clean it up.
+  return pthread_join(t, NULL);
 }
diff --git a/libc/bionic/pthread_exit.cpp b/libc/bionic/pthread_exit.cpp
index 9603a7903..d0d64b0ec 100644
--- a/libc/bionic/pthread_exit.cpp
+++ b/libc/bionic/pthread_exit.cpp
@@ -99,6 +99,9 @@ void pthread_exit(void* return_value) {
     // pthread_internal_t is freed below with stack, not here.
     _pthread_internal_remove_locked(thread, false);
     free_mapped_space = true;
+  } else {
+    // Mark the thread as exiting without freeing pthread_internal_t.
+    thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
   }
   pthread_mutex_unlock(&g_thread_list_lock);
 
diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 80002e93f..8fbaf2213 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -38,6 +38,9 @@
 /* Has the thread been joined by another thread? */
 #define PTHREAD_ATTR_FLAG_JOINED 0x00000002
 
+/* Did the thread exit without freeing pthread_internal_t? */
+#define PTHREAD_ATTR_FLAG_ZOMBIE 0x00000004
+
 /* Is this the main thread? */
 #define PTHREAD_ATTR_FLAG_MAIN_THREAD 0x80000000
 
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index 2398f234b..cb3207923 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -431,7 +431,7 @@ TEST(pthread, pthread_detach__no_such_thread) {
   ASSERT_EQ(ESRCH, pthread_detach(dead_thread));
 }
 
-TEST(pthread, pthread_detach__leak) {
+TEST(pthread, pthread_detach_no_leak) {
   size_t initial_bytes = 0;
   // Run this loop more than once since the first loop causes some memory
   // to be allocated permenantly. Run an extra loop to help catch any subtle
@@ -464,9 +464,7 @@ TEST(pthread, pthread_detach__leak) {
   size_t final_bytes = mallinfo().uordblks;
   int leaked_bytes = (final_bytes - initial_bytes);
 
-  // User code (like this test) doesn't know how large pthread_internal_t is.
-  // We can be pretty sure it's more than 128 bytes.
-  ASSERT_LT(leaked_bytes, 32 /*threads*/ * 128 /*bytes*/);
+  ASSERT_EQ(0, leaked_bytes);
 }
 
 TEST(pthread, pthread_getcpuclockid__clock_gettime) {