Re: [PATCH 1/2] object-file: fix SEGV on free() regression in v2.34.0-rc2
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 11, 2021, 18:41 UTC
- Message-ID
- <xmqqtugilel1.fsf@gitster.g>
- In-Reply-To
- <patch-1.2-811242178e4-20211111T051800Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
Show 36 quoted lines
> 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