On Thu, Aug 21, 2025 at 10:13 AM Branko Čibej <br...@apache.org> wrote:
>
>> On 21. 8. 25 09:58, Joe Orton wrote:
>>
>> The pool-debug tests for httpd have started failing with SIGSEGV, I am
>> guessing related to this change since these have been very stable for a
>> long time. I think this is the relevant part of the traceback:
>
> It's probably related to what Yann pointed out yesterday. My bad. Will fix as
> soon as I can, or indeed Yann could just commit and backport his patches; I'm
> a bit swamped ATM.
Looks like it's not related, my patch only fixes some error cases
(which need some cleanup after the code move), but I don't think httpd
fails on creating a pool.
I think what happens is that now apr_pool_create_ex_debug() will
attach the pool to its parent before creating the mutex, but then an
apr_pool_walk_tree() on the parent (or any ancestor) running
concurrently might find the new pool with ->mutex == NULL, and
segfault in pool_lock().
ISTM that apr_pool_check_lifetime() could simply ignore any pool with
->parent == NULL (this includes the global pool and unmanaged pools),
and then we can move back the mutex creation before the pool is
attached to its parent.
Something like the attached patch?
Cheers;
Yann.
Index: memory/unix/apr_pools.c
===================================================================
--- memory/unix/apr_pools.c (revision 1927913)
+++ memory/unix/apr_pools.c (working copy)
@@ -600,7 +600,6 @@ struct apr_pool_t {
apr_os_thread_t owner;
apr_thread_mutex_t *mutex;
#endif /* APR_HAS_THREADS */
- int unmanaged;
#endif /* APR_POOL_DEBUG */
#ifdef NETWARE
apr_os_proc_t owner_proc;
@@ -1619,9 +1618,11 @@ static void apr_pool_check_lifetime(apr_pool_t *po
* ok, since the only user is apr_pools.c. Unless
* people have searched for the top level parent and
* started to use that...
+ * Like the global pool, unmanaged pools have their
+ * own lifetime and no ->parent, ignore both here.
+ *
*/
- if (pool == global_pool || global_pool == NULL
- || (pool->parent == NULL && pool->unmanaged))
+ if (pool->parent == NULL)
return;
/* Lifetime
@@ -2065,22 +2066,6 @@ APR_DECLARE(apr_status_t) apr_pool_create_ex_debug
pool->owner_proc = (apr_os_proc_t)getnlmhandle();
#endif /* defined(NETWARE) */
- if ((pool->parent = parent) != NULL) {
- pool_lock(parent);
-
- if ((pool->sibling = parent->child) != NULL)
- pool->sibling->ref = &pool->sibling;
-
- parent->child = pool;
- pool->ref = &parent->child;
-
- pool_unlock(parent);
- }
- else {
- pool->sibling = NULL;
- pool->ref = NULL;
- }
-
#if APR_HAS_THREADS
if (parent == NULL || parent->allocator != allocator) {
apr_status_t rv;
@@ -2104,6 +2089,22 @@ APR_DECLARE(apr_status_t) apr_pool_create_ex_debug
}
#endif /* APR_HAS_THREADS */
+ if ((pool->parent = parent) != NULL) {
+ pool_lock(parent);
+
+ if ((pool->sibling = parent->child) != NULL)
+ pool->sibling->ref = &pool->sibling;
+
+ parent->child = pool;
+ pool->ref = &parent->child;
+
+ pool_unlock(parent);
+ }
+ else {
+ pool->sibling = NULL;
+ pool->ref = NULL;
+ }
+
#if (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE)
apr_pool_log_event(pool, "CREATE", file_line, 1);
#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
@@ -2132,7 +2133,6 @@ APR_DECLARE(apr_status_t) apr_pool_create_unmanage
memset(pool, 0, SIZEOF_POOL_T);
- pool->unmanaged = 1;
pool->abort_fn = abort_fn;
pool->tag = file_line;
pool->file_line = file_line;
@@ -2169,6 +2169,9 @@ APR_DECLARE(apr_status_t) apr_pool_create_unmanage
*/
if ((rv = apr_thread_mutex_create(&pool->mutex,
APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
+ /* Don't leak the (own) allocator created above eventually */
+ if (pool_allocator->owner == pool)
+ apr_allocator_destroy(pool_allocator);
free(pool);
return rv;
}
