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

Han Xin [off-list ref] writes:

quoted

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?

On Fri, Jun 10, 2022 at 2:27 AM Junio C Hamano [off-list ref] wrote:

Han Xin [off-list ref] writes:

quoted

quoted

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

On Fri, Jun 10 2022, Han Xin wrote:

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@gmail.com/
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.

On Fri, Jun 10, 2022 at 10:07 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Fri, Jun 10 2022, Han Xin wrote:

quoted

On Fri, Jun 10, 2022 at 2:27 AM Junio C Hamano [off-list ref] wrote:

quoted

Han Xin [off-list ref] writes:

quoted

quoted

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@gmail.com/
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