diff --git a/changes-entries/cache-disk-long-lines.txt b/changes-entries/cache-disk-long-lines.txt new file mode 100644 index 00000000000..9df432b9242 --- /dev/null +++ b/changes-entries/cache-disk-long-lines.txt @@ -0,0 +1,2 @@ + *) mod_cache_disk: Preserve cached response headers longer than + MAX_STRING_LEN when recalling entries. [Oz Kaiser] diff --git a/modules/cache/mod_cache_disk.c b/modules/cache/mod_cache_disk.c index 17e22a32f18..50ef7fa3067 100644 --- a/modules/cache/mod_cache_disk.c +++ b/modules/cache/mod_cache_disk.c @@ -26,6 +26,7 @@ #include "util_filter.h" #include "util_script.h" #include "util_charset.h" +#include "util_varbuf.h" /* * mod_cache_disk: Disk Based HTTP 1.1 Cache. @@ -709,14 +710,46 @@ static int remove_url(cache_handle_t *h, request_rec *r) return OK; } +static apr_status_t read_line_tail(request_rec *r, apr_file_t *file, + char *buf, apr_size_t buf_size, + apr_size_t first_len, char **line, + apr_size_t *line_len) +{ + struct ap_varbuf vb; + apr_size_t len; + apr_status_t rv; + + ap_varbuf_init(r->pool, &vb, MAX_STRING_LEN); + vb.strlen = 0; + vb.buf[0] = '\0'; + ap_varbuf_strmemcat(&vb, buf, first_len); + + do { + rv = apr_file_gets(buf, buf_size, file); + if (rv != APR_SUCCESS) { + ap_varbuf_free(&vb); + return rv; + } + len = strlen(buf); + ap_varbuf_strmemcat(&vb, buf, len); + } while (len == 0 || buf[len - 1] != '\n'); + + *line_len = vb.strlen; + *line = ap_varbuf_pdup(r->pool, &vb, NULL, 0, NULL, 0, NULL); + ap_varbuf_free(&vb); + return APR_SUCCESS; +} + static apr_status_t read_array(request_rec *r, apr_array_header_t* arr, apr_file_t *file) { - char w[MAX_STRING_LEN]; + char buf[MAX_STRING_LEN]; + char *w; apr_size_t p; apr_status_t rv; while (1) { + w = buf; rv = apr_file_gets(w, MAX_STRING_LEN - 1, file); if (rv != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00716) @@ -725,6 +758,15 @@ static apr_status_t read_array(request_rec *r, apr_array_header_t* arr, } p = strlen(w); + if (p == 0 || w[p - 1] != '\n') { + rv = read_line_tail(r, file, buf, MAX_STRING_LEN - 1, + p, &w, &p); + if (rv != APR_SUCCESS) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00716) + "Premature end of vary array."); + return rv; + } + } if (p > 0 && w[p - 1] == '\n') { if (p > 1 && w[p - 2] == CR) { w[p - 2] = '\0'; @@ -776,7 +818,8 @@ static apr_status_t store_array(apr_file_t *fd, apr_array_header_t* arr) static apr_status_t read_table(cache_handle_t *handle, request_rec *r, apr_table_t *table, apr_file_t *file) { - char w[MAX_STRING_LEN]; + char buf[MAX_STRING_LEN]; + char *w; char *l; apr_size_t p; apr_status_t rv; @@ -784,6 +827,7 @@ static apr_status_t read_table(cache_handle_t *handle, request_rec *r, while (1) { /* ### What about APR_EOF? */ + w = buf; rv = apr_file_gets(w, MAX_STRING_LEN - 1, file); if (rv != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00717) @@ -794,6 +838,15 @@ static apr_status_t read_table(cache_handle_t *handle, request_rec *r, /* Delete terminal (CR?)LF */ p = strlen(w); + if (p == 0 || w[p - 1] != '\n') { + rv = read_line_tail(r, file, buf, MAX_STRING_LEN - 1, + p, &w, &p); + if (rv != APR_SUCCESS) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00717) + "Premature end of cache headers."); + return rv; + } + } /* Indeed, the host's '\n': '\012' for UNIX; '\015' for MacOS; '\025' for OS/390 -- whatever the script generates. diff --git a/test/pytest_suite/t/conf/cache.conf.in b/test/pytest_suite/t/conf/cache.conf.in index 91284d13644..71e3d09b393 100644 --- a/test/pytest_suite/t/conf/cache.conf.in +++ b/test/pytest_suite/t/conf/cache.conf.in @@ -20,6 +20,10 @@ CacheDirLength 1 + + Header echo ^TS + Header set Cache-Control "public, max-age=600" + DocumentRoot @SERVERROOT@/htdocs/modules/cache diff --git a/test/pytest_suite/tests/t/modules/test_cache.py b/test/pytest_suite/tests/t/modules/test_cache.py index 76f10b107a7..6a68586c554 100644 --- a/test/pytest_suite/tests/t/modules/test_cache.py +++ b/test/pytest_suite/tests/t/modules/test_cache.py @@ -9,10 +9,32 @@ """ import os +import shutil from apache_pytest import need_min_apache_version, need_module, t_cmp +def _raw_get(http, path, headers=()): + request = [ + f"GET {path} HTTP/1.1", + f"Host: {http.hostport('mod_cache')}", + "Connection: close", + ] + request.extend(f"{name}: {value}" for name, value in headers) + + with http.vhost_socket("mod_cache") as sock: + sock.print("\r\n".join(request) + "\r\n\r\n") + response = sock.read() + + head = response.split("\r\n\r\n", 1)[0].split("\r\n") + fields = [] + for line in head[1:]: + name, separator, value = line.partition(":") + if separator: + fields.append((name.lower(), value.lstrip())) + return head[0], fields + + @need_module("cache", "cache_disk") @need_min_apache_version("2.1.9") def test_cache(http): @@ -29,3 +51,35 @@ def test_cache(http): r = http.GET("/cache/") assert t_cmp(r.status_code, 200), "cached call to index.html" + + +@need_module("cache", "cache_disk", "headers") +@need_min_apache_version("2.1.9") +def test_cache_long_response_header(http): + """A cache metadata line longer than MAX_STRING_LEN stays one header.""" + http.module("mod_cache") + + cacheroot = os.path.join(http.vars("statedir"), "cacheroot") + shutil.rmtree(cacheroot, ignore_errors=True) + os.makedirs(cacheroot, exist_ok=True) + + # Align a header-shaped tail with the old fixed-size read boundary. + tail = "X-Cache-Tail: preserved" + name = "TS" + "A" * (len(tail) - 4) + first = "a" * 4000 + second = "b" * (8190 - len(first) - 2 - len(tail)) + tail + expected = first + ", " + second + + status, prime = _raw_get( + http, + "/cache/index.html?long-response-header", + ((name, first), (name, second)), + ) + assert status.startswith("HTTP/1.1 200"), "cache prime returned 200" + assert (name.lower(), expected) in prime, "prime response kept the long header" + + status, recalled = _raw_get(http, "/cache/index.html?long-response-header") + assert status.startswith("HTTP/1.1 200"), "cached response returned 200" + assert (name.lower(), expected) in recalled, "cache recall kept the long header" + assert not any(name == "x-cache-tail" for name, _ in recalled), \ + "cache recall did not split a continuation chunk into another header"