Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"

From: René Scharfe <l.s.r@web.de>
Date: Nov 28, 2022, 17:57 UTC
Message-ID: <2488058d-dc59-e8c1-0611-fbcaeb083d73@web.de>
In-Reply-To: <59431916-9f55-d0f4-da54-e7369803eb4c@web.de>

Am 28.11.2022 um 16:56 schrieb René Scharfe:>

> The problem is "How to use struct rev_info without leaks?".  No matter
> where you move it, the leak will be present until the TODO in
> release_revisions() is done.

Wrong.  The following sequence leaks:

	struct rev_info revs;
	repo_init_revisions(the_repository, &revs, NULL);
	release_revisions(&revs);

... and this here doesn't:

	struct rev_info revs;
	repo_init_revisions(the_repository, &revs, NULL);
	setup_revisions(0, NULL, &revs, NULL);  // leak plugger
	release_revisions(&revs);

That's because setup_revisions() calls diff_setup_done(), which frees revs->diffopt.parseopts, and release_revisions() doesn't.

And since builtin/pack-objects.c::get_object_list() calls setup_revisions(), it really frees that memory, as you claimed from the start. Sorry, I was somehow assuming that a setup function wouldn't clean up. D'oh!

The first sequence is used in some other places. e.g. builtin/prune.c. But there LeakSanitizer doesn't complain for some reason; Valgrind reports the parseopts allocation as "possibly lost".

I still think the assumption that "init_x(x); release_x(x);" doesn't leak is reasonable. Let's make it true. How about this? It's safe in the sense that we don't risk double frees and it's close to the TODO comment so we probably won't forget removing it once diff_free() becomes used.

---
 revision.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.c b/revision.c
index 439e34a7c5..6a51ef9418 100644
--- a/revision.c
+++ b/revision.c
@@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)
 	release_revisions_mailmap(revs->mailmap);
 	free_grep_patterns(&revs->grep_filter);
 	/* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */
+	FREE_AND_NULL(revs->diffopt.parseopts);
 	diff_free(&revs->pruning);
 	reflog_walk_info_release(revs->reflog_info);
 	release_revisions_topo_walk_info(revs->topo_walk_info);
--
2.30.2