Re: [PATCH] Remove the hard coded length limit on variable names in config files

2012-09-30 Thread Ben Walton 
Hi Michael,

 The patch doesn't apply to the current master; it appears to have been
 built against master 883a2a3504 (2012-02-23) or older.  It will have to
 be rebased to the current master.

Junio had asked that it be based on maint so that's what I (thought
I?) did.  I'm happy to redo it against master if that's better though.

 The preferred format for multiline comments in the git project is

 /*
  * Truncate the var name back to the section header prior to
  * grabbing the suffix part of the name and the value.
  */

Oops; Will fix.

 In the old code, get_base_var() read the string into var and returned
 var's length (or -1 on error).  The fact that the length of var was
 first reset to zero is somewhat implicit in the fact that no length
 parameter is being passed to get_base_var().

 But in the new version, get_base_var() is passed a strbuf.  Often,
 operations with strbufs append to the strbuf, and this is what I first
 assumed.  It took me a while to realize that get_base_var() calls
 strbuf_reset() before getting to work.  Moreover, get_base_var() still
 returns the length of what it found, which is redundant with a strbuf
 and therefore unexpected.  So when the return value of get_base_var() is
 stored into baselen, it is not really obvious that it is the string's
 length.

Ok, that's a fair criticism.  When I was creating the patch, I thought
that placing
the strbuf_reset in get_base_var() seemed nicer as it matched the
baselen = 0 which
effectively reset the old character array.  Your point is well taken
though and I think
it makes sense to switch things around the way you've suggested.

 Finally, I realize that the MAXNAME constant was not exported and I
 can't find the old length limits documented anywhere, but I nevertheless
 worry a little bit that one of the users of the config API has a
 built-in assumption that names can never be longer than 256 characters
 (for example, a config_fn_t function might try to store the name into a
 fixed-length buffer).  Hopefully such code would never have been written
 or accepted, but...?  If you have thought about this or audited the
 callers, please mention that in your commit message.

I did look through the code to see if anything was relying on fixed
size buffers and I didn't see anything.  I'll update the commit
message with this info too.

I'll send a modified patch shortly.

Thanks for the review!
-Ben
-- 
---
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove the hard coded length limit on variable names in config files

2012-09-30 Thread Ben Walton 
Previously while reading the variable names in config files, there was
a 256 character limit with at most 128 of those characters being used
by the section header portion of the variable name.  This limitation
was only enforced while reading the config files.  It was possible to
write a config file that was not subsequently readable.

Instead of enforcing this limitation for both reading and writing,
remove it entirely by changing the var member of the config_file
struct to a strbuf instead of a fixed length buffer.  Update all of
the parsing functions in config.c to use the strbuf instead of the
static buffer.

The parsing functions that returned the base length of the variable
name now return simply 0 for success and -1 for failure.  The base
length information is obtained through the strbuf's len member.

We now send the buf member of the strbuf to external callback
functions to preserve the external api.  None of the external callers
rely on the old size limitation for sizing their own buffers so
removing the limit should have no externally visible effect.

Signed-off-by: Ben Walton bdwal...@gmail.com
---
 config.c |   59 +--
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/config.c b/config.c
index 08e47e2..24fb2d2 100644
--- a/config.c
+++ b/config.c
@@ -10,8 +10,6 @@
 #include strbuf.h
 #include quote.h
 
-#define MAXNAME (256)
-
 typedef struct config_file {
struct config_file *prev;
FILE *f;
@@ -19,7 +17,7 @@ typedef struct config_file {
int linenr;
int eof;
struct strbuf value;
-   char var[MAXNAME];
+   struct strbuf var;
 } config_file;
 
 static config_file *cf;
@@ -260,7 +258,7 @@ static inline int iskeychar(int c)
return isalnum(c) || c == '-';
 }
 
-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
+static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
int c;
char *value;
@@ -272,11 +270,9 @@ static int get_value(config_fn_t fn, void *data, char 
*name, unsigned int len)
break;
if (!iskeychar(c))
break;
-   name[len++] = tolower(c);
-   if (len = MAXNAME)
-   return -1;
+   strbuf_addch(name, tolower(c));
}
-   name[len] = 0;
+
while (c == ' ' || c == '\t')
c = get_next_char();
 
