On 7/12/21 1:53 PM, Christophe JAILLET wrote:
> Le 12/07/2021 à 12:32, [email protected] 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
