List for cgit developers and users
 help / color / mirror / Atom feed
* configfile.c:63:14: warning: array subscript is above array [-Warray-bounds]
@ 2013-06-03 20:21 koverstreet
  2013-06-04 10:34 ` Jason
  0 siblings, 1 reply; 17+ messages in thread
From: koverstreet @ 2013-06-03 20:21 UTC (permalink / raw)


Just saw that when building 52c926c with gcc 4.6.2, the line of code is
        line[i + 1] = 0;
        return i;

Took a look at the code and it looks like a real buffer overflow. But it
also looks like the code would be a lot simpler if you just used
getline() from glibc (and if you don't want to be tied to glibc, just
reimplement it and ideally push it to some other C library).


^ permalink raw reply	[flat|nested] 17+ messages in thread

* configfile.c:63:14: warning: array subscript is above array [-Warray-bounds]
  2013-06-03 20:21 configfile.c:63:14: warning: array subscript is above array [-Warray-bounds] koverstreet
@ 2013-06-04 10:34 ` Jason
  2013-06-04 13:57   ` cgit
  0 siblings, 1 reply; 17+ messages in thread
From: Jason @ 2013-06-04 10:34 UTC (permalink / raw)


Here is the function in question:

static int read_config_line(FILE *f, char *line, const char **value,
int bufsize)
{
        int i = 0, isname = 0;

        *value = NULL;
        while (i < bufsize - 1) {
                int c = next_char(f);
                if (!isname && (c == '#' || c == ';')) {
                        skip_line(f);
                        continue;
                }
                if (!isname && isspace(c))
                        continue;

                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++;
        }
        line[i + 1] = 0;
        return i;
}

The ways out of this loop are when it encounters a \n or EOF after
finding a name, or when i == bufsize - 1. In the former case, an extra
null terminator is needlessly added. In the latter, we write a null
into line[bufsize], which, as you've pointed out, is out of bounds.

I'll adjust it to this:

static int read_config_line(FILE *f, char *line, const char **value,
int bufsize)
{
        int i = 0, isname = 0;

        *value = NULL;
        while (i < bufsize - 1) {
                int c = next_char(f);
                if (!isname && (c == '#' || c == ';')) {
                        skip_line(f);
                        continue;
                }
                if (!isname && isspace(c))
                        continue;

                if (c == '=' && !*value) {
                        line[i] = 0;
                        *value = &line[i + 1];
                } else if (c == '\n' && !isname) {
                        i = 0;
                        continue;
                } else if (c == '\n' || c == EOF) {
                        break;
                } else {
                        line[i] = c;
                }
                isname = 1;
                i++;
        }
        line[i] = 0;
        return i;
}

Look good?

Thanks Kent.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* configfile.c:63:14: warning: array subscript is above array [-Warray-bounds]
  2013-06-04 10:34 ` Jason
@ 2013-06-04 13:57   ` cgit
  2013-06-04 14:47     ` [PATCH] Use strbuf for reading configuration files cgit
  0 siblings, 1 reply; 17+ messages in thread
From: cgit @ 2013-06-04 13:57 UTC (permalink / raw)


On Tue, Jun 04, 2013 at 12:34:49PM +0200, Jason A. Donenfeld wrote:
> Here is the function in question:
> 
> [...]
> 
> I'll adjust it to this:
> 
> static int read_config_line(FILE *f, char *line, const char **value,
> int bufsize)
> {
> [...]
>                 } else if (c == '\n' || c == EOF) {
>                         break;
>                 } else {
>                         line[i] = c;
>                 }
>                 isname = 1;
>                 i++;
>         }
>         line[i] = 0;
>         return i;
> }
> 
> Look good?

Ack. Maybe we can further improve this by using strbuf_*() instead of
fixed-size buffers?

> 
> Thanks Kent.
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 13:57   ` cgit
@ 2013-06-04 14:47     ` cgit
  2013-06-04 15:43       ` cgit
                         ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: cgit @ 2013-06-04 14:47 UTC (permalink / raw)


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>
---
 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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 14:47     ` [PATCH] Use strbuf for reading configuration files cgit
@ 2013-06-04 15:43       ` cgit
  2013-06-04 17:29         ` Jason
  2013-06-04 18:23       ` john
  2013-06-05 11:17       ` [PATCH v2] " cgit
  2 siblings, 1 reply; 17+ messages in thread
