From 51ae0608db8c071b66cd896e931bf5224ae8d0cb Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 1 Oct 2026 20:12:58 +0100 Subject: [PATCH 1/4] * test/modules/proxy/test_07_balancer_manager.py: New test suite. Co-Authored-By: Claude Fable 5.1 GitHub: PR #628 --- .../modules/proxy/test_07_balancer_manager.py | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 test/modules/proxy/test_07_balancer_manager.py diff --git a/test/modules/proxy/test_07_balancer_manager.py b/test/modules/proxy/test_07_balancer_manager.py new file mode 100644 index 00000000000..762460d5725 --- /dev/null +++ b/test/modules/proxy/test_07_balancer_manager.py @@ -0,0 +1,137 @@ +import re + +import pytest + +from pyhttpd.conf import HttpdConf + +# balancer-manager parameter handling. The manager only acts on a POST +# whose Referer is this server and whose nonce matches the balancer's, so the +# nonce is pinned in the config and every request carries a Referer; a +# request which fails either check is silently ignored, which is why the +# first test is a positive control. +# +# Two members are needed: with a single member recalc_factors() pins its load +# factor to 100 whatever was set, so a w_lf change would be unobservable. The +# second must differ in hostname, not just path, or mod_proxy shares the +# first worker for it (AH01145) and there is still only one. +BALANCER = "pr628" +NONCE = "pr628nonce" +ALT_HOST = "localhost" + + +class TestBalancerManager: + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env): + domain = f"test1.{env.http_tld}" + conf = HttpdConf(env) + conf.start_vhost(domains=[domain], port=env.http_port, + doc_root="htdocs/test1") + conf.add([ + f"", + f" BalancerMember {self.member(env)}", + f" BalancerMember http://{ALT_HOST}:{env.http_port}", + f" ProxySet nonce={NONCE}", + "", + f"ProxyPass /{BALANCER} balancer://{BALANCER}", + "", + " SetHandler balancer-manager", + "", + ]) + conf.end_vhost() + conf.install() + assert env.apache_restart() == 0 + + # -- helpers --------------------------------------------------------- + + @staticmethod + def member(env): + """The member every setting is applied to.""" + return f"http://127.0.0.1:{env.http_port}" + + def url(self, env): + return env.mkurl("http", "test1", "/balancer-manager") + + def post(self, env, **params): + """Submit worker/balancer settings as the manager's own form does.""" + url = self.url(env) + body = "&".join([f"b={BALANCER}", f"w={self.member(env)}", + f"nonce={NONCE}"] + + [f"{k}={v}" for k, v in params.items()]) + r = env.curl_post_data(url, data=body, + options=["-H", f"Referer: {url}"]) + assert r.exit_code == 0, f"{r.stdout}{r.stderr}" + assert r.response["status"] == 200 + return r + + def workers(self, env): + """{name: (status tokens, load factor)} for every member, from the + XML view.""" + url = self.url(env) + r = env.curl_get(f"{url}?xml=1&b={BALANCER}", + options=["-H", f"Referer: {url}"]) + assert r.response["status"] == 200 + body = r.response["body"] + if isinstance(body, bytes): + body = body.decode("utf-8", "replace") + found = {} + for block in re.findall(r"(.*?)", + body, re.S): + name = re.search(r"([^<]*)", block) + status = re.search(r"([^<]*)", block) + lf = re.search(r"([^<]*)", + block) + assert name and status and lf, f"incomplete worker:\n{block}" + found[name.group(1)] = (status.group(1).split(), lf.group(1)) + assert len(found) == 2, f"expected two members:\n{body}" + return found + + def worker(self, env): + """(status tokens, load factor) of the member settings are applied + to - the one that is not on ALT_HOST.""" + return next(v for k, v in self.workers(env).items() + if ALT_HOST not in k) + + # -- tests ----------------------------------------------------------- + + # Control: a well-formed value is applied and cleared. Proves the + # nonce/Referer/POST plumbing, so the cases below fail for the right reason. + def test_proxy_07_001(self, env): + self.post(env, w_status_D="1") + assert "Dis" in self.worker(env)[0] + self.post(env, w_status_D="0") + assert "Dis" not in self.worker(env)[0] + + # An out-of-range flag value must be rejected, not treated as "set". + def test_proxy_07_002(self, env): + self.post(env, w_status_D="0") + self.post(env, w_status_D="2") + status, _ = self.worker(env) + assert "Dis" not in status, \ + f"w_status_D=2 was accepted as 'set': {status}" + + # A non-numeric value must be rejected, not coerced to 0 and used to + # clear a flag that was set. + def test_proxy_07_003(self, env): + self.post(env, w_status_D="1") + assert "Dis" in self.worker(env)[0] + self.post(env, w_status_D="junk") + status, _ = self.worker(env) + try: + assert "Dis" in status, \ + f"w_status_D=junk cleared the flag: {status}" + finally: + self.post(env, w_status_D="0") + + # A load factor with trailing garbage must be rejected, not parsed up to + # the garbage and applied. + def test_proxy_07_004(self, env): + assert self.worker(env)[1] == "1.00" + self.post(env, w_lf="1.5junk") + workers = self.workers(env) + _, lf = self.worker(env) + try: + assert lf == "1.00", \ + f"w_lf=1.5junk was applied: {workers}" + finally: + self.post(env, w_lf="1") From 2d4903d7330be15fc08704e7518023f8903d2718 Mon Sep 17 00:00:00 2001 From: metsw24-max Date: Mon, 6 Apr 2026 16:02:01 +0530 Subject: [PATCH 2/4] input validation and removal of unsafe numeric parsing in balancer-manager (cherry picked from commit bacde3167cf34c5e576d37f7a3ee3a6af13cef98) --- modules/proxy/mod_proxy_balancer.c | 113 +++++++++++++++++++++++------ 1 file changed, 91 insertions(+), 22 deletions(-) diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c index 892bde38ae2..555f4db5da8 100644 --- a/modules/proxy/mod_proxy_balancer.c +++ b/modules/proxy/mod_proxy_balancer.c @@ -26,6 +26,9 @@ #include "apr_escape.h" #include "mod_watchdog.h" +#include +#include + static const char *balancer_mutex_type = "proxy-balancer-shm"; ap_slotmem_provider_t *storage = NULL; @@ -40,6 +43,54 @@ static APR_OPTIONAL_FN_TYPE(hc_show_exprs) *hc_show_exprs_f = NULL; static APR_OPTIONAL_FN_TYPE(hc_select_exprs) *hc_select_exprs_f = NULL; static APR_OPTIONAL_FN_TYPE(hc_valid_expr) *hc_valid_expr_f = NULL; +static int balancer_parse_int(const char *val, int min, int max, int *result) +{ + apr_int64_t parsed; + char *end = NULL; + + if (!val || !*val) { + return 0; + } + + errno = 0; + parsed = apr_strtoi64(val, &end, 10); + if (errno == ERANGE || end == val || *end != '\0') { + return 0; + } + + if (parsed < min || parsed > max) { + return 0; + } + + *result = (int)parsed; + return 1; +} + +static int balancer_parse_lbfactor(const char *val, int *result) +{ + char *end = NULL; + double parsed; + double scaled; + + if (!val || !*val) { + return 0; + } + + errno = 0; + parsed = strtod(val, &end); + if (errno == ERANGE || end == val || *end != '\0') { + return 0; + } + + scaled = parsed * 100.0; + if (scaled < 100.0 || scaled > 10000.0) { + return 0; + } + + *result = (int)scaled; + return 1; +} + /* * Register our mutex type before the config is read so we @@ -1114,9 +1165,7 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c if ((val = apr_table_get(params, "w_lf"))) { int ival; - double fval = atof(val); - ival = fval * 100.0; - if (ival >= 100 && ival <= 10000) { + if (balancer_parse_lbfactor(val, &ival)) { wsel->s->lbfactor = ival; if (bsel) recalc_factors(bsel); @@ -1139,29 +1188,50 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c * on that # character, since the character == the flag */ if ((val = apr_table_get(params, "w_status_I"))) { - ap_proxy_set_wstatus(PROXY_WORKER_IGNORE_ERRORS_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_IGNORE_ERRORS_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_N"))) { - ap_proxy_set_wstatus(PROXY_WORKER_DRAIN_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_DRAIN_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_D"))) { - ap_proxy_set_wstatus(PROXY_WORKER_DISABLED_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_DISABLED_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_H"))) { - ap_proxy_set_wstatus(PROXY_WORKER_HOT_STANDBY_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_HOT_STANDBY_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_R"))) { - ap_proxy_set_wstatus(PROXY_WORKER_HOT_SPARE_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_HOT_SPARE_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_S"))) { - ap_proxy_set_wstatus(PROXY_WORKER_STOPPED_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_STOPPED_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_status_C"))) { - ap_proxy_set_wstatus(PROXY_WORKER_HC_FAIL_FLAG, atoi(val), wsel); + int ival; + if (balancer_parse_int(val, 0, 1, &ival)) { + ap_proxy_set_wstatus(PROXY_WORKER_HC_FAIL_FLAG, ival, wsel); + } } if ((val = apr_table_get(params, "w_ls"))) { - int ival = atoi(val); - if (ival >= 0 && ival <= 99) { + int ival; + if (balancer_parse_int(val, 0, 99, &ival)) { wsel->s->lbset = ival; } } @@ -1174,14 +1244,14 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c } } if ((val = apr_table_get(params, "w_hp"))) { - int ival = atoi(val); - if (ival >= 1) { + int ival; + if (balancer_parse_int(val, 1, APR_INT32_MAX, &ival)) { wsel->s->passes = ival; } } if ((val = apr_table_get(params, "w_hf"))) { - int ival = atoi(val); - if (ival >= 1) { + int ival; + if (balancer_parse_int(val, 1, APR_INT32_MAX, &ival)) { wsel->s->fails = ival; } } @@ -1234,21 +1304,20 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c } } if ((val = apr_table_get(params, "b_tmo"))) { - ival = atoi(val); - if (ival >= 0 && ival <= 7200) { /* 2 hrs enuff? */ + if (balancer_parse_int(val, 0, 7200, &ival)) { /* 2 hrs enuff? */ bsel->s->timeout = apr_time_from_sec(ival); } } if ((val = apr_table_get(params, "b_max"))) { - ival = atoi(val); - if (ival >= 0 && ival <= 99) { + if (balancer_parse_int(val, 0, 99, &ival)) { bsel->s->max_attempts = ival; bsel->s->max_attempts_set = 1; } } if ((val = apr_table_get(params, "b_sforce"))) { - ival = atoi(val); - bsel->s->sticky_force = (ival != 0); + if (balancer_parse_int(val, 0, 1, &ival)) { + bsel->s->sticky_force = (ival != 0); + } } if ((val = apr_table_get(params, "b_ss")) && *val) { if (strlen(val) < (sizeof(bsel->s->sticky_path)-1)) { From da9853efb18c2addc2387ab9efda8201ddf45b42 Mon Sep 17 00:00:00 2001 From: Sayed Kaif Date: Sat, 18 Jul 2026 14:22:49 +0530 Subject: [PATCH 3/4] Address review nits: declare ival once per scope, drop redundant includes (cherry picked from commit d4d447871649b55db1ecc9c9e55704892430e454) --- modules/proxy/mod_proxy_balancer.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c index 555f4db5da8..aba414453e0 100644 --- a/modules/proxy/mod_proxy_balancer.c +++ b/modules/proxy/mod_proxy_balancer.c @@ -26,9 +26,6 @@ #include "apr_escape.h" #include "mod_watchdog.h" -#include -#include - static const char *balancer_mutex_type = "proxy-balancer-shm"; ap_slotmem_provider_t *storage = NULL; @@ -1159,12 +1156,12 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c /* First set the params */ if (wsel) { const char *val; + int ival; int was_usable = PROXY_WORKER_IS_USABLE(wsel); ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01192) "settings worker params"); if ((val = apr_table_get(params, "w_lf"))) { - int ival; if (balancer_parse_lbfactor(val, &ival)) { wsel->s->lbfactor = ival; if (bsel) @@ -1188,49 +1185,41 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c * on that # character, since the character == the flag */ if ((val = apr_table_get(params, "w_status_I"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_IGNORE_ERRORS_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_N"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_DRAIN_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_D"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_DISABLED_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_H"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_HOT_STANDBY_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_R"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_HOT_SPARE_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_S"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_STOPPED_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_status_C"))) { - int ival; if (balancer_parse_int(val, 0, 1, &ival)) { ap_proxy_set_wstatus(PROXY_WORKER_HC_FAIL_FLAG, ival, wsel); } } if ((val = apr_table_get(params, "w_ls"))) { - int ival; if (balancer_parse_int(val, 0, 99, &ival)) { wsel->s->lbset = ival; } @@ -1244,13 +1233,11 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c } } if ((val = apr_table_get(params, "w_hp"))) { - int ival; if (balancer_parse_int(val, 1, APR_INT32_MAX, &ival)) { wsel->s->passes = ival; } } if ((val = apr_table_get(params, "w_hf"))) { - int ival; if (balancer_parse_int(val, 1, APR_INT32_MAX, &ival)) { wsel->s->fails = ival; } From 6bd6c2c35799d686cd4487e430ed12c5348ee0cb Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 1 Oct 2026 22:20:49 +0100 Subject: [PATCH 4/4] * test/modules/proxy/env.py (TCPFaker._read_request): New hook, reading the request with a single recv() as before. (TCPFaker._process): Use it. * test/modules/proxy/test_05_uwsgi.py (_UWSGIFaker._read_request): Read the whole uwsgi packet as framed by its header; a single recv() returns only the first 4096 bytes on Windows, failing the length check. Co-Authored-By: Claude Fable 5.1 (cherry picked from commit a2e5a2cbdf297191ef4c8d037ff65ca080c4cc6f) --- test/modules/proxy/env.py | 9 ++++++++- test/modules/proxy/test_05_uwsgi.py | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/test/modules/proxy/env.py b/test/modules/proxy/env.py index d73f9db9be9..cbbe00898d3 100644 --- a/test/modules/proxy/env.py +++ b/test/modules/proxy/env.py @@ -46,12 +46,19 @@ def _make_response(self, data): Hello""".encode() + def _read_request(self, conn): + """Read the request httpd sent; a single recv() is enough for the + tests which do not inspect it. A faker which checks the request + must read it whole, as its protocol frames it, since one recv() + returns only what the stack has delivered so far.""" + return conn.recv(4096) + def _process(self): while not self._done: try: c, client_address = self._socket.accept() try: - data = c.recv(4096) + data = self._read_request(c) # capture request to backend self._request = data c.sendall(self._make_response(data)) diff --git a/test/modules/proxy/test_05_uwsgi.py b/test/modules/proxy/test_05_uwsgi.py index 0b360b89ff6..04b04fed521 100644 --- a/test/modules/proxy/test_05_uwsgi.py +++ b/test/modules/proxy/test_05_uwsgi.py @@ -6,6 +6,24 @@ class _UWSGIFaker(TCPFaker): + def _read_request(self, conn): + """Read the whole uwsgi packet: a 4-byte header whose bytes 1-2 + give the little-endian size of the data block which follows.""" + conn.settimeout(5) + data = b"" + while len(data) < 4: + chunk = conn.recv(4096) + if not chunk: + return data + data += chunk + total = 4 + data[1] + (data[2] * 256) + while len(data) < total: + chunk = conn.recv(total - len(data)) + if not chunk: + break + data += chunk + return data + @staticmethod def hello(data): body = b"Hello"