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

From: Jeff King <peff@peff.net>
Date: Nov 16, 2009, 04:55 UTC
Message-ID: <20091116045532.GC14664@coredump.intra.peff.net>
In-Reply-To: <20091116013654.GX11919@spearce.org>

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