apr_thread: Allocate the apr_thread_t struct on the thread's pool.

apr_thread_create() was allocating the created apr_thread_t on the given pool,
which caused e.g. short-living threads to leak memory on that pool without a
way to clear it (while some threads are still running).

Change this by allocating the apr_thread_t on the thread's pool itself, which
is safe in the implementations of all archs because none uses the apr_thread_t
after the thread exits, and it's safe for the users provided they don't use the
apr_thread_t for detached threads or for attached threads after the call to
apr_thread_join(). These are hardly new requirements though.


apr_thread: Follow up to r1897197: Safer apr_thread_join().

Make sure apr_thread_join() behaves correctly w.r.t. the returned value
and pool destroy for all archs.


Merge r1897197, r1897198 from trunk.
Submitted by: ylavic


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.8.x@1897221 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 86 insertions, 84 deletions

diff --git a/threadproc/beos/thread.c b/threadproc/beos/thread.c
index 269ee9cb6..880bba2b6 100644
--- a/threadproc/beos/thread.c
+++ b/threadproc/beos/thread.c
@@ -81,12 +81,8 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     int32 temp;
     apr_status_t stat;
     apr_allocator_t *allocator;
+    apr_pool_t *p;
     
-    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-    if ((*new) == NULL) {
-        return APR_ENOMEM;
-    }
-
     /* The thread can be detached anytime (from the creation or later with
      * apr_thread_detach), so it needs its own pool and allocator to not
      * depend on a parent pool which could be destroyed before the thread
@@ -97,15 +93,21 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     if (stat != APR_SUCCESS) {
         return stat;
     }
-    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                        apr_pool_abort_get(pool),
+    stat = apr_pool_create_unmanaged_ex(&p, apr_pool_abort_get(pool),
                                         allocator);
     if (stat != APR_SUCCESS) {
         apr_allocator_destroy(allocator);
         return stat;
     }
-    apr_allocator_owner_set(allocator, (*new)->pool);
+    apr_allocator_owner_set(allocator, p);
+
+    (*new) = (apr_thread_t *)apr_pcalloc(p, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        apr_pool_destroy(p);
+        return APR_ENOMEM;
+    }
 
+    (*new)->pool = p;
     (*new)->data = data;
     (*new)->func = func;
     (*new)->exitval = -1;
@@ -118,14 +120,12 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
 
     /* First we create the new thread...*/
     (*new)->td = spawn_thread((thread_func)dummy_worker, 
-                              "apr thread", 
-                              temp, 
-                              (*new));
+                              "apr thread", temp, (*new));
 
     /* Now we try to run it...*/
     if (resume_thread((*new)->td) != B_NO_ERROR) {
         stat = errno;
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return stat;
     }
 
@@ -162,21 +162,19 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *th
     }
 
     ret = wait_for_thread(thd->td, &rv);
