Correct a race condition in the pthread join logic. Sometimes the join structure was being deallocated while it was still needed.

git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@180 42af7a65-404d-4744-a932-0658087f49c3

4 files changed, 69 insertions, 49 deletions

diff --git a/nuttx/sched/pthread_completejoin.c b/nuttx/sched/pthread_completejoin.c
index 571bb6862..76b2e9f80 100644
--- a/nuttx/sched/pthread_completejoin.c
+++ b/nuttx/sched/pthread_completejoin.c
@@ -65,16 +65,16 @@
  ************************************************************/
 
 /************************************************************
- * Function:  pthread_destroyjoininfo
+ * Function:  pthread_notifywaiters
  *
  * Description:
- *   Destroy a join_t structure.  This must
- *   be done by the child thread at child thread destruction
- *   time.
+ *   Notify all other threads waiting in phread join for this
+ *   thread's exit data.  This must  be done by the child
+ *   at child thread destruction time.
  *
  ************************************************************/
 
-static void pthread_destroyjoininfo(FAR join_t *pjoin)
+static boolean pthread_notifywaiters(FAR join_t *pjoin)
 {
   int ntasks_waiting;
   int status;
@@ -111,14 +111,9 @@ static void pthread_destroyjoininfo(FAR join_t *pjoin)
        */
 
       (void)pthread_takesemaphore(&pjoin->data_sem);
-      (void)sem_destroy(&pjoin->data_sem);
+      return TRUE;
     }
-
-  /* All of the joined threads have had received the exit value.
-   * Now we can destroy this thread's exit semaphore
-   */
-
-  (void)sem_destroy(&pjoin->exit_sem);
+  return FALSE;
 }
 
 /************************************************************
@@ -148,7 +143,6 @@ static void pthread_destroyjoininfo(FAR join_t *pjoin)
 int pthread_completejoin(pid_t pid, FAR void *exit_value)
 {
   FAR join_t *pjoin;
-  boolean detached = FALSE;
 
   dbg("process_id=%d exit_value=%p\n", pid, exit_value);
 
@@ -164,48 +158,72 @@ int pthread_completejoin(pid_t pid, FAR void *exit_value)
     }
   else
     {
-      /* Has the thread been marked as detached? */
+      boolean waiters;
+
+      /* Save the return exit value in the thread structure. */
 
       pjoin->terminated = TRUE;
-      detached = pjoin->detached;
-      if (detached)
-        {
-          dbg("Detaching\n");
+      pjoin->exit_value = exit_value;
 
-          /* If so, then remove the thread's structure from the private
-           * data set. After this point, no other thread can perform a join
-           * operation.
-           */
+      /* Notify waiters of the availability of the exit value */
 
-          (void)pthread_removejoininfo(pid);
-          (void)pthread_givesemaphore(&g_join_semaphore);
+      waiters = pthread_notifywaiters(pjoin);
 
-          /* Destroy this thread data structure. */
+      /* If there are no waiters and if the thread is marked as detached.
+       * then discard the join information now.  Otherwise, the pthread
+       * join logic will call pthread_destroyjoin() when all of the threads
+       * have sampled the exit value.
+       */
 
-          pthread_destroyjoininfo(pjoin);
+      if (!waiters && pjoin->detached)
+        {
+           pthread_destroyjoin(pjoin);
+        }
 
-          /* Deallocate the join entry if it was detached. */
+      /* Giving the following semaphore will allow the waiters
+       * to call pthread_destroyjoin.
+       */
 
-          sched_free((FAR void*)pjoin);
-        }
+      (void)pthread_givesemaphore(&g_join_semaphore);
+    }
 
-     /* No, then we can assume that some other thread is waiting for the join info */
+  return OK;
+}
 