@@ -288,10 +284,10 @@ static int get_value(config_fn_t fn, void *data, char 
*name, unsigned int len)
if (!value)
return -1;
}
-   return fn(name, value, data);
+   return fn(name-buf, value, data);
 }
 
-static int get_extended_base_var(char *name, int baselen, int c)
+static int get_extended_base_var(struct strbuf *name, int c)
 {
do {
if (c == '\n')
@@ -302,7 +298,7 @@ static int get_extended_base_var(char *name, int baselen, 
int c)
/* We require the format to be '[base extension]' */
if (c != '')
return -1;
-   name[baselen++] = '.';
+   strbuf_addch(name, '.');
 
for (;;) {
int c = get_next_char();
@@ -315,37 +311,31 @@ static int get_extended_base_var(char *name, int baselen, 
int c)
if (c == '\n')
goto error_incomplete_line;
}
-   name[baselen++] = c;
-   if (baselen  MAXNAME / 2)
-   return -1;
+   strbuf_addch(name, c);
}
 
/* Final ']' */
if (get_next_char() != ']')
return -1;
-   return baselen;
+   return 0;
 error_incomplete_line:
cf-linenr--;
return -1;
 }
 
-static int get_base_var(char *name)
+static int get_base_var(struct strbuf *name)
 {
-   int baselen = 0;
-
for (;;) {
int c = get_next_char();
if (cf-eof)
return -1;
if (c == ']')
-   return baselen;
+   return 0;
if (isspace(c))
-   return get_extended_base_var(name, baselen, c);
+   return get_extended_base_var(name, c);
if (!iskeychar(c)  c != '.')
return -1;
-   if (baselen  MAXNAME / 2)
-   return -1;
-   name[baselen++] = tolower(c);
+   strbuf_addch(name, tolower(c));
}
 }
 
@@ -353,7 +343,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 {
int comment = 0;
int baselen = 0;
-   char *var = cf-var;
+   struct strbuf *var = cf-var;
 
/* U+FEFF Byte Order Mark in UTF8 */
static const unsigned char *utf8_bom = (unsigned char *) \xef\xbb\xbf;
@@ -389,17 +379,24 @@ static int git_parse_file(config_fn_t fn, void *data)

Re: [PATCH] Remove the hard coded length limit on variable names in config files

2012-09-30 Thread Michael Haggerty 
On 09/30/2012 08:20 PM, Ben Walton wrote:
 The patch doesn't apply to the current master; it appears to have been
 built against master 883a2a3504 (2012-02-23) or older.  It will have to
 be rebased to the current master.
 
 Junio had asked that it be based on maint so that's what I (thought
 I?) did.  I'm happy to redo it against master if that's better though.

That explains it.  Sorry, I hadn't seen that conversation.  (In the
future, if a patch applies against something other than master, please
add a note in the cover letter or in the patch comment after the --.)

It is OK with me to leave the patch against maint, if that is what Junio
wanted.  It would help, though, if you rebase it to the latest maint
(the conflict seems easy to fix).

Thanks,
Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove the hard coded length limit on variable names in config files

2012-09-29 Thread Ben Walton 
Previously while reading the variable names in config files, there was
a 256 character limit with at most 128 of those characters being used
by the section header portion of the variable name.  This limitation
was only enforced while reading the config files.  It was possible to
write a config file that was not subsequently readable.

Instead of enforcing this limitation for both reading and writing,
remove it entirely by changing the var member of the config_file
struct to a strbuf instead of a fixed length buffer.  Update all of
the parsing functions in config.c to use the strbuf instead of the
static buffer.  Send the buf member of the strbuf to external callback
functions to preserve the external api.

Signed-off-by: Ben Walton bdwal...@gmail.com
---
Hi Junio,

