Skip to content

Commit

Permalink
merge-ort: fix small memory leak in unique_path()
Browse files Browse the repository at this point in the history
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 6697ee0 (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>
  • Loading branch information
newren committed Feb 19, 2022
1 parent f0308de commit 73bc1e5
Showing 1 changed file with 12 additions and 9 deletions.
21 changes: 12 additions & 9 deletions merge-ort.c
Original file line number Diff line number Diff line change
Expand Up @@ -722,13 +722,15 @@ static void add_flattened_path(struct strbuf *out, const char *s)
out->buf[i] = '_';
}

static char *unique_path(struct strmap *existing_paths,
static char *unique_path(struct merge_options *opt,
const char *path,
const char *branch)
{
char *ret = NULL;
struct strbuf newpath = STRBUF_INIT;
int suffix = 0;
size_t base_len;
struct strmap *existing_paths = &opt->priv->paths;

strbuf_addf(&newpath, "%s~", path);
add_flattened_path(&newpath, branch);
Expand All @@ -739,7 +741,11 @@ static char *unique_path(struct strmap *existing_paths,
strbuf_addf(&newpath, "_%d", suffix++);
}

return strbuf_detach(&newpath, NULL);
/* Track the new path in our memory pool */
ret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);
memcpy(ret, newpath.buf, newpath.len + 1);
strbuf_release(&newpath);
return ret;
}

/*** Function Grouping: functions related to collect_merge_info() ***/
Expand Down Expand Up @@ -3679,7 +3685,7 @@ static void process_entry(struct merge_options *opt,
*/
df_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;
branch = (df_file_index == 1) ? opt->branch1 : opt->branch2;
path = unique_path(&opt->priv->paths, path, branch);
path = unique_path(opt, path, branch);
strmap_put(&opt->priv->paths, path, new_ci);

path_msg(opt, path, 0,
Expand Down Expand Up @@ -3804,14 +3810,12 @@ static void process_entry(struct merge_options *opt,
/* Insert entries into opt->priv_paths */
assert(rename_a || rename_b);
if (rename_a) {
a_path = unique_path(&opt->priv->paths,
path, opt->branch1);
a_path = unique_path(opt, path, opt->branch1);
strmap_put(&opt->priv->paths, a_path, ci);
}

if (rename_b)
b_path = unique_path(&opt->priv->paths,
path, opt->branch2);
b_path = unique_path(opt, path, opt->branch2);
else
b_path = path;
strmap_put(&opt->priv->paths, b_path, new_ci);
Expand Down Expand Up @@ -4199,15 +4203,14 @@ static int record_conflicted_index_entries(struct merge_options *opt)
struct stat st;

if (!lstat(path, &st)) {
char *new_name = unique_path(&opt->priv->paths,
char *new_name = unique_path(opt,
path,
"cruft");

path_msg(opt, path, 1,
_("Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s"),
path, new_name);
errs |= rename(path, new_name);
free(new_name);
}
errs |= checkout_entry(ce, &state, NULL, NULL);
}
Expand Down

0 comments on commit 73bc1e5

Please sign in to comment.