Re: configfile.c:63:14: warning: array subscript is above array [-Warray-bounds]
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.
___
CGit mailing list
CGit@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/cgit
[PATCH] Use strbuf for reading configuration files
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 c...@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
___
CGit mailing list
CGit@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH] Use strbuf for reading configuration files
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 c...@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. */
[...]
___
CGit mailing list
CGit@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH] Use strbuf for reading configuration files
Looks mostly okay. While you're at it, what about fixing the BUG mentioned at the bottom of cgitrc.5.txt? ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