(Sorry that this patch took so long to submit.  I've been busy moving.)

I think this should remove the length limitations enforced while reading
configuration file variable names.

Thanks
-Ben

 config.c |   50 +++---
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/config.c b/config.c
index 40f9c6d..ee860a7 100644
--- a/config.c
+++ b/config.c
@@ -10,8 +10,6 @@
 #include strbuf.h
 #include quote.h
 
-#define MAXNAME (256)
-
 typedef struct config_file {
struct config_file *prev;
FILE *f;
@@ -19,7 +17,7 @@ typedef struct config_file {
int linenr;
int eof;
struct strbuf value;
-   char var[MAXNAME];
+   struct strbuf var;
 } config_file;
 
 static config_file *cf;
@@ -191,7 +189,7 @@ static inline int iskeychar(int c)
return isalnum(c) || c == '-';
 }
 
-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
+static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
int c;
char *value;
@@ -203,11 +201,9 @@ static int get_value(config_fn_t fn, void *data, char 
*name, unsigned int len)
break;
if (!iskeychar(c))
break;
-   name[len++] = tolower(c);
-   if (len = MAXNAME)
-   return -1;
+   strbuf_addch(name, tolower(c));
}
-   name[len] = 0;
+
while (c == ' ' || c == '\t')
c = get_next_char();
 
@@ -219,10 +215,10 @@ static int get_value(config_fn_t fn, void *data, char 
*name, unsigned int len)
if (!value)
return -1;
}
-   return fn(name, value, data);
+   return fn(name-buf, value, data);
 }
 
-static int get_extended_base_var(char *name, int baselen, int c)
+static int get_extended_base_var(struct strbuf *name, int c)
 {
do {
if (c == '\n')
@@ -233,7 +229,7 @@ static int get_extended_base_var(char *name, int baselen, 
int c)
/* We require the format to be '[base extension]' */
if (c != '')
return -1;
-   name[baselen++] = '.';
+   strbuf_addch(name, '.');
 
for (;;) {
int c = get_next_char();
@@ -246,34 +242,30 @@ static int get_extended_base_var(char *name, int baselen, 
int c)
if (c == '\n')
return -1;
}
-   name[baselen++] = c;
-   if (baselen  MAXNAME / 2)
-   return -1;
+   strbuf_addch(name, c);
}
 
/* Final ']' */
if (get_next_char() != ']')
return -1;
-   return baselen;
+   return name-len;
 }
 
-static int get_base_var(char *name)
+static int get_base_var(struct strbuf *name)
 {
-   int baselen = 0;
+   strbuf_reset(name);
 
for (;;) {
int c = get_next_char();
if (cf-eof)
return -1;
if (c == ']')
-   return baselen;
+   return name-len;
if (isspace(c))
-   return get_extended_base_var(name, baselen, c);
+   return get_extended_base_var(name, c);
if (!iskeychar(c)  c != '.')
return -1;
-   if (baselen  MAXNAME / 2)
-   return -1;
-   name[baselen++] = tolower(c);
+   strbuf_addch(name, tolower(c));
}
 }
 
