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

[karthik nayak <karthik.188@gmail.com>]

On 04/21/2015 12:21 AM, Jeff King wrote:
> 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).
>
But why would we want it to be a part of unpack_sha1_header?

>> +       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.
Noted. Thanks :)
>
> 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.
>
Yes, even I though about doing that, but wasn't keen on combining those 
two functions, they're meant to do two different things.
> -Peff
>