Re: [PATCH 2/3] check_stream_sha1(): handle input underflow

From: Jeff King <peff@peff.net>
Date: Oct 31, 2018, 04:30 UTC
Message-ID: <20181031043051.GA5601@sigill.intra.peff.net>
In-Reply-To: <xmqqpnvqyc9x.fsf@gitster-ct.c.googlers.com>

On Wed, Oct 31, 2018 at 01:23:54PM +0900, Junio C Hamano wrote:

Show 22 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The bug comes from commit f6371f9210 (sha1_file: add
> > read_loose_object() function, 2017-01-13), which
> > reimplemented some of the existing loose object functions.
> > So it's worth checking if this bug was inherited from any of
> > those. The answers seems to be no. The two obvious
> > candidates are both OK:
> >
> >   1. unpack_sha1_rest(); this doesn't need to loop on
> >      Z_BUF_ERROR at all, since it allocates the expected
> >      output buffer in advance (which we can't do since we're
> >      explicitly streaming here)
> >
> >   2. check_object_signature(); the streaming path relies on
> >      the istream interface, which uses read_istream_loose()
> >      for this case. That function uses a similar "is our
> >      output buffer full" check with Z_BUF_ERROR (which is
> >      where I stole it from for this patch!)
> 
> See 692f0bc7 to find who did the fix you stole from, and for what
> kind of breakage the original fix was made.

Heh. I did not dig into it, but actually thought "I'll bet Junio had to get this right when he wrote the streaming code. No wonder he spotted my mistake so quickly!".

> By the way, a very similar loop for pack_non_delta istream iterates
> while total_read is smaller than sz, but it does not have the same
> check upon BUF_ERROR to see if we've read everything.

Indeed. Did you find that one by inspection, or did you peek at:

  https://public-inbox.org/git/20130325202114.GD16019@sigill.intra.peff.net/

? :)

-Peff