Re: [PATCH] This fixes a minor memory leak (detected by LeakSanitizer) in git merge.

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2023, 21:41 UTC
Message-ID: <xmqqo7j4t4dm.fsf@gitster.g>
In-Reply-To: <pull.1577.git.1692389061490.gitgitgadget@gmail.com>

"Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 23 quoted lines

> From: Kevin Backhouse <kevinbackhouse@github.com>
>
> To reproduce (with an ASAN build):
>
> ```
> mkdir test
> cd test
> git init
> echo x > x.txt
> git add .
> git commit -m "WIP"
> git checkout -b dev
> echo y > x.txt
> git add .
> git commit -m "WIP"
> git checkout main
> echo z > x.txt
> git add .
> git commit -m "WIP"
> echo a > x.txt
> git add .
> git merge dev
> ```

We'd rather not to see the above in the proposed log message; can't we add (a variation of) it to our test suite?

> The fix is to call free_commit_list(merge_bases) when an error occurs.

We usually have the description of what the problem is and give an analysis on why/how it happens, before presenting a solution. Write it more like:

    The caller of merge_ort_recursive() expects the commit list
    passed in as the merge_bases parameter to be fully consumed by
    the function and does not free it when the function returns.  In
    normal cases, the commit list does get consumed, but when the
    function returns early upon encountering an error, it forgets to
    clean it up.

    Fix this by freeing the list in the code paths for error returns.

>  merge-ort-wrappers.c | 4 +++-
>  merge-ort.c          | 4 +++-

These two places and their fixes seem OK, but I have to wonder if these are complete fixes.

Show 16 quoted lines

> diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
> index 4acedf3c338..aeb56c9970c 100644
> --- a/merge-ort-wrappers.c
> +++ b/merge-ort-wrappers.c
> @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,
>  	struct tree *head = repo_get_commit_tree(opt->repo, side1);
>  	struct merge_result tmp;
>  
> -	if (unclean(opt, head))
> +	if (unclean(opt, head)) {
> +		free_commit_list(merge_bases);
>  		return -1;
> +	}
>  
>  	memset(&tmp, 0, sizeof(tmp));
>  	merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);

The function before this hunk appears to have very similar code structure. Does it need the same fix, or if not why not?

Show 13 quoted lines

> diff --git a/merge-ort.c b/merge-ort.c
> index 8631c997002..a0eb91fb011 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,
>  		opt->branch1 = "Temporary merge branch 1";
>  		opt->branch2 = "Temporary merge branch 2";
>  		merge_ort_internal(opt, NULL, prev, next, result);
> -		if (result->clean < 0)
> +		if (result->clean < 0) {
> +			free_commit_list(merge_bases);
>  			return;
> +		}

Before this function, there is a comment that this came from another function and it seems to still have a very similar code structure. Does the other function need the same fix, or if not why not?

Thanks.