diff mbox series

[1/2] merge-ort: fix small memory leak in detect_and_process_renames()

Message ID f0308de28e49fb9bb1239fdcbc839097f5afa62a.1645290601.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix a couple small leaks in merge-ort | expand

Commit Message

Elijah Newren Feb. 19, 2022, 5:09 p.m. UTC 
From: Elijah Newren <newren@gmail.com>

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:

	ALLOC_GROW(combined.queue,
		   renames->pairs[1].nr + renames->pairs[2].nr,
		   combined.alloc);

It then collects the items from each side:

	collect_renames(opt, &combined, MERGE_SIDE1, ...)
	collect_renames(opt, &combined, MERGE_SIDE2, ...)

Note, though, that collect_renames() sometimes determines that some
pairs are unnecessary and does not include them in the combined array.
When it is done, detect_and_process_renames() frees this memory:

	if (combined.nr) {
                ...
		free(combined.queue);
        }

The problem is that sometimes even when there are pairs, none of them are
necessary.  Instead of checking combined.nr, we should check
combined.alloc.  Doing so fixes the following memory leak, as reported
by valgrind:

==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134
==PID==    at 0xADDRESS: malloc
==PID==    by 0xADDRESS: realloc
==PID==    by 0xADDRESS: xrealloc (wrapper.c:126)
==PID==    by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)
==PID==    by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)
==PID==    by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)
==PID==    by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)
==PID==    by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)
==PID==    by 0xADDRESS: try_merge_strategy (merge.c:753)
==PID==    by 0xADDRESS: cmd_merge (merge.c:1676)
==PID==    by 0xADDRESS: run_builtin (git.c:461)
==PID==    by 0xADDRESS: handle_builtin (git.c:713)
==PID==    by 0xADDRESS: run_argv (git.c:780)
==PID==    by 0xADDRESS: cmd_main (git.c:911)
==PID==    by 0xADDRESS: main (common-main.c:52)

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 19, 2022, 9:44 p.m. UTC | #1 
On Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> 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:
>
> 	ALLOC_GROW(combined.queue,
> 		   renames->pairs[1].nr + renames->pairs[2].nr,
> 		   combined.alloc);
>
> It then collects the items from each side:
>
> 	collect_renames(opt, &combined, MERGE_SIDE1, ...)
> 	collect_renames(opt, &combined, MERGE_SIDE2, ...)
>
> Note, though, that collect_renames() sometimes determines that some
> pairs are unnecessary and does not include them in the combined array.
> When it is done, detect_and_process_renames() frees this memory:
>
> 	if (combined.nr) {
>                 ...
> 		free(combined.queue);
>         }
>
> The problem is that sometimes even when there are pairs, none of them are
> necessary.  Instead of checking combined.nr, we should check
> combined.alloc.  Doing so fixes the following memory leak, as reported
> by valgrind:
>
> ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134
> ==PID==    at 0xADDRESS: malloc
> ==PID==    by 0xADDRESS: realloc
> ==PID==    by 0xADDRESS: xrealloc (wrapper.c:126)
> ==PID==    by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)
> ==PID==    by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)
> ==PID==    by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)
> ==PID==    by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)
> ==PID==    by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)
> ==PID==    by 0xADDRESS: try_merge_strategy (merge.c:753)
> ==PID==    by 0xADDRESS: cmd_merge (merge.c:1676)
> ==PID==    by 0xADDRESS: run_builtin (git.c:461)
> ==PID==    by 0xADDRESS: handle_builtin (git.c:713)
> ==PID==    by 0xADDRESS: run_argv (git.c:780)
> ==PID==    by 0xADDRESS: cmd_main (git.c:911)
> ==PID==    by 0xADDRESS: main (common-main.c:52)
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index d85b1cd99e9..4f5abc558c5 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3175,7 +3175,7 @@ simple_cleanup:
>  		free(renames->pairs[s].queue);
>  		DIFF_QUEUE_CLEAR(&renames->pairs[s]);
>  	}
> -	if (combined.nr) {
> +	if (combined.alloc) {
>  		int i;
>  		for (i = 0; i < combined.nr; i++)
>  			pool_diff_free_filepair(&opt->priv->pool,

This looks correct in that it'll work, but I still don't get why the
pre-image or post-image is going through this indirection of checking
"alloc" at all. Wouldn't this be correct & more straightforward (the
memset part is optional, just did it for good measure)?:

diff --git a/merge-ort.c b/merge-ort.c
index 40ae4dc4e92..a01f28586a1 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3092,12 +3092,12 @@ 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;
 	unsigned detection_run = 0;
+	int i;
 
-	memset(&combined, 0, sizeof(combined));
 	if (!possible_renames(renames))
 		goto cleanup;
 
@@ -3181,13 +3181,10 @@ static int detect_and_process_renames(struct merge_options *opt,
 		free(renames->pairs[s].queue);
 		DIFF_QUEUE_CLEAR(&renames->pairs[s]);
 	}
-	if (combined.alloc) {
-		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;
 }
Elijah Newren Feb. 20, 2022, 12:26 a.m. UTC | #2 
On Sat, Feb 19, 2022 at 1:45 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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:
> >
> >       ALLOC_GROW(combined.queue,
> >                  renames->pairs[1].nr + renames->pairs[2].nr,
> >                  combined.alloc);
> >
> > It then collects the items from each side:
> >
> >       collect_renames(opt, &combined, MERGE_SIDE1, ...)
> >       collect_renames(opt, &combined, MERGE_SIDE2, ...)
> >
> > Note, though, that collect_renames() sometimes determines that some
> > pairs are unnecessary and does not include them in the combined array.
> > When it is done, detect_and_process_renames() frees this memory:
> >
> >       if (combined.nr) {
> >                 ...
> >               free(combined.queue);
> >         }
> >
> > The problem is that sometimes even when there are pairs, none of them are
> > necessary.  Instead of checking combined.nr, we should check
> > combined.alloc.  Doing so fixes the following memory leak, as reported
> > by valgrind:
> >
> > ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134
> > ==PID==    at 0xADDRESS: malloc
> > ==PID==    by 0xADDRESS: realloc
> > ==PID==    by 0xADDRESS: xrealloc (wrapper.c:126)
> > ==PID==    by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)
> > ==PID==    by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)
> > ==PID==    by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)
> > ==PID==    by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)
> > ==PID==    by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)
> > ==PID==    by 0xADDRESS: try_merge_strategy (merge.c:753)
> > ==PID==    by 0xADDRESS: cmd_merge (merge.c:1676)
> > ==PID==    by 0xADDRESS: run_builtin (git.c:461)
> > ==PID==    by 0xADDRESS: handle_builtin (git.c:713)
> > ==PID==    by 0xADDRESS: run_argv (git.c:780)
> > ==PID==    by 0xADDRESS: cmd_main (git.c:911)
> > ==PID==    by 0xADDRESS: main (common-main.c:52)
> >
> > Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  merge-ort.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index d85b1cd99e9..4f5abc558c5 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -3175,7 +3175,7 @@ simple_cleanup:
> >               free(renames->pairs[s].queue);
> >               DIFF_QUEUE_CLEAR(&renames->pairs[s]);
> >       }
> > -     if (combined.nr) {
> > +     if (combined.alloc) {
> >               int i;
> >               for (i = 0; i < combined.nr; i++)
> >                       pool_diff_free_filepair(&opt->priv->pool,
>
> This looks correct in that it'll work, but I still don't get why the
> pre-image or post-image is going through this indirection of checking
> "alloc" at all. Wouldn't this be correct & more straightforward (the
> memset part is optional, just did it for good measure)?:
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 40ae4dc4e92..a01f28586a1 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3092,12 +3092,12 @@ 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;
>         unsigned detection_run = 0;
> +       int i;
>
> -       memset(&combined, 0, sizeof(combined));
>         if (!possible_renames(renames))
>                 goto cleanup;
>
> @@ -3181,13 +3181,10 @@ static int detect_and_process_renames(struct merge_options *opt,
>                 free(renames->pairs[s].queue);
>                 DIFF_QUEUE_CLEAR(&renames->pairs[s]);
>         }
> -       if (combined.alloc) {
> -               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;
>  }

Hmm, good point; I like your version better.  I'll change it, thanks.
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index d85b1cd99e9..4f5abc558c5 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3175,7 +3175,7 @@  simple_cleanup:
 		free(renames->pairs[s].queue);
 		DIFF_QUEUE_CLEAR(&renames->pairs[s]);
 	}
-	if (combined.nr) {
+	if (combined.alloc) {
 		int i;
 		for (i = 0; i < combined.nr; i++)
 			pool_diff_free_filepair(&opt->priv->pool,