diff mbox series

This fixes a minor memory leak (detected by LeakSanitizer) in git merge.

Message ID pull.1577.git.1692389061490.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series This fixes a minor memory leak (detected by LeakSanitizer) in git merge. | expand

Commit Message

Kevin Backhouse Aug. 18, 2023, 8:04 p.m. UTC 
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
```

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

Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>
---
    This fixes a minor memory leak (detected by LeakSanitizer) in git merge
    
    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
    
    
    The fix is to call free_commit_list(merge_bases) when an error occurs.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1577%2Fkevinbackhouse%2Ffree-merge-bases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1577/kevinbackhouse/free-merge-bases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1577

 merge-ort-wrappers.c | 4 +++-
 merge-ort.c          | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)


base-commit: f9972720e9a405e4f6924a7cde0ed5880687f4d0

Comments

Junio C Hamano Aug. 18, 2023, 9:41 p.m. UTC | #1 
"Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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?

> 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.
Elijah Newren Sept. 12, 2023, 3:06 p.m. UTC | #2 
On Fri, Aug 18, 2023 at 2:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> > 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?
>
> > 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?

The other function would need a more involved fix, which would
basically involve porting a59b8dd94f (merge-ort: fix memory leak in
merge_ort_internal(), 2022-01-20) to merge-recursive as a preparatory
step.  This particular cleanup cannot be ported in its current form to
merge-recursive.c until then.
diff mbox series

Patch

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);
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;
+		}
 		opt->branch1 = saved_b1;
 		opt->branch2 = saved_b2;
 		opt->priv->call_depth--;