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

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

[Tarmigan Casebolt]

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

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

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

[Tarmigan Casebolt]

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

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

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

[Shawn O. Pearce]

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.

-- 
Shawn.

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

[Shawn O. Pearce]

Tarmigan Casebolt <tarmigan+git@gmail.com> wrote:
> 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.

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

[Jeff King]

On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:

> 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

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

[Junio C Hamano]

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';

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

[Tarmigan]

On Sun, Nov 15, 2009 at 10:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Brian Gernhardt]

On Nov 16, 2009, at 1:12 AM, Junio C Hamano wrote:

> 	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