From: cgit @ 2013-06-04 15:43 UTC (permalink / raw)


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>
> ---
>  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)
> [...]
> +	/* Skip comments and preceding spaces. */
> +	while (c == '#' || c == ';') {
> +		skip_line(f);
> +		c = next_char(f);
> +	}
> +	while (isspace(c))
> +		c = next_char(f);

Just noticed that this no longer detects indented comments. Not sure if
it makes sense to accept these (since we already only allow full
fill-line comments) but given that no longer allowing them is probably
considered a regression I will amend the patch and resubmit as far and
as soon as there aren't any other comments.

> +
> +	/* Read variable name. */
> [...]


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 15:43       ` cgit
@ 2013-06-04 17:29         ` Jason
  2013-06-04 17:35           ` Jason
  2013-06-04 17:39           ` cgit
  0 siblings, 2 replies; 17+ messages in thread
From: Jason @ 2013-06-04 17:29 UTC (permalink / raw)


Looks mostly okay. While you're at it, what about fixing the BUG mentioned
at the bottom of cgitrc.5.txt?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130604/e993f468/attachment.html>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 17:29         ` Jason
@ 2013-06-04 17:35           ` Jason
  2013-06-04 17:39           ` cgit
  1 sibling, 0 replies; 17+ messages in thread
From: Jason @ 2013-06-04 17:35 UTC (permalink / raw)


It looks like we can also borrow routines directly from git, off of which
it appears config.c was initial based:

http://git.zx2c4.com/git/tree/config.c

In fact, what about reworking our config in general to use git's system?
This would involve supporting both our:

repo.blah = val

syntax as well as

[repo]
blah = val

which might be a little bit nice.

http://git.zx2c4.com/git/tree/config.c#n898

git_config_from_file might be a possibility.

We'd get nice escaping too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130604/77e78961/attachment.html>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 17:29         ` Jason
  2013-06-04 17:35           ` Jason
@ 2013-06-04 17:39           ` cgit
  2013-06-04 17:40             ` Jason
  1 sibling, 1 reply; 17+ messages in thread
From: cgit @ 2013-06-04 17:39 UTC (permalink / raw)


On Tue, Jun 04, 2013 at 07:29:07PM +0200, Jason A. Donenfeld wrote:
> Looks mostly okay. While you're at it, what about fixing the BUG mentioned at
> the bottom of cgitrc.5.txt?

Mh. I don't think that it is trivial to fix -- especially with regard to
backwards compatibility. In order to allow inline comments we would
probably have to add proper quoting (think about descriptions and file
names including "#") and break existing configurations. Any other ideas?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 17:39           ` cgit
@ 2013-06-04 17:40             ` Jason
  2013-06-04 17:49               ` Jason
  0 siblings, 1 reply; 17+ messages in thread
From: Jason @ 2013-06-04 17:40 UTC (permalink / raw)


Implementing the git_config_parse_parameter as we speak! Standby.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 17:40             ` Jason
@ 2013-06-04 17:49               ` Jason
  0 siblings, 0 replies; 17+ messages in thread
From: Jason @ 2013-06-04 17:49 UTC (permalink / raw)


On Tue, Jun 4, 2013 at 7:40 PM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:

> Implementing the git_config_parse_parameter as we speak! Standby.


Just sent some general sketch of what I have in mind if you'd like to try
your hand with it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130604/d636c89e/attachment.html>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 14:47     ` [PATCH] Use strbuf for reading configuration files cgit
  2013-06-04 15:43       ` cgit
@ 2013-06-04 18:23       ` john
  2013-06-05 11:20         ` cgit
  2013-06-05 11:17       ` [PATCH v2] " cgit
  2 siblings, 1 reply; 17+ messages in thread
From: john @ 2013-06-04 18:23 UTC (permalink / raw)


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?

>  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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Use strbuf for reading configuration files
  2013-06-04 14:47     ` [PATCH] Use strbuf for reading configuration files cgit
  2013-06-04 15:43       ` cgit
  2013-06-04 18:23       ` john
@ 2013-06-05 11:17       ` cgit
  2013-06-13 21:25         ` cgit
  2 siblings, 1 reply; 17+ messages in thread
From: cgit @ 2013-06-05 11:17 UTC (permalink / raw)


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>
---
 configfile.c | 64 +++++++++++++++++++++++++++++++-----------------------------
 configfile.h |  2 ++
 2 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/configfile.c b/configfile.c