-      else
-        {
-          /* Save the return exit value in the thread structure. */
+/************************************************************
+ * Function:  pthread_destroyjoin
+ *
+ * Description:
+ *   This is called from pthread_completejoin if the join
+ *   info was detached or from pthread_join when the last
+ *   waiting thread has received the thread exit info.
+ *
+ *   Or it may never be called if the join info was never
+ *   detached or if no thread ever calls pthread_join.  In
+ *   case, there is a memory leak!
+ *
+ * Assumptions:
+ *   The caller holds g_join_semaphore
+ *
+ ************************************************************/
+
+void pthread_destroyjoin(FAR join_t *pjoin)
+{
+  int status;
 
-          pjoin->exit_value = exit_value;
+  dbg("pjoin=0x%p\n", pjoin);
 
-          /* Destroy this thread data structure. */
+  /* Remove the join info from the set of joins */
 
-          pthread_destroyjoininfo(pjoin);
+  (void)pthread_removejoininfo((pid_t)pjoin->thread);
 
-          /* pthread_join may now access the thread entry structure. */
+  /* Destroy its semaphores */
 
-          (void)pthread_givesemaphore(&g_join_semaphore);
-        }
-    }
+  (void)sem_destroy(&pjoin->data_sem);
+  (void)sem_destroy(&pjoin->exit_sem);
 
-  return OK;
+  /* And deallocate the pjoin structure */
+
+  sched_free(pjoin);
 }
+
diff --git a/nuttx/sched/pthread_detach.c b/nuttx/sched/pthread_detach.c
index ca3f732f4..a1a60acf5 100644
--- a/nuttx/sched/pthread_detach.c
+++ b/nuttx/sched/pthread_detach.c
@@ -109,9 +109,7 @@ int pthread_detach(pthread_t thread)
         {
           /* YES.. just remove the thread entry. */
 
-          (void)pthread_removejoininfo((pid_t)thread);
-          sched_free(pjoin);
-          pjoin = NULL;
+          pthread_destroyjoin(pjoin);
         }
       else
         {
diff --git a/nuttx/sched/pthread_internal.h b/nuttx/sched/pthread_internal.h
index 34a2f518d..ec7c5ed71 100644
--- a/nuttx/sched/pthread_internal.h
+++ b/nuttx/sched/pthread_internal.h
@@ -64,6 +64,7 @@
 struct join_s 
 {
   FAR struct join_s *next;       /* Implements link list */
+  ubyte          crefs;          /* Reference count */
   boolean        started;        /* TRUE: pthread started. */
   boolean        detached;       /* TRUE: pthread_detached'ed */
   boolean        terminated;     /* TRUE: detach'ed+exit'ed */
@@ -115,6 +116,7 @@ extern "C" {
 
 EXTERN void weak_function pthread_initialize(void);
 EXTERN int                pthread_completejoin(pid_t pid, FAR void *exit_value);
+EXTERN void               pthread_destroyjoin(FAR join_t *pjoin);
 EXTERN FAR join_t        *pthread_findjoininfo(pid_t pid);
 EXTERN int                pthread_givesemaphore(sem_t *sem);
 EXTERN FAR join_t        *pthread_removejoininfo(pid_t pid);
diff --git a/nuttx/sched/pthread_join.c b/nuttx/sched/pthread_join.c
index 3819c1fa3..71ea7e9f0 100644
--- a/nuttx/sched/pthread_join.c
+++ b/nuttx/sched/pthread_join.c
@@ -181,6 +181,7 @@ int pthread_join(pthread_t thread, pthread_addr_t *pexit_value)
        */
 
       sched_lock();
+      pjoin->crefs++;
       (void)pthread_givesemaphore(&g_join_semaphore);
 
       /* Take the thread's join semaphore */
@@ -195,10 +196,6 @@ int pthread_join(pthread_t thread, pthread_addr_t *pexit_value)
           dbg("exit_value=0x%p\n", pjoin->exit_value);
         }
 
-      /* Then remove the thread entry. */
-
-      (void)pthread_removejoininfo((pid_t)thread);
-
       /* Post the thread's join semaphore so that exitting thread
        * will know that we have received the data.
        */
@@ -211,7 +208,12 @@ int pthread_join(pthread_t thread, pthread_addr_t *pexit_value)
 
       /* Deallocate the thread entry */
 
-      sched_free(pjoin);
+      (void)pthread_takesemaphore(&g_join_semaphore);
+      if (--pjoin->crefs <= 0)
+        {
+         (void)pthread_destroyjoin(pjoin);
+        }
+      (void)pthread_givesemaphore(&g_join_semaphore);
       ret = OK;
     }