Re: [PATCH] packfile: Correct zlib buffer handling

Junio C Hamano [off-list ref] writes:

Duy Nguyen [off-list ref] writes:

quoted

On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano [off-list ref] wrote:

quoted

Duy Nguyen [off-list ref] writes:

quoted

On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton [off-list ref] wrote:

quoted

@@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
                return NULL;
        memset(&stream, 0, sizeof(stream));
        stream.next_out = buffer;
-       stream.avail_out = size + 1;
+       stream.avail_out = size;

You may want to include in your commit message a reference to
39eea7bdd9 (Fix incorrect error check while reading deflated pack data
- 2009-10-21) which adds this plus one with a fascinating story
behind.

A bit puzzled---are you saying that this recent patch breaks the old
fix and must be done in some other way?

No. I actually wanted to answer that question when I tried to track
down the commit that adds " + 1" but I did not spend enough time to
understand the old problem. I guess your puzzle means you didn't think
it would break anything, which is good.

No it merely means I am puzzled how the posted patch that goes
directly opposite to what an earlier "fix" did is a correct solution
to anything X-<.

Specifically, I was worried about this assertion:

    Lets rely on the fact that the source buffer will only be fully
    consumed when the when the destination buffer is inflated to the
    correct size.

which I think is the exact bad thinking that caused troubles for us
in the past; isn't the explanation in 456cdf6e ("Fix loose object
uncompression check.", 2007-03-19) relevant here?

-	stream.avail_out = size + 1;
+	stream.avail_out = size;
	...
 		stream.next_in = in;
 		st = git_inflate(&stream, Z_FINISH);
 		if (!stream.avail_out)
-			break; /* the payload is larger than it should be */
+			break; /* done, st indicates if source fully consumed */
 		curpos += stream.next_in - in;
 	} while (st == Z_OK || st == Z_BUF_ERROR);
 	git_inflate_end(&stream);
 	if ((st != Z_STREAM_END) || stream.total_out != size) {
 		free(buffer);
 		return NULL;
 	}

With minimum stream.avail_out without slack, when !avail_out, i.e.
when we fully filled the output buffer, it could be that we had
correct input that deflates to the correct size, in which case we
are happy---st would say Z_STREAM_END, we would leave the loop
because it is neither OK nor BUF_ERROR, and total_out would report
the size we expected.  Or the input zlib stream may have ended with
bytes that express "this concludes the stream", and the input bytes
before that was sufficient to construct the original payload fully,
and we may have just fed the bytes before that "this concludes the
stream" to git_inflate().

In such a case, we haven't consumed all the avail_in.  We may
already have all the correct output, i.e. !avail_out, but because we
haven't consumed the "this concludes the stream", st is not
STREAM_END in such a case.  

Our existing while() loop, with one-byte slack in avail_out, would
have let us continue and the next iteration of the loop would have
consumed the input without producing any more output (i.e. avail_out
would have been left to 1 in both of these final two rounds) and we
would have exited the loop.  After calling inflate_end(), we would
have noticed STREAM_END and correct size and we would have been
happy.

The updated code would handle this latter case rather badly, no?  We
leave the loop early, notice st is not STREAM_END, and be very
unhappy, because this patch did not give us to consume the very end
of the input stream and left the loop early.

quoted

This yields two problems, first a single byte overrun won't be detected
properly because the Z_STREAM_END will then be set, but the null
terminator will have been overwritten.

Because we compare total_out and size at the end, we would detect it
as an error in this function, no?  Then zlib overwriting NUL would
not be a problem, as we would free the buffer and return NULL, no?

quoted

The other problem is that
more recent zlib patches have been poisoning the unconsumed portions
of the buffers which also overwrites the null, while correctly
returning length and status.

Isn't that a bug in zlib, though?  Or do they do that deliberately?

I think a workaround with lower impact would be to manually restore
NUL at the end of the buffer.

Hi,

