[PATCH 3/3] nfs.conf tidy ups

[PATCH 3/3] nfs.conf tidy ups

[Justin Mitchell]

Remove the line length parameter and associated code which
led to buffer overruns in the line parsing code.
Also drops the undocumented 'include' directive.

Signed-off-by: Justin Mitchell <jumitche@rehat.com>

---
 support/nfs/conffile.c | 201 ++++++++++++++++++++++++++++---------------------
 1 file changed, 117 insertions(+), 84 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 7c607c0..449ac1c 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -212,15 +212,10 @@ conf_set_now(char *section, char *arg, char *tag,
  * headers and feed tag-value pairs into our configuration database.
  */
 static void
-conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsection)
+conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
 {
 	char *val, *ptr;
-	size_t i;
-	size_t j;
-	static int ln = 0;
 
-	/* Lines starting with '#' or ';' are comments.  */
-	ln++;
 	/* Ignore blank lines */
 	if (*line == '\0')
 		return;
@@ -229,113 +224,149 @@ conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsect
 	while (isblank(*line)) 
 		line++;
 
+	/* Lines starting with '#' or ';' are comments.  */
 	if (*line == '#' || *line == ';')
 		return;
 
 	/* '[section]' parsing...  */
 	if (*line == '[') {
 		line++;
+
+		if (*section) {
+			free(*section);
+			*section = NULL;
+		}
+		if (*subsection) {
+			free(*subsection);
+			*subsection = NULL;
+		}
+
 		/* Strip off any blanks after '[' */
 		while (isblank(*line)) 
 			line++;
-		for (i = 0; i < sz; i++) {
-			if (line[i] == ']') {
-				break;
-			}
-		}
-		if (*section)
-			free(*section);
-		if (i == sz) {
+
+		/* find the closing ] */
+		ptr = strchr(line, ']');
+		if (ptr == NULL) {
 			xlog_warn("config file error: line %d: "
- 				"non-matched ']', ignoring until next section", ln);
-			*section = NULL;
+ 				"non-matched ']', ignoring until next section", lineno);
 			return;
 		}
+
+		/* just ignore everything after the closing ] */
+		*(ptr--) = '\0';
+
 		/* Strip off any blanks before ']' */
-		val = line;
-		j=0;
-		while (*val && !isblank(*val)) 
-			val++, j++;
-		if (*val)
-			i = j;
-		*section = malloc(i+1);
+		while (ptr >= line && isblank(*ptr)) 
+			*(ptr--)='\0';
+
+		/* look for an arg to split from the section name */
+		val = strchr(line, '"');
+		if (val != NULL) {
+			ptr = val - 1;
+			*(val++) = '\0';
+
+			/* trim away any whitespace before the " */
+			while (ptr > line && isblank(*ptr))
+				*(ptr--)='\0';
+		}
+
+		/* copy the section name */
+		*section = strdup(line);
 		if (!*section) {
-			xlog_warn("conf_parse_line: %d: malloc (%lu) failed", ln,
-						(unsigned long)i);
+			xlog_warn("conf_parse_line: %d: malloc failed", lineno);
 			return;
 		}
-		strncpy(*section, line, i);
-		(*section)[i] = '\0';
 
-		if (*subsection) {
-			free(*subsection);
-			*subsection = NULL;
-		}
+		/* there is no arg, we are done */
+		if (val == NULL) return;
 
+		/* check for the closing " */
 		ptr = strchr(val, '"');
-		if (ptr == NULL)
-			return;
-		line = ++ptr;
-		while (*ptr && *ptr != '"' && *ptr != ']')
-			ptr++;
-		if (*ptr == '\0' || *ptr == ']') {
+		if (ptr == NULL) {
 			xlog_warn("config file error: line %d: "
- 				"non-matched '\"', ignoring until next section", ln);
-		}  else {
-			*ptr = '\0';
-			*subsection = strdup(line);
-			if (!*subsection) 
-				xlog_warn("conf_parse_line: %d: malloc arg failed", ln);
+ 				"non-matched '\"', ignoring until next section", lineno);
+			return;
 		}
+		*ptr = '\0';
+		*subsection = strdup(val);
+		if (!*subsection) 
+			xlog_warn("conf_parse_line: %d: malloc arg failed", lineno);
 		return;
 	}
 
 	/* Deal with assignments.  */
-	for (i = 0; i < sz; i++) {
-		if (line[i] == '=') {
-			/* If no section, we are ignoring the lines.  */
-			if (!*section) {
+	ptr = strchr(line, '=');
+
+	/* not an assignment line */
+	if (ptr == NULL) {
+		/* Other non-empty lines are weird.  */
+		if (line[strspn(line, " \t")])
 			xlog_warn("config file error: line %d: "
-				"ignoring line due to no section", ln);
-				return;
-			}
-			line[strcspn (line, " \t=")] = '\0';
-			val = line + i + 1 + strspn (line + i + 1, " \t");
-
-			if (val[0] == '"') {
-				val ++;
-				j = strcspn(val, "\"");
-				val[j] = 0;
-			} else if (val[0] == '\'') {
-				val ++;
-				j = strcspn(val, "'");
-				val[j] = 0;
-			} else {
-				/* Skip trailing spaces and comments */
-				for (j = 0; val[j]; j++) {
-					if ((val[j] == '#' || val[j] == ';')
-					    && (j == 0 || isspace(val[j-1]))) {
-						val[j] = '\0';
-						break;
-					}
-				}
-				while (j && isspace(val[j-1]))
-					val[--j] = '\0';
-			}
-			if (strcasecmp(line, "include") == 0)
-				conf_load(trans, val);
-			else
-				/* XXX Perhaps should we not ignore errors?  */
-				conf_set(trans, *section, *subsection, line, val, 0, 0);
+				"line not empty and not an assignment", lineno);
+		return;
+	}
+
+	/* If no section, we are ignoring the line.  */
+	if (!*section) {
+		xlog_warn("config file error: line %d: "
+			"ignoring line due to no section", lineno);
+		return;
+	}
+
+	val = ptr + 1;
+	*(ptr--) = '\0';
+
+	/* strip spaces before and after the = */
+	while (ptr >= line && isblank(*ptr))
+		*(ptr--)='\0';
+	while (*val != '\0' && isblank(*val))
+		val++;
+
+	if (*val == '"') {
+		val++;
+		ptr = strchr(val, '"');
+		if (ptr == NULL) {
+			xlog_warn("config file error: line %d: "
+				"unmatched quotes", lineno);
 			return;
 		}
+		*ptr = '\0';
+	} else
+	if (*val == '\'') {
+		val++;
+		ptr = strchr(val, '\'');
+		if (ptr == NULL) {
+			xlog_warn("config file error: line %d: "
+				"unmatched quotes", lineno);
+			return;
+		}
+		*ptr = '\0';
+	} else {
+		/* Trim any trailing spaces and comments */
+		if ((ptr=strchr(val, '#'))!=NULL)
+			*ptr = '\0';
+		if ((ptr=strchr(val, ';'))!=NULL)
+			*ptr = '\0';
+
+		ptr = val + strlen(val) - 1;
+		while (ptr > val && isspace(*ptr))
+			*(ptr--) = '\0';
 	}
-	/* Other non-empty lines are weird.  */
-	i = strspn(line, " \t");
-	if (line[i])
-		xlog_warn("config file error: line %d:", ln);
 
-	return;
+	if (*line == '\0') {
+		xlog_warn("config file error: line %d: "
+			"missing tag in assignment", lineno);
+		return;
+	}
+	if (*val == '\0') {
+		xlog_warn("config file error: line %d: "
+			"missing value in assignment", lineno);
+		return;
+	}
+
+	/* XXX Perhaps should we not ignore errors?  */
+	conf_set(trans, *section, *subsection, line, val, 0, 0);
 }
 
 /* Parse the mapped configuration file.  */
@@ -347,6 +378,7 @@ conf_parse(int trans, char *buf, size_t sz)
 	char *line;
 	char *section = NULL;
 	char *subsection = NULL;
+	int lineno = 0;
 
 	line = cp;
 	while (cp < bufend) {
@@ -356,7 +388,8 @@ conf_parse(int trans, char *buf, size_t sz)
 				*(cp - 1) = *cp = ' ';
 			else {
 				*cp = '\0';
-				conf_parse_line(trans, line, cp - line, &section, &subsection);
+				lineno++;
+				conf_parse_line(trans, line, lineno, &section, &subsection);
 				line = cp + 1;
 			}
 		}
-- 
1.8.3.1

Re: [PATCH 3/3] nfs.conf tidy ups

[Steve Dickson]

Hello,

On 05/22/2017 11:52 AM, Justin Mitchell wrote:
> Remove the line length parameter and associated code which
> led to buffer overruns in the line parsing code.
Where these buffer overruns caused by the first patch
or where they originally there? If it is the latter
what had to happen to cause them, being we never
saw them. 

> Also drops the undocumented 'include' directive.
Good call!

steved.
> 
> Signed-off-by: Justin Mitchell <jumitche@rehat.com>
> 
> ---
>  support/nfs/conffile.c | 201 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 117 insertions(+), 84 deletions(-)
> 
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 7c607c0..449ac1c 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -212,15 +212,10 @@ conf_set_now(char *section, char *arg, char *tag,
>   * headers and feed tag-value pairs into our configuration database.
>   */
>  static void
> -conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsection)
> +conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
>  {
>  	char *val, *ptr;
> -	size_t i;
> -	size_t j;
> -	static int ln = 0;
>  
> -	/* Lines starting with '#' or ';' are comments.  */
> -	ln++;
>  	/* Ignore blank lines */
>  	if (*line == '\0')
>  		return;
> @@ -229,113 +224,149 @@ conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsect
>  	while (isblank(*line)) 
>  		line++;
>  
> +	/* Lines starting with '#' or ';' are comments.  */
>  	if (*line == '#' || *line == ';')
>  		return;
>  
>  	/* '[section]' parsing...  */
>  	if (*line == '[') {
>  		line++;
> +
> +		if (*section) {
> +			free(*section);
> +			*section = NULL;
> +		}
> +		if (*subsection) {
> +			free(*subsection);
> +			*subsection = NULL;
> +		}
> +
>  		/* Strip off any blanks after '[' */
>  		while (isblank(*line)) 
>  			line++;
> -		for (i = 0; i < sz; i++) {
> -			if (line[i] == ']') {
> -				break;
> -			}
> -		}
> -		if (*section)
> -			free(*section);
> -		if (i == sz) {
> +
> +		/* find the closing ] */
> +		ptr = strchr(line, ']');
> +		if (ptr == NULL) {
>  			xlog_warn("config file error: line %d: "
> - 				"non-matched ']', ignoring until next section", ln);
> -			*section = NULL;
> + 				"non-matched ']', ignoring until next section", lineno);
>  			return;
>  		}
> +
> +		/* just ignore everything after the closing ] */
> +		*(ptr--) = '\0';
> +
>  		/* Strip off any blanks before ']' */
> -		val = line;
> -		j=0;
> -		while (*val && !isblank(*val)) 
> -			val++, j++;
> -		if (*val)
> -			i = j;
> -		*section = malloc(i+1);
> +		while (ptr >= line && isblank(*ptr)) 
> +			*(ptr--)='\0';
> +
> +		/* look for an arg to split from the section name */
> +		val = strchr(line, '"');
> +		if (val != NULL) {
> +			ptr = val - 1;
> +			*(val++) = '\0';
> +
> +			/* trim away any whitespace before the " */
> +			while (ptr > line && isblank(*ptr))
> +				*(ptr--)='\0';
> +		}
> +
> +		/* copy the section name */
> +		*section = strdup(line);
>  		if (!*section) {
> -			xlog_warn("conf_parse_line: %d: malloc (%lu) failed", ln,
> -						(unsigned long)i);
> +			xlog_warn("conf_parse_line: %d: malloc failed", lineno);
>  			return;
>  		}
> -		strncpy(*section, line, i);
> -		(*section)[i] = '\0';
>  
> -		if (*subsection) {
> -			free(*subsection);
> -			*subsection = NULL;
> -		}
> +		/* there is no arg, we are done */
> +		if (val == NULL) return;
>  
> +		/* check for the closing " */
>  		ptr = strchr(val, '"');
> -		if (ptr == NULL)
> -			return;
> -		line = ++ptr;
> -		while (*ptr && *ptr != '"' && *ptr != ']')
> -			ptr++;
> -		if (*ptr == '\0' || *ptr == ']') {
> +		if (ptr == NULL) {
>  			xlog_warn("config file error: line %d: "
> - 				"non-matched '\"', ignoring until next section", ln);
> -		}  else {
> -			*ptr = '\0';
> -			*subsection = strdup(line);
> -			if (!*subsection) 
> -				xlog_warn("conf_parse_line: %d: malloc arg failed", ln);
> + 				"non-matched '\"', ignoring until next section", lineno);
> +			return;
>  		}
> +		*ptr = '\0';
> +		*subsection = strdup(val);
> +		if (!*subsection) 
> +			xlog_warn("conf_parse_line: %d: malloc arg failed", lineno);
>  		return;
>  	}
>  
>  	/* Deal with assignments.  */
> -	for (i = 0; i < sz; i++) {
> -		if (line[i] == '=') {
> -			/* If no section, we are ignoring the lines.  */
> -			if (!*section) {
> +	ptr = strchr(line, '=');
> +
> +	/* not an assignment line */
> +	if (ptr == NULL) {
> +		/* Other non-empty lines are weird.  */
> +		if (line[strspn(line, " \t")])
>  			xlog_warn("config file error: line %d: "
> -				"ignoring line due to no section", ln);
> -				return;
> -			}
> -			line[strcspn (line, " \t=")] = '\0';
> -			val = line + i + 1 + strspn (line + i + 1, " \t");
> -
> -			if (val[0] == '"') {
> -				val ++;
> -				j = strcspn(val, "\"");
> -				val[j] = 0;
> -			} else if (val[0] == '\'') {
> -				val ++;
> -				j = strcspn(val, "'");
> -				val[j] = 0;
> -			} else {
> -				/* Skip trailing spaces and comments */
> -				for (j = 0; val[j]; j++) {
> -					if ((val[j] == '#' || val[j] == ';')
> -					    && (j == 0 || isspace(val[j-1]))) {
> -						val[j] = '\0';
> -						break;
> -					}
> -				}
> -				while (j && isspace(val[j-1]))
> -					val[--j] = '\0';
> -			}
> -			if (strcasecmp(line, "include") == 0)
> -				conf_load(trans, val);
> -			else
> -				/* XXX Perhaps should we not ignore errors?  */
> -				conf_set(trans, *section, *subsection, line, val, 0, 0);
> +				"line not empty and not an assignment", lineno);
> +		return;
> +	}
> +
> +	/* If no section, we are ignoring the line.  */
> +	if (!*section) {
> +		xlog_warn("config file error: line %d: "
> +			"ignoring line due to no section", lineno);
> +		return;
> +	}
> +
> +	val = ptr + 1;
> +	*(ptr--) = '\0';
> +
> +	/* strip spaces before and after the = */
> +	while (ptr >= line && isblank(*ptr))
> +		*(ptr--)='\0';
> +	while (*val != '\0' && isblank(*val))
> +		val++;
> +
> +	if (*val == '"') {
> +		val++;
> +		ptr = strchr(val, '"');
> +		if (ptr == NULL) {
> +			xlog_warn("config file error: line %d: "
> +				"unmatched quotes", lineno);
>  			return;
>  		}
> +		*ptr = '\0';
> +	} else
> +	if (*val == '\'') {
> +		val++;
> +		ptr = strchr(val, '\'');
> +		if (ptr == NULL) {
> +			xlog_warn("config file error: line %d: "
> +				"unmatched quotes", lineno);
> +			return;
> +		}
> +		*ptr = '\0';
> +	} else {
> +		/* Trim any trailing spaces and comments */
> +		if ((ptr=strchr(val, '#'))!=NULL)
> +			*ptr = '\0';
> +		if ((ptr=strchr(val, ';'))!=NULL)
> +			*ptr = '\0';
> +
> +		ptr = val + strlen(val) - 1;
> +		while (ptr > val && isspace(*ptr))
> +			*(ptr--) = '\0';
>  	}
> -	/* Other non-empty lines are weird.  */
> -	i = strspn(line, " \t");
> -	if (line[i])
> -		xlog_warn("config file error: line %d:", ln);
>  
> -	return;
> +	if (*line == '\0') {
> +		xlog_warn("config file error: line %d: "
> +			"missing tag in assignment", lineno);
> +		return;
> +	}
> +	if (*val == '\0') {
> +		xlog_warn("config file error: line %d: "
> +			"missing value in assignment", lineno);
> +		return;
> +	}
> +
> +	/* XXX Perhaps should we not ignore errors?  */
> +	conf_set(trans, *section, *subsection, line, val, 0, 0);
>  }
>  
>  /* Parse the mapped configuration file.  */
> @@ -347,6 +378,7 @@ conf_parse(int trans, char *buf, size_t sz)
>  	char *line;
>  	char *section = NULL;
>  	char *subsection = NULL;
> +	int lineno = 0;
>  
>  	line = cp;
>  	while (cp < bufend) {
> @@ -356,7 +388,8 @@ conf_parse(int trans, char *buf, size_t sz)
>  				*(cp - 1) = *cp = ' ';
>  			else {
>  				*cp = '\0';
> -				conf_parse_line(trans, line, cp - line, &section, &subsection);
> +				lineno++;
> +				conf_parse_line(trans, line, lineno, &section, &subsection);
>  				line = cp + 1;
>  			}
>  		}
> 

Re: [PATCH 3/3] nfs.conf tidy ups

[Justin Mitchell]

On Thu, 2017-06-01 at 11:07 -0400, Steve Dickson wrote:
> Hello,
> 
> On 05/22/2017 11:52 AM, Justin Mitchell wrote:
> > Remove the line length parameter and associated code which
> > led to buffer overruns in the line parsing code.
> Where these buffer overruns caused by the first patch
> or where they originally there? If it is the latter
> what had to happen to cause them, being we never
> saw them. 

Already there, the conf_parse_line function is passed the length of the
'line' it is processing (out of a larger buffer containing the whole
file). early on the start of the line pointer can get moved to strip
leading whitespace but it later iterates from this new point for an
original-length worth of characters, overrunning the line and
potentially the buffer as a whole.

This is likely from long term code creep as some parts of the line
parsing used this naive string length looping, and other parts use
smarter functions. so rather than repeatedly updating and counting
lengths it seemed more robust to replace these loops with code which
obeys the existing nul markers.

> 
> > Also drops the undocumented 'include' directive.
> Good call!
my motive for this is that when we get to a point of being able to
write/modify the configs through an API it will be a potential headache
trying to figure out which file a given change should be written to.

> 
> steved.
> > 
> > Signed-off-by: Justin Mitchell <jumitche@rehat.com>
> > 
> > ---
> >  support/nfs/conffile.c | 201 ++++++++++++++++++++++++++++---------------------
> >  1 file changed, 117 insertions(+), 84 deletions(-)
> > 
> > diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> > index 7c607c0..449ac1c 100644
> > --- a/support/nfs/conffile.c
> > +++ b/support/nfs/conffile.c
> > @@ -212,15 +212,10 @@ conf_set_now(char *section, char *arg, char *tag,
> >   * headers and feed tag-value pairs into our configuration database.
> >   */
> >  static void
> > -conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsection)
> > +conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
> >  {
> >  	char *val, *ptr;
> > -	size_t i;
> > -	size_t j;
> > -	static int ln = 0;
> >  
> > -	/* Lines starting with '#' or ';' are comments.  */
> > -	ln++;
> >  	/* Ignore blank lines */
> >  	if (*line == '\0')
> >  		return;
> > @@ -229,113 +224,149 @@ conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsect
> >  	while (isblank(*line)) 
> >  		line++;
> >  
> > +	/* Lines starting with '#' or ';' are comments.  */
> >  	if (*line == '#' || *line == ';')
> >  		return;
> >  
> >  	/* '[section]' parsing...  */
> >  	if (*line == '[') {
> >  		line++;
> > +
> > +		if (*section) {
> > +			free(*section);
> > +			*section = NULL;
> > +		}
> > +		if (*subsection) {
> > +			free(*subsection);
> > +			*subsection = NULL;
> > +		}
> > +
> >  		/* Strip off any blanks after '[' */
> >  		while (isblank(*line)) 
> >  			line++;
> > -		for (i = 0; i < sz; i++) {
> > -			if (line[i] == ']') {
> > -				break;
> > -			}
> > -		}
> > -		if (*section)
> > -			free(*section);
> > -		if (i == sz) {
> > +
> > +		/* find the closing ] */
> > +		ptr = strchr(line, ']');
> > +		if (ptr == NULL) {
> >  			xlog_warn("config file error: line %d: "
> > - 				"non-matched ']', ignoring until next section", ln);
> > -			*section = NULL;
> > + 				"non-matched ']', ignoring until next section", lineno);
> >  			return;
> >  		}
> > +
> > +		/* just ignore everything after the closing ] */
> > +		*(ptr--) = '\0';
> > +
> >  		/* Strip off any blanks before ']' */
> > -		val = line;
> > -		j=0;
> > -		while (*val && !isblank(*val)) 
> > -			val++, j++;
> > -		if (*val)
> > -			i = j;
> > -		*section = malloc(i+1);
> > +		while (ptr >= line && isblank(*ptr)) 
> > +			*(ptr--)='\0';
> > +
> > +		/* look for an arg to split from the section name */
> > +		val = strchr(line, '"');
> > +		if (val != NULL) {
> > +			ptr = val - 1;
> > +			*(val++) = '\0';
> > +
> > +			/* trim away any whitespace before the " */
> > +			while (ptr > line && isblank(*ptr))
> > +				*(ptr--)='\0';
> > +		}
> > +
> > +		/* copy the section name */
> > +		*section = strdup(line);
> >  		if (!*section) {
> > -			xlog_warn("conf_parse_line: %d: malloc (%lu) failed", ln,
> > -						(unsigned long)i);
> > +			xlog_warn("conf_parse_line: %d: malloc failed", lineno);
> >  			return;
> >  		}
> > -		strncpy(*section, line, i);
> > -		(*section)[i] = '\0';
> >  
> > -		if (*subsection) {
> > -			free(*subsection);
> > -			*subsection = NULL;
> > -		}
> > +		/* there is no arg, we are done */
> > +		if (val == NULL) return;
> >  
> > +		/* check for the closing " */
> >  		ptr = strchr(val, '"');
> > -		if (ptr == NULL)
> > -			return;
> > -		line = ++ptr;
> > -		while (*ptr && *ptr != '"' && *ptr != ']')
> > -			ptr++;
> > -		if (*ptr == '\0' || *ptr == ']') {
> > +		if (ptr == NULL) {
> >  			xlog_warn("config file error: line %d: "
> > - 				"non-matched '\"', ignoring until next section", ln);
> > -		}  else {
> > -			*ptr = '\0';
> > -			*subsection = strdup(line);
> > -			if (!*subsection) 
> > -				xlog_warn("conf_parse_line: %d: malloc arg failed", ln);
> > + 				"non-matched '\"', ignoring until next section", lineno);
> > +			return;
> >  		}
> > +		*ptr = '\0';
> > +		*subsection = strdup(val);
> > +		if (!*subsection) 
> > +			xlog_warn("conf_parse_line: %d: malloc arg failed", lineno);
> >  		return;
> >  	}
> >  
> >  	/* Deal with assignments.  */
> > -	for (i = 0; i < sz; i++) {
> > -		if (line[i] == '=') {
> > -			/* If no section, we are ignoring the lines.  */
> > -			if (!*section) {
> > +	ptr = strchr(line, '=');
> > +
> > +	/* not an assignment line */
> > +	if (ptr == NULL) {
> > +		/* Other non-empty lines are weird.  */
> > +		if (line[strspn(line, " \t")])
> >  			xlog_warn("config file error: line %d: "
> > -				"ignoring line due to no section", ln);
> > -				return;
> > -			}
> > -			line[strcspn (line, " \t=")] = '\0';
> > -			val = line + i + 1 + strspn (line + i + 1, " \t");
> > -
> > -			if (val[0] == '"') {
> > -				val ++;
> > -				j = strcspn(val, "\"");
> > -				val[j] = 0;
> > -			} else if (val[0] == '\'') {
> > -				val ++;
> > -				j = strcspn(val, "'");
> > -				val[j] = 0;
> > -			} else {
> > -				/* Skip trailing spaces and comments */
> > -				for (j = 0; val[j]; j++) {
> > -					if ((val[j] == '#' || val[j] == ';')
> > -					    && (j == 0 || isspace(val[j-1]))) {
> > -						val[j] = '\0';
> > -						break;
> > -					}
> > -				}
> > -				while (j && isspace(val[j-1]))
> > -					val[--j] = '\0';
> > -			}
> > -			if (strcasecmp(line, "include") == 0)
> > -				conf_load(trans, val);
> > -			else
> > -				/* XXX Perhaps should we not ignore errors?  */
> > -				conf_set(trans, *section, *subsection, line, val, 0, 0);
> > +				"line not empty and not an assignment", lineno);
> > +		return;
> > +	}
> > +
> > +	/* If no section, we are ignoring the line.  */
> > +	if (!*section) {
> > +		xlog_warn("config file error: line %d: "
> > +			"ignoring line due to no section", lineno);
> > +		return;
> > +	}
> > +
> > +	val = ptr + 1;
> > +	*(ptr--) = '\0';
> > +
> > +	/* strip spaces before and after the = */
> > +	while (ptr >= line && isblank(*ptr))
> > +		*(ptr--)='\0';
> > +	while (*val != '\0' && isblank(*val))
> > +		val++;
> > +
> > +	if (*val == '"') {
> > +		val++;
> > +		ptr = strchr(val, '"');
> > +		if (ptr == NULL) {
> > +			xlog_warn("config file error: line %d: "
> > +				"unmatched quotes", lineno);
> >  			return;
> >  		}
> > +		*ptr = '\0';
> > +	} else
> > +	if (*val == '\'') {
> > +		val++;
> > +		ptr = strchr(val, '\'');
> > +		if (ptr == NULL) {
> > +			xlog_warn("config file error: line %d: "
> > +				"unmatched quotes", lineno);
> > +			return;
> > +		}
> > +		*ptr = '\0';
> > +	} else {
> > +		/* Trim any trailing spaces and comments */
> > +		if ((ptr=strchr(val, '#'))!=NULL)
> > +			*ptr = '\0';
> > +		if ((ptr=strchr(val, ';'))!=NULL)
> > +			*ptr = '\0';
> > +
> > +		ptr = val + strlen(val) - 1;
> > +		while (ptr > val && isspace(*ptr))
> > +			*(ptr--) = '\0';
> >  	}
> > -	/* Other non-empty lines are weird.  */
> > -	i = strspn(line, " \t");
> > -	if (line[i])
> > -		xlog_warn("config file error: line %d:", ln);
> >  
> > -	return;
> > +	if (*line == '\0') {
> > +		xlog_warn("config file error: line %d: "
> > +			"missing tag in assignment", lineno);
> > +		return;
> > +	}
> > +	if (*val == '\0') {
> > +		xlog_warn("config file error: line %d: "
> > +			"missing value in assignment", lineno);
> > +		return;
> > +	}
> > +
> > +	/* XXX Perhaps should we not ignore errors?  */
> > +	conf_set(trans, *section, *subsection, line, val, 0, 0);
> >  }
> >  
> >  /* Parse the mapped configuration file.  */
> > @@ -347,6 +378,7 @@ conf_parse(int trans, char *buf, size_t sz)
> >  	char *line;
> >  	char *section = NULL;
> >  	char *subsection = NULL;
> > +	int lineno = 0;
> >  
> >  	line = cp;
> >  	while (cp < bufend) {
> > @@ -356,7 +388,8 @@ conf_parse(int trans, char *buf, size_t sz)
> >  				*(cp - 1) = *cp = ' ';
> >  			else {
> >  				*cp = '\0';
> > -				conf_parse_line(trans, line, cp - line, &section, &subsection);
> > +				lineno++;
> > +				conf_parse_line(trans, line, lineno, &section, &subsection);
> >  				line = cp + 1;
> >  			}
> >  		}
> > 

Re: [PATCH 3/3] nfs.conf tidy ups

[Steve Dickson]

On 05/22/2017 11:52 AM, Justin Mitchell wrote:
> Remove the line length parameter and associated code which
> led to buffer overruns in the line parsing code.
> Also drops the undocumented 'include' directive.
> 
> Signed-off-by: Justin Mitchell <jumitche@rehat.com>
Committed... 

steved.

> 
> ---
>  support/nfs/conffile.c | 201 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 117 insertions(+), 84 deletions(-)
> 
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 7c607c0..449ac1c 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -212,15 +212,10 @@ conf_set_now(char *section, char *arg, char *tag,
>   * headers and feed tag-value pairs into our configuration database.
>   */
>  static void
> -conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsection)
> +conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
>  {
>  	char *val, *ptr;
> -	size_t i;
> -	size_t j;
> -	static int ln = 0;
>  
> -	/* Lines starting with '#' or ';' are comments.  */
> -	ln++;
>  	/* Ignore blank lines */
>  	if (*line == '\0')
>  		return;
> @@ -229,113 +224,149 @@ conf_parse_line(int trans, char *line, size_t sz, char **section, char **subsect
>  	while (isblank(*line)) 
>  		line++;
>  
> +	/* Lines starting with '#' or ';' are comments.  */
>  	if (*line == '#' || *line == ';')
>  		return;
>  
>  	/* '[section]' parsing...  */
>  	if (*line == '[') {
>  		line++;
> +
> +		if (*section) {
> +			free(*section);
> +			*section = NULL;
> +		}
> +		if (*subsection) {
> +			free(*subsection);
> +			*subsection = NULL;
> +		}
> +
>  		/* Strip off any blanks after '[' */
>  		while (isblank(*line)) 
>  			line++;
> -		for (i = 0; i < sz; i++) {
> -			if (line[i] == ']') {
> -				break;
> -			}
> -		}
> -		if (*section)
> -			free(*section);
> -		if (i == sz) {
> +
> +		/* find the closing ] */
> +		ptr = strchr(line, ']');
> +		if (ptr == NULL) {
>  			xlog_warn("config file error: line %d: "
> - 				"non-matched ']', ignoring until next section", ln);
> -			*section = NULL;
> + 				"non-matched ']', ignoring until next section", lineno);
>  			return;
>  		}
> +
> +		/* just ignore everything after the closing ] */
> +		*(ptr--) = '\0';
> +
>  		/* Strip off any blanks before ']' */
> -		val = line;
> -		j=0;
> -		while (*val && !isblank(*val)) 
> -			val++, j++;
> -		if (*val)
> -			i = j;
> -		*section = malloc(i+1);
> +		while (ptr >= line && isblank(*ptr)) 
> +			*(ptr--)='\0';
> +
> +		/* look for an arg to split from the section name */
> +		val = strchr(line, '"');
> +		if (val != NULL) {
> +			ptr = val - 1;
> +			*(val++) = '\0';
> +
> +			/* trim away any whitespace before the " */
> +			while (ptr > line && isblank(*ptr))
> +				*(ptr--)='\0';
> +		}
> +
> +		/* copy the section name */
> +		*section = strdup(line);
>  		if (!*section) {
> -			xlog_warn("conf_parse_line: %d: malloc (%lu) failed", ln,
> -						(unsigned long)i);
> +			xlog_warn("conf_parse_line: %d: malloc failed", lineno);
>  			return;
>  		}
> -		strncpy(*section, line, i);
> -		(*section)[i] = '\0';
>  
> -		if (*subsection) {
> -			free(*subsection);
> -			*subsection = NULL;
> -		}
> +		/* there is no arg, we are done */
> +		if (val == NULL) return;
>  
> +		/* check for the closing " */
>  		ptr = strchr(val, '"');
> -		if (ptr == NULL)
> -			return;
> -		line = ++ptr;
> -		while (*ptr && *ptr != '"' && *ptr != ']')
> -			ptr++;
> -		if (*ptr == '\0' || *ptr == ']') {
> +		if (ptr == NULL) {
>  			xlog_warn("config file error: line %d: "
> - 				"non-matched '\"', ignoring until next section", ln);
> -		}  else {
> -			*ptr = '\0';
> -			*subsection = strdup(line);
> -			if (!*subsection) 
> -				xlog_warn("conf_parse_line: %d: malloc arg failed", ln);
> + 				"non-matched '\"', ignoring until next section", lineno);
> +			return;
>  		}
> +		*ptr = '\0';
> +		*subsection = strdup(val);
> +		if (!*subsection) 
> +			xlog_warn("conf_parse_line: %d: malloc arg failed", lineno);
>  		return;
>  	}
>  
>  	/* Deal with assignments.  */
> -	for (i = 0; i < sz; i++) {
> -		if (line[i] == '=') {
> -			/* If no section, we are ignoring the lines.  */
> -			if (!*section) {
> +	ptr = strchr(line, '=');
> +
> +	/* not an assignment line */
> +	if (ptr == NULL) {
> +		/* Other non-empty lines are weird.  */
> +		if (line[strspn(line, " \t")])
>  			xlog_warn("config file error: line %d: "
> -				"ignoring line due to no section", ln);
> -				return;
> -			}
> -			line[strcspn (line, " \t=")] = '\0';
> -			val = line + i + 1 + strspn (line + i + 1, " \t");
> -
> -			if (val[0] == '"') {
> -				val ++;
> -				j = strcspn(val, "\"");
> -				val[j] = 0;
> -			} else if (val[0] == '\'') {
> -				val ++;
> -				j = strcspn(val, "'");
> -				val[j] = 0;
> -			} else {
> -				/* Skip trailing spaces and comments */
> -				for (j = 0; val[j]; j++) {
> -					if ((val[j] == '#' || val[j] == ';')
> -					    && (j == 0 || isspace(val[j-1]))) {
> -						val[j] = '\0';
> -						break;
> -					}
> -				}
> -				while (j && isspace(val[j-1]))
> -					val[--j] = '\0';
> -			}
> -			if (strcasecmp(line, "include") == 0)
> -				conf_load(trans, val);
> -			else
> -				/* XXX Perhaps should we not ignore errors?  */
> -				conf_set(trans, *section, *subsection, line, val, 0, 0);
> +				"line not empty and not an assignment", lineno);
> +		return;
> +	}
> +
> +	/* If no section, we are ignoring the line.  */
> +	if (!*section) {
> +		xlog_warn("config file error: line %d: "
> +			"ignoring line due to no section", lineno);
> +		return;
> +	}
> +
> +	val = ptr + 1;
> +	*(ptr--) = '\0';
> +
> +	/* strip spaces before and after the = */
> +	while (ptr >= line && isblank(*ptr))
> +		*(ptr--)='\0';
> +	while (*val != '\0' && isblank(*val))
> +		val++;
> +
> +	if (*val == '"') {
> +		val++;
> +		ptr = strchr(val, '"');
> +		if (ptr == NULL) {
> +			xlog_warn("config file error: line %d: "
> +				"unmatched quotes", lineno);
>  			return;
>  		}
> +		*ptr = '\0';
> +	} else
> +	if (*val == '\'') {
> +		val++;
> +		ptr = strchr(val, '\'');
> +		if (ptr == NULL) {
> +			xlog_warn("config file error: line %d: "
> +				"unmatched quotes", lineno);
> +			return;
> +		}
> +		*ptr = '\0';
> +	} else {
> +		/* Trim any trailing spaces and comments */
> +		if ((ptr=strchr(val, '#'))!=NULL)
> +			*ptr = '\0';
> +		if ((ptr=strchr(val, ';'))!=NULL)
> +			*ptr = '\0';
> +
> +		ptr = val + strlen(val) - 1;
> +		while (ptr > val && isspace(*ptr))
> +			*(ptr--) = '\0';
>  	}
> -	/* Other non-empty lines are weird.  */
> -	i = strspn(line, " \t");
> -	if (line[i])
> -		xlog_warn("config file error: line %d:", ln);
>  
> -	return;
> +	if (*line == '\0') {
> +		xlog_warn("config file error: line %d: "
> +			"missing tag in assignment", lineno);
> +		return;
> +	}
> +	if (*val == '\0') {
> +		xlog_warn("config file error: line %d: "
> +			"missing value in assignment", lineno);
> +		return;
> +	}
> +
> +	/* XXX Perhaps should we not ignore errors?  */
> +	conf_set(trans, *section, *subsection, line, val, 0, 0);
>  }
>  
>  /* Parse the mapped configuration file.  */
> @@ -347,6 +378,7 @@ conf_parse(int trans, char *buf, size_t sz)
>  	char *line;
>  	char *section = NULL;
>  	char *subsection = NULL;
> +	int lineno = 0;
>  
>  	line = cp;
>  	while (cp < bufend) {
> @@ -356,7 +388,8 @@ conf_parse(int trans, char *buf, size_t sz)
>  				*(cp - 1) = *cp = ' ';
>  			else {
>  				*cp = '\0';
> -				conf_parse_line(trans, line, cp - line, &section, &subsection);
> +				lineno++;
> +				conf_parse_line(trans, line, lineno, &section, &subsection);
>  				line = cp + 1;
>  			}
>  		}
> 

Re: [PATCH 3/3] nfs.conf tidy ups

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Mon, May 22 2017, Justin Mitchell wrote:

> Remove the line length parameter and associated code which
> led to buffer overruns in the line parsing code.
> Also drops the undocumented 'include' directive.

No, please don't drop the "include" directive.
It was there for a reason.  See commit  be38fd8b8011c6bd.

Yes, it should be documented.  I'm happy to provide documentation.  But
please do remove things without first understanding why they are there.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 3/3] nfs.conf tidy ups

[J. Bruce Fields]

On Wed, Jun 07, 2017 at 01:11:13PM +1000, NeilBrown wrote:
> On Mon, May 22 2017, Justin Mitchell wrote:
> 
> > Remove the line length parameter and associated code which
> > led to buffer overruns in the line parsing code.
> > Also drops the undocumented 'include' directive.
> 
> No, please don't drop the "include" directive.
> It was there for a reason.  See commit  be38fd8b8011c6bd.
> 
> Yes, it should be documented.  I'm happy to provide documentation.  But
> please do [not] remove things without first understanding why they are there.

Looks like that commit did add documentation to nfs.conf.man.

Note also we try to give extra scrutiny to user visible changes like
this which might break existing user setups, so they should really come
in their own patch that mentions the change in the subject line.

--b.

Re: [PATCH 3/3] nfs.conf tidy ups

[Steve Dickson]

Justin,

On 06/06/2017 11:11 PM, NeilBrown wrote:
> On Mon, May 22 2017, Justin Mitchell wrote:
> 
>> Remove the line length parameter and associated code which
>> led to buffer overruns in the line parsing code.
>> Also drops the undocumented 'include' directive.
> 
> No, please don't drop the "include" directive.
> It was there for a reason.  See commit  be38fd8b8011c6bd.
> 
> Yes, it should be documented.  I'm happy to provide documentation.  But
> please do remove things without first understanding why they are there.
Would you mind posting a patch that adds this back?

steved.