[PATCH] repack: free geometry struct

[PATCH] repack: free geometry struct

[Jeff King]

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

Re: [PATCH] repack: free geometry struct

[Taylor Blau]

On Tue, Aug 08, 2023 at 02:50:23PM -0400, Jeff King wrote:
> 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.

The patch you have here looks good, but...

> 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").

...I think that storing the geometric_factor value on the pack_geometry
struct itself gets around most of these issues.

This version is still somewhat noisy, but it's not too bad IMHO. I'm
fine with either your approach or mine :-).

--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index aea5ca9d44..13e4f0094e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -303,6 +303,8 @@ struct pack_geometry {
 	struct packed_git **pack;
 	uint32_t pack_nr, pack_alloc;
 	uint32_t split;
+
+	int split_factor;
 };

 static uint32_t geometry_pack_weight(struct packed_git *p)
@@ -324,17 +326,13 @@ static int geometry_cmp(const void *va, const void *vb)
 	return 0;
 }

-static void init_pack_geometry(struct pack_geometry **geometry_p,
+static void init_pack_geometry(struct pack_geometry *geometry,
 			       struct string_list *existing_kept_packs,
 			       const struct pack_objects_args *args)
 {
 	struct packed_git *p;
-	struct pack_geometry *geometry;
 	struct strbuf buf = STRBUF_INIT;

-	*geometry_p = xcalloc(1, sizeof(struct pack_geometry));
-	geometry = *geometry_p;
-
 	for (p = get_all_packs(the_repository); p; p = p->next) {
 		if (args->local && !p->pack_local)
 			/*
@@ -380,7 +378,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,
 	strbuf_release(&buf);
 }

-static void split_pack_geometry(struct pack_geometry *geometry, int factor)
+static void split_pack_geometry(struct pack_geometry *geometry)
 {
 	uint32_t i;
 	uint32_t split;
@@ -399,12 +397,14 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)
 		struct packed_git *ours = geometry->pack[i];
 		struct packed_git *prev = geometry->pack[i - 1];

-		if (unsigned_mult_overflows(factor, geometry_pack_weight(prev)))
+		if (unsigned_mult_overflows(geometry->split_factor,
+					    geometry_pack_weight(prev)))
 			die(_("pack %s too large to consider in geometric "
 			      "progression"),
 			    prev->pack_name);

-		if (geometry_pack_weight(ours) < factor * geometry_pack_weight(prev))
+		if (geometry_pack_weight(ours) <
+		    geometry->split_factor * geometry_pack_weight(prev))
 			break;
 	}

@@ -439,10 +439,12 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)
 	for (i = split; i < geometry->pack_nr; i++) {
 		struct packed_git *ours = geometry->pack[i];

-		if (unsigned_mult_overflows(factor, total_size))
+		if (unsigned_mult_overflows(geometry->split_factor,
+					    total_size))
 			die(_("pack %s too large to roll up"), ours->pack_name);

-		if (geometry_pack_weight(ours) < factor * total_size) {
+		if (geometry_pack_weight(ours) <
+		    geometry->split_factor * total_size) {
 			if (unsigned_add_overflows(total_size,
 						   geometry_pack_weight(ours)))
 				die(_("pack %s too large to roll up"),
@@ -781,7 +783,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list names = STRING_LIST_INIT_DUP;
 	struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
 	struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
-	struct pack_geometry *geometry = NULL;
+	struct pack_geometry geometry = { 0 };
 	struct strbuf line = STRBUF_INIT;
 	struct tempfile *refs_snapshot = NULL;
 	int i, ext, ret;
@@ -795,7 +797,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
 	struct pack_objects_args po_args = {NULL};
 	struct pack_objects_args cruft_po_args = {NULL};
-	int geometric_factor = 0;
 	int write_midx = 0;
 	const char *cruft_expiration = NULL;
 	const char *expire_to = NULL;
@@ -844,7 +845,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("repack objects in packs marked with .keep")),
 		OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
 				N_("do not repack this pack")),
-		OPT_INTEGER('g', "geometric", &geometric_factor,
+		OPT_INTEGER('g', "geometric", &geometry.split_factor,
 			    N_("find a geometric progression with factor <N>")),
 		OPT_BOOL('m', "write-midx", &write_midx,
 			   N_("write a multi-pack index of the resulting packs")),
@@ -920,11 +921,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
 			       &keep_pack_list);

-	if (geometric_factor) {
+	if (geometry.split_factor) {
 		if (pack_everything)
 			die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
 		init_pack_geometry(&geometry, &existing_kept_packs, &po_args);
-		split_pack_geometry(geometry, geometric_factor);
+		split_pack_geometry(&geometry);
 	}

 	prepare_pack_objects(&cmd, &po_args, packtmp);
@@ -938,7 +939,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		strvec_pushf(&cmd.args, "--keep-pack=%s",
 			     keep_pack_list.items[i].string);
 	strvec_push(&cmd.args, "--non-empty");
-	if (!geometry) {
+	if (!geometry.split_factor) {
 		/*
 		 * We need to grab all reachable objects, including those that
 		 * are reachable from reflogs and the index.
@@ -985,7 +986,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				strvec_push(&cmd.args, "--pack-loose-unreachable");
 			}
 		}
-	} else if (geometry) {
+	} else if (geometry.split_factor) {
 		strvec_push(&cmd.args, "--stdin-packs");
 		strvec_push(&cmd.args, "--unpacked");
 	} else {
@@ -993,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		strvec_push(&cmd.args, "--incremental");
 	}

-	if (geometry)
+	if (geometry.split_factor)
 		cmd.in = -1;
 	else
 		cmd.no_stdin = 1;
@@ -1002,17 +1003,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (ret)
 		goto cleanup;

-	if (geometry) {
+	if (geometry.split_factor) {
 		FILE *in = xfdopen(cmd.in, "w");
 		/*
 		 * The resulting pack should contain all objects in packs that
 		 * are going to be rolled up, but exclude objects in packs which
 		 * are being left alone.
 		 */
-		for (i = 0; i < geometry->split; i++)
-			fprintf(in, "%s\n", pack_basename(geometry->pack[i]));
-		for (i = geometry->split; i < geometry->pack_nr; i++)
-			fprintf(in, "^%s\n", pack_basename(geometry->pack[i]));
+		for (i = 0; i < geometry.split; i++)
+			fprintf(in, "%s\n", pack_basename(geometry.pack[i]));
+		for (i = geometry.split; i < geometry.pack_nr; i++)
+			fprintf(in, "^%s\n", pack_basename(geometry.pack[i]));
 		fclose(in);
 	}

@@ -1155,9 +1156,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (write_midx) {
 		struct string_list include = STRING_LIST_INIT_NODUP;
 		midx_included_packs(&include, &existing_nonkept_packs,
-				    &existing_kept_packs, &names, geometry);
+				    &existing_kept_packs, &names, &geometry);

-		ret = write_midx_included_packs(&include, geometry,
+		ret = write_midx_included_packs(&include, &geometry,
 						refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
 						show_progress, write_bitmaps > 0);

@@ -1180,12 +1181,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			remove_redundant_pack(packdir, item->string);
 		}

-		if (geometry) {
+		if (geometry.split_factor) {
 			struct strbuf buf = STRBUF_INIT;

 			uint32_t i;
-			for (i = 0; i < geometry->split; i++) {
-				struct packed_git *p = geometry->pack[i];
+			for (i = 0; i < geometry.split; i++) {
+				struct packed_git *p = geometry.pack[i];
 				if (string_list_has_string(&names,
 							   hash_to_hex(p->hash)))
 					continue;
@@ -1228,7 +1229,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);
+	clear_pack_geometry(&geometry);

 	return ret;
 }
--- >8 ---

Thanks,
Taylor

Re: [PATCH] repack: free geometry struct

[Jeff King]

On Tue, Aug 08, 2023 at 03:59:43PM -0400, Taylor Blau wrote:

> > 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").
> 
> ...I think that storing the geometric_factor value on the pack_geometry
> struct itself gets around most of these issues.
> 
> This version is still somewhat noisy, but it's not too bad IMHO. I'm
> fine with either your approach or mine :-).

Yeah, that is much better than what I had put together, as it means that
pack_geometry is always in a consistent state, even if we didn't call
init_pack_geometry(). So it is safe to clear() it, for example, without
checking geometric_factor again. And any sub-functions can likewise
check whether it contains any useful data. So you'd need this fixup on
top of your patch:

diff --git a/builtin/repack.c b/builtin/repack.c
index 13e4f0094e..80bffc36d4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -579,7 +579,7 @@ static void midx_included_packs(struct string_list *include,
 		string_list_insert(include, xstrfmt("%s.idx", item->string));
 	for_each_string_list_item(item, names)
 		string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
-	if (geometry) {
+	if (geometry->split_factor) {
 		struct strbuf buf = STRBUF_INIT;
 		uint32_t i;
 		for (i = geometry->split; i < geometry->pack_nr; i++) {

or else t5326 (and I think a few others) will fail.

I'd be happy to proceed that way if you want to clean it up with a
commit message, but I think we should do it on top of the leak-fix (I
wanted to say that the heap-to-stack conversion was low-risk, but both
of us introduced bugs while trying it ;) ).

-Peff

[PATCH 0/2] repack: prevent geometry struct leak

[Taylor Blau]

This short series combines a patch from Peff and a patch from me to
prevent leaking the 'struct pack_geometry *' pointer from the repack
code.

The first patch free()s the heap-allocated struct, and the second
patch moves the variable to be allocated on the stack.

Jeff King (1):
  repack: free geometry struct

Taylor Blau (1):
  repack: move `pack_geometry` struct to the stack

 builtin/repack.c | 66 +++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

-- 
2.42.0.rc0.27.g2e2a760381

[PATCH 1/2] repack: free geometry struct

[Taylor Blau]

From: Jeff King <peff@peff.net>

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>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 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.27.g2e2a760381

[PATCH 2/2] repack: move `pack_geometry` struct to the stack

[Taylor Blau]

The `pack_geometry` struct is used to maintain and partition a list of
packfiles into a "frozen" set (to be left alone), and a non-frozen set
(to be combined into a single new pack). In the previous commit, we
removed a leak caused by neglecting to free() the heap allocated space
used to store the structure itself.

But there is no need for this structure to live on the heap anyway.
Instead, let's move it to be stack allocated, eliminating the
possibility of a direct leak like the one addressed in the previous
patch.

The one minor hitch is that we use the NULL-ness of the pack_geometry's
struct pointer to determine whether or not we are performing a geometric
repack with `--geometric=<d>`. But since we only initialize the
pack_geometry structure when the `geometric_factor` is non-zero, we can
use that variable (based on whether or not it is equal to zero) to
determine whether or not we are performing a geometric repack.

There are a couple of spots that have access to a pointer to the
pack_geometry struct, but not the geometric_factor itself. Instead of
passing in an additional variable, let's make the geometric_factor a
field of the pack_geometry struct.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 62 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 97051479e4..2b43a5be08 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -303,6 +303,8 @@ struct pack_geometry {
 	struct packed_git **pack;
 	uint32_t pack_nr, pack_alloc;
 	uint32_t split;
+
+	int split_factor;
 };
 
 static uint32_t geometry_pack_weight(struct packed_git *p)
@@ -324,17 +326,13 @@ static int geometry_cmp(const void *va, const void *vb)
 	return 0;
 }
 
-static void init_pack_geometry(struct pack_geometry **geometry_p,
+static void init_pack_geometry(struct pack_geometry *geometry,
 			       struct string_list *existing_kept_packs,
 			       const struct pack_objects_args *args)
 {
 	struct packed_git *p;
-	struct pack_geometry *geometry;
 	struct strbuf buf = STRBUF_INIT;
 
-	*geometry_p = xcalloc(1, sizeof(struct pack_geometry));
-	geometry = *geometry_p;
-
 	for (p = get_all_packs(the_repository); p; p = p->next) {
 		if (args->local && !p->pack_local)
 			/*
@@ -380,7 +378,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,
 	strbuf_release(&buf);
 }
 
-static void split_pack_geometry(struct pack_geometry *geometry, int factor)
+static void split_pack_geometry(struct pack_geometry *geometry)
 {
 	uint32_t i;
 	uint32_t split;
@@ -399,12 +397,14 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)
 		struct packed_git *ours = geometry->pack[i];
 		struct packed_git *prev = geometry->pack[i - 1];
 
-		if (unsigned_mult_overflows(factor, geometry_pack_weight(prev)))
+		if (unsigned_mult_overflows(geometry->split_factor,
+					    geometry_pack_weight(prev)))
 			die(_("pack %s too large to consider in geometric "
 			      "progression"),
 			    prev->pack_name);
 
-		if (geometry_pack_weight(ours) < factor * geometry_pack_weight(prev))
+		if (geometry_pack_weight(ours) <
+		    geometry->split_factor * geometry_pack_weight(prev))
 			break;
 	}
 
@@ -439,10 +439,12 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)
 	for (i = split; i < geometry->pack_nr; i++) {
 		struct packed_git *ours = geometry->pack[i];
 
-		if (unsigned_mult_overflows(factor, total_size))
+		if (unsigned_mult_overflows(geometry->split_factor,
+					    total_size))
 			die(_("pack %s too large to roll up"), ours->pack_name);
 
-		if (geometry_pack_weight(ours) < factor * total_size) {
+		if (geometry_pack_weight(ours) <
+		    geometry->split_factor * total_size) {
 			if (unsigned_add_overflows(total_size,
 						   geometry_pack_weight(ours)))
 				die(_("pack %s too large to roll up"),
@@ -498,7 +500,6 @@ static void free_pack_geometry(struct pack_geometry *geometry)
 		return;
 
 	free(geometry->pack);
-	free(geometry);
 }
 
 struct midx_snapshot_ref_data {
@@ -575,7 +576,7 @@ static void midx_included_packs(struct string_list *include,
 		string_list_insert(include, xstrfmt("%s.idx", item->string));
 	for_each_string_list_item(item, names)
 		string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
-	if (geometry) {
+	if (geometry->split_factor) {
 		struct strbuf buf = STRBUF_INIT;
 		uint32_t i;
 		for (i = geometry->split; i < geometry->pack_nr; i++) {
@@ -779,7 +780,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list names = STRING_LIST_INIT_DUP;
 	struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
 	struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
-	struct pack_geometry *geometry = NULL;
+	struct pack_geometry geometry = { 0 };
 	struct strbuf line = STRBUF_INIT;
 	struct tempfile *refs_snapshot = NULL;
 	int i, ext, ret;
@@ -793,7 +794,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
 	struct pack_objects_args po_args = {NULL};
 	struct pack_objects_args cruft_po_args = {NULL};
-	int geometric_factor = 0;
 	int write_midx = 0;
 	const char *cruft_expiration = NULL;
 	const char *expire_to = NULL;
@@ -842,7 +842,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("repack objects in packs marked with .keep")),
 		OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
 				N_("do not repack this pack")),
-		OPT_INTEGER('g', "geometric", &geometric_factor,
+		OPT_INTEGER('g', "geometric", &geometry.split_factor,
 			    N_("find a geometric progression with factor <N>")),
 		OPT_BOOL('m', "write-midx", &write_midx,
 			   N_("write a multi-pack index of the resulting packs")),
@@ -918,11 +918,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
 			       &keep_pack_list);
 
-	if (geometric_factor) {
+	if (geometry.split_factor) {
 		if (pack_everything)
 			die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
 		init_pack_geometry(&geometry, &existing_kept_packs, &po_args);
-		split_pack_geometry(geometry, geometric_factor);
+		split_pack_geometry(&geometry);
 	}
 
 	prepare_pack_objects(&cmd, &po_args, packtmp);
@@ -936,7 +936,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		strvec_pushf(&cmd.args, "--keep-pack=%s",
 			     keep_pack_list.items[i].string);
 	strvec_push(&cmd.args, "--non-empty");
-	if (!geometry) {
+	if (!geometry.split_factor) {
 		/*
 		 * We need to grab all reachable objects, including those that
 		 * are reachable from reflogs and the index.
@@ -983,7 +983,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				strvec_push(&cmd.args, "--pack-loose-unreachable");
 			}
 		}
-	} else if (geometry) {
+	} else if (geometry.split_factor) {
 		strvec_push(&cmd.args, "--stdin-packs");
 		strvec_push(&cmd.args, "--unpacked");
 	} else {
@@ -991,7 +991,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		strvec_push(&cmd.args, "--incremental");
 	}
 
-	if (geometry)
+	if (geometry.split_factor)
 		cmd.in = -1;
 	else
 		cmd.no_stdin = 1;
@@ -1000,17 +1000,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (ret)
 		goto cleanup;
 
-	if (geometry) {
+	if (geometry.split_factor) {
 		FILE *in = xfdopen(cmd.in, "w");
 		/*
 		 * The resulting pack should contain all objects in packs that
 		 * are going to be rolled up, but exclude objects in packs which
 		 * are being left alone.
 		 */
-		for (i = 0; i < geometry->split; i++)
-			fprintf(in, "%s\n", pack_basename(geometry->pack[i]));
-		for (i = geometry->split; i < geometry->pack_nr; i++)
-			fprintf(in, "^%s\n", pack_basename(geometry->pack[i]));
+		for (i = 0; i < geometry.split; i++)
+			fprintf(in, "%s\n", pack_basename(geometry.pack[i]));
+		for (i = geometry.split; i < geometry.pack_nr; i++)
+			fprintf(in, "^%s\n", pack_basename(geometry.pack[i]));
 		fclose(in);
 	}
 
@@ -1153,9 +1153,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (write_midx) {
 		struct string_list include = STRING_LIST_INIT_NODUP;
 		midx_included_packs(&include, &existing_nonkept_packs,
-				    &existing_kept_packs, &names, geometry);
+				    &existing_kept_packs, &names, &geometry);
 
-		ret = write_midx_included_packs(&include, geometry,
+		ret = write_midx_included_packs(&include, &geometry,
 						refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
 						show_progress, write_bitmaps > 0);
 
@@ -1178,12 +1178,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			remove_redundant_pack(packdir, item->string);
 		}
 
-		if (geometry) {
+		if (geometry.split_factor) {
 			struct strbuf buf = STRBUF_INIT;
 
 			uint32_t i;
-			for (i = 0; i < geometry->split; i++) {
-				struct packed_git *p = geometry->pack[i];
+			for (i = 0; i < geometry.split; i++) {
+				struct packed_git *p = geometry.pack[i];
 				if (string_list_has_string(&names,
 							   hash_to_hex(p->hash)))
 					continue;
@@ -1226,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);
-	free_pack_geometry(geometry);
+	free_pack_geometry(&geometry);
 
 	return ret;
 }
-- 
2.42.0.rc0.27.g2e2a760381

Re: [PATCH 0/2] repack: prevent geometry struct leak

[Jeff King]

On Wed, Aug 09, 2023 at 04:32:36PM -0400, Taylor Blau wrote:

> This short series combines a patch from Peff and a patch from me to
> prevent leaking the 'struct pack_geometry *' pointer from the repack
> code.
> 
> The first patch free()s the heap-allocated struct, and the second
> patch moves the variable to be allocated on the stack.

Thanks for cleaning up your patch. I think Junio has the first one in
'next' already as jk/repack-leakfix, but your patch 2 can go on top.

The result looks good to me. It's a little unusual that the "init"
function expects the caller to have already initialized-to-zero and set
the split_factor field, but I think that's OK for a struct that is
limited to this one file.

-Peff