Bug 844784: Add a reference count to PRMonitor because PR_ExitMonitorNSPR_4_10_4_BETA3

may access struct members after unlocking the internal mutex. r=thuang.

4 files changed, 101 insertions, 7 deletions

diff --git a/pr/include/private/primpl.h b/pr/include/private/primpl.h
index 6437d80d..4b087928 100644
--- a/pr/include/private/primpl.h
+++ b/pr/include/private/primpl.h
@@ -1459,6 +1459,14 @@ struct PRMonitor {
     pthread_cond_t entryCV;     /* for threads waiting to enter the monitor */
 
     pthread_cond_t waitCV;      /* for threads waiting on the monitor */
+    PRInt32 refCount;           /* reference count, an atomic variable.
+                                 * PR_NewMonitor adds a reference to the
+                                 * newly created PRMonitor, and
+                                 * PR_DestroyMonitor releases that reference.
+                                 * PR_ExitMonitor adds a reference before
+                                 * unlocking the internal lock if it needs to
+                                 * signal entryCV, and releases the reference
+                                 * after signaling entryCV. */
 #else  /* defined(_PR_PTHREADS) */
     PRCondVar *cvar;            /* associated lock and condition variable queue */
 #endif /* defined(_PR_PTHREADS) */
diff --git a/pr/src/pthreads/ptsynch.c b/pr/src/pthreads/ptsynch.c
index a0582cc0..7a099613 100644
--- a/pr/src/pthreads/ptsynch.c
+++ b/pr/src/pthreads/ptsynch.c
@@ -503,6 +503,7 @@ PR_IMPLEMENT(PRMonitor*) PR_NewMonitor(void)
 
     mon->notifyTimes = 0;
     mon->entryCount = 0;
+    mon->refCount = 1;
     return mon;
 
 error3:
@@ -526,14 +527,18 @@ PR_IMPLEMENT(PRMonitor*) PR_NewNamedMonitor(const char* name)
 PR_IMPLEMENT(void) PR_DestroyMonitor(PRMonitor *mon)
 {
     int rv;
+
     PR_ASSERT(mon != NULL);
-    rv = pthread_cond_destroy(&mon->waitCV); PR_ASSERT(0 == rv);
-    rv = pthread_cond_destroy(&mon->entryCV); PR_ASSERT(0 == rv);
-    rv = pthread_mutex_destroy(&mon->lock); PR_ASSERT(0 == rv);
+    if (PR_ATOMIC_DECREMENT(&mon->refCount) == 0)
+    {
+        rv = pthread_cond_destroy(&mon->waitCV); PR_ASSERT(0 == rv);
+        rv = pthread_cond_destroy(&mon->entryCV); PR_ASSERT(0 == rv);
+        rv = pthread_mutex_destroy(&mon->lock); PR_ASSERT(0 == rv);
 #if defined(DEBUG)
-    memset(mon, 0xaf, sizeof(PRMonitor));
+        memset(mon, 0xaf, sizeof(PRMonitor));
 #endif
-    PR_Free(mon);    
+        PR_Free(mon);
+    }
 }  /* PR_DestroyMonitor */
 
 /* The GC uses this; it is quite arguably a bad interface.  I'm just 
@@ -626,15 +631,21 @@ PR_IMPLEMENT(PRStatus) PR_ExitMonitor(PRMonitor *mon)
         notifyEntryWaiter = PR_TRUE;
         notifyTimes = mon->notifyTimes;
         mon->notifyTimes = 0;
+        /* We will access the members of 'mon' after unlocking mon->lock.
+         * Add a reference. */
+        PR_ATOMIC_INCREMENT(&mon->refCount);
     }
     rv = pthread_mutex_unlock(&mon->lock);
     PR_ASSERT(0 == rv);
-    if (notifyTimes)
-        pt_PostNotifiesFromMonitor(&mon->waitCV, notifyTimes);
     if (notifyEntryWaiter)
     {
+        if (notifyTimes)
+            pt_PostNotifiesFromMonitor(&mon->waitCV, notifyTimes);
         rv = pthread_cond_signal(&mon->entryCV);
         PR_ASSERT(0 == rv);
+        /* We are done accessing the members of 'mon'. Release the
+         * reference. */
+        PR_DestroyMonitor(mon);
     }
     return PR_SUCCESS;
 }  /* PR_ExitMonitor */
diff --git a/pr/tests/Makefile.in b/pr/tests/Makefile.in
index 6634a05f..4dc55b81 100644
--- a/pr/tests/Makefile.in
+++ b/pr/tests/Makefile.in
@@ -79,6 +79,7 @@ CSRCS =             \
 	multiacc.c		\
 	multiwait.c		\
 	many_cv.c		\
+	monref.c		\
 	nameshm1.c      \
 	nbconn.c		\
 	nblayer.c		\
diff --git a/pr/tests/monref.c b/pr/tests/monref.c
new file mode 100644
index 00000000..3e2ae637
--- /dev/null
+++ b/pr/tests/monref.c
@@ -0,0 +1,74 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/*
+ * This test program demonstrates that PR_ExitMonitor needs to add a
+ * reference to the PRMonitor object before unlocking the internal
+ * mutex.
+ */
+
+#include "prlog.h"
+#include "prmon.h"
+#include "prthread.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+/* Protected by the PRMonitor 'mon' in the main function. */
+static PRBool done = PR_FALSE;
+
+static void ThreadFunc(void *arg)
+{
+    PRMonitor *mon = (PRMonitor *)arg;
+    PRStatus rv;
+
+    PR_EnterMonitor(mon);
+    done = PR_TRUE;
+    rv = PR_Notify(mon);
+    PR_ASSERT(rv == PR_SUCCESS);
+    rv = PR_ExitMonitor(mon);
+    PR_ASSERT(rv == PR_SUCCESS);
+}
+
+int main()
+{
+    PRMonitor *mon;
+    PRThread *thread;
+    PRStatus rv;
+
+    mon = PR_NewMonitor();
+    if (!mon) {
+        fprintf(stderr, "PR_NewMonitor failed\n");
+        exit(1);
+    }
+
+    thread = PR_CreateThread(PR_USER_THREAD, ThreadFunc, mon,
+                             PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD,
+                             PR_JOINABLE_THREAD, 0);
+    if (!thread) {
+        fprintf(stderr, "PR_CreateThread failed\n");
+        exit(1);
+    }
+
+    PR_EnterMonitor(mon);
+    while (!done) {
+        rv = PR_Wait(mon, PR_INTERVAL_NO_TIMEOUT);
+        PR_ASSERT(rv == PR_SUCCESS);
+    }
+    rv = PR_ExitMonitor(mon);
+    PR_ASSERT(rv == PR_SUCCESS);
+
+    /*
+     * Do you agree it should be safe to destroy 'mon' now?
+     * See bug 844784 comment 27.
+     */
+    PR_DestroyMonitor(mon);
+
+    rv = PR_JoinThread(thread);
+    PR_ASSERT(rv == PR_SUCCESS);
+
+    printf("PASS\n");
+    return 0;
+}