Re: [PATCH v2 2/4] fsck: do not die when not enough memory to examine a pack entry

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2014, 18:09 UTC
Message-ID: <xmqqa98z5yrg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1403610336-27761-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> fsck is a tool that error() is more preferred than die(), but many

"more preferred" without justifying why it is "more preferred" is not quite a justification, is it? Also, an object failing to load in-core is not a missing object, so if your aim is to let "fsck" diagnose a too-large-to-load object as missing and let it continue, I do not know if it is "more preferred" in the first place. Adding a "too large--cannot check" bin of objects may be needed for it to be useful. Also, we might need to give at the end "oh by the way, because we couldn't read some objects to even determine its type, the unreachable report from this fsck run is totally useless."

The log message tries to justify that this may be a good thing for "fsck", but the patch actually tries to change the behaviour of all code paths that try to load an object in-core without considering the ramifications of such a change. I _think_ all callers should be prepared to receive NULL when we encounter a corrupt object (and otherwise we should fix them), but it is unclear how much audit of the callers (if any) was done to prepare this change.

Not very excited X-<.

Show 42 quoted lines

> functions embed die() inside beyond fsck's control.
> unpack_compressed_entry()'s using xmallocz is such a function,
> triggered from verify_packfile() -> unpack_entry(). Make it use
> xmallocz_gentle() instead.
>
> Noticed-by: Dale R. Worley <worley@alum.mit.edu>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  sha1_file.c      | 4 +++-
>  t/t1050-large.sh | 6 ++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 34d527f..eb69c78 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1925,7 +1925,9 @@ static void *unpack_compressed_entry(struct packed_git *p,
>  	git_zstream stream;
>  	unsigned char *buffer, *in;
>  
> -	buffer = xmallocz(size);
> +	buffer = xmallocz_gentle(size);
> +	if (!buffer)
> +		return NULL;
>  	memset(&stream, 0, sizeof(stream));
>  	stream.next_out = buffer;
>  	stream.avail_out = size + 1;
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index aea4936..5642f84 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -163,4 +163,10 @@ test_expect_success 'zip achiving, deflate' '
>  	git archive --format=zip HEAD >/dev/null
>  '
>  
> +test_expect_success 'fsck' '
> +	test_must_fail git fsck 2>err &&
> +	n=$(grep "error: attempting to allocate .* over limit" err | wc -l) &&
> +	test "$n" -gt 1
> +'
> +
>  test_done