threads / patch / 21619

patch, 2 partshttp-backend: Let gcc check the format of more printf-type functions.

Subject: [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions.

## tl;dr

8 messages between Nov 14, 2009 and Nov 23, 2009. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Tarmigan Casebolt· Nov 14, 2009, 21:10 UTC · lore

[PATCH 1/2] http-backend: Fix access beyond end of string.

Found with valgrind while looking for Content-Length corruption in smart http.

Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com>
---
 http-backend.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to http-backend.c +1 −1
diff --git a/http-backend.c b/http-backend.c
index f8ea9d7..ab9433d 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -634,7 +634,7 @@ int main(int argc, char **argv)
 			cmd = c;
 			cmd_arg = xmalloc(n);
 			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
-			cmd_arg[n] = '\0';
+			cmd_arg[n-1] = '\0';
 			dir[out[0].rm_so] = 0;
 			break;
 		}

-- 
1.6.5.51.g191f5
Tarmigan Casebolt· Nov 14, 2009, 21:10 UTC · re: Tarmigan Casebolt · lore

We already have these checks in many printf-type functions that have prototypes which are in header files. Add these same checks to static functions in http-backend.c

Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com>
---

Shawn, please consider this patch in addition to the one that you posted that actually fixes the bug. With this patch, gcc will warn about that bug.

 http-backend.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to http-backend.c +3 −0
diff --git a/http-backend.c b/http-backend.c
index ab9433d..110b166 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -108,6 +108,7 @@ static const char *get_parameter(const char *name)
 	return i ? i->util : NULL;
 }
 
+__attribute__((format (printf, 2, 3)))
 static void format_write(int fd, const char *fmt, ...)
 {
 	static char buffer[1024];
@@ -165,6 +166,7 @@ static void end_headers(void)
 	safe_write(1, "\r\n", 2);
 }
 
+__attribute__((format (printf, 1, 2)))
 static NORETURN void not_found(const char *err, ...)
 {
 	va_list params;
@@ -180,6 +182,7 @@ static NORETURN void not_found(const char *err, ...)
 	exit(0);
 }
 
+__attribute__((format (printf, 1, 2)))
 static NORETURN void forbidden(const char *err, ...)
 {
 	va_list params;

-- 
1.6.5.51.g191f5
Shawn O. Pearce· Nov 16, 2009, 01:39 UTC · re: Tarmigan Casebolt · lore

Re: [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions.

Tarmigan Casebolt <tarmigan+git@gmail.com> wrote:
Show 9 quoted lines
> We already have these checks in many printf-type functions that have
> prototypes which are in header files.  Add these same checks to
> static functions in http-backend.c
> 
> Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com>
> ---
> 
> Shawn, please consider this patch in addition to the one that you posted 
> that actually fixes the bug.  With this patch, gcc will warn about that bug.
Yup, it would have caught it, thanks.
Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
-- 
Shawn.
Shawn O. Pearce· Nov 16, 2009, 01:36 UTC · re: Tarmigan Casebolt · lore

Re: [PATCH 1/2] http-backend: Fix access beyond end of string.

Tarmigan Casebolt <tarmigan+git@gmail.com> wrote:
Show 12 quoted lines
> diff --git a/http-backend.c b/http-backend.c
> index f8ea9d7..ab9433d 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -634,7 +634,7 @@ int main(int argc, char **argv)
>  			cmd = c;
>  			cmd_arg = xmalloc(n);
>  			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
> -			cmd_arg[n] = '\0';
> +			cmd_arg[n-1] = '\0';
>  			dir[out[0].rm_so] = 0;
>  			break;
Shouldn't this instead be:
Show changes to http-backend.c +1 −1
diff --git a/http-backend.c b/http-backend.c
index 9021266..16ec635 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -626,7 +626,7 @@ int main(int argc, char **argv)
 			}
 
 			cmd = c;
-			cmd_arg = xmalloc(n);
+			cmd_arg = xmalloc(n + 1);
 			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
 			cmd_arg[n] = '\0';
 			dir[out[0].rm_so] = 0;

The cmd_arg string was simply allocated too small.  Your fix is
terminating the string one character too short which would cause
get_loose_object and get_pack_file to break.

-- 
Shawn.
Jeff King· Nov 16, 2009, 04:55 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 1/2] http-backend: Fix access beyond end of string.

On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:
Show 33 quoted lines
> Tarmigan Casebolt <tarmigan+git@gmail.com> wrote:
> > diff --git a/http-backend.c b/http-backend.c
> > index f8ea9d7..ab9433d 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -634,7 +634,7 @@ int main(int argc, char **argv)
> >  			cmd = c;
> >  			cmd_arg = xmalloc(n);
> >  			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
> > -			cmd_arg[n] = '\0';
> > +			cmd_arg[n-1] = '\0';
> >  			dir[out[0].rm_so] = 0;
> >  			break;
> 
> Shouldn't this instead be:
> 
> diff --git a/http-backend.c b/http-backend.c
> index 9021266..16ec635 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -626,7 +626,7 @@ int main(int argc, char **argv)
>  			}
>  
>  			cmd = c;
> -			cmd_arg = xmalloc(n);
> +			cmd_arg = xmalloc(n + 1);
>  			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
>  			cmd_arg[n] = '\0';
>  			dir[out[0].rm_so] = 0;
> 
> The cmd_arg string was simply allocated too small.  Your fix is
> terminating the string one character too short which would cause
> get_loose_object and get_pack_file to break.

