git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] repack: free geometry struct
@ 2023-08-08 18:50 Jeff King
  2023-08-08 19:59 ` Taylor Blau
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff King @ 2023-08-08 18:50 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau

When the program is ending, we call clear_pack_geometry() to free any
resources in the pack_geometry struct. But the struct itself is
allocated on the heap, and leak-checkers will complain about the
resulting small leak.

This one was marked by Coverity as a "new" leak, though it has existed
since 0fabafd0b9 (builtin/repack.c: add '--geometric' option,
2021-02-22). This might be because recent unrelated changes in the file
confused it about what is new and what is not. But regardless, it is
worth addressing.

We can fix it easily by free-ing the struct. We'll convert our "clear"
function to "free", since the allocation happens in the matching init()
function (though since there is only one call to each, and the struct is
local to this file, it's mostly academic).

Another option would be to put the struct on the stack rather than the
heap. However, this gets tricky, as we check the pointer against NULL in
several places to decide whether we're in geometric mode.

Signed-off-by: Jeff King <peff@peff.net>
---
I did actually put together the "put it on the stack" patch, which swaps
out:

  if (geometry)

for:

  if (geometric_factor)

to get around the NULL checks. But besides being noisy for little gain,
it ends up with some subtle gotchas, because we pass "&geometry" into
some functions which don't have access to "geometric_factor" (so now
extra call-sites have to keep track of "is this struct valid enough to
pass").

 builtin/repack.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index aea5ca9d44..97051479e4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -492,15 +492,13 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)
 	return NULL;
 }
 
-static void clear_pack_geometry(struct pack_geometry *geometry)
+static void free_pack_geometry(struct pack_geometry *geometry)
 {
 	if (!geometry)
 		return;
 
 	free(geometry->pack);
-	geometry->pack_nr = 0;
-	geometry->pack_alloc = 0;
-	geometry->split = 0;
+	free(geometry);
 }
 
 struct midx_snapshot_ref_data {
@@ -1228,7 +1226,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&names, 1);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
-	clear_pack_geometry(geometry);
+	free_pack_geometry(geometry);
 
 	return ret;
 }
-- 
2.42.0.rc0.376.g66bfc4f195

^ permalink raw reply related	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-10  0:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-08 18:50 [PATCH] repack: free geometry struct Jeff King
2023-08-08 19:59 ` Taylor Blau
2023-08-08 21:17   ` Jeff King
2023-08-09 20:32     ` [PATCH 0/2] repack: prevent geometry struct leak Taylor Blau
2023-08-09 20:32       ` [PATCH 1/2] repack: free geometry struct Taylor Blau
2023-08-09 20:32       ` [PATCH 2/2] repack: move `pack_geometry` struct to the stack Taylor Blau
2023-08-10  0:19       ` [PATCH 0/2] repack: prevent geometry struct leak Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).