Flaky SANITIZE=leak test "regression" in v2.36.0 (was: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames())

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Sun, Feb 20 2022, Elijah Newren via GitGitGadget wrote:

[I fat-fingered an empty E-Mail reply to this earlier in
https://lore.kernel.org/git/220601.86h7541yqj.gmgdl@xxxxxxxxxxxxxxxxxxx/,
sorry!]

> From: Elijah Newren <newren@xxxxxxxxx>
>
> detect_and_process_renames() detects renames on both sides of history
> and then combines these into a single diff_queue_struct.  The combined
> diff_queue_struct needs to be able to hold the renames found on either
> side, and since it knows the (maximum) size it needs, it pre-emptively
> grows the array to the appropriate size:
> [...]
> diff --git a/merge-ort.c b/merge-ort.c
> index d85b1cd99e9..3d7f9feb6f7 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,
>  				      struct tree *side1,
>  				      struct tree *side2)
>  {
> -	struct diff_queue_struct combined;
> +	struct diff_queue_struct combined = { 0 };
>  	struct rename_info *renames = &opt->priv->renames;
> -	int need_dir_renames, s, clean = 1;
> +	int need_dir_renames, s, i, clean = 1;
>  	unsigned detection_run = 0;
>  
> -	memset(&combined, 0, sizeof(combined));
>  	if (!possible_renames(renames))
>  		goto cleanup;
>  
> @@ -3175,13 +3174,9 @@ simple_cleanup:
>  		free(renames->pairs[s].queue);
>  		DIFF_QUEUE_CLEAR(&renames->pairs[s]);
>  	}
> -	if (combined.nr) {
> -		int i;
> -		for (i = 0; i < combined.nr; i++)
> -			pool_diff_free_filepair(&opt->priv->pool,
> -						combined.queue[i]);
> -		free(combined.queue);
> -	}
> +	for (i = 0; i < combined.nr; i++)
> +		pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);
> +	free(combined.queue);
>  
>  	return clean;
>  }

I haven't been able to find whether this actually causes anything bad,
but when I started digging into SANITIZE=leak failures I found that this
change made t6435-merge-sparse.sh flaky in the v2.36.0 release when
compiled with SANITIZE=leak.

I.e. it "should" fail, but with 8d60e9d2010 (merge-ort: fix small memory
leak in detect_and_process_renames(), 2022-02-20) it will sometimes
succeed.

I bisected it between 2.35.0..2.36.0 with:

	git bisect run sh -c 'make SANITIZE=leak && (cd t && ! (for i in $(seq 1 20); do ./t6435-merge-sparse.sh >/dev/null && echo $? || echo $?; done | grep -q 0))'

I.e. "fail if we succeed" (there's some redundancy in the ad-hoc
one-liner). Manually debugging it with:
	
	diff --git a/dir.c b/dir.c
	index d91295f2bcd..3e860769c26 100644
	--- a/dir.c
	+++ b/dir.c
	@@ -878,6 +878,8 @@ void add_pattern(const char *string, const char *base,
	 	unsigned flags;
	 	int nowildcardlen;
	 
	+	fprintf(stderr, "adding pattern %s\n", string);
	+
	 	parse_path_pattern(&string, &patternlen, &flags, &nowildcardlen);
	 	if (flags & PATTERN_FLAG_MUSTBEDIR) {
	 		FLEXPTR_ALLOC_MEM(pattern, pattern, string, patternlen);

Turns up this odd case, i.e. we "should" fail at the end of the "setup"
in this bit of test code:

	[...]
        test_commit_this ours &&
        git config core.sparseCheckout true &&
        echo "/checked-out" >.git/info/sparse-checkout &&
        git reset --hard &&
        test_must_fail git merge their

Here we leak in "git reset", so we "should" get a leak like:
	
	[...]	
	+ git tag ours
	+ git config core.sparseCheckout true
	+ echo /checked-out
	+ git reset --hard
	adding pattern /checked-out
	adding pattern /checked-out
	adding pattern /checked-out
	HEAD is now at 3dfe889 ours
	
	=================================================================
	==25385==ERROR: LeakSanitizer: detected memory leaks
	
	Indirect leak of 512 byte(s) in 1 object(s) allocated from:
	    #0 0x7f9652ef50aa in __interceptor_calloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:90
	    #1 0x6ab6dc in xcalloc /home/avar/g/git/wrapper.c:140
	    #2 0x58e5ac in alloc_table /home/avar/g/git/hashmap.c:79
	    #3 0x58e8e9 in hashmap_init /home/avar/g/git/hashmap.c:168
	    #4 0x569808 in add_patterns_from_buffer /home/avar/g/git/dir.c:1136
	    #5 0x5697a1 in add_patterns /home/avar/g/git/dir.c:1124
	    #6 0x56996a in add_patterns_from_file_to_list /home/avar/g/git/dir.c:1164
	    #7 0x56e0b2 in get_sparse_checkout_patterns /home/avar/g/git/dir.c:3273
	    #8 0x56a240 in init_sparse_checkout_patterns /home/avar/g/git/dir.c:1451
	[...]
	
That's consistent with what I see before, but sometimes it'll succeed
like this:

	[...]		
	+ git tag ours
	+ git config core.sparseCheckout true
	+ echo /checked-out
	+ git reset --hard
	adding pattern /checked-out   
	HEAD is now at 3dfe889 ours   
        [...]

I.e. for some reason the same "reset --hard" now adds one, not three
patterns (which before were all identical).

Now, the "flaky success" with SANITIZE=leak does appear to be new in
8d60e9d2010, but before that running this in a loop reveals that the 2nd
test sometimes succeeds, so perhaps the underlying "issue" isn't new.

I say "issue" because I haven't dug enough to see if this has any impact
on anything, and the failure I discovered doesn't per-se matter now.

But perhaps this observed indeterminism is pointing the way to some
deeper bug here? Or maybe it isn't...