From: Willy Tarreau <w@1wt.eu>
Date: Thu, 25 Jun 2020 05:35:42 +0000 (+0200)
Subject: MINOR: tools: make parse_line() always terminate the args list
X-Git-Tag: v2.2-dev11~22
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=61dd44bbc1d7b98710ddd922b766170e55defe73;p=thirdparty%2Fhaproxy.git

MINOR: tools: make parse_line() always terminate the args list

parse_line() as added in commit c8d167bcf ("MINOR: tools: add a new
configurable line parse, parse_line()") presents an difficult usage
because it's up to the caller to determine the last written argument
based on what was passed to it. In practice the only way to safely
use it is for the caller to always pass nbarg-1 and make that last
entry point to the last arg + its strlen. This is annoying because
it makes it as painful to use as the infamous strncpy() while it has
all the information the caller needs.

This patch changes its behavior so that it guarantees that at least
one argument will point to the trailing zero at the end of the output
string, as long as there is at least one argument. The caller just
has to pass +1 to the arg count to make sure at least a last one is
empty.
---

diff --git a/src/cfgparse.c b/src/cfgparse.c
index 191698e2b6..9f65d83894 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -1938,7 +1938,7 @@ next_line:
 			uint32_t err;
 			char *errptr;
 
-			arg = MAX_LINE_ARGS;
+			arg = MAX_LINE_ARGS + 1;
 			outlen = outlinesize;
 			err = parse_line(line, outline, &outlen, args, &arg,
 					 PARSE_OPT_ENV | PARSE_OPT_DQUOTE | PARSE_OPT_SQUOTE |
@@ -1977,7 +1977,7 @@ next_line:
 			}
 
 			if (err & PARSE_ERR_TOOMANY) {
-				ha_alert("parsing [%s:%d]: too many words, truncating at word %d, position %ld: <%s>.\n",
+				ha_alert("parsing [%s:%d]: too many words, truncating after word %d, position %ld: <%s>.\n",
 					 file, linenum, MAX_LINE_ARGS, (long)(args[MAX_LINE_ARGS-1] - outline + 1), args[MAX_LINE_ARGS-1]);
 				err_code |= ERR_ALERT | ERR_FATAL;
 				fatal++;
diff --git a/src/tools.c b/src/tools.c
index 10c973761a..964170065e 100644
--- a/src/tools.c
+++ b/src/tools.c
@@ -4738,7 +4738,9 @@ void ha_generate_uuid(struct buffer *output)
  * extraneous ones are not emitted but <outlen> is updated so that the caller
  * knows how much to realloc. Similarly, <args> are not updated beyond <nbargs>
  * but the returned <nbargs> indicates how many were found. All trailing args
- * up to <nbargs> point to the trailing zero.
+ * up to <nbargs> point to the trailing zero, and as long as <nbargs> is > 0,
+ * it is guaranteed that at least one arg will point to the zero. It is safe
+ * to call it with a NULL <args> if <nbargs> is 0.
  *
  * <out> may overlap with <in> provided that it never goes further, in which
  * case the parser will accept to perform in-place parsing and unquoting/
@@ -4761,7 +4763,7 @@ uint32_t parse_line(char *in, char *out, size_t *outlen, char **args, int *nbarg
 	char *brace = NULL;
 	unsigned char hex1, hex2;
 	size_t outmax = *outlen;
-	int argsmax = *nbargs;
+	int argsmax = *nbargs - 1;
 	size_t outpos = 0;
 	int squote = 0;
 	int dquote = 0;
@@ -4771,7 +4773,10 @@ uint32_t parse_line(char *in, char *out, size_t *outlen, char **args, int *nbarg
 	*nbargs = 0;
 	*outlen = 0;
 
-	args[arg] = out;
+	/* argsmax may be -1 here, protecting args[] from any write */
+	if (arg < argsmax)
+		args[arg] = out;
+
 	while (1) {
 		if (*in >= '-' && *in != '\\') {
 			/* speedup: directly send all regular chars starting
@@ -4972,8 +4977,13 @@ uint32_t parse_line(char *in, char *out, size_t *outlen, char **args, int *nbarg
 	*nbargs = arg;
 	*outlen = outpos;
 
-	/* empty all trailing args by making them point to the trailing zero */
-	while (arg < argsmax)
+	/* empty all trailing args by making them point to the trailing zero,
+	 * at least the last one in any case.
+	 */
+	if (arg > argsmax)
+		arg = argsmax;
+
+	while (arg >= 0 && arg <= argsmax)
 		args[arg++] = out + outpos - 1;
 
 	return err;