Re: [PATCH v13 1/7] unpack-objects: low memory footprint for get_data() in dry_run mode

> On Fri, Jun 10, 2022 at 2:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Han Xin <chiyutianyi@gmail.com> 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