From 8d60e9d2010d34f8c8ca65967ed02a2b06d74dc5 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sun, 20 Feb 2022 01:29:50 +0000 Subject: [PATCH 1/2] merge-ort: fix small memory leak in detect_and_process_renames() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, just remove the if-check; free() knows to skip NULL pointers. This change 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 Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index d85b1cd99e..3d7f9feb6f 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -3086,12 +3086,11 @@ 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; + int need_dir_renames, s, i, clean = 1; unsigned detection_run = 0; - memset(&combined, 0, sizeof(combined)); if (!possible_renames(renames)) goto cleanup; @@ -3175,13 +3174,9 @@ static int detect_and_process_renames(struct merge_options *opt, free(renames->pairs[s].queue); DIFF_QUEUE_CLEAR(&renames->pairs[s]); } - if (combined.nr) { - 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; } From 81afc7941294cec828daaff86c040b1edf099f25 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sun, 20 Feb 2022 01:29:51 +0000 Subject: [PATCH 2/2] merge-ort: fix small memory leak in unique_path() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 3d7f9feb6f..71134d5150 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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); @@ -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() ***/ @@ -3674,7 +3680,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, @@ -3799,14 +3805,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); @@ -4194,7 +4198,7 @@ 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"); @@ -4202,7 +4206,6 @@ static int record_conflicted_index_entries(struct merge_options *opt) _("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); }