Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type

[Jeff King <peff@peff.net>]

On Tue, Apr 21, 2015 at 12:13:30AM +0530, karthik nayak wrote:

> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char
> *map,
> +                                       unsigned long mapsize, void *buffer,
> +                                       unsigned long bufsiz, struct strbuf
> *header)
> +{
> +       unsigned char *cp;
> +       int status;
> +       int i = 0;
> +
> +       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);

I wonder if we would feel comfortable just running this NUL-check as
part of unpack_sha1_header (i.e., in all code paths). It _shouldn't_
trigger in normal use, but I wonder if there would be any downsides
(e.g., maliciously crafted objects getting us to allocate memory or
something; I think it is fairly easy to convince git to allocate memory,
though).

> +       for (cp = buffer; cp < stream->next_out; cp++)
> +               if (!*cp) {
> +                       /* Found the NUL at the end of the header */
> +                       return 0;
> +               }

I think we can spell this as:

  if (memchr(buffer, '\0', stream->next_out - buffer))
	return 0;

which is shorter and possibly more efficient.

In theory we could also just start trying to parse the type/size header,
and notice there when we don't find the NUL. That's probably not worth
doing, though. The parsing is separated from the unpacking here, so it
would require combining those two operations in a single function. And
the extra NUL search here is likely not very expensive.

-Peff