svn commit: r1883802 - /apr/apr/trunk/memory/unix/apr_pools.c

[yl...@apache.org]

Author: ylavic
Date: Tue Nov 24 23:16:52 2020
New Revision: 1883802

URL: http://svn.apache.org/viewvc?rev=1883802&view=rev
Log:
apr_pools: follow up to r1883750.

Don't release and re-acquire the lock in apr_pool_destroy_debug() between
pool_clear_debug() and the removal of the pool from the parent, so that
apr_pool_walk_tree() has no chance to find a pool being destroyed.

This is done like in apr_pool_clear_debug(), all the necessary locking goes
to apr_pool_destroy_debug() which then calls pool_destroy_debug() in the
critical section.

Note that pool_destroy_debug() when called from pool_clear_debug() is not
locking the parent mutex by itself anymore, thus the pool being cleared there
is not locked when its children are destroyed (just like before r1883750). This
is not an issue though because the parent mutex is supposed to protect against
concurrent access from apr_pool_walk_tree() on an ancestor, not an illegal
operation from another thread on a pool being cleared. That is always undefined
behaviour with debug and non-debug pools, though the latter might detect it
with apr_pool_check_owner(), not the pool being cleared's business anyway.

Modified:
    apr/apr/trunk/memory/unix/apr_pools.c

Modified: apr/apr/trunk/memory/unix/apr_pools.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/memory/unix/apr_pools.c?rev=1883802&r1=1883801&r2=1883802&view=diff
==============================================================================
--- apr/apr/trunk/memory/unix/apr_pools.c (original)
+++ apr/apr/trunk/memory/unix/apr_pools.c Tue Nov 24 23:16:52 2020
@@ -1895,6 +1895,10 @@ static void pool_clear_debug(apr_pool_t
 
     pool->stat_alloc = 0;
     pool->stat_clear++;
+
+#if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
+    apr_pool_log_event(pool, "CLEARED", file_line, 1);
+#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
 }
 
 APR_DECLARE(void) apr_pool_clear_debug(apr_pool_t *pool,
@@ -1917,12 +1921,12 @@ APR_DECLARE(void) apr_pool_clear_debug(a
     }
 #endif
 
-    pool_clear_debug(pool, file_line);
-
 #if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
     apr_pool_log_event(pool, "CLEAR", file_line, 1);
 #endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
 
+    pool_clear_debug(pool, file_line);
+
 #if APR_HAS_THREADS
     /* If we had our own mutex, it will have been destroyed by
      * the registered cleanups.  Recreate the mutex.  Unlock
@@ -1945,26 +1949,15 @@ APR_DECLARE(void) apr_pool_clear_debug(a
 
 static void pool_destroy_debug(apr_pool_t *pool, const char *file_line)
 {
-    apr_pool_clear_debug(pool, file_line);
-
-#if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
-    apr_pool_log_event(pool, "DESTROY", file_line, 1);
-#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
-
-    /* Remove the pool from the parents child list */
-    if (pool->parent) {
-#if APR_HAS_THREADS
-        apr_thread_mutex_lock(pool->parent->mutex);
-#endif /* APR_HAS_THREADS */
-
-        if ((*pool->ref = pool->sibling) != NULL)
-            pool->sibling->ref = pool->ref;
+    pool_clear_debug(pool, file_line);
 
-#if APR_HAS_THREADS
-        apr_thread_mutex_unlock(pool->parent->mutex);
-#endif /* APR_HAS_THREADS */
+    /* Remove the pool from the parent's child list */
+    if (pool->parent != NULL
+        && (*pool->ref = pool->sibling) != NULL) {
+        pool->sibling->ref = pool->ref;
     }
 
+    /* Destroy the allocator if the pool owns it */
     if (pool->allocator != NULL
         && apr_allocator_owner_get(pool->allocator) == pool) {
         apr_allocator_destroy(pool->allocator);
@@ -1977,6 +1970,12 @@ static void pool_destroy_debug(apr_pool_
 APR_DECLARE(void) apr_pool_destroy_debug(apr_pool_t *pool,
                                          const char *file_line)
 {
+#if APR_HAS_THREADS
+    apr_thread_mutex_t *mutex = NULL;
+#endif
+
+    apr_pool_check_lifetime(pool);
+
     if (pool->joined) {
         /* Joined pools must not be explicitly destroyed; the caller
          * has broken the guarantee. */
@@ -1987,7 +1986,29 @@ APR_DECLARE(void) apr_pool_destroy_debug
 
         abort();
     }
+
+#if APR_HAS_THREADS
+    /* Lock the parent mutex before destroying so that it's not accessed
+     * concurrently by apr_pool_walk_tree.
+     */
+    if (pool->parent != NULL) {
+        mutex = pool->parent->mutex;
+        apr_thread_mutex_lock(mutex);
+    }
+#endif
+
+#if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
+    apr_pool_log_event(pool, "DESTROY", file_line, 1);
+#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
+
     pool_destroy_debug(pool, file_line);
+
+#if APR_HAS_THREADS
+    /* Unlock the mutex we obtained above */
+    if (mutex != NULL) {
+        apr_thread_mutex_unlock(mutex);
+    }
+#endif /* APR_HAS_THREADS */
 }
 
 APR_DECLARE(apr_status_t) apr_pool_create_ex_debug(apr_pool_t **newpool,