Sorry about the delay here (bit of a mix-up and didn't reply to the list).

(see inline )

On Sun, May 27, 2018 at 9:41 PM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Duy Nguyen [off-list ref] writes:

quoted

On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano [off-list ref] wrote:

(trimming)

Specifically, I was worried about this assertion:

    Lets rely on the fact that the source buffer will only be fully
    consumed when the when the destination buffer is inflated to the
    correct size.

which I think is the exact bad thinking that caused troubles for us
in the past; isn't the explanation in 456cdf6e ("Fix loose object
uncompression check.", 2007-03-19) relevant here?

-       stream.avail_out = size + 1;
+       stream.avail_out = size;
        ...
                stream.next_in = in;
                st = git_inflate(&stream, Z_FINISH);
                if (!stream.avail_out)
-                       break; /* the payload is larger than it should be */
+                       break; /* done, st indicates if source fully consumed */
                curpos += stream.next_in - in;
        } while (st == Z_OK || st == Z_BUF_ERROR);
        git_inflate_end(&stream);
        if ((st != Z_STREAM_END) || stream.total_out != size) {
                free(buffer);
                return NULL;
        }

With minimum stream.avail_out without slack, when !avail_out, i.e.
when we fully filled the output buffer, it could be that we had
correct input that deflates to the correct size, in which case we
are happy---st would say Z_STREAM_END, we would leave the loop
because it is neither OK nor BUF_ERROR, and total_out would report
the size we expected.  Or the input zlib stream may have ended with
bytes that express "this concludes the stream", and the input bytes
before that was sufficient to construct the original payload fully,
and we may have just fed the bytes before that "this concludes the
stream" to git_inflate().

In such a case, we haven't consumed all the avail_in.  We may
already have all the correct output, i.e. !avail_out, but because we
haven't consumed the "this concludes the stream", st is not
STREAM_END in such a case.

If I understand correctly your concerned the avail_in is longer than
what is required to fill the output buffer..

I'm fairly sure that won't result in a Z_STREAM_END, as you rightfully
point out, but the loop _will_ terminate due to the output buffer
being full and then since its not Z_STREAM_END the
unpack_compressed_entry fails, as it should.

Our existing while() loop, with one-byte slack in avail_out, would
have let us continue and the next iteration of the loop would have
consumed the input without producing any more output (i.e. avail_out
would have been left to 1 in both of these final two rounds) and we
would have exited the loop.  After calling inflate_end(), we would
have noticed STREAM_END and correct size and we would have been
happy.

Your assuming that zlib will terminate with an error, but a fully
decompressed buffer, because it hasn't consumed the entire input
buffer. I don't think that is how it works (its not how the
documentation is written, nor the bits of code i've looked at seem to
work, which granted i'm not a zlib maintainer).

The updated code would handle this latter case rather badly, no?  We
leave the loop early, notice st is not STREAM_END, and be very
unhappy, because this patch did not give us to consume the very end
of the input stream and left the loop early.

Your correct if the above case is a valid zlib behavior then there
would be a problem. But, I don't think the termination is dicated by
insufficient output space until there is an attempt to utilize that
space.

quoted

quoted

This yields two problems, first a single byte overrun won't be detected
properly because the Z_STREAM_END will then be set, but the null
terminator will have been overwritten.

Because we compare total_out and size at the end, we would detect it
as an error in this function, no?  Then zlib overwriting NUL would
not be a problem, as we would free the buffer and return NULL, no?

quoted

quoted

The other problem is that
more recent zlib patches have been poisoning the unconsumed portions
of the buffers which also overwrites the null, while correctly
returning length and status.

Isn't that a bug in zlib, though?  Or do they do that deliberately?

I think a workaround with lower impact would be to manually restore
NUL at the end of the buffer.

I agree, just resetting the NULL its likely safer, and I will repost a
patch soon which if nothing else makes git more robust to errant zlib
behavior.