From mboxrd@z Thu Jan  1 00:00:00 1970
From: cgit at cryptocrack.de (Lukas Fleischer)
Date: Wed, 5 Jun 2013 13:20:19 +0200
Subject: [PATCH] Use strbuf for reading configuration files
In-Reply-To: <20130604182303.GO1072@serenity.lan>
References: <20130604135705.GA15648@blizzard>
 <1370357273-4532-1-git-send-email-cgit@cryptocrack.de>
 <20130604182303.GO1072@serenity.lan>
Message-ID: <20130605112019.GC19702@blizzard>

On Tue, Jun 04, 2013 at 07:23:03PM +0100, John Keeping wrote:
> On Tue, Jun 04, 2013 at 04:47:53PM +0200, Lukas Fleischer wrote:
> > Use struct strbuf from Git instead of fixed-size buffers to remove the
> > limit on the length of configuration file lines and refactor
> > read_config_line() to improve readability.
> > 
> > Note that this also fixes a buffer overflow that existed with the
> > original fixed-size buffer implementation.
> > 
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> 
> Is there any reason not to use strbuf_getline and
> string_list_split_in_place if we're using strbuf here?

* We would need to deal with CRLF separately (check what next_char()
  currently does).

* I don't think it is much shorter or significantly improves
  readability.

* It might be a tad slower (we need to look for the delimiter after
  reading the whole line vs. doing it on the fly).

* Git uses strbuf_addch() in their configuration file parser as well
  (although their format is a tad more complex and there might be other
  reasons for using it there).

I will resubmit v2 that still uses strbuf_addch() -- feel free to submit
an improved version that uses strbuf_getline() and
string_list_split_in_place() :)

> 
> >  configfile.c | 65 ++++++++++++++++++++++++++++++------------------------------
> >  configfile.h |  2 ++
> >  2 files changed, 35 insertions(+), 32 deletions(-)
> > 
> > diff --git a/configfile.c b/configfile.c
> > index d98989c..e6ad1d6 100644
> > --- a/configfile.c
> > +++ b/configfile.c
> > @@ -31,45 +31,44 @@ static void skip_line(FILE *f)
> >  		;
> >  }
> >  
> > -static int read_config_line(FILE *f, char *line, const char **value, int bufsize)
> > +static int read_config_line(FILE *f, struct strbuf *name, struct strbuf *value)
> >  {
> > -	int i = 0, isname = 0;
> > +	int c = next_char(f);
> >  
> > -	*value = NULL;
> > -	while (i < bufsize - 1) {
> > -		int c = next_char(f);
> > -		if (!isname && (c == '#' || c == ';')) {
> > -			skip_line(f);
> > -			continue;
> > -		}
> > -		if (!isname && isspace(c))
> > -			continue;
> > +	strbuf_reset(name);
> > +	strbuf_reset(value);
> >  
> > -		if (c == '=' && !*value) {
> > -			line[i] = 0;
> > -			*value = &line[i + 1];
> > -		} else if (c == '\n' && !isname) {
> > -			i = 0;
> > -			continue;
> > -		} else if (c == '\n' || c == EOF) {
> > -			line[i] = 0;
> > -			break;
> > -		} else {
> > -			line[i] = c;
> > -		}
> > -		isname = 1;
> > -		i++;
> > +	/* Skip comments and preceding spaces. */
> > +	while (c == '#' || c == ';') {
> > +		skip_line(f);
> > +		c = next_char(f);
> > +	}
> > +	while (isspace(c))
> > +		c = next_char(f);
> > +
> > +	/* Read variable name. */
> > +	while (c != '=') {
> > +		if (c == '\n' || c == EOF)
> > +			return 0;
> > +		strbuf_addch(name, c);
> > +		c = next_char(f);
> >  	}
> > -	line[i + 1] = 0;
> > -	return i;
> > +
> > +	/* Read variable value. */
> > +	c = next_char(f);
> > +	while (c != '\n' && c != EOF) {
> > +		strbuf_addch(value, c);
> > +		c = next_char(f);
> > +	}
> > +
> > +	return 1;
> >  }
> >  
> >  int parse_configfile(const char *filename, configfile_value_fn fn)
> >  {
> >  	static int nesting;
> > -	int len;
> > -	char line[256];
> > -	const char *value;
> > +	struct strbuf name = STRBUF_INIT;
> > +	struct strbuf value = STRBUF_INIT;
> >  	FILE *f;
> >  
> >  	/* cancel deeply nested include-commands */
> > @@ -78,10 +77,12 @@ int parse_configfile(const char *filename, configfile_value_fn fn)
> >  	if (!(f = fopen(filename, "r")))
> >  		return -1;
> >  	nesting++;
> > -	while ((len = read_config_line(f, line, &value, sizeof(line))) > 0)
> > -		fn(line, value);
> > +	while (read_config_line(f, &name, &value))
> > +		fn(name.buf, value.buf);
> >  	nesting--;
> >  	fclose(f);
> > +	strbuf_release(&name);
> > +	strbuf_release(&value);
> >  	return 0;
> >  }
> >  
> > diff --git a/configfile.h b/configfile.h
> > index 04235e5..af7ca19 100644
> > --- a/configfile.h
> > +++ b/configfile.h
> > @@ -1,6 +1,8 @@
> >  #ifndef CONFIGFILE_H
> >  #define CONFIGFILE_H
> >  
> > +#include "cgit.h"
> > +
> >  typedef void (*configfile_value_fn)(const char *name, const char *value);
> >  
> >  extern int parse_configfile(const char *filename, configfile_value_fn fn);
> > -- 
> > 1.8.3.450.gf3f2a46