You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2020/11/24 21:02:28 UTC
svn commit: r1883800 - /apr/apr/trunk/memory/unix/apr_pools.c
Author: ylavic
Date: Tue Nov 24 21:02:27 2020
New Revision: 1883800
URL: http://svn.apache.org/viewvc?rev=1883800&view=rev
Log:
apr_pools: follow up to r1883750.
Make apr_pool_clear_debug() always lock the parent mutex, regardless of
whether its own mutex is the same (inherited), otherwise the race with
apr_pool_walk_tree() is still not addressed in any case.
There is no risk of the mutex being destroyed while locked since it's been
created on an ancestor pool, and the cleanups can still use the mutex since
it's APR_THREAD_MUTEX_NESTED.
Also, in apr_pool_create_ex_debug(), create/inherit the mutex before the pool
is registered in the parent, or it can be accessed by apr_pool_walk_tree()
with the a NULL mutex and thus breaking the rules.
For unmanaged pools, even though they have no parent pool, they can still
have children that can be cleared/destroyed so the synchronization with
apr_pool_walk_tree() still applies. And since the allocator plays no role in
APR_POOL_DEBUG mode, apr_pool_create_unmanaged_ex_debug() must always create
the mutex, regardless of whether an allocator is provided.
While doing this change in apr_pool_create_unmanaged_ex_debug(), this commit
also moves the corresponding code (creating the mutex) at the same place as
it's moved in apr_pool_create_ex_debug(), to ease reading/comparing the two
functions.
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=1883800&r1=1883799&r2=1883800&view=diff
==============================================================================
--- apr/apr/trunk/memory/unix/apr_pools.c (original)
+++ apr/apr/trunk/memory/unix/apr_pools.c Tue Nov 24 21:02:27 2020
@@ -1911,14 +1911,12 @@ APR_DECLARE(void) apr_pool_clear_debug(a
apr_pool_check_lifetime(pool);
#if APR_HAS_THREADS
- if (pool->parent != NULL)
- mutex = pool->parent->mutex;
-
/* Lock the parent mutex before clearing so that if we have our
* own mutex it won't be accessed by apr_pool_walk_tree after
* it has been destroyed.
*/
- if (mutex != NULL && mutex != pool->mutex) {
+ if (pool->parent != NULL) {
+ mutex = pool->parent->mutex;
apr_thread_mutex_lock(mutex);
}
#endif
@@ -1942,9 +1940,9 @@ APR_DECLARE(void) apr_pool_clear_debug(a
pool->mutex = NULL;
(void)apr_thread_mutex_create(&pool->mutex,
APR_THREAD_MUTEX_NESTED, pool);
-
- if (mutex != NULL)
- (void)apr_thread_mutex_unlock(mutex);
+ }
+ if (mutex != NULL) {
+ apr_thread_mutex_unlock(mutex);
}
#endif /* APR_HAS_THREADS */
}
@@ -2037,6 +2035,29 @@ APR_DECLARE(apr_status_t) apr_pool_creat
pool->tag = file_line;
pool->file_line = file_line;
+#if APR_HAS_THREADS
+ if (parent == NULL || parent->allocator != allocator) {
+ apr_status_t rv;
+
+ /* No matter what the creation flags say, always create
+ * a lock. Without it integrity_check and apr_pool_num_bytes
+ * blow up (because they traverse pools child lists that
+ * possibly belong to another thread, in combination with
+ * the pool having no lock). However, this might actually
+ * hide problems like creating a child pool of a pool
+ * belonging to another thread.
+ */
+ if ((rv = apr_thread_mutex_create(&pool->mutex,
+ APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
+ free(pool);
+ return rv;
+ }
+ }
+ else {
+ pool->mutex = parent->mutex;
+ }
+#endif /* APR_HAS_THREADS */
+
if ((pool->parent = parent) != NULL) {
#if APR_HAS_THREADS
if (parent->mutex)
@@ -2069,32 +2090,6 @@ APR_DECLARE(apr_status_t) apr_pool_creat
apr_pool_log_event(pool, "CREATE", file_line, 1);
#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
- if (parent == NULL || parent->allocator != allocator) {
-#if APR_HAS_THREADS
- apr_status_t rv;
-
- /* No matter what the creation flags say, always create
- * a lock. Without it integrity_check and apr_pool_num_bytes
- * blow up (because they traverse pools child lists that
- * possibly belong to another thread, in combination with
- * the pool having no lock). However, this might actually
- * hide problems like creating a child pool of a pool
- * belonging to another thread.
- */
- if ((rv = apr_thread_mutex_create(&pool->mutex,
- APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
- free(pool);
- return rv;
- }
-#endif /* APR_HAS_THREADS */
- }
- else {
-#if APR_HAS_THREADS
- if (parent)
- pool->mutex = parent->mutex;
-#endif /* APR_HAS_THREADS */
- }
-
*newpool = pool;
return APR_SUCCESS;
@@ -2124,6 +2119,26 @@ APR_DECLARE(apr_status_t) apr_pool_creat
pool->file_line = file_line;
#if APR_HAS_THREADS
+ {
+ apr_status_t rv;
+
+ /* No matter what the creation flags say, always create
+ * a lock. Without it integrity_check and apr_pool_num_bytes
+ * blow up (because they traverse pools child lists that
+ * possibly belong to another thread, in combination with
+ * the pool having no lock). However, this might actually
+ * hide problems like creating a child pool of a pool
+ * belonging to another thread.
+ */
+ if ((rv = apr_thread_mutex_create(&pool->mutex,
+ APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
+ free(pool);
+ return rv;
+ }
+ }
+#endif /* APR_HAS_THREADS */
+
+#if APR_HAS_THREADS
pool->owner = apr_os_thread_current();
#endif /* APR_HAS_THREADS */
#ifdef NETWARE
@@ -2145,26 +2160,6 @@ APR_DECLARE(apr_status_t) apr_pool_creat
apr_pool_log_event(pool, "CREATEU", file_line, 1);
#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
- if (pool->allocator != allocator) {
-#if APR_HAS_THREADS
- apr_status_t rv;
-
- /* No matter what the creation flags say, always create
- * a lock. Without it integrity_check and apr_pool_num_bytes
- * blow up (because they traverse pools child lists that
- * possibly belong to another thread, in combination with
- * the pool having no lock). However, this might actually
- * hide problems like creating a child pool of a pool
- * belonging to another thread.
- */
- if ((rv = apr_thread_mutex_create(&pool->mutex,
- APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
- free(pool);
- return rv;
- }
-#endif /* APR_HAS_THREADS */
- }
-
*newpool = pool;
return APR_SUCCESS;