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

[Jeff King <peff@peff.net>]

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