https://bz.apache.org/bugzilla/show_bug.cgi?id=70293
Bug ID: 70293
Summary: mod_proxy_balancer: balancer_handler ignores
PROXY_THREAD_LOCK failure yet still calls
PROXY_THREAD_UNLOCK, unlike the other three call sites
in the same file
Product: Apache httpd-2
Version: 2.4.69
Hardware: PC
Status: NEW
Severity: minor
Priority: P2
Component: mod_proxy
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
Description
-----------
In modules/proxy/mod_proxy_balancer.c, balancer_handler() walks conf->balancers
and
performs:
for (i = 0; i < conf->balancers->nelts; i++, balancer++) {
#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_LOCK(balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01189)
"%s: Lock failed for balancer_handler",
balancer->s->name);
}
#endif
ap_proxy_sync_balancer(balancer, r->server, conf);
#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01190)
"%s: Unlock failed for balancer_handler",
balancer->s->name);
}
#endif
}
When PROXY_THREAD_LOCK() fails, the error is logged and execution continues
into
the very region the lock was supposed to protect: ap_proxy_sync_balancer() is
then
called without holding balancer->tmutex. The matching PROXY_THREAD_UNLOCK() is
also issued unconditionally, so on the failure path we unlock a mutex we do not
own (EPERM on an error-checking mutex, undefined behaviour on a default one).
There is no flag by which the caller can tell "locked" from "did not lock",
because the macro is NULL-safe by design:
#define PROXY_THREAD_LOCK(x) ((x) && (x)->tmutex ?
apr_thread_mutex_lock((x)->tmutex) : APR_SUCCESS)
#define PROXY_THREAD_UNLOCK(x) ((x) && (x)->tmutex ?
apr_thread_mutex_unlock((x)->tmutex) : APR_SUCCESS)
The same applies to the APLOGNO(01190) branch: a failed unlock means the mutex
may
still be held, and merely logging it leaves the balancer in a state that cannot
be
diagnosed from the log. balancer->tmutex is the same mutex used by
find_best_worker(), proxy_balancer_pre_request() and
proxy_balancer_post_request(),
i.e. it is on the request path of every proxied request for that balancer, so a
leaked lock does not degrade a single admin request - it stalls request routing
for that balancer.
Inconsistency within the same file
----------------------------------
The other three call sites all handle the identical condition by bailing out:
* find_best_worker() - APLOGNO(01163), then "return NULL"
* proxy_balancer_pre_request() - APLOGNO(01166), then "return DECLINED"
* proxy_balancer_post_request() - APLOGNO(01173), then "return
HTTP_INTERNAL_SERVER_ERROR"
Only balancer_handler() logs and continues, which looks like an oversight
rather
than an intentional policy. The same log-and-continue pattern is also present
in
balancer_process_balancer_worker(), APLOGNO(01194): after a failed
PROXY_GLOBAL_LOCK(bsel) the code still calls ap_proxy_define_worker(), which
does
an apr_array_push() on balancer->workers. In that same function the failure
path of
ap_proxy_define_worker() calls PROXY_GLOBAL_UNLOCK(bsel) without even
inspecting
the return value, so three different error-handling styles coexist within one
function.
Version: 2.4.69 (code identical in the 2.4.x branch tip at the time of filing)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
