Re: svn commit: r1927658 - apr/apr/trunk/memory/unix

On 20. 8. 25 14:56, Yann Ylavic wrote:
> On Thu, Aug 7, 2025 at 8:19 AM<[email protected]> wrote:
>> Author: brane
>> Date: Thu Aug  7 06:19:52 2025
>> New Revision: 1927658
>>
>> Log:
>> Fix pool debugging. With lifetime or owner checks enabled, pools couldn't
>> even be created, except for the global pool which is a special case.
>>
>> * memory/unix/apr_pools.c
>>    (struct apr_pool_t): Add an 'unmanaged' member for pool-debug mode.
>>     The lifetime checks must be skipped for unmanaged pools, since they
>>     inevitably fail: apr_pool_is_child_of() expects all pools to have a
>>     parent, which obviously is not the case for unmanaged pools.
>>
>>    (apr_pool_create_ex_debug): Create the pool's mutex after the parent
>>     has been assigned, because that involves an allocation which triggers
>>     a lifetime check which ... well, see above.
>>
>>    (apr_pool_create_unmanaged): Set the pool->unmanaged flag and create the
>>     pool's mutex after the owner and allocator have been assigned. A pool
>>     without the owner set fails the ownershhip check, and without an
>>     allocator it's sort of hard to allocate space for the mutex.
>>
>> Modified:
>>     apr/apr/trunk/memory/unix/apr_pools.c
> []
>> --- apr/apr/trunk/memory/unix/apr_pools.c       Thu Aug  7 05:08:51 2025        (r1927657)
>> +++ apr/apr/trunk/memory/unix/apr_pools.c       Thu Aug  7 06:19:52 2025        (r1927658)
> []
>> @@ -2063,6 +2065,22 @@ APR_DECLARE(apr_status_t) apr_pool_creat
>>       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;
> Here we might fail and need to rewind the above if parent != NULL,
> maybe something like this:
>
> Index: memory/unix/apr_pools.c
> ===================================================================
> --- memory/unix/apr_pools.c    (revision 1927913)
> +++ memory/unix/apr_pools.c    (working copy)
> @@ -2095,7 +2095,15 @@ APR_DECLARE(apr_status_t) apr_pool_create_ex_debug
>            */
>           if ((rv = apr_thread_mutex_create(&pool->mutex,
>                   APR_THREAD_MUTEX_NESTED, pool)) != APR_SUCCESS) {
> -            free(pool);
> +            /* Unwind parent binding from above eventually */
> +            if (parent) {
> +                pool_lock(parent);
> +                pool_destroy_debug(pool, file_line);
> +                pool_unlock(parent);
> +            }
> +            else {
> +                free(pool);
> +            }
>               return rv;
>           }
>       }
> ?
>
> []
>> @@ -2130,11 +2132,30 @@ APR_DECLARE(apr_status_t) apr_pool_creat
>>
>>       memset(pool, 0, SIZEOF_POOL_T);
>>
>> +    pool->unmanaged = 1;
>>       pool->abort_fn = abort_fn;
>>       pool->tag = file_line;
>>       pool->file_line = file_line;
>>
>>   #if APR_HAS_THREADS
>> +    pool->owner = apr_os_thread_current();
>> +#endif /* APR_HAS_THREADS */
>> +#ifdef NETWARE
>> +    pool->owner_proc = (apr_os_proc_t)getnlmhandle();
>> +#endif /* defined(NETWARE) */
>> +
>> +    if ((pool_allocator = allocator) == NULL) {
>> +        apr_status_t rv;
>> +        if ((rv = apr_allocator_create(&pool_allocator)) != APR_SUCCESS) {
>> +            if (abort_fn)
>> +                abort_fn(rv);
>> +            return rv;
>> +        }
>> +        pool_allocator->owner = pool;
>> +    }
>> +    pool->allocator = pool_allocator;
>> +
>> +#if APR_HAS_THREADS
>>       {
>>           apr_status_t rv;
> Likewise here we need to destroy pool_allocator eventually on failure,
> something like this:
>
> Index: memory/unix/apr_pools.c
> ===================================================================
> --- memory/unix/apr_pools.c    (revision 1927913)
> +++ memory/unix/apr_pools.c    (working copy)
> @@ -2169,6 +2177,10 @@ 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 own allocator created above eventually */
> +            if (pool_allocator->owner == pool) {
> +                apr_allocator_destroy(pool_allocator);
> +            }
>               free(pool);
>               return rv;
>           }
> ?
>


I'll check. I didn't dig that deep, I only moved the code around so that 
the data dependencies lined up.

-- Brane