| Message ID | 2a23df9a6869f58a231cfe4947b322690d48cb1a.1726484308.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.7) | expand |
Patrick Steinhardt <ps@pks.im> 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@pks.im> > --- > 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.
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; + while (commit->parents != parent) + pop_commit(&commit->parents); commit->parents = parent; /* @@ -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 */ diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh index be6c3f472c1..49d28166da0 100755 --- a/t/t1414-reflog-walk.sh +++ b/t/t1414-reflog-walk.sh @@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'set up some reflog entries' ' diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index a6de7c57643..7044c7d7c6d 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -2,6 +2,7 @@ test_description='exercise basic bitmap functionality' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-bitmap.sh diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh index 832b92619c0..6eaa692f33b 100755 --- a/t/t5326-multi-pack-bitmaps.sh +++ b/t/t5326-multi-pack-bitmaps.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='exercise basic multi-pack bitmap functionality' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh" diff --git a/t/t6004-rev-list-path-optim.sh b/t/t6004-rev-list-path-optim.sh index cd4f420e2a1..5416241edea 100755 --- a/t/t6004-rev-list-path-optim.sh +++ b/t/t6004-rev-list-path-optim.sh @@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh index 738da23628b..1aabab69568 100755 --- a/t/t6019-rev-list-ancestry-path.sh +++ b/t/t6019-rev-list-ancestry-path.sh @@ -29,6 +29,7 @@ test_description='--ancestry-path' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_merge () { diff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh index 90ff1416400..f63bc8d3da6 100755 --- a/t/t6111-rev-list-treesame.sh +++ b/t/t6111-rev-list-treesame.sh @@ -16,6 +16,7 @@ test_description='TREESAME and limiting' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh note () {
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@pks.im> --- 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(+)