threads / patch / 11197

patchFix off-by-one error: don't read the byte before a malloc'd buffer.

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

## tl;dr

3 messages between Dec 8, 2007 and Dec 9, 2007. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Jim Meyering· Dec 8, 2007, 15:48 UTC · lore

* 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(-)

Show changes to config.c +1 −2

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

Junio C Hamano· Dec 9, 2007, 18:15 UTC · re: Jim Meyering · lore

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

Jim Meyering <jim@meyering.net> writes:

Show 20 quoted lines

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

Brian Gernhardt· Dec 9, 2007, 19:24 UTC · re: Junio C Hamano · lore

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

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.

Show 6 quoted lines

> 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