@matheusbsbs / matheus-bagliano / commits / 81afc79412

merge-ort: fix small memory leak in unique_path()

The struct strmap paths member of merge_options_internal is perhaps the most central data structure to all of merge-ort. Because all the paths involved in the merge need to be kept until the merge is complete, this "paths" data structure traditionally took responsibility for owning all the allocated paths. When the merge is over, those paths were free()d as part of free()ing this strmap. In commit 6697ee01b5d3 (merge-ort: switch our strmaps over to using memory pools, 2021-07-30), we changed the allocations for pathnames to come from a memory pool. That meant the ownership changed slightly; there were no individual free() calls to make, instead the memory pool owned all those paths and they were free()d all at once. Unfortunately unique_path() was written presuming the pre-memory-pool model, and allocated a path on the heap and left it in the strmap for later free()ing. Modify it to return a path allocated from the memory pool instead. Note that there's one instance -- in record_conflicted_index_entries() -- where the returned string from unique_path() was only used very temporarily and thus had been immediately free()'d. This codepath was associated with an ugly skip-worktree workaround that has since been better fixed by the in-flight en/present-despite-skipped topic. This workaround probably makes sense to excise once that topic merges down, but for now, just remove the immediate free() and allow the returned string to be free()d when the memory pool is released. This fixes the following memory leak as reported by valgrind: ==PID== 65 bytes in 1 blocks are definitely lost in loss record 79 of 134 ==PID== at 0xADDRESS: malloc ==PID== by 0xADDRESS: realloc ==PID== by 0xADDRESS: xrealloc (wrapper.c:126) ==PID== by 0xADDRESS: strbuf_grow (strbuf.c:98) ==PID== by 0xADDRESS: strbuf_vaddf (strbuf.c:394) ==PID== by 0xADDRESS: strbuf_addf (strbuf.c:335) ==PID== by 0xADDRESS: unique_path (merge-ort.c:733) ==PID== by 0xADDRESS: process_entry (merge-ort.c:3678) ==PID== by 0xADDRESS: process_entries (merge-ort.c:4037) ==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4621) ==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) Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Feb 20, 2022 at 01:29 UTC 81afc7941294cec828daaff86c040b1edf099f25

1 file changed +12 -9

merge-ort.c

+12 -9

@@ -722,13 +722,15 @@ static void add_flattened_path(struct strbuf *out, const char *s)

722 out->buf[i] = '_';

723 }

724

725 -static char *unique_path(struct strmap *existing_paths,

725 +static char *unique_path(struct merge_options *opt,

726 const char *path,

727 const char *branch)

728 {

729 + char *ret = NULL;

730 struct strbuf newpath = STRBUF_INIT;

731 int suffix = 0;

732 size_t base_len;

733 + struct strmap *existing_paths = &opt->priv->paths;

734

735 strbuf_addf(&newpath, "%s~", path);

736 add_flattened_path(&newpath, branch);

@@ -739,7 +741,11 @@ static char *unique_path(struct strmap *existing_paths,

741 strbuf_addf(&newpath, "_%d", suffix++);

742 }

743

742 - return strbuf_detach(&newpath, NULL);

744 + /* Track the new path in our memory pool */

745 + ret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);

746 + memcpy(ret, newpath.buf, newpath.len + 1);

747 + strbuf_release(&newpath);

748 + return ret;

749 }

750

751 /*** Function Grouping: functions related to collect_merge_info() ***/

@@ -3674,7 +3680,7 @@ static void process_entry(struct merge_options *opt,

3680 */

3681 df_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;

3682 branch = (df_file_index == 1) ? opt->branch1 : opt->branch2;

3677 - path = unique_path(&opt->priv->paths, path, branch);

3683 + path = unique_path(opt, path, branch);

3684 strmap_put(&opt->priv->paths, path, new_ci);

3685

3686 path_msg(opt, path, 0,

@@ -3799,14 +3805,12 @@ static void process_entry(struct merge_options *opt,

3805 /* Insert entries into opt->priv_paths */

3806 assert(rename_a || rename_b);

3807 if (rename_a) {

3802 - a_path = unique_path(&opt->priv->paths,

3803 - path, opt->branch1);

3808 + a_path = unique_path(opt, path, opt->branch1);

3809 strmap_put(&opt->priv->paths, a_path, ci);

3810 }

3811

3812 if (rename_b)

3808 - b_path = unique_path(&opt->priv->paths,

3809 - path, opt->branch2);

3813 + b_path = unique_path(opt, path, opt->branch2);

3814 else

3815 b_path = path;

3816 strmap_put(&opt->priv->paths, b_path, new_ci);

@@ -4194,7 +4198,7 @@ static int record_conflicted_index_entries(struct merge_options *opt)

4198 struct stat st;

4199

4200 if (!lstat(path, &st)) {

4197 - char *new_name = unique_path(&opt->priv->paths,

4201 + char *new_name = unique_path(opt,

4202 path,

4203 "cruft");

4204

@@ -4202,7 +4206,6 @@ static int record_conflicted_index_entries(struct merge_options *opt)

4206 _("Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s"),

4207 path, new_name);

4208 errs |= rename(path, new_name);

4205 - free(new_name);

4209 }

4210 errs |= checkout_entry(ce, &state, NULL, NULL);

4211 }