Actually, from my reading, I think his fix is right, because you trim the first character during the strncpy (using "out[0].rm_so + 1"). But it's not clear when you create 'n' that you are dropping that character. IOW, you are doing:

  /* string + '\0' - '/' */
  size_t n = out[0].rm_eo - (out[0].rm_so + 1) + 1;

which ends up the same as your n, but means that the NUL goes at cmd_arg[n-1]. But I didn't actually run it, so if his fix is breaking things, then both Tarmigan and I are counting wrong. ;)

-Peff
Junio C Hamano· Nov 16, 2009, 06:12 UTC · re: Jeff King · lore

Re: [PATCH 1/2] http-backend: Fix access beyond end of string.

Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:
> ...
>> Shouldn't this instead be:
>> 
>> diff --git a/http-backend.c b/http-backend.c
>> index 9021266..16ec635 100644
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -626,7 +626,7 @@ int main(int argc, char **argv)
>>  			}
>>  
>>  			cmd = c;
>> -			cmd_arg = xmalloc(n);
>> +			cmd_arg = xmalloc(n + 1);
>>  			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
>>  			cmd_arg[n] = '\0';
>>  			dir[out[0].rm_so] = 0;
>> 
>> The cmd_arg string was simply allocated too small.  Your fix is
>> terminating the string one character too short which would cause
>> get_loose_object and get_pack_file to break.
>
> Actually, from my reading, I think his fix is right, because you trim
> the first character during the strncpy (using "out[0].rm_so + 1").

Your regexps all start with leading "/", and rm_so+1 points at the character after the slash; the intention being that you would copy the rest of the matched sequence without the leading "/".

So allocating n = rm_eo - rm_so is Ok. It counts the space for terminating NUL. But copying "up to n bytes" using strncpy(), only to NUL terminate immediately later, is dubious. You would want to copy only n-1 bytes. I.e.

	n = out[0].rm_eo - out[0].rm_so; /* allocation */
        ... validate and fail invalid method ...
        cmd_arg = xmalloc(n);
        memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);
        cmd_arg[n-1] = '\0';
Tarmigan· Nov 17, 2009, 04:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] http-backend: Fix access beyond end of string.

On Sun, Nov 15, 2009 at 10:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 42 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:
>> ...
>>> Shouldn't this instead be:
>>>
>>> diff --git a/http-backend.c b/http-backend.c
>>> index 9021266..16ec635 100644
>>> --- a/http-backend.c
>>> +++ b/http-backend.c
>>> @@ -626,7 +626,7 @@ int main(int argc, char **argv)
>>>                      }
>>>
>>>                      cmd = c;
>>> -                    cmd_arg = xmalloc(n);
>>> +                    cmd_arg = xmalloc(n + 1);
>>>                      strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
>>>                      cmd_arg[n] = '\0';
>>>                      dir[out[0].rm_so] = 0;
>>>
>>> The cmd_arg string was simply allocated too small.  Your fix is
>>> terminating the string one character too short which would cause
>>> get_loose_object and get_pack_file to break.
>>
>> Actually, from my reading, I think his fix is right, because you trim
>> the first character during the strncpy (using "out[0].rm_so + 1").
>
> Your regexps all start with leading "/", and rm_so+1 points at the
> character after the slash; the intention being that you would copy
> the rest of the matched sequence without the leading "/".
>
> So allocating n = rm_eo - rm_so is Ok.  It counts the space for
> terminating NUL.  But copying "up to n bytes" using strncpy(), only to NUL
> terminate immediately later, is dubious.  You would want to copy only n-1
> bytes.  I.e.
>
>        n = out[0].rm_eo - out[0].rm_so; /* allocation */
>        ... validate and fail invalid method ...
>        cmd_arg = xmalloc(n);
>        memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);
>        cmd_arg[n-1] = '\0';
>

I think the strncpy( , ,n) would not harm anything because we won't overflow dir because it's NUL terminated in getdir(), and the '\0' shouldn't match the regex. But I agree that strncpy( , , n-1) is better and memcpy( , , n-1) is better still.

Better eyes than mine have now looked at this and see different things each time. I wonder if some parts could be made a little less subtle (perhaps along with the dir[out[0].rm_so] = 0;)?

Thanks, Tarmigan

Brian Gernhardt· Nov 23, 2009, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] http-backend: Fix access beyond end of string.

On Nov 16, 2009, at 1:12 AM, Junio C Hamano wrote:
Show 5 quoted lines
> 	n = out[0].rm_eo - out[0].rm_so; /* allocation */
>        ... validate and fail invalid method ...
>        cmd_arg = xmalloc(n);
>        memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);
>        cmd_arg[n-1] = '\0';
I just thought I'd point out that this change (committed as 48aec1b) fixed the problem I was having with t5541-http-push (and a couple others) hanging.  Looks like that one extra byte was overwriting something that malloc/free wanted to keep intact on OS X.
~~ Brian

← back to recent threads