Patrick Steinhardt <ps@xxxxxx> writes:
> When simplifying commits, e.g. because they are treesame with their
> parents, we unset the commit's parent pointers but never free them. Plug
> the resulting memory leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> revision.c | 5 +++++
> t/t1414-reflog-walk.sh | 1 +
> t/t5310-pack-bitmaps.sh | 1 +
> t/t5326-multi-pack-bitmaps.sh | 2 ++
> t/t6004-rev-list-path-optim.sh | 1 +
> t/t6019-rev-list-ancestry-path.sh | 1 +
> t/t6111-rev-list-treesame.sh | 1 +
> 7 files changed, 12 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 2d7ad2bddff..e79f39e5555 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
> ts->treesame[nth_parent] = 1;
> continue;
> }
> +
> + free_commit_list(parent->next);
> parent->next = NULL;
We have been walking commit->parents linked list, "parent" is the
current parent we are looking at. We are simplifying the history
and decided that later parents are not needed, hence we discard the
remaining commit_list entries starting from parents->next. But we
didn't discard; we just updated parent->next pointer and made the
pointed data unreachable, leaking. So we free_commit_list().
> + while (commit->parents != parent)
> + pop_commit(&commit->parents);
> commit->parents = parent;
And this is the other direction, discarding the parents before the
current one we are looking at.
Of course it makes me wonder if we can just free_commit_list() the
whole thing and then add the current parent commit (in "p", which
was taken with "p = parent->item" before) as the sole parent, with
something like
free_commit_list(commit->parents);
commit->parents = NULL;
/* parent = */ commit_list_insert(p, &commit->parents);
to replace the 5-line (2 to discard later parents, 3 to discard
earlier ones) code, but I do not think it becomes particularly
easier to read, so let's drop that.
> @@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
> die("cannot simplify commit %s (invalid %s)",
> oid_to_hex(&commit->object.oid),
> oid_to_hex(&p->object.oid));
> + free_commit_list(p->parents);
> p->parents = NULL;
> }
> /* fallthrough */
Looking good.
Thanks.
