On Fri, Jun 10, 2022 at 10:07 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
>
> 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.
OK, I am glad to do that.
Thank you very much.
-Han Xin
