apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From E Holyat <ehol...@yahoo.com>
Subject Re: apr_pools and thread safety
Date Thu, 30 Jun 2005 01:51:18 GMT
What if one thread destroys the allocator while this function is trying to dereference it.
 No matter what, you are going to lock in 99% of the time, why not be safe and lock it before
dereferencing it.
Besides if you are going to fall into an if and wait on a mutex, shouldn't you check the if
statement again too?  What is the purpose of the if statement when by the time a thread acquires
the mutex, the if statement may no longer evaluate to TRUE?

Sander Striker <striker@apache.org> wrote:
E Holyat wrote:

[...]
> But I can see the following still holds true for allocator_alloc and 
> allocator_free. allocator_alloc allows a free read on max_index when 
> either allocator_alloc / allocator_free can change that value.
>
> If 40 threads from the same parent create child pools. 40 threads can 
> fall into the first "if statement". The access within that "if 
> statement" is mutually exclusive, but, if max_index is being decremented 
> within the "if statement", then the condition that put the 13 or 14th or 
> nth thread into that "if statement" may no longer hold true. Wouldn't 
> that invalidate the index alignment check and give a thread an 
> invalid child pool reference?

I don't think this would be the case. Comments inline.

> The mutex should be moved outside of the if and else if

> if (index <= allocator->max_index) { can be changed while being read

While allocator->max_index can indeed be changed, this is just a quick
scan to see if a node (aka chunk of memory) *might* be available. If
this seems to be the case a mutex is taken out and the value is read
again:

apr_pools.c:196

if (index <= allocator->max_index) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */

/* Walk the free list to see if there are
* any nodes on it of the requested size

...

max_index = allocator->max_index;
ref = &allocator->free[index];
i = index;
while (*ref == NULL && i < max_index) {
ref++;
i++;
}

At this point we either have a node or we don't. If we do, we unlock
the mutex and return the node. If we don't, we unlock the mutex and
fall through, and back, on this piece of code:

apr_pools.c:297

/* If we haven't got a suitable node, malloc a new one
* and initialize it.
*/
if ((node = malloc(size)) == NULL)
return NULL;

...

return node;


> }
> 
> /* If we found nothing, seek the sink (at index 0), if
> * it is not empty.
> */
> else if (allocator->free[0]) { 2 threads can enter.

The same holds true for the above, we grab the lock and check again.
Again with the risk of falling through to the block at line 297:

apr_pools.c:260

else if (allocator->free[0]) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */

/* Walk the free list to see if there are
* any nodes on it of the requested size
*/
ref = &allocator->free[0];
while ((node = *ref) != NULL && index > node->index)
ref = &node->next;


Sander


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 
Mime
View raw message