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:12:37 UTC
svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c
Author: ylavic
Date: Tue Nov 24 21:12:37 2020
New Revision: 1883801
URL: http://svn.apache.org/viewvc?rev=1883801&view=rev
Log:
apr_pools: follow up to r1883750 and r1883800.
After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
remove useless NULL checks around locking.
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=1883801&r1=1883800&r2=1883801&view=diff
==============================================================================
--- apr/apr/trunk/memory/unix/apr_pools.c (original)
+++ apr/apr/trunk/memory/unix/apr_pools.c Tue Nov 24 21:12:37 2020
@@ -1501,9 +1501,7 @@ static int apr_pool_walk_tree(apr_pool_t
return rv;
#if APR_HAS_THREADS
- if (pool->mutex) {
- apr_thread_mutex_lock(pool->mutex);
- }
+ apr_thread_mutex_lock(pool->mutex);
#endif /* APR_HAS_THREADS */
child = pool->child;
@@ -1516,9 +1514,7 @@ static int apr_pool_walk_tree(apr_pool_t
}
#if APR_HAS_THREADS
- if (pool->mutex) {
- apr_thread_mutex_unlock(pool->mutex);
- }
+ apr_thread_mutex_unlock(pool->mutex);
#endif /* APR_HAS_THREADS */
return rv;
@@ -1958,18 +1954,14 @@ static void pool_destroy_debug(apr_pool_
/* Remove the pool from the parents child list */
if (pool->parent) {
#if APR_HAS_THREADS
- apr_thread_mutex_t *mutex;
-
- if ((mutex = pool->parent->mutex) != NULL)
- apr_thread_mutex_lock(mutex);
+ apr_thread_mutex_lock(pool->parent->mutex);
#endif /* APR_HAS_THREADS */
if ((*pool->ref = pool->sibling) != NULL)
pool->sibling->ref = pool->ref;
#if APR_HAS_THREADS
- if (mutex)
- apr_thread_mutex_unlock(mutex);
+ apr_thread_mutex_unlock(pool->parent->mutex);
#endif /* APR_HAS_THREADS */
}
@@ -2060,9 +2052,9 @@ APR_DECLARE(apr_status_t) apr_pool_creat
if ((pool->parent = parent) != NULL) {
#if APR_HAS_THREADS
- if (parent->mutex)
- apr_thread_mutex_lock(parent->mutex);
+ apr_thread_mutex_lock(parent->mutex);
#endif /* APR_HAS_THREADS */
+
if ((pool->sibling = parent->child) != NULL)
pool->sibling->ref = &pool->sibling;
@@ -2070,8 +2062,7 @@ APR_DECLARE(apr_status_t) apr_pool_creat
pool->ref = &parent->child;
#if APR_HAS_THREADS
- if (parent->mutex)
- apr_thread_mutex_unlock(parent->mutex);
+ apr_thread_mutex_unlock(parent->mutex);
#endif /* APR_HAS_THREADS */
}
else {
Re: svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/25/20 11:45 AM, Yann Ylavic wrote:
> On Wed, Nov 25, 2020 at 10:20 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 11/24/20 10:12 PM, ylavic@apache.org wrote:
>>> Author: ylavic
>>> Date: Tue Nov 24 21:12:37 2020
>>> New Revision: 1883801
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1883801&view=rev
>>> Log:
>>> apr_pools: follow up to r1883750 and r1883800.
>>>
>>> After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
>>> remove useless NULL checks around locking.
>>
>> I am struggling a bit to see when the mutex could have been NULL before r1883800.
>
> It could have been NULL because apr_pool_create_ex_debug() was adding
> the pool to the parent's children list before creating the mutex.
> In this window, apr_pool_walk_tree() (like apr_pool_find() starting
> from the global_pool) could have found the pool with its NULL mutex
> and walked it unlocked.
Ahh. Thanks for explaining. I did not consider this short period of time and that another thread doing
apr_pool_walk_tree could then hit the NULL mutex.
Regards
Rüdiger
Re: svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Nov 25, 2020 at 10:20 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 11/24/20 10:12 PM, ylavic@apache.org wrote:
> > Author: ylavic
> > Date: Tue Nov 24 21:12:37 2020
> > New Revision: 1883801
> >
> > URL: http://svn.apache.org/viewvc?rev=1883801&view=rev
> > Log:
> > apr_pools: follow up to r1883750 and r1883800.
> >
> > After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
> > remove useless NULL checks around locking.
>
> I am struggling a bit to see when the mutex could have been NULL before r1883800.
It could have been NULL because apr_pool_create_ex_debug() was adding
the pool to the parent's children list before creating the mutex.
In this window, apr_pool_walk_tree() (like apr_pool_find() starting
from the global_pool) could have found the pool with its NULL mutex
and walked it unlocked.
For a short leaving pool, the pool could then have been cleared while
a walker was accessing a child => boom!
Regards;
Yann.
Re: svn commit: r1883801 - /apr/apr/trunk/memory/unix/apr_pools.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/24/20 10:12 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Tue Nov 24 21:12:37 2020
> New Revision: 1883801
>
> URL: http://svn.apache.org/viewvc?rev=1883801&view=rev
> Log:
> apr_pools: follow up to r1883750 and r1883800.
>
> After r1883800, the mutex of a pool in APR_POOL_DEBUG can't be NULL, so
> remove useless NULL checks around locking.
I am struggling a bit to see when the mutex could have been NULL before r1883800.
Regards
Rüdiger