You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2021/07/12 10:32:22 UTC
svn commit: r1891477 - in /httpd/httpd/trunk:
changes-entries/ap_proxy_sync_balancer.txt modules/proxy/proxy_util.c
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
Re: svn commit: r1891477 - in /httpd/httpd/trunk:
changes-entries/ap_proxy_sync_balancer.txt modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
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
Re: svn commit: r1891477 - in /httpd/httpd/trunk:
changes-entries/ap_proxy_sync_balancer.txt modules/proxy/proxy_util.c
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
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?
If this is fine, maybe the palloc could be done outside the mutex too?
If not, maybe pcalloc instead of an explicit memset?
Also, the #if APR_HAS_THREADS block could be removed, tmutex will be
NULL'ed by the memset.
just my 2c.
CJ