-    if (ret == B_NO_ERROR) {
-        *retval = rv;
-        return APR_SUCCESS;
-    }
-    else {
+    if (ret != B_NO_ERROR) {
         /* if we've missed the thread's death, did we set an exit value prior
          * to it's demise?  If we did return that.
          */
-        if (thd->exitval != -1) {
-            *retval = thd->exitval;
-            apr_pool_destroy(thd->pool);
-            return APR_SUCCESS;
-        } else 
-            return ret;
+        if (thd->exitval == -1) {
+            return errno;
+        }
+        rv = thd->exitval;
     }
+
+    *retval = rv;
+    apr_pool_destroy(thd->pool);
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
diff --git a/threadproc/netware/thread.c b/threadproc/netware/thread.c
index 2f126539b..f86281980 100644
--- a/threadproc/netware/thread.c
+++ b/threadproc/netware/thread.c
@@ -86,12 +86,8 @@ apr_status_t apr_thread_create(apr_thread_t **new,
     unsigned long flags = NX_THR_BIND_CONTEXT;
     size_t stack_size = APR_DEFAULT_STACK_SIZE;
     apr_allocator_t *allocator;
+    apr_pool_t *p;
     
-    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-    if ((*new) == NULL) {
-        return APR_ENOMEM;
-    }
-
     /* The thread can be detached anytime (from the creation or later with
      * apr_thread_detach), so it needs its own pool and allocator to not
      * depend on a parent pool which could be destroyed before the thread
@@ -102,29 +98,35 @@ apr_status_t apr_thread_create(apr_thread_t **new,
     if (stat != APR_SUCCESS) {
         return stat;
     }
-    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                        apr_pool_abort_get(pool),
+    stat = apr_pool_create_unmanaged_ex(&p, apr_pool_abort_get(pool),
                                         allocator);
     if (stat != APR_SUCCESS) {
         apr_allocator_destroy(allocator);
         return stat;
     }
-    apr_allocator_owner_set(allocator, (*new)->pool);
+    apr_allocator_owner_set(allocator, p);
+
+    (*new) = (apr_thread_t *)apr_pcalloc(p, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        apr_pool_destroy(p);
+        return APR_ENOMEM;
+    }
 
+    (*new)->pool = p;
     (*new)->data = data;
     (*new)->func = func;
     (*new)->exitval = -1;
     (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
     if (attr && attr->thread_name) {
-        (*new)->thread_name = apr_pstrndup(pool, ttr->thread_name,
+        (*new)->thread_name = apr_pstrndup(p, ttr->thread_name,
                                            NX_MAX_OBJECT_NAME_LEN);
     }
     else {
-        (*new)->thread_name = apr_psprintf(pool, "APR_thread %04d",
+        (*new)->thread_name = apr_psprintf(p, "APR_thread %04d",
                                            ++thread_count);
     }
     if ((*new)->thread_name == NULL) {
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return APR_ENOMEM;
     }
 
@@ -159,7 +161,7 @@ apr_status_t apr_thread_create(apr_thread_t **new,
         /* NXThreadId_t *thread_id */ &(*new)->td);
 
     if (stat) {
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return stat;
     }
         
diff --git a/threadproc/os2/thread.c b/threadproc/os2/thread.c
index 43bc4a1b0..ff896a43b 100644
--- a/threadproc/os2/thread.c
+++ b/threadproc/os2/thread.c
@@ -84,14 +84,9 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
                                             apr_pool_t *pool)
 {
     apr_status_t stat;
-    apr_thread_t *thread;
     apr_allocator_t *allocator;
+    apr_pool_t *p;
     
-    *new = thread = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-    if (thread == NULL) {
-        return APR_ENOMEM;
-    }
-
     /* The thread can be detached anytime (from the creation or later with
      * apr_thread_detach), so it needs its own pool and allocator to not
      * depend on a parent pool which could be destroyed before the thread
@@ -102,33 +97,39 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     if (stat != APR_SUCCESS) {
         return stat;
     }
-    stat = apr_pool_create_unmanaged_ex(&thread->pool,
-                                        apr_pool_abort_get(pool),
+    stat = apr_pool_create_unmanaged_ex(&p, apr_pool_abort_get(pool),
                                         allocator);
     if (stat != APR_SUCCESS) {
         apr_allocator_destroy(allocator);
         return stat;
     }
-    apr_allocator_owner_set(allocator, thread->pool);
+    apr_allocator_owner_set(allocator, p);
+
+    (*new) = (apr_thread_t *)apr_pcalloc(p, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        apr_pool_destroy(p);
+        return APR_ENOMEM;
+    }
 
-    thread->func = func;
-    thread->data = data;
+    (*new)->pool = p;
+    (*new)->func = func;
+    (*new)->data = data;
     if (attr == NULL) {
-        stat = apr_threadattr_create(&attr, thread->pool);
+        stat = apr_threadattr_create(&attr, p);
         if (stat != APR_SUCCESS) {
-            apr_pool_destroy(thread->pool);
+            apr_pool_destroy(p);
             return stat;
         }
     }
-    thread->attr = attr;
+    (*new)->attr = attr;
 
-    thread->tid = _beginthread(dummy_worker, NULL, 
-                               thread->attr->stacksize > 0 ?
-                               thread->attr->stacksize : APR_THREAD_STACKSIZE,
-                               thread);
-    if (thread->tid < 0) {
+    (*new)->tid = _beginthread(dummy_worker, NULL, 
+                               (*new)->attr->stacksize > 0 ?
+                               (*new)->attr->stacksize : APR_THREAD_STACKSIZE,
+                               (*new));
+    if ((*new)->tid < 0) {
         stat = errno;
-        apr_pool_destroy(thread->pool);
+        apr_pool_destroy(p);
         return stat;
     }
 
diff --git a/threadproc/unix/thread.c b/threadproc/unix/thread.c
index e5919a4b8..d8c0509fe 100644
--- a/threadproc/unix/thread.c
+++ b/threadproc/unix/thread.c
@@ -157,12 +157,8 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
     apr_status_t stat;
     pthread_attr_t *temp;
     apr_allocator_t *allocator;
+    apr_pool_t *p;
     
-    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-    if ((*new) == NULL) {
-        return APR_ENOMEM;
-    }
-
     /* The thread can be detached anytime (from the creation or later with
      * apr_thread_detach), so it needs its own pool and allocator to not
      * depend on a parent pool which could be destroyed before the thread
@@ -173,21 +169,27 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
     if (stat != APR_SUCCESS) {
         return stat;
     }
-    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                        apr_pool_abort_get(pool),
+    stat = apr_pool_create_unmanaged_ex(&p, apr_pool_abort_get(pool),
                                         allocator);
     if (stat != APR_SUCCESS) {
         apr_allocator_destroy(allocator);
         return stat;
     }
-    apr_allocator_owner_set(allocator, (*new)->pool);
+    apr_allocator_owner_set(allocator, p);
+
+    (*new) = (apr_thread_t *)apr_pcalloc(p, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        apr_pool_destroy(p);
+        return APR_ENOMEM;
+    }
 
+    (*new)->pool = p;
     (*new)->data = data;
     (*new)->func = func;
     (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
-    (*new)->td = (pthread_t *)apr_pcalloc(pool, sizeof(pthread_t));
+    (*new)->td = (pthread_t *)apr_pcalloc(p, sizeof(pthread_t));
     if ((*new)->td == NULL) {
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return APR_ENOMEM;
     }
 
@@ -200,7 +202,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 #ifdef HAVE_ZOS_PTHREADS
         stat = errno;
 #endif
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return stat;
     }
 
@@ -233,24 +235,22 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval,
                                           apr_thread_t *thd)
 {
     apr_status_t stat;
-    apr_status_t *thread_stat;
+    void *thread_stat;
 
     if (thd->detached) {
         return APR_EINVAL;
     }
 
-    if ((stat = pthread_join(*thd->td,(void *)&thread_stat)) == 0) {
-        *retval = thd->exitval;
-        apr_pool_destroy(thd->pool);
-        return APR_SUCCESS;
-    }
-    else {
+    if ((stat = pthread_join(*thd->td, &thread_stat))) {
 #ifdef HAVE_ZOS_PTHREADS
         stat = errno;
 #endif
-
         return stat;
     }
+
+    *retval = thd->exitval;
+    apr_pool_destroy(thd->pool);
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
diff --git a/threadproc/win32/thread.c b/threadproc/win32/thread.c
index 6b04abb98..0dd26a53b 100644
--- a/threadproc/win32/thread.c
+++ b/threadproc/win32/thread.c
@@ -94,12 +94,8 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
     unsigned temp;
     HANDLE handle;
     apr_allocator_t *allocator;
+    apr_pool_t *p;
     
-    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-    if ((*new) == NULL) {
-        return APR_ENOMEM;
-    }
-
     /* The thread can be detached anytime (from the creation or later with
      * apr_thread_detach), so it needs its own pool and allocator to not
      * depend on a parent pool which could be destroyed before the thread
@@ -110,15 +106,21 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
     if (stat != APR_SUCCESS) {
         return stat;
     }
-    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                        apr_pool_abort_get(pool),
+    stat = apr_pool_create_unmanaged_ex(&p, apr_pool_abort_get(pool),
                                         allocator);
     if (stat != APR_SUCCESS) {
         apr_allocator_destroy(allocator);
         return stat;
     }
-    apr_allocator_owner_set(allocator, (*new)->pool);
+    apr_allocator_owner_set(allocator, p);
+
+    (*new) = (apr_thread_t *)apr_pcalloc(p, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        apr_pool_destroy(p);
+        return APR_ENOMEM;
+    }
 
+    (*new)->pool = p;
     (*new)->data = data;
     (*new)->func = func;
 
@@ -131,7 +133,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
                         (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
                         (*new), 0, &temp)) == 0) {
         stat = APR_FROM_OS_ERROR(_doserrno);
-        apr_pool_destroy((*new)->pool);
+        apr_pool_destroy(p);
         return stat;
     }
 #else
@@ -195,7 +197,6 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval,
     if (rv == APR_SUCCESS) {
         CloseHandle(thd->td);
         apr_pool_destroy(thd->pool);
-        thd->td = NULL;
     }
 
     return rv;