On Fri, Jun 10 2022, Han Xin wrote:
> 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
Han, do you want to pick this up again for a v14? It looks like you're
very on top of it already, and I re-sent your patches because I saw that
your
https://lore.kernel.org/git/cover.1653015534.git.chiyutianyi@xxxxxxxxx/
wasn't picked up in the interim & you hadn't been active on-list
otherwise.
But it looks like there's some interest now, and that you have more time
to test & follow-up on this topic than I do at the moment, so if you
wanted to do the work of properly rebasing ot in tho recent fsync
changes that would be great. Thanks.
