From: Willy Tarreau <w@1wt.eu>
Date: Thu, 5 May 2022 14:00:45 +0000 (+0200)
Subject: BUG/MEDIUM: resolvers: make "show resolvers" properly yield
X-Git-Tag: v2.6-dev9~84
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4e047e7d0e2c1d11c0c9d8621060c080fc017d46;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: resolvers: make "show resolvers" properly yield

The "show resolvers" command is bogus, it tries to implement a yielding
mechanism except that if it yields it restarts from the beginning, until
it manages to fill the buffer with only line breaks, and faces error -2
that lets it reach the final state and exit.

The risk is low since it requires about 50 name servers to reach that
state, but it's not impossible, especially when using multiple sections.

In addition, the extraneous line breaks, if sent over an interactive
connection, will desynchronize the commands and make the client believe
the end was reached after the first nameserver. This cannot be fixed
separately because that would turn this bug into an infinite loop since
it's the line feed that manages to fill the buffer and stop it.

The fix consists in saving the current resolvers section into ctx.cli.p1
and the current nameserver into ctx.cli.p2.

This should be backported, but that code moved a lot since it was
introduced and has always been bogus. It looks like it has mostly
stabilized in 2.4 with commit c943799c86 so the fix might be backportable
to 2.4 without too much effort.
---

diff --git a/src/resolvers.c b/src/resolvers.c
index 7cbddef31d..7d7ced6fda 100644
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -2742,12 +2742,13 @@ static int cli_parse_stat_resolvers(char **args, char *payload, struct appctx *a
 /* Dumps counters from all resolvers section and associated name servers. It
  * returns 0 if the output buffer is full and it needs to be called again,
  * otherwise non-zero. It may limit itself to the resolver pointed to by
- * <cli.p0> if it's not null.
+ * <cli.p0> if it's not null, and takes the current resolver section from p1
+ * and the current resolver from p2.
  */
 static int cli_io_handler_dump_resolvers_to_buffer(struct appctx *appctx)
 {
 	struct conn_stream *cs = appctx->owner;
-	struct resolvers    *resolvers;
+	struct resolvers    *resolvers = appctx->ctx.cli.p1;
 	struct dns_nameserver   *ns;
 
 	chunk_reset(&trash);
@@ -2759,15 +2760,31 @@ static int cli_io_handler_dump_resolvers_to_buffer(struct appctx *appctx)
 
 	case STAT_ST_LIST:
 		if (LIST_ISEMPTY(&sec_resolvers)) {
-			chunk_appendf(&trash, "No resolvers found\n");
+			if (ci_putstr(cs_ic(cs), "No resolvers found\n") == -1)
+				goto full;
 		}
 		else {
-			list_for_each_entry(resolvers, &sec_resolvers, list) {
+			if (!resolvers)
+				resolvers = LIST_ELEM(sec_resolvers.n, typeof(resolvers), list);
+
+			list_for_each_entry_from(resolvers, &sec_resolvers, list) {
 				if (appctx->ctx.cli.p0 != NULL && appctx->ctx.cli.p0 != resolvers)
 					continue;
 
-				chunk_appendf(&trash, "Resolvers section %s\n", resolvers->id);
-				list_for_each_entry(ns, &resolvers->nameservers, list) {
+				appctx->ctx.cli.p1 = resolvers;
+				ns = appctx->ctx.cli.p2;
+
+				if (!ns) {
+					chunk_printf(&trash, "Resolvers section %s\n", resolvers->id);
+					if (ci_putchk(cs_ic(cs), &trash) == -1)
+						goto full;
+
+					ns = LIST_ELEM(resolvers->nameservers.n, typeof(ns), list);
+					appctx->ctx.cli.p2 = ns;
+				}
+
+				list_for_each_entry_from(ns, &resolvers->nameservers, list) {
+					chunk_reset(&trash);
 					chunk_appendf(&trash, " nameserver %s:\n", ns->id);
 					chunk_appendf(&trash, "  sent:        %lld\n", ns->counters->sent);
 					chunk_appendf(&trash, "  snd_error:   %lld\n", ns->counters->snd_error);
@@ -2784,25 +2801,29 @@ static int cli_io_handler_dump_resolvers_to_buffer(struct appctx *appctx)
 					chunk_appendf(&trash, "  too_big:     %lld\n", ns->counters->app.resolver.too_big);
 					chunk_appendf(&trash, "  truncated:   %lld\n", ns->counters->app.resolver.truncated);
 					chunk_appendf(&trash, "  outdated:    %lld\n",  ns->counters->app.resolver.outdated);
+					if (ci_putchk(cs_ic(cs), &trash) == -1)
+						goto full;
+					appctx->ctx.cli.p2 = ns;
 				}
-				chunk_appendf(&trash, "\n");
+
+				appctx->ctx.cli.p2 = NULL;
+
+				/* was this the only section to dump ? */
+				if (appctx->ctx.cli.p0)
+					break;
 			}
 		}
 
-		/* display response */
-		if (ci_putchk(cs_ic(cs), &trash) == -1) {
-			/* let's try again later from this session. We add ourselves into
-			 * this session's users so that it can remove us upon termination.
-			 */
-			cs_rx_room_blk(cs);
-			return 0;
-		}
 		/* fall through */
 
 	default:
 		appctx->st2 = STAT_ST_FIN;
 		return 1;
 	}
+ full:
+	/* the output buffer is full, retry later */
+	cs_rx_room_blk(cs);
+	return 0;
 }
 
 /* register cli keywords */