[PATCH] Fix off-by-one error: don't read the byte before a malloc'd buffer.

[PATCH] Fix off-by-one error: don't read the byte before a malloc'd buffer.

[Jim Meyering]

* config.c (store_write_pair): Don't read value[-1].

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 config.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index ed96213..6031b38 100644
--- a/config.c
+++ b/config.c
@@ -652,7 +652,7 @@ static int store_write_pair(int fd, const char* key, const char* value)
 	for (i = 0; value[i]; i++)
 		if (value[i] == ';' || value[i] == '#')
 			quote = 1;
-	if (value[i-1] == ' ')
+	if (i && value[i-1] == ' ')
 		quote = 1;

 	if (write_in_full(fd, "\t", 1) != 1 ||
--
1.5.3.7.1116.gae2a9

Re: [PATCH] Fix off-by-one error: don't read the byte before a malloc'd buffer.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> * config.c (store_write_pair): Don't read value[-1].
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  config.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/config.c b/config.c
> index ed96213..6031b38 100644
> --- a/config.c
> +++ b/config.c
> @@ -652,7 +652,7 @@ static int store_write_pair(int fd, const char* key, const char* value)
>  	for (i = 0; value[i]; i++)
>  		if (value[i] == ';' || value[i] == '#')
>  			quote = 1;
> -	if (value[i-1] == ' ')
> +	if (i && value[i-1] == ' ')
>  		quote = 1;
>
>  	if (write_in_full(fd, "\t", 1) != 1 ||

Thanks.

The fix may be correct but the comment above the part the patch fixes is
very misleading and I wasted a few minutes checking to see what it is
doing is correct.

The "quote" variable does not really control quoting; the quoting with
backslash is always done for the value, and this check is about adding a
dq pair around it, so that the parser does not lose leading or trailing
SP or string after start-of-comment marker characters.

Re: [PATCH] Fix off-by-one error: don't read the byte before a malloc'd buffer.

[Brian Gernhardt]

On Dec 9, 2007, at 1:15 PM, Junio C Hamano wrote:

> The fix may be correct but the comment above the part the patch  
> fixes is
> very misleading and I wasted a few minutes checking to see what it is
> doing is correct.

The fix is correct.  I did not anticipate empty strings when I wrote  
the loop in question.  Although I wonder if that should be expanded to  
use isspace instead of checking for just ' '.  I'm less familiar with  
the config parsing than I was.

> The "quote" variable does not really control quoting; the quoting with
> backslash is always done for the value, and this check is about  
> adding a
> dq pair around it, so that the parser does not lose leading or  
> trailing
> SP or string after start-of-comment marker characters.

It's for adding quotes, which made the comment make sense to me.  But  
your expanded description is correct and matches the commit message I  
wrote.  Thanks for clarifying it in the code.

~~ Brian