On Fri, Jun 10, 2022 at 2:27 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Han Xin <chiyutianyi@xxxxxxxxx> writes:
>
> >> I am not sure if this is not loosening the error checking in the
> >> dry-run case, though. In the original code, we set the avail_out
> >> to the total expected size so
> >>
> >> (1) if the caller gives too small a size, git_inflate() would stop
> >> at stream.total_out with ret that is not STREAM_END nor OK,
> >> bypassing the "break", and we catch the error.
> >>
> >> (2) if the caller gives too large a size, git_inflate() would stop
> >> at the true size of inflated zstream, with STREAM_END and would
> >> not hit this "break", and we catch the error.
> >>
> >> With the new code, since we keep refreshing avail_out (see below),
> >> git_inflate() does not even learn how many bytes we are _expecting_
> >> to see. Is the error checking in the loop, with the updated code,
> >> catch the mismatch between expected and actual size (plausibly
> >> caused by a corrupted zstream) the same way as we do in the
> >> non dry-run code path?
> >>
> >
> > Unlike the original implementation, if we get a corrupted zstream, we
> > won't break at Z_BUFFER_ERROR, maybe until we've read all the
> > input. I think it can still catch the mismatch between expected and
> > actual size when "fill(1)" gets an EOF, if it's not too late.
>
> That is only one half of the two possible failure cases, i.e. input
> is shorter than the expected size. If the caller specified size is
> smaller than what the stream inflates to, I do not see the new code
> to be limiting the .avail_out near the end of the iteration, which
> would be necessary to catch such an error, even if we are not
> interested in using the inflated contents, no?
>
Yes, you are right.
Instead of always using a fixed "bufsize" even if there is not enough
expected output remaining, we can get a more accurate one by comparing
"total_out" to "size", so we can catch problems early by getting
Z_BUFFER_ERROR.
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 64abba8dba..5d59144883 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -139,7 +139,8 @@ static void *get_data(unsigned long size)
if (dry_run) {
/* reuse the buffer in dry_run mode */
stream.next_out = buf;
- stream.avail_out = bufsize;
+ stream.avail_out = bufsize > size - stream.total_out ?
+ size - stream.total_out : bufsize;
}
}
git_inflate_end(&stream);
Thanks
-Han Xin
