mod_proxy_balancer: test coverage for #628 (strict balancer-manager numeric parsing)

modules/proxy/mod_proxy_balancer.c

@@ -40,6 +40,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
@@ -1108,15 +1156,13 @@ 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;
-            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 +1185,42 @@ 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);
+            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);
+            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);
+            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);
+            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);
+            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);
+            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);
+            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) {
+            if (balancer_parse_int(val, 0, 99, &ival)) {
                 wsel->s->lbset = ival;
              }
         }
@@ -1174,14 +1233,12 @@ 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) {
+            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) {
+            if (balancer_parse_int(val, 1, APR_INT32_MAX, &ival)) {
                 wsel->s->fails = ival;
              }
         }
@@ -1234,21 +1291,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)) {

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))

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"

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"<Proxy balancer://{BALANCER}>",
+            f"  BalancerMember {self.member(env)}",
+            f"  BalancerMember http://{ALT_HOST}:{env.http_port}",
+            f"  ProxySet nonce={NONCE}",
+            "</Proxy>",
+            f"ProxyPass /{BALANCER} balancer://{BALANCER}",
+            "<Location /balancer-manager>",
+            "  SetHandler balancer-manager",
+            "</Location>",
+        ])
+        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"<httpd:worker>(.*?)</httpd:worker>",
+                                body, re.S):
+            name = re.search(r"<httpd:name>([^<]*)</httpd:name>", block)
+            status = re.search(r"<httpd:status>([^<]*)</httpd:status>", block)
+            lf = re.search(r"<httpd:loadfactor>([^<]*)</httpd:loadfactor>",
+                           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")