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.



git-svn-id: https://svn.apache.org/repos/asf/apr/apr/trunk@1897197 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 71 insertions, 64 deletions

diff --git a/threadproc/beos/thread.c b/threadproc/beos/thread.c
index 5287ded8d..8dd95ea8b 100644
--- a/threadproc/beos/thread.c
+++ b/threadproc/beos/thread.c
@@ -82,12 +82,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
@@ -98,15 +94,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;
@@ -119,14 +121,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;
     }
 
diff --git a/threadproc/netware/thread.c b/threadproc/netware/thread.c
index 34a5691f5..a661ef886 100644
--- a/threadproc/netware/thread.c
+++ b/threadproc/netware/thread.c
@@ -87,12 +87,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
@@ -103,29 +99,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;
     }
 
@@ -160,7 +162,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 1e164ebd6..2891f0577 100644
--- a/threadproc/os2/thread.c
+++ b/threadproc/os2/thread.c
@@ -85,14 +85,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
@@ -103,33 +98,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 a91d8cd31..3b6ef2809 100644
--- a/threadproc/unix/thread.c
+++ b/threadproc/unix/thread.c
@@ -158,12 +158,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
@@ -174,21 +170,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;
     }
 
@@ -201,7 +203,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;
     }
 
diff --git a/threadproc/win32/thread.c b/threadproc/win32/thread.c
index ae0c40794..02956c59f 100644
--- a/threadproc/win32/thread.c
+++ b/threadproc/win32/thread.c
@@ -95,12 +95,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
@@ -111,15 +107,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;
     }
     if (attr && attr->detach) {