Re: [PATCH v3 3/6] unpack-objects: continue when fail to malloc due to large objects
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 14, 2014, 16:58 UTC
- Message-ID
- <xmqq7g2b2ele.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1407927454-9268-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 48 quoted lines
> As a recovery tool, unpack-objects should go on unpacking as many
> objects as it can.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/unpack-objects.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> t/t1050-large.sh | 7 +++++++
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 99cde45..8b5c67e 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -88,10 +88,50 @@ static void use(int bytes)
> consumed_bytes += bytes;
> }
>
> +static void inflate_and_throw_away(unsigned long size)
> +{
> + git_zstream stream;
> + char buf[8192];
> +
> + memset(&stream, 0, sizeof(stream));
> + stream.next_out = (unsigned char *)buf;
> + stream.avail_out = sizeof(buf);
> + stream.next_in = fill(1);
> + stream.avail_in = len;
> + git_inflate_init(&stream);
> +
> + for (;;) {
> + int ret = git_inflate(&stream, 0);
> + use(len - stream.avail_in);
> + if (stream.total_out == size && ret == Z_STREAM_END)
> + break;
> + if (ret != Z_OK) {
> + error("inflate returned %d", ret);
> + if (!recover)
> + exit(1);
> + has_errors = 1;
> + break;
> + }
> + stream.next_out = (unsigned char *)buf;
> + stream.avail_out = sizeof(buf);
> + stream.next_in = fill(1);
> + stream.avail_in = len;
> + }
> + git_inflate_end(&stream);
> +}This looks wrong in a few ways.
You already know that we saw an error when you get to this function, whether you will see an out-of-sync stream in this loop or not, so there is no reason to copy the assignment to has_errors from the other function. You also know 'recover' is true---otherwise you wouldn't be here.
But more importantly, the basic structure of this loop is the same as the loop we already have in the only caller of this new function, not just the regular "zlib produced this much that is not yet the expected size, go on reading more" and "we are at the end of the stream with Z_STREAM_END, and we are done", but even to "the stream is corrupt, we need to exit the loop", they are identical. Is a copy-and-paste like this the best we can do to add this "skip to the end of the current stream"? We would really want to keep the number of copies of this loop down; we saw a same bug introduced on the termination condition multiple times to different copies X-<.
Show 32 quoted lines
> static void *get_data(unsigned long size)
> {
> git_zstream stream;
> - void *buf = xmalloc(size);
> + void *buf = xmalloc_gentle(size);
> +
> + if (!buf) {
> + if (!recover)
> + exit(1);
> + has_errors = 1;
> + inflate_and_throw_away(size);
> + return NULL;
> + }
>
> memset(&stream, 0, sizeof(stream));
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index 5642f84..eec2cca 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -169,4 +169,11 @@ test_expect_success 'fsck' '
> test "$n" -gt 1
> '
>
> +test_expect_success 'unpack-objects' '
> + P=`ls .git/objects/pack/*.pack` &&
> + git unpack-objects -n -r <$P 2>err
> + test $? = 1 &&
> + grep "error: attempting to allocate .* over limit" err
> +'
> +
> test_done