Mailing List Archive

On 7/12/21 1:53 PM, Christophe JAILLET wrote:
> Le 12/07/2021 à 12:32, ylavic@apache.org a écrit :
>> Author: ylavic
>> Date: Mon Jul 12 10:32:21 2021
>> New Revision: 1891477
>>
>> URL: http://svn.apache.org/viewvc?rev=1891477&view=rev
>> Log:
>> mod_proxy: Fix icomplete initialization of BalancerMember(s) from the manager.
>>
>> Clear the workers created in ap_proxy_sync_balancer(), notably ->local_status
>> for below ap_proxy_initialize_worker() to initialize all the child structures
>> like ->cp and ->cp->reslist, avoiding a possible crash when the workers are
>> used at runtime.
>>
>> Added:
>>      httpd/httpd/trunk/changes-entries/ap_proxy_sync_balancer.txt
>> Modified:
>>      httpd/httpd/trunk/modules/proxy/proxy_util.c
>>
>> Added: httpd/httpd/trunk/changes-entries/ap_proxy_sync_balancer.txt
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/ap_proxy_sync_balancer.txt?rev=1891477&view=auto
>> ==============================================================================
>> --- httpd/httpd/trunk/changes-entries/ap_proxy_sync_balancer.txt (added)
>> +++ httpd/httpd/trunk/changes-entries/ap_proxy_sync_balancer.txt Mon Jul 12 10:32:21 2021
>> @@ -0,0 +1,2 @@
>> +  *) mod_proxy: Fix icomplete initialization of BalancerMember(s) from the
>> +     balancer-manager, which can lead to a crash.  [Yann Ylavic]
>>
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1891477&r1=1891476&r2=1891477&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jul 12 10:32:21 2021
>> @@ -3681,9 +3681,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_syn
>>               runtime = apr_array_push(b->workers);
>>               *runtime = apr_palloc(conf->pool, sizeof(proxy_worker));
>>               apr_global_mutex_unlock(proxy_mutex);
>> +            memset(*runtime, 0, sizeof(proxy_worker));
>>               (*runtime)->hash = shm->hash;
>> -            (*runtime)->context = NULL;
>> -            (*runtime)->cp = NULL;
>>               (*runtime)->balancer = b;
>>               (*runtime)->s = shm;
>>   #if APR_HAS_THREADS
>>
>
> Hi,
> just wondering if it is safe to array_push+palloc within a mutex and finalize the initialization of this memory outside the mutex?

To be honest I ask myself why we need a global mutex here as this seems to be data that is local to the process. Hence a a thread
mutex might be sufficient.

>
> If this is fine, maybe the palloc could be done outside the mutex too?
> If not, maybe pcalloc instead of an explicit memset?

I would suggest to use apr_pcalloc instead of memset as this seems to be the standard approach for this kind of initialization.

>
>
> Also, the #if APR_HAS_THREADS block could be removed, tmutex will be NULL'ed by the memset.

+1

So something like this?

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1891497)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3679,15 +3679,11 @@
proxy_worker **runtime;
apr_global_mutex_lock(proxy_mutex);
runtime = apr_array_push(b->workers);
- *runtime = apr_palloc(conf->pool, sizeof(proxy_worker));
+ *runtime = apr_pcalloc(conf->pool, sizeof(proxy_worker));
apr_global_mutex_unlock(proxy_mutex);
- memset(*runtime, 0, sizeof(proxy_worker));
(*runtime)->hash = shm->hash;
(*runtime)->balancer = b;
(*runtime)->s = shm;
-#if APR_HAS_THREADS
- (*runtime)->tmutex = NULL;
-#endif
rv = ap_proxy_initialize_worker(*runtime, s, conf->pool);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(00966) "Cannot init worker");


Regards

Rüdiger