Test coverage for SubstituteMaxLineLength on the unmatched tail (#755)

changes-entries/substitute-maxlinelength-tail.txt

@@ -0,0 +1,2 @@
+  *) mod_substitute: Enforce SubstituteMaxLineLength on the unmatched tail
+     after substitutions.  [Robert McConnell]

modules/filters/mod_substitute.c

@@ -282,8 +282,12 @@ static apr_status_t do_pattmatch(ap_filter_t *f, apr_bucket *inb,
                             /* XXX: we should check for AP_MAX_BUCKETS here and
                              * XXX: call ap_pass_brigade accordingly
                              */
-                            char *copy = ap_varbuf_pdup(pool, &vb, NULL, 0,
-                                                        buff, bytes, &len);
+                            char *copy;
+                            if (vb.strlen > cfg->max_line_length
+                                    || bytes > cfg->max_line_length - vb.strlen)
+                                return APR_ENOMEM;
+                            copy = ap_varbuf_pdup(pool, &vb, NULL, 0,
+                                                  buff, bytes, &len);
                             ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, f->r,
                                           "New line (%" APR_SIZE_T_FMT " bytes): %.*s",
                                           len, CAP2LINEMAX(len), copy);
@@ -389,6 +393,9 @@ static apr_status_t do_pattmatch(ap_filter_t *f, apr_bucket *inb,
                         /* Copy result plus the part after the last match into
                          * a bucket.
                          */
+                        if (vb.strlen > cfg->max_line_length
+                                || left > cfg->max_line_length - vb.strlen)
+                            return APR_ENOMEM;
                         copy = ap_varbuf_pdup(pool, &vb, NULL, 0, pos, left,
                                               &len);
                         ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, f->r,
@@ -400,6 +407,9 @@ static apr_status_t do_pattmatch(ap_filter_t *f, apr_bucket *inb,
                         apr_bucket_delete(b);
                         b = tmp_b;
                     }
+                    else if (have_match && left > space_left) {
+                        return APR_ENOMEM;
+                    }
                 }
                 else {
                     ap_assert(0);

test/modules/filters/conftest.py

@@ -0,0 +1,30 @@
+import logging
+import os
+import sys
+
+import pytest
+
+from .env import FiltersTestEnv
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '../..'))
+
+
+def pytest_report_header(config, start_path):
+    env = FiltersTestEnv()
+    return f"filters [apache: {env.get_httpd_version()}, mpm: {env.mpm_module}, {env.prefix}]"
+
+
+@pytest.fixture(scope="package")
+def env(pytestconfig) -> FiltersTestEnv:
+    logging.getLogger('').setLevel(level=logging.INFO)
+    env = FiltersTestEnv(pytestconfig=pytestconfig)
+    env.setup_httpd()
+    env.apache_access_log_clear()
+    env.httpd_error_log.clear_log()
+    return env
+
+
+@pytest.fixture(autouse=True, scope="package")
+def _stop_package_scope(env):
+    yield
+    assert env.apache_stop() == 0

test/modules/filters/env.py

@@ -0,0 +1,25 @@
+import inspect
+import logging
+import os
+
+from pyhttpd.env import HttpdTestEnv, HttpdTestSetup
+
+log = logging.getLogger(__name__)
+
+
+class FiltersTestSetup(HttpdTestSetup):
+
+    def __init__(self, env: 'HttpdTestEnv'):
+        super().__init__(env=env)
+        self.add_source_dir(os.path.dirname(inspect.getfile(FiltersTestSetup)))
+        self.add_modules(["substitute"])
+
+
+class FiltersTestEnv(HttpdTestEnv):
+
+    def __init__(self, pytestconfig=None):
+        super().__init__(pytestconfig=pytestconfig)
+        self.add_httpd_log_modules(["substitute", "core"])
+
+    def setup_httpd(self, setup: HttpdTestSetup = None):
+        super().setup_httpd(setup=FiltersTestSetup(env=self))

test/modules/filters/test_001_substitute_maxlen.py

@@ -0,0 +1,76 @@
+import os
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+# SubstituteMaxLineLength caps the length of a line after substitution.  Each
+# flag combination reaches a different branch of do_pattmatch(): regex or
+# literal ('n'), flattened into one bucket or left as separate ones ('q').
+MODES = {
+    'regex_flatten': 'f',
+    'regex_quick': 'q',
+    'literal_flatten': 'nf',
+    'literal_quick': 'nq',
+}
+
+MAXLEN = 10
+# "x" expands to "yy", so each line grows by exactly one byte.
+OVER = "x" + "a" * 9        # 10 bytes in, 11 out -- over the limit
+OVER_MID = "aaaa" + "x" + "aaaaa"   # same, with the match off the start
+OVER_END = "a" * 9 + "x"    # same again, but with no unmatched tail
+EXACT = "x" + "a" * 8       # 9 bytes in, 10 out -- exactly at the limit
+
+
+class TestSubstituteMaxLineLength:
+
+    @pytest.fixture(autouse=True, scope='class')
+    def _class_scope(self, env):
+        doc_dir = os.path.join(env.server_dir, "htdocs", "test1")
+        for name, text in [("over.html", OVER), ("over_mid.html", OVER_MID),
+                           ("over_end.html", OVER_END), ("exact.html", EXACT)]:
+            with open(os.path.join(doc_dir, name), "w") as f:
+                f.write(text)
+        # APLOGNO(01328) "Line too long" is the expected rejection.
+        env.httpd_error_log.add_ignored_lognos(["AH01328"])
+
+    def configure(self, env, flags):
+        conf = HttpdConf(env, extras={
+            f"test1.{env.http_tld}": f"""
+            SubstituteMaxLineLength {MAXLEN}
+            <Location "/">
+                AddOutputFilterByType SUBSTITUTE text/html
+                Substitute "s/x/yy/{flags}"
+            </Location>
+            """,
+        })
+        conf.add_vhost_test1()
+        conf.install()
+        assert env.apache_restart() == 0
+
+    def get(self, env, path):
+        return env.curl_get(env.mkurl("http", "test1", path))
+
+    # A line that would grow past the limit must be rejected, not truncated
+    # and not returned over-length.
+    @pytest.mark.parametrize("doc", ["over.html", "over_mid.html", "over_end.html"])
+    @pytest.mark.parametrize("mode", list(MODES), ids=list(MODES))
+    def test_filters_001_01(self, env, mode, doc):
+        self.configure(env, MODES[mode])
+        r = self.get(env, f"/{doc}")
+        body = r.response["body"].decode() if r.response else None
+        assert not (r.response and r.response["status"] == 200
+                    and len(body) > MAXLEN), \
+            f"{mode} {doc}: over-length line returned: " \
+            f"{len(body) if body else None} bytes {body!r}"
+
+    # A line that lands exactly on the limit is still served.
+    @pytest.mark.parametrize("mode", list(MODES), ids=list(MODES))
+    def test_filters_001_02(self, env, mode):
+        self.configure(env, MODES[mode])
+        r = self.get(env, "/exact.html")
+        assert r.response, f"{mode}: no response"
+        assert r.response["status"] == 200, f"{mode}: status {r.response['status']}"
+        body = r.response["body"].decode()
+        assert body == "yy" + "a" * 8, f"{mode}: body {body!r}"
+        assert len(body) == MAXLEN