[Bug 48535] Potential data race on allocator->max_index in allocator_alloc()

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=48535

--- Comment #6 from Ionut Oancea <io...@yahoo.com> ---
I stumbled into this recently and I was very surprised that such an old report
wasn't concluded yet. In our MT application, we use APR pools extensively and
an issue there would definitely be a big problem.

Therefore, I have decided to do an analysis of the affected code (aka
allocator_alloc and allocator_free functions) to hopefully help in closing this
bug or just help others stuck with this.

Note: Unfortunately, I have only analyzed the code for the latest APR release
(version 1.7.0) so I won't address the version being reported (hopefully, no
one uses that old version by now)

The thread analyzer tools (helgrind, drd, tsan) trigger warnings about
unprotected read access in 2 places:
===================== 1 ===========================
if (index <= allocator->max_index) { // `allocator->max_index` is read w/o
mutex protection 
#if APR_HAS_THREADS
        if (allocator->mutex)
            apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
===================================================

===================== 2 ===========================
else if (allocator->free[0]) { // `allocator->free[0]` is read w/o mutex
protection
#if APR_HAS_THREADS
        if (allocator->mutex)
            apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
===================================================

To check whether the reports are harmless or not we'll have to see what happens
when the unprotected condition isn't `true` after the mutex is acquired.

For case 1, this means `index > allocator->max_index`. The line that deserves
attention is the one which uses `index` - aka `ref = &allocator->free[index]`.
In our scenario, `ref` will point beyond the current `max_index` which looks
dangerous at a 1st sight. After a closer inspection, the access is ok because: 
  * the `free[]` field is a MAX_INDEX (where `allocator->max_index <
MAX_INDEX`) array - so it can't be an out-of-bounds read.
  * the emptied entries from `allocator->free` will always point to NULL making
`*ref == NULL` - so the reference won't point to dangling or in-use node.

For case 2, the race condition will be `allocator->free[0] == NULL` that will
affect the `ref = &allocator->free[0]` line. This branch is a bit more simple
and one can easily see that the result from `*ref` is properly checked against
NULL - aka `while ((node = *ref) != NULL ...` and later `if (node) {`.

In conclusion, for above read races, the existing code won't create any issue
and, in both cases, the new `node` will end up being allocated.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org