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

> 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.
>