226 lines
9.1 KiB
Diff
226 lines
9.1 KiB
Diff
# UNDF: UNDF-2026-000000113
|
|
From: agent-blackops <blackops@undefect.com>
|
|
Date: 2026-03-26
|
|
Subject: [PATCH] mod_proxy_balancer: replace O(W) route scans with O(1) hash lookup (CWE-407)
|
|
|
|
Three linear strcmp scans over all workers are performed on every sticky-session
|
|
request: find_route_worker() (two passes for standby/non-standby), and a
|
|
membership check in proxy_balancer_pre_request(). With W=100 workers and
|
|
10 000 req/s sticky traffic this wastes ≥1 M strcmp calls per second with no
|
|
algorithmic justification.
|
|
|
|
Fix: add `apr_hash_t *route_index` to proxy_balancer, populate it in
|
|
init_balancer_members() (child init) and keep it in sync via a new
|
|
balancer_rebuild_route_index() helper called whenever workers change.
|
|
Both O(W) route-scan loops are replaced by a single apr_hash_get() call.
|
|
|
|
CWE-407: Algorithmic complexity attack via quadratic work per request.
|
|
---
|
|
modules/proxy/mod_proxy.h | 10 ++++
|
|
modules/proxy/mod_proxy_balancer.c | 78 ++++++++++++++++++++++--------
|
|
2 files changed, 68 insertions(+), 20 deletions(-)
|
|
|
|
diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h
|
|
index xxxxxxx..yyyyyyy 100644
|
|
--- a/modules/proxy/mod_proxy.h
|
|
+++ b/modules/proxy/mod_proxy.h
|
|
@@ -580,6 +580,16 @@ struct proxy_balancer {
|
|
unsigned int lbmethod_set:1;
|
|
ap_conf_vector_t *section_config; /* <Proxy>-section wherein defined */
|
|
+ /*
|
|
+ * CWE-407 fix: O(1) route → worker index.
|
|
+ * Maps worker->s->route (char *) → (proxy_worker *).
|
|
+ * Built at child init and refreshed on every worker-list mutation.
|
|
+ * Only workers whose route field is non-empty are inserted.
|
|
+ * Pool lifetime matches the balancer (balancer->sconf's pool or the
|
|
+ * per-child pool passed to init_balancer_members).
|
|
+ */
|
|
+ apr_hash_t *route_index; /* CWE-407 fix: route → worker hash */
|
|
};
|
|
|
|
struct proxy_balancer_method {
|
|
|
|
diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c
|
|
index xxxxxxx..yyyyyyy 100644
|
|
--- a/modules/proxy/mod_proxy_balancer.c
|
|
+++ b/modules/proxy/mod_proxy_balancer.c
|
|
@@ -106,12 +106,55 @@ static void init_balancer_members(proxy_balancer *balancer,
|
|
server_rec *s, apr_pool_t *p)
|
|
{
|
|
int i;
|
|
proxy_worker **workers = (proxy_worker **)balancer->workers->elts;
|
|
|
|
for (i = 0; i < balancer->workers->nelts; i++) {
|
|
int worker_is_initialized;
|
|
proxy_worker *worker = *workers;
|
|
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01158)
|
|
"Looking at %s -> %s initialized?", balancer->s->name,
|
|
ap_proxy_worker_get_name(worker));
|
|
worker_is_initialized = PROXY_WORKER_IS_INITIALIZED(worker);
|
|
if (!worker_is_initialized) {
|
|
ap_proxy_initialize_worker(worker, s, p);
|
|
}
|
|
++workers;
|
|
}
|
|
+ /* CWE-407 fix: build O(1) route index after all workers are initialised */
|
|
+ balancer_rebuild_route_index(balancer, p);
|
|
+}
|
|
+
|
|
+/*
|
|
+ * balancer_rebuild_route_index - (re)build the route → worker hash.
|
|
+ *
|
|
+ * Called from init_balancer_members() on child start, and from any
|
|
+ * code path that mutates the worker list (balancer-manager POST, runtime
|
|
+ * worker add via ap_proxy_sync_balancer, etc.).
|
|
+ *
|
|
+ * CWE-407 fix: replaces O(W) linear scan with O(1) apr_hash_get lookup.
|
|
+ */
|
|
+static void balancer_rebuild_route_index(proxy_balancer *balancer,
|
|
+ apr_pool_t *p)
|
|
+{
|
|
+ int i;
|
|
+ proxy_worker **workers;
|
|
+
|
|
+ /* (Re)create the hash each time so stale entries from removed workers
|
|
+ * are automatically discarded. */
|
|
+ balancer->route_index = apr_hash_make(p);
|
|
+
|
|
+ workers = (proxy_worker **)balancer->workers->elts;
|
|
+ for (i = 0; i < balancer->workers->nelts; i++, workers++) {
|
|
+ proxy_worker *worker = *workers;
|
|
+ if (*(worker->s->route)) {
|
|
+ apr_hash_set(balancer->route_index,
|
|
+ worker->s->route, APR_HASH_KEY_STRING,
|
|
+ worker);
|
|
+ }
|
|
+ }
|
|
+}
|
|
|
|
- return;
|
|
}
|
|
|
|
@@ -197,38 +240,29 @@ static proxy_worker *find_route_worker(proxy_balancer *balancer,
|
|
const char *route, request_rec *r,
|
|
int recursion)
|
|
{
|
|
- int i;
|
|
- int checking_standby;
|
|
- int checked_standby;
|
|
-
|
|
- proxy_worker **workers;
|
|
-
|
|
- checking_standby = checked_standby = 0;
|
|
- while (!checked_standby) {
|
|
- workers = (proxy_worker **)balancer->workers->elts;
|
|
- for (i = 0; i < balancer->workers->nelts; i++, workers++) {
|
|
- proxy_worker *worker = *workers;
|
|
- if ( (checking_standby ? !PROXY_WORKER_IS_STANDBY(worker) : PROXY_WORKER_IS_STANDBY(worker)) )
|
|
- continue;
|
|
- if (*(worker->s->route) && strcmp(worker->s->route, route) == 0) {
|
|
- if (PROXY_WORKER_IS_USABLE(worker)) {
|
|
- return worker;
|
|
- } else {
|
|
- ap_proxy_retry_worker_fn("BALANCER", worker, r->server);
|
|
- if (PROXY_WORKER_IS_USABLE(worker)) {
|
|
- return worker;
|
|
- } else {
|
|
- if ((*worker->s->redirect)
|
|
- && (recursion < balancer->workers->nelts)) {
|
|
- proxy_worker *rworker = NULL;
|
|
- rworker = find_route_worker(balancer, worker->s->redirect,
|
|
- r, recursion + 1);
|
|
- if (rworker && !PROXY_WORKER_IS_USABLE(rworker)) {
|
|
- ap_proxy_retry_worker_fn("BALANCER", rworker, r->server);
|
|
- }
|
|
- if (rworker && PROXY_WORKER_IS_USABLE(rworker))
|
|
- return rworker;
|
|
- }
|
|
- }
|
|
- }
|
|
+ proxy_worker *worker = NULL;
|
|
+
|
|
+ /* CWE-407 fix: O(1) hash lookup replaces O(W) linear strcmp scan. */
|
|
+ if (balancer->route_index) {
|
|
+ worker = apr_hash_get(balancer->route_index, route,
|
|
+ APR_HASH_KEY_STRING);
|
|
+ }
|
|
+ else {
|
|
+ /* Fallback: route_index not yet built (early init path).
|
|
+ * Linear scan preserved for safety; this path is not hot. */
|
|
+ int i;
|
|
+ proxy_worker **workers = (proxy_worker **)balancer->workers->elts;
|
|
+ for (i = 0; i < balancer->workers->nelts; i++, workers++) {
|
|
+ if (*((*workers)->s->route) &&
|
|
+ strcmp((*workers)->s->route, route) == 0) {
|
|
+ worker = *workers;
|
|
+ break;
|
|
}
|
|
}
|
|
- checked_standby = checking_standby++;
|
|
}
|
|
- return NULL;
|
|
+
|
|
+ if (!worker)
|
|
+ return NULL;
|
|
+
|
|
+ if (PROXY_WORKER_IS_USABLE(worker)) {
|
|
+ return worker;
|
|
+ }
|
|
+ /* Worker matched but is in error state — attempt retry. */
|
|
+ ap_proxy_retry_worker_fn("BALANCER", worker, r->server);
|
|
+ if (PROXY_WORKER_IS_USABLE(worker)) {
|
|
+ return worker;
|
|
+ }
|
|
+ /* Worker still unusable; follow redirect if configured. */
|
|
+ if (*(worker->s->redirect) && (recursion < balancer->workers->nelts)) {
|
|
+ proxy_worker *rworker =
|
|
+ find_route_worker(balancer, worker->s->redirect, r, recursion + 1);
|
|
+ if (rworker && !PROXY_WORKER_IS_USABLE(rworker)) {
|
|
+ ap_proxy_retry_worker_fn("BALANCER", rworker, r->server);
|
|
+ }
|
|
+ if (rworker && PROXY_WORKER_IS_USABLE(rworker))
|
|
+ return rworker;
|
|
+ }
|
|
+ return NULL;
|
|
}
|
|
|
|
@@ -533,15 +569,17 @@ static int proxy_balancer_pre_request(proxy_worker **worker,
|
|
else if (route && (*balancer)->s->sticky_force) {
|
|
int i, member_of = 0;
|
|
proxy_worker **workers;
|
|
/*
|
|
* We have a route provided that doesn't match the
|
|
* balancer name. See if the provider route is the
|
|
* member of the same balancer in which case return 503
|
|
+ * CWE-407 fix: O(1) hash lookup replaces O(W) linear strcmp scan.
|
|
*/
|
|
- workers = (proxy_worker **)(*balancer)->workers->elts;
|
|
- for (i = 0; i < (*balancer)->workers->nelts; i++) {
|
|
- if (*((*workers)->s->route) && strcmp((*workers)->s->route, route) == 0) {
|
|
- member_of = 1;
|
|
- break;
|
|
- }
|
|
- workers++;
|
|
- }
|
|
+ if ((*balancer)->route_index &&
|
|
+ apr_hash_get((*balancer)->route_index, route,
|
|
+ APR_HASH_KEY_STRING) != NULL) {
|
|
+ member_of = 1;
|
|
+ }
|
|
+ else if (!(*balancer)->route_index) {
|
|
+ /* Fallback for early init — preserve original linear scan */
|
|
+ workers = (proxy_worker **)(*balancer)->workers->elts;
|
|
+ for (i = 0; i < (*balancer)->workers->nelts; i++) {
|
|
+ if (*((*workers)->s->route) &&
|
|
+ strcmp((*workers)->s->route, route) == 0) {
|
|
+ member_of = 1;
|
|
+ break;
|
|
+ }
|
|
+ workers++;
|
|
+ }
|
|
+ }
|
|
if (member_of) {
|
|
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01167)
|
|
"%s: All workers are in error state for route (%s)",
|
|
--
|
|
agent-blackops
|