Skip to content
  • Junio C Hamano's avatar
    Fix incorrect error check while reading deflated pack data · 39eea7bd
    Junio C Hamano authored
    The loop in get_size_from_delta() feeds a deflated delta data from the
    pack stream _until_ we get inflated result of 20 bytes[*] or we reach the
    end of stream.
    
        Side note. This magic number 20 does not have anything to do with the
        size of the hash we use, but comes from 1a3b55c6 (reduce delta head
        inflated size, 2006-10-18).
    
    The loop reads like this:
    
        do {
            in = use_pack();
            stream.next_in = in;
            st = git_inflate(&stream, Z_FINISH);
            curpos += stream.next_in - in;
        } while ((st == Z_OK || st == Z_BUF_ERROR) &&
                 stream.total_out < sizeof(delta_head));
    
    This git_inflate() can return:
    
     - Z_STREAM_END, if use_pack() fed it enough input and the delta itself
       was smaller than 20 bytes;
    
     - Z_OK, when some progress has been made;
    
     - Z_BUF_ERROR, if no progress is possible, because we either ran out of
       input (due to corrupt pack), or we ran out of output before we saw the
       end of the stream.
    
    The fix b3118bdc (sha1_file: Fix infinite loop when pack is corrupted,
    2009-10-14) attempted was against a corruption that appears to be a valid
    stream that produces a result larger than the output buffer, but we are
    not even trying to read the stream to the end in this loop.  If avail_out
    becomes zero, total_out will be the same as sizeof(delta_head) so the loop
    will terminate without the "fix".  There is no fix from b3118bdc needed for
    this loop, in other words.
    
    The loop in unpack_compressed_entry() is quite a different story.  It
    feeds a deflated stream (either delta or base) and allows the stream to
    produce output up to what we expect but no more.
    
        do {
            in = use_pack();
            stream.next_in = in;
            st = git_inflate(&stream, Z_FINISH);
            curpos += stream.next_in - in;
        } while (st == Z_OK || st == Z_BUF_ERROR)
    
    This _does_ risk falling into an endless interation, as we can exhaust
    avail_out if the length we expect is smaller than what the stream wants to
    produce (due to pack corruption).  In such a case, avail_out will become
    zero and inflate() will return Z_BUF_ERROR, while avail_in may (or may
    not) be zero.
    
    But this is not a right fix:
    
        do {
            in = use_pack();
            stream.next_in = in;
            st = git_inflate(&stream, Z_FINISH);
    +       if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out)
    +               break; /* wants more input??? */
            curpos += stream.next_in - in;
        } while (st == Z_OK || st == Z_BUF_ERROR)
    
    as Z_BUF_ERROR from inflate() may be telling us that avail_in has also run
    out before reading the end of stream marker.  In such a case, both avail_in
    and avail_out would be zero, and the loop should iterate to allow the end
    of stream marker to be seen by inflate from the input stream.
    
    The right fix for this loop is likely to be to increment the initial
    avail_out by one (we allocate one extra byte to terminate it with NUL
    anyway, so there is no risk to overrun the buffer), and break out if we
    see that avail_out has become zero, in order to detect that the stream
    wants to produce more than what we expect.  After the loop, we have a
    check that exactly tests this condition:
    
        if ((st != Z_STREAM_END) || stream.total_out != size) {
            free(buffer);
            return NULL;
        }
    
    So here is a patch (without my previous botched attempts) to fix this
    issue.  The first hunk reverts the corresponding hunk from b3118bdc
    
    , and
    the second hunk is the same fix proposed earlier.
    
    Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
    39eea7bd