@@ -281,7 +273,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 {
int comment = 0;
int baselen = 0;
-   char *var = cf-var;
+   struct strbuf *var = cf-var;
 
/* U+FEFF Byte Order Mark in UTF8 */
static const unsigned char *utf8_bom = (unsigned char *) \xef\xbb\xbf;
@@ -320,14 +312,16 @@ static int git_parse_file(config_fn_t fn, void *data)
baselen = get_base_var(var);
if (baselen = 0)
break;
-   var[baselen++] = '.';
-   var[baselen] = 0;
+

Re: [PATCH] Remove the hard coded length limit on variable names in config files

2012-09-29 Thread Michael Haggerty 
On 09/29/2012 12:19 PM, Ben Walton wrote:
 Previously while reading the variable names in config files, there was
 a 256 character limit with at most 128 of those characters being used
 by the section header portion of the variable name.  This limitation
 was only enforced while reading the config files.  It was possible to
 write a config file that was not subsequently readable.
 
 Instead of enforcing this limitation for both reading and writing,
 remove it entirely by changing the var member of the config_file
 struct to a strbuf instead of a fixed length buffer.  Update all of
 the parsing functions in config.c to use the strbuf instead of the
 static buffer.  Send the buf member of the strbuf to external callback
 functions to preserve the external api.
 
 Signed-off-by: Ben Walton bdwal...@gmail.com
 ---
 Hi Junio,
 
 (Sorry that this patch took so long to submit.  I've been busy moving.)

The patch doesn't apply to the current master; it appears to have been
built against master 883a2a3504 (2012-02-23) or older.  It will have to
be rebased to the current master.

Nevertheless I will add a few comments below.

Overall, I like your approach of using strbuf here, as it is simpler to
use and less error-prone.  It is also nice to get rid of an arbitrary
length limit, especially since it was not consistently enforced.

 I think this should remove the length limitations enforced while reading
 configuration file variable names.
 
 Thanks
 -Ben
 
  config.c |   50 +++---
  1 file changed, 23 insertions(+), 27 deletions(-)
 
 diff --git a/config.c b/config.c
 index 40f9c6d..ee860a7 100644
 --- a/config.c
 +++ b/config.c
 @@ -10,8 +10,6 @@
  #include strbuf.h
  #include quote.h
  
 -#define MAXNAME (256)
 -
  typedef struct config_file {
   struct config_file *prev;
   FILE *f;
 @@ -19,7 +17,7 @@ typedef struct config_file {
   int linenr;
   int eof;
   struct strbuf value;
 - char var[MAXNAME];
 + struct strbuf var;
  } config_file;
  
  static config_file *cf;
 @@ -191,7 +189,7 @@ static inline int iskeychar(int c)
   return isalnum(c) || c == '-';
  }
  
 -static int get_value(config_fn_t fn, void *data, char *name, unsigned int 
 len)
 +static int get_value(config_fn_t fn, void *data, struct strbuf *name)
  {
   int c;
   char *value;
 @@ -203,11 +201,9 @@ static int get_value(config_fn_t fn, void *data, char 
 *name, unsigned int len)
   break;
   if (!iskeychar(c))
   break;
 - name[len++] = tolower(c);
 - if (len = MAXNAME)
 - return -1;
 + strbuf_addch(name, tolower(c));
   }
 - name[len] = 0;
 +
   while (c == ' ' || c == '\t')
   c = get_next_char();
  
 @@ -219,10 +215,10 @@ static int get_value(config_fn_t fn, void *data, char 
 *name, unsigned int len)
   if (!value)
   return -1;
   }
 - return fn(name, value, data);
 + return fn(name-buf, value, data);
  }
  
 -static int get_extended_base_var(char *name, int baselen, int c)
 +static int get_extended_base_var(struct strbuf *name, int c)
  {
   do {
   if (c == '\n')
 @@ -233,7 +229,7 @@ static int get_extended_base_var(char *name, int baselen, 
 int c)
   /* We require the format to be '[base extension]' */
   if (c != '')
   return -1;
 - name[baselen++] = '.';
 + strbuf_addch(name, '.');
  
   for (;;) {
   int c = get_next_char();
 @@ -246,34 +242,30 @@ static int get_extended_base_var(char *name, int 
 baselen, int c)
   if (c == '\n')
   return -1;
   }
 - name[baselen++] = c;
 - if (baselen  MAXNAME / 2)
 - return -1;
 + strbuf_addch(name, c);
   }
  
   /* Final ']' */
   if (get_next_char() != ']')
   return -1;
 - return baselen;
 + return name-len;
  }
  
 -static int get_base_var(char *name)
 +static int get_base_var(struct strbuf *name)
  {
 - int baselen = 0;
 + strbuf_reset(name);
  
   for (;;) {
   int c = get_next_char();
   if (cf-eof)
   return -1;
   if (c == ']')
 - return baselen;
 + return name-len;
   if (isspace(c))
 - return get_extended_base_var(name, baselen, c);
 + return get_extended_base_var(name, c);
   if (!iskeychar(c)  c != '.')
   return -1;
 - if (baselen  MAXNAME / 2)
 - return -1;
 - name[baselen++] = tolower(c);
 + strbuf_addch(name, tolower(c));
   }
  }
  
 @@ -281,7 +273,7 @@ static int git_parse_file(config_fn_t fn, void *data)
  {
   int comment = 0;
   int baselen = 0;
 - char