From: Willy Tarreau <w@1wt.eu>
Date: Thu, 11 Aug 2022 14:12:11 +0000 (+0200)
Subject: BUG/MEDIUM: ring: fix too lax 'size' parser
X-Git-Tag: v2.7-dev4~50
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=18d1306abd6578f2d955f67578ff2458795bba95;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: ring: fix too lax 'size' parser

It took me a while to figure why a ring declared with "size 1M" was causing
strange effects in a ring, it's just because it's parsed as "1", which is
smaller than the default 16384 size and errors are silently ignored.

This commit tries to address this the best possible way without breaking
existing configs that would work by accident, by warning that the size is
ignored if it's smaller than the current one, and by printing the parsed
size instead of the input string in warnings and errors. This way if some
users have "size 10000" or "size 100k" it will continue to work as 16kB
like today but they will now be aware of it.

In addition the error messages were a bit poor in context in that they
only provided line numbers. The ring name was added to ease locating the
problem.

As the issue was present since day one and was introduced in 2.2 with
commit 99c453df9d ("MEDIUM: ring: new section ring to declare custom ring
buffers."), it could make sense to backport this as far as 2.2, but with
2.2 being quite old now it doesn't seem very reasonable to start emitting
new config warnings in config that apparently worked well.

Thus it looks more reasonable to backport this as far as 2.4.
---

diff --git a/src/sink.c b/src/sink.c
index 889acd2f46..04bc8c87bd 100644
--- a/src/sink.c
+++ b/src/sink.c
@@ -760,7 +760,7 @@ int cfg_parse_ring(const char *file, int linenum, char **args, int kwm)
 	size_t size = BUFSIZE;
 	struct proxy *p;
 
-	if (strcmp(args[0], "ring") == 0) { /* new peers section */
+	if (strcmp(args[0], "ring") == 0) { /* new ring section */
 		if (!*args[1]) {
 			ha_alert("parsing [%s:%d] : missing ring name.\n", file, linenum);
 			err_code |= ERR_ALERT | ERR_FATAL;
@@ -804,6 +804,12 @@ int cfg_parse_ring(const char *file, int linenum, char **args, int kwm)
 		cfg_sink->forward_px = p;
 	}
 	else if (strcmp(args[0], "size") == 0) {
+		if (!cfg_sink || (cfg_sink->type != SINK_TYPE_BUFFER)) {
+			ha_alert("parsing [%s:%d] : 'size' directive not usable with this type of sink.\n", file, linenum);
+			err_code |= ERR_ALERT | ERR_FATAL;
+			goto err;
+		}
+
 		size = atol(args[1]);
 		if (!size) {
 			ha_alert("parsing [%s:%d] : invalid size '%s' for new sink buffer.\n", file, linenum, args[1]);
@@ -811,9 +817,16 @@ int cfg_parse_ring(const char *file, int linenum, char **args, int kwm)
 			goto err;
 		}
 
-		if (!cfg_sink || (cfg_sink->type != SINK_TYPE_BUFFER)
-		              || !ring_resize(cfg_sink->ctx.ring, size)) {
-			ha_alert("parsing [%s:%d] : fail to set sink buffer size '%s'.\n", file, linenum, args[1]);
+		if (size < cfg_sink->ctx.ring->buf.size) {
+			ha_warning("parsing [%s:%d] : ignoring new size '%llu' that is smaller than current size '%llu' for ring '%s'.\n",
+				   file, linenum, (ullong)size, (ullong)cfg_sink->ctx.ring->buf.size, cfg_sink->name);
+			err_code |= ERR_ALERT | ERR_FATAL;
+			goto err;
+		}
+
+		if (!ring_resize(cfg_sink->ctx.ring, size)) {
+			ha_alert("parsing [%s:%d] : fail to set sink buffer size '%llu' for ring '%s'.\n", file, linenum,
+				 (ullong)cfg_sink->ctx.ring->buf.size, cfg_sink->name);
 			err_code |= ERR_ALERT | ERR_FATAL;
 			goto err;
 		}