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
