Subject: Re: [PATCH 1/2] object-file: fix SEGV on free() regression in v2.34.0-rc2

From: Junio C Hamano <[email-masked]>
Date: Thu, 11 Nov 2021 10:41:46 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> Fix a regression introduced in my 96e41f58fe1 (fsck: report invalid
> object type-path combinations, 2021-10-01). When fsck-ing blobs larger
> than core.bigFileThreshold we'd free() a pointer to uninitialized
> memory.

s/d we'd/d, we'd/; no need to resend.

> This issue would have been caught by SANITIZE=address, but since it
> involves core.bigFileThreshold none of the existing tests in our test
> suite covered it.

s/d none/d, none/; likewise.

> Running them with the "big_file_threshold" in "environment.c" changed
> to say "6" would have shown this failure, but let's add a dedicated
> test for this scenario based on Han Xin's report[1].

Yeah, it is a good and focused test.  

By the way, I do not think changing big_file_threshold _blindly_ to
smaller values, instead of in a focused test like this, is a good
idea in general.  Some tests check if a file with a normal size that
is smaller than the threshold correctly is treated as a binary file,
and lowering threshold for them without understanding what they are
meant to test would trigger a "bug" that is not a bug at all, for
example.

> It would be a good follow-up change to add a GIT_TEST_* mode to run
> all the tests with a low core.bigFileThreshold threshold.

So, no, please don't do that.

>  object-file.c    | 2 ++
>  t/t1050-large.sh | 8 ++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/object-file.c b/object-file.c
> index 02b79702748..ac476653a06 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2528,6 +2528,8 @@ int read_loose_object(const char *path,
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long *size = oi->sizep;
>  
> +	*contents = NULL;
> +
>  	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
>  	if (!map) {
>  		error_errno(_("unable to mmap %s"), path);
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index 4bab6a513c5..6bc1d76fb10 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -17,6 +17,14 @@ test_expect_success setup '
>  	export GIT_ALLOC_LIMIT
>  '
>  
> +test_expect_success 'enter "large" codepath, with small core.bigFileThreshold' '
> +	test_when_finished "rm -rf repo" &&
> +
> +	git init --bare repo &&
> +	echo large | git -C repo hash-object -w --stdin &&
> +	git -C repo -c core.bigfilethreshold=4 fsck
> +'
> +
>  # add a large file with different settings
>  while read expect config
>  do