Fix some old memory leaks in merge-ort and builtin/merge #1200
Conversation
|
There are issues in commit a341d7a: |
|
There are issues in commit 0f0cf35: |
newren
changed the title
The documentation for merge_incore_recursive(), modelled after
merge_recursive(), notes that
merge_bases will be consumed (emptied) so make a copy if you need it
However, in merge_ort_internal() (which merge_incore_recursive() calls),
it runs
merged_merge_bases = pop_commit(&merge_bases);
...
for (iter = merge_bases; iter; iter = iter->next) {
...
}
In other words, it only consumes the *first* entry of merge_bases, and
the rest it iterates through. If it iterated through all of them, the
caller could be responsible for free'ing the memory. If it consumed all
of them, the current documentation would be correct and the callers
would need to do nothing. The current middle ground makes it impossible
for callers to avoid memory leaks, since any attempt to use the
merge_bases it passes in would result in a use-after-free.
It turns out this part of the code was copied from merge-recursive.c,
which has had the same bug for 15.5 years. However, since we are trying
to keep merge-recursive.c stable as we sunset it, let's just fix the
leak in in merge_ort_internal() by having it actually consume all the
elements of the merge_bases commit_list.
Testing this commit against t6404 (the first testcase specifically
about recursive merges) under valgrind shows that this patch fixes
the following leak:
32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost \
in loss record 49 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x47EC86: try_merge_strategy (merge.c:751)
by 0x48143B: cmd_merge (merge.c:1679)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
by 0x4E7DFA: main (common-main.c:56)
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
There were two commit_lists created in cmd_merge() that were only
conditionally free()'d. Add a quick conditional call to
free_commit_list() for each of them at the end of the function.
Testing this commit against t6404 under valgrind shows that this patch
fixes the following two leaks:
16 bytes in 1 blocks are definitely lost in loss record 16 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x47FC93: reduce_parents (merge.c:1114)
by 0x4801EE: collect_parents (merge.c:1214)
by 0x480B56: cmd_merge (merge.c:1465)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
by 0x4E7DFA: main (common-main.c:56)
8 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost in \
loss record 61 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x52A8F2: commit_list_insert_by_date (commit.c:620)
by 0x5270AC: get_merge_bases_many_0 (commit-reach.c:413)
by 0x52716C: repo_get_merge_bases (commit-reach.c:438)
by 0x480E5A: cmd_merge (merge.c:1520)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
There are still 3 leaks in chdir_notify_register() after this, but
chdir_notify_register() has been brought up on the list before and folks
were not a fan of fixing those, so I'm not touching them.
Signed-off-by: Elijah Newren <newren@gmail.com>
|
/submit |
|
Submitted as pull.1200.git.git.1642664835.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -1273,7 +1273,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) | |||
| int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> done:
> + if (!automerge_was_ok) {
> + free_commit_list(common);
> + free_commit_list(remoteheads);
> + }
I wondered what happens upon a successful automerge.
We call finish_automerge() and come here, and I do see a call to
free_commit_list(common) in finish_automerge(), but the way
remoteheads is used looked a bit iffy.
In finish_automerge(), we use a temporary variable parents to point
remoteheads at it, and conditionally prepend the current HEAD at the
beginning of the parents list. This is passed to commit_tree(),
which does pop_commit() to consume all commits on the list.
So after commit_tree() returns, all commit_list instances on
remoteheads list, and possibly the one finish_automerge() prepended
for the current HEAD, are all consumed, and there is no need to
call, and it would be wrong to call, free_commit_list(), at this
point.
So, I agree that this conditional freeing is correct. It was just
it was a bit hard to see.
Thanks.
|
This branch is now known as |
|
This patch series was integrated into seen via beb78a6. |
|
There was a status update in the "New Topics" section about the branch Leakfix. Will merge to 'next'. source: <pull.1200.git.git.1642664835.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via d330798. |
|
This patch series was integrated into seen via 7620cc5. |
|
This patch series was integrated into seen via dc7e36e. |
|
There was a status update in the "Cooking" section about the branch Leakfix. Will merge to 'next'. source: <pull.1200.git.git.1642664835.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 722e1df. |
|
This patch series was integrated into seen via 3faf353. |
|
This patch series was integrated into next via 0cf6aa0. |
|
This patch series was integrated into seen via 8e50a26. |
|
This patch series was integrated into seen via 904138c. |
|
This patch series was integrated into seen via e2a6438. |
|
There was a status update in the "Cooking" section about the branch Leakfix. Will merge to 'master'. source: <pull.1200.git.git.1642664835.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via a1d47d4. |
|
This patch series was integrated into seen via 38e0f0f. |
|
This patch series was integrated into seen via 3aa8df7. |
|
This patch series was integrated into seen via c70b5e7. |
|
This patch series was integrated into next via c70b5e7. |
|
This patch series was integrated into master via c70b5e7. |
|
Closed via c70b5e7. |
This series fixes three memory leaks.
The first comes from a report at [1]. It's a leak in merge-ort that pre-dates the remerge-diff topic (technically traceable back 15.5 years ago across merge-recursive.c history) and is triggerable from a variety of testcases. I think I may have overlooked it previously just because there's other leaks in revision walking and this looks like one of those.
The next two leaks were ones in builtin/merge.c found while fixing the above leak; they are fixed together in the second patch.
[1] https://lore.kernel.org/git/220119.86pmonn2mb.gmgdl@evledraar.gmail.com/
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com