index d98989c..31fe5c8 100644
--- a/configfile.c
+++ b/configfile.c
@@ -31,45 +31,45 @@ 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;
+	/* Skip comments and preceding spaces. */
+	for(;;) {
+		if (c == '#' || c == ';')
+			skip_line(f);
+		else if (!isspace(c))
 			break;
-		} else {
-			line[i] = c;
-		}
-		isname = 1;
-		i++;
+		c = next_char(f);
 	}
-	line[i + 1] = 0;
-	return i;
+
+	/* Read variable name. */
+	while (c != '=') {
+		if (c == '\n' || c == EOF)
+			return 0;
+		strbuf_addch(name, c);
+		c = next_char(f);
+	}
+
+	/* 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 +78,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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] Use strbuf for reading configuration files
  2013-06-04 18:23       ` john
@ 2013-06-05 11:20         ` cgit
  0 siblings, 0 replies; 17+ messages in thread
From: cgit @ 2013-06-05 11:20 UTC (permalink / raw)


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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Use strbuf for reading configuration files
  2013-06-05 11:17       ` [PATCH v2] " cgit
@ 2013-06-13 21:25         ` cgit
  2013-08-02 17:13           ` cgit
  0 siblings, 1 reply; 17+ messages in thread
From: cgit @ 2013-06-13 21:25 UTC (permalink / raw)


On Wed, Jun 05, 2013 at 01:17:00PM +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>
> ---
>  configfile.c | 64 +++++++++++++++++++++++++++++++-----------------------------
>  configfile.h |  2 ++
>  2 files changed, 35 insertions(+), 31 deletions(-)
> 

Status...?

> [...]


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Use strbuf for reading configuration files
  2013-06-13 21:25         ` cgit
@ 2013-08-02 17:13           ` cgit
  2013-08-02 18:14             ` Jason
  2013-08-12 18:52             ` Jason
  0 siblings, 2 replies; 17+ messages in thread
From: cgit @ 2013-08-02 17:13 UTC (permalink / raw)


On Thu, Jun 13, 2013 at 11:25:53PM +0200, Lukas Fleischer wrote:
> On Wed, Jun 05, 2013 at 01:17:00PM +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>
> > ---
> >  configfile.c | 64 +++++++++++++++++++++++++++++++-----------------------------
> >  configfile.h |  2 ++
> >  2 files changed, 35 insertions(+), 31 deletions(-)
> > 
> 
> Status...?

*bump*

I put all pending patches on the "for-jason" branch of my public cgit
repository [1], so it shouldn't take much longer than 20 seconds to
merge them into master...

> 
> > [...]
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[1] http://git.cryptocrack.de/cgit.git/log/?h=for-jason


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Use strbuf for reading configuration files
  2013-08-02 17:13           ` cgit
@ 2013-08-02 18:14             ` Jason
  2013-08-12 18:52             ` Jason
  1 sibling, 0 replies; 17+ messages in thread
From: Jason @ 2013-08-02 18:14 UTC (permalink / raw)


Thanks for consolidating things Lukas. I'm still on vacation (til
mid-August) but I'll be getting to it asap.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130802/f8779f0b/attachment.html>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Use strbuf for reading configuration files
  2013-08-02 17:13           ` cgit
  2013-08-02 18:14             ` Jason
@ 2013-08-12 18:52             ` Jason
  1 sibling, 0 replies; 17+ messages in thread
From: Jason @ 2013-08-12 18:52 UTC (permalink / raw)


Merged at last! Thanks Lukas.


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2013-08-12 18:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-03 20:21 configfile.c:63:14: warning: array subscript is above array [-Warray-bounds] koverstreet
2013-06-04 10:34 ` Jason
2013-06-04 13:57   ` cgit
2013-06-04 14:47     ` [PATCH] Use strbuf for reading configuration files cgit
2013-06-04 15:43       ` cgit
2013-06-04 17:29         ` Jason
2013-06-04 17:35           ` Jason
2013-06-04 17:39           ` cgit
2013-06-04 17:40             ` Jason
2013-06-04 17:49               ` Jason
2013-06-04 18:23       ` john
2013-06-05 11:20         ` cgit
2013-06-05 11:17       ` [PATCH v2] " cgit
2013-06-13 21:25         ` cgit
2013-08-02 17:13           ` cgit
2013-08-02 18:14             ` Jason
2013-08-12 18:52             ` Jason

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).