Since commit 3ef263987 ("MEDIUM: chunks: Realloc trash buffers only after
the config is parsed and checked") in 1.8, the trash keeps its early
default size (BUFSIZE) until init_trash_buffers() is called after
check_config_validity(). Pattern files (maps, ACL "-f" files and
set-vars-from-map files) are loaded before that point, and their lines are
read with fgets() into the trash. As a result, a line longer than BUFSIZE-1
(16383 by default) is silently split regardless of tune.bufsize: the first
part is loaded with a truncated value and the remainder is loaded as one or
more extra entries, without any warning. For ACLs, this also means that the
16383-char prefix of a long pattern matches while the full pattern does
not. The only workarounds are to shorten such lines or to set the entries
again from the CLI after each reload.
Let's read pattern file lines into a dedicated heap buffer that grows as
needed, as readcfgfile() does for configuration lines. getline() is not
used since it was removed for portability reasons by commit fbc3d46b9
("BUILD: stats: remove non portable getline() usage"), and resizing the
trash earlier was not an option since ACL files are loaded while parsing.
Values remain bounded at runtime by tune.bufsize since pattern_exec_match()
copies the matched sample into a trash chunk, which is consistent with what
can be set from the CLI.
A reg-test was added which checks that a map file and an ACL file holding
a single 20000-char line are each loaded as a single entry, and that the
map value is returned in full.
This should be backported to all stable versions. Before 3.0, the two
pat_ref_read_from_file*() functions take the file name as an argument,
which only changes the patch context.
---
I hit this on 3.2.21 with a map holding base64-encoded response bodies
returned by "http-request return", which were served truncated after each
reload. It is still present in master and in the 3.2, 3.3 and 3.4 branches.
Tested on vanilla 3.2.21, 3.4.6 and master, on macOS (clang 17) and on
Linux x86_64 (Rocky 9, gcc 11.5): the new reg-test fails without the fix
and passes with it, and regular map files load exactly as before. On
master, the existing map/ACL reg-tests and the full suite give the same
results as without the patch, and loading a 2M-line map takes the same
time.
I'll open a GitHub issue pointing to this thread. Please keep me CCed,
I'm not subscribed to the list.
Thanks,
Dongwon Jang
reg-tests/http-rules/map_acl_long_lines.vtc | 58 +++++++++++++++++++
src/pattern.c | 64 +++++++++++++++++++--
2 files changed, 118 insertions(+), 4 deletions(-)
create mode 100644 reg-tests/http-rules/map_acl_long_lines.vtc
diff --git a/reg-tests/http-rules/map_acl_long_lines.vtc
b/reg-tests/http-rules/map_acl_long_lines.vtc
new file mode 100644
index 000000000..7fddc43e9
--- /dev/null
+++ b/reg-tests/http-rules/map_acl_long_lines.vtc
@@ -0,0 +1,58 @@
+varnishtest "map and ACL file lines longer than the default buffer size"
+
+# Pattern files (maps and ACL "-f" files) used to be read with the trash as
+# the line buffer before it was resized to tune.bufsize, so that any line
+# longer than 16383 chars was silently split into several entries and map
+# values were truncated. Check that a 20000-char line is loaded as a single
+# entry from both a map file and an ACL file, and that the map value is
+# returned in full.
+
+feature ignore_unknown_macro
+
+#REGTEST_TYPE=bug
+
+# Each file holds a single 20000-char line. They are generated here so that
+# the repository does not have to carry them.
+shell {
+ printf 'k %s\n' "$(printf '%20000s' '' | tr ' ' a)" >
${tmpdir}/long_line.map
+ printf '%s\n' "$(printf '%20000s' '' | tr ' ' a)" > ${tmpdir}/long_line.acl
+}
+
+haproxy h1 -conf {
+ global
+ # the value must fit in a buffer at runtime
+ tune.bufsize 65536
+ .if feature(THREAD)
+ thread-groups 1
+ .endif
+
+ defaults
+ mode http
+ timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+ frontend fe1
+ bind "fd@${fe1}"
+ # the ACL is only used to load its file, its entries are checked from the
CLI
+ acl long req.hdr(x-test) -f ${tmpdir}/long_line.acl
+ http-request return status 200 hdr x-map-len
"%[str(k),map(${tmpdir}/long_line.map),length]" if !long
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+ txreq -url "/"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.x-map-len == "20000"
+} -run
+
+# each line must be loaded as a single entry, not split into several ones
+haproxy h1 -cli {
+ send "show map"
+ expect ~ "long_line.map'.* entry_cnt=1\\n"
+}
+
+haproxy h1 -cli {
+ send "show acl"
+ expect ~ "long_line.acl'.* entry_cnt=1\\n"
+}
diff --git a/src/pattern.c b/src/pattern.c
index ff6e4d54a..b9d899169 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2396,6 +2396,44 @@ struct pattern_expr *pattern_new_expr(struct
pattern_head *head, struct pat_ref
return expr;
}
+/* Reads the next line of pattern file <file> into <*buf>, whose allocated
+ * size is <*size>, growing the buffer as needed. Lines are not read into the
+ * trash because pattern files are loaded while the configuration is parsed and
+ * checked, when the trash still has its early default size (tune.bufsize is
+ * only applied afterwards), so longer lines used to be silently split into
+ * several entries. <*buf> must either be NULL, in which case it is allocated,
+ * or point to the area of <*size> bytes it was given by a previous call, and
+ * it must be released by the caller with free(). Returns 1 if a line was read,
+ * 0 at the end of the file or on read error, and -1 on memory allocation
+ * failure.
+ */
+static int pat_ref_read_line(FILE *file, char **buf, size_t *size)
+{
+ size_t len = 0;
+ char *new_buf;
+
+ if (!*buf) {
+ *buf = malloc(LINESIZE);
+ if (!*buf)
+ return -1;
+ *size = LINESIZE;
+ }
+
+ while (fgets(*buf + len, *size - len, file) != NULL) {
+ len += strlen(*buf + len);
+ if (len < *size - 1 || (*buf)[len - 1] == '\n')
+ return 1;
+
+ /* the buffer is full and the line is not complete yet */
+ new_buf = realloc(*buf, *size * 2);
+ if (!new_buf)
+ return -1;
+ *buf = new_buf;
+ *size *= 2;
+ }
+ return len ? 1 : 0;
+}
+
/* Reads patterns from a file. If <err_msg> is non-NULL, an error message will
* be returned there on errors and the caller will have to free it.
*
@@ -2424,6 +2462,9 @@ int pat_ref_read_from_file_smp(struct pat_ref *ref, char
**err)
char *c;
int ret = 0;
int line = 0;
+ char *buf = NULL;
+ size_t buf_size = 0;
+ int rd;
char *key_beg;
char *key_end;
char *value_beg;
@@ -2445,9 +2486,9 @@ int pat_ref_read_from_file_smp(struct pat_ref *ref, char
**err)
* followed by one value per line. The start spaces, separator spaces
* and and spaces are stripped. Each can contain comment started by '#'
*/
- while (fgets(trash.area, trash.size, file) != NULL) {
+ while ((rd = pat_ref_read_line(file, &buf, &buf_size)) > 0) {
line++;
- c = trash.area;
+ c = buf;
/* ignore lines beginning with a dash */
if (*c == '#')
@@ -2493,6 +2534,11 @@ int pat_ref_read_from_file_smp(struct pat_ref *ref, char
**err)
}
}
+ if (rd < 0) {
+ memprintf(err, "out of memory");
+ goto out_close;
+ }
+
if (ferror(file)) {
memprintf(err, "error encountered while reading <%s> : %s",
ref->reference, strerror(errno));
@@ -2502,6 +2548,7 @@ int pat_ref_read_from_file_smp(struct pat_ref *ref, char
**err)
ret = 1;
out_close:
+ free(buf);
fclose(file);
return ret;
}
@@ -2516,6 +2563,9 @@ int pat_ref_read_from_file(struct pat_ref *ref, char
**err)
char *arg;
int ret = 0;
int line = 0;
+ char *buf = NULL;
+ size_t buf_size = 0;
+ int rd;
file = fopen(ref->reference, "r");
if (!file) {
@@ -2532,9 +2582,9 @@ int pat_ref_read_from_file(struct pat_ref *ref, char
**err)
* line. If the line contains spaces, they will be part of the pattern.
* The pattern stops at the first CR, LF or EOF encountered.
*/
- while (fgets(trash.area, trash.size, file) != NULL) {
+ while ((rd = pat_ref_read_line(file, &buf, &buf_size)) > 0) {
line++;
- c = trash.area;
+ c = buf;
/* ignore lines beginning with a dash */
if (*c == '#')
@@ -2560,6 +2610,11 @@ int pat_ref_read_from_file(struct pat_ref *ref, char
**err)
}
}
+ if (rd < 0) {
+ memprintf(err, "out of memory when loading patterns from file
<%s>", ref->reference);
+ goto out_close;
+ }
+
if (ferror(file)) {
memprintf(err, "error encountered while reading <%s> : %s",
ref->reference, strerror(errno));
@@ -2568,6 +2623,7 @@ int pat_ref_read_from_file(struct pat_ref *ref, char
**err)
ret = 1; /* success */
out_close:
+ free(buf);
fclose(file);
return ret;
}
--
2.50.1 (Apple Git-155)
