{"thread":{"id":"60082","subject":"[PATCH] repack: free geometry struct","startedAt":"2023-08-08T19:26:07Z","lastAt":"2023-08-10T00:19:25Z","messageCount":7,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480311","messageId":"20230808185023.GA3498623@coredump.intra.peff.net","threadId":"60082","inReplyTo":null,"subject":"[PATCH] repack: free geometry struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T18:50:23Z","receivedAt":"2023-08-08T19:26:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the program is ending, we call clear_pack_geometry() to free any\nresources in the pack_geometry struct. But the struct itself is\nallocated on the heap, and leak-checkers will complain about the\nresulting small leak.\n\nThis one was marked by Coverity as a \"new\" leak, though it has existed\nsince 0fabafd0b9 (builtin/repack.c: add '--geometric' option,\n2021-02-22). This might be because recent unrelated changes in the file\nconfused it about what is new and what is not. But regardless, it is\nworth addressing.\n\nWe can fix it easily by free-ing the struct. We'll convert our \"clear\"\nfunction to \"free\", since the allocation happens in the matching init()\nfunction (though since there is only one call to each, and the struct is\nlocal to this file, it's mostly academic).\n\nAnother option would be to put the struct on the stack rather than the\nheap. However, this gets tricky, as we check the pointer against NULL in\nseveral places to decide whether we're in geometric mode.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI did actually put together the \"put it on the stack\" patch, which swaps\nout:\n\n  if (geometry)\n\nfor:\n\n  if (geometric_factor)\n\nto get around the NULL checks. But besides being noisy for little gain,\nit ends up with some subtle gotchas, because we pass \"&geometry\" into\nsome functions which don't have access to \"geometric_factor\" (so now\nextra call-sites have to keep track of \"is this struct valid enough to\npass\").\n\n builtin/repack.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex aea5ca9d44..97051479e4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -492,15 +492,13 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n \treturn NULL;\n }\n \n-static void clear_pack_geometry(struct pack_geometry *geometry)\n+static void free_pack_geometry(struct pack_geometry *geometry)\n {\n \tif (!geometry)\n \t\treturn;\n \n \tfree(geometry->pack);\n-\tgeometry->pack_nr = 0;\n-\tgeometry->pack_alloc = 0;\n-\tgeometry->split = 0;\n+\tfree(geometry);\n }\n \n struct midx_snapshot_ref_data {\n@@ -1228,7 +1226,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n-\tclear_pack_geometry(geometry);\n+\tfree_pack_geometry(geometry);\n \n \treturn ret;\n }\n-- \n2.42.0.rc0.376.g66bfc4f195\n"},{"id":"480327","messageId":"ZNKer8BfwaKEeV+W@nand.local","threadId":"60082","inReplyTo":"20230808185023.GA3498623@coredump.intra.peff.net","subject":"Re: [PATCH] repack: free geometry struct","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T19:59:43Z","receivedAt":"2023-08-08T20:34:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 02:50:23PM -0400, Jeff King wrote:\n> Another option would be to put the struct on the stack rather than the\n> heap. However, this gets tricky, as we check the pointer against NULL in\n> several places to decide whether we're in geometric mode.\n\nThe patch you have here looks good, but...\n\n> I did actually put together the \"put it on the stack\" patch, which swaps\n> out:\n>\n>   if (geometry)\n>\n> for:\n>\n>   if (geometric_factor)\n>\n> to get around the NULL checks. But besides being noisy for little gain,\n> it ends up with some subtle gotchas, because we pass \"&geometry\" into\n> some functions which don't have access to \"geometric_factor\" (so now\n> extra call-sites have to keep track of \"is this struct valid enough to\n> pass\").\n\n...I think that storing the geometric_factor value on the pack_geometry\nstruct itself gets around most of these issues.\n\nThis version is still somewhat noisy, but it's not too bad IMHO. I'm\nfine with either your approach or mine :-).\n\n--- 8< ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex aea5ca9d44..13e4f0094e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -303,6 +303,8 @@ struct pack_geometry {\n \tstruct packed_git **pack;\n \tuint32_t pack_nr, pack_alloc;\n \tuint32_t split;\n+\n+\tint split_factor;\n };\n\n static uint32_t geometry_pack_weight(struct packed_git *p)\n@@ -324,17 +326,13 @@ static int geometry_cmp(const void *va, const void *vb)\n \treturn 0;\n }\n\n-static void init_pack_geometry(struct pack_geometry **geometry_p,\n+static void init_pack_geometry(struct pack_geometry *geometry,\n \t\t\t       struct string_list *existing_kept_packs,\n \t\t\t       const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n-\tstruct pack_geometry *geometry;\n \tstruct strbuf buf = STRBUF_INIT;\n\n-\t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n-\tgeometry = *geometry_p;\n-\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (args->local && !p->pack_local)\n \t\t\t/*\n@@ -380,7 +378,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tstrbuf_release(&buf);\n }\n\n-static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n+static void split_pack_geometry(struct pack_geometry *geometry)\n {\n \tuint32_t i;\n \tuint32_t split;\n@@ -399,12 +397,14 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \t\tstruct packed_git *ours = geometry->pack[i];\n \t\tstruct packed_git *prev = geometry->pack[i - 1];\n\n-\t\tif (unsigned_mult_overflows(factor, geometry_pack_weight(prev)))\n+\t\tif (unsigned_mult_overflows(geometry->split_factor,\n+\t\t\t\t\t    geometry_pack_weight(prev)))\n \t\t\tdie(_(\"pack %s too large to consider in geometric \"\n \t\t\t      \"progression\"),\n \t\t\t    prev->pack_name);\n\n-\t\tif (geometry_pack_weight(ours) < factor * geometry_pack_weight(prev))\n+\t\tif (geometry_pack_weight(ours) <\n+\t\t    geometry->split_factor * geometry_pack_weight(prev))\n \t\t\tbreak;\n \t}\n\n@@ -439,10 +439,12 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \tfor (i = split; i < geometry->pack_nr; i++) {\n \t\tstruct packed_git *ours = geometry->pack[i];\n\n-\t\tif (unsigned_mult_overflows(factor, total_size))\n+\t\tif (unsigned_mult_overflows(geometry->split_factor,\n+\t\t\t\t\t    total_size))\n \t\t\tdie(_(\"pack %s too large to roll up\"), ours->pack_name);\n\n-\t\tif (geometry_pack_weight(ours) < factor * total_size) {\n+\t\tif (geometry_pack_weight(ours) <\n+\t\t    geometry->split_factor * total_size) {\n \t\t\tif (unsigned_add_overflows(total_size,\n \t\t\t\t\t\t   geometry_pack_weight(ours)))\n \t\t\t\tdie(_(\"pack %s too large to roll up\"),\n@@ -781,7 +783,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n-\tstruct pack_geometry *geometry = NULL;\n+\tstruct pack_geometry geometry = { 0 };\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n \tint i, ext, ret;\n@@ -795,7 +797,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tstruct pack_objects_args po_args = {NULL};\n \tstruct pack_objects_args cruft_po_args = {NULL};\n-\tint geometric_factor = 0;\n \tint write_midx = 0;\n \tconst char *cruft_expiration = NULL;\n \tconst char *expire_to = NULL;\n@@ -844,7 +845,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n-\t\tOPT_INTEGER('g', \"geometric\", &geometric_factor,\n+\t\tOPT_INTEGER('g', \"geometric\", &geometry.split_factor,\n \t\t\t    N_(\"find a geometric progression with factor <N>\")),\n \t\tOPT_BOOL('m', \"write-midx\", &write_midx,\n \t\t\t   N_(\"write a multi-pack index of the resulting packs\")),\n@@ -920,11 +921,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n \t\t\t       &keep_pack_list);\n\n-\tif (geometric_factor) {\n+\tif (geometry.split_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n \t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n-\t\tsplit_pack_geometry(geometry, geometric_factor);\n+\t\tsplit_pack_geometry(&geometry);\n \t}\n\n \tprepare_pack_objects(&cmd, &po_args, packtmp);\n@@ -938,7 +939,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n \t\t\t     keep_pack_list.items[i].string);\n \tstrvec_push(&cmd.args, \"--non-empty\");\n-\tif (!geometry) {\n+\tif (!geometry.split_factor) {\n \t\t/*\n \t\t * We need to grab all reachable objects, including those that\n \t\t * are reachable from reflogs and the index.\n@@ -985,7 +986,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t\t\t}\n \t\t}\n-\t} else if (geometry) {\n+\t} else if (geometry.split_factor) {\n \t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t} else {\n@@ -993,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrvec_push(&cmd.args, \"--incremental\");\n \t}\n\n-\tif (geometry)\n+\tif (geometry.split_factor)\n \t\tcmd.in = -1;\n \telse\n \t\tcmd.no_stdin = 1;\n@@ -1002,17 +1003,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (ret)\n \t\tgoto cleanup;\n\n-\tif (geometry) {\n+\tif (geometry.split_factor) {\n \t\tFILE *in = xfdopen(cmd.in, \"w\");\n \t\t/*\n \t\t * The resulting pack should contain all objects in packs that\n \t\t * are going to be rolled up, but exclude objects in packs which\n \t\t * are being left alone.\n \t\t */\n-\t\tfor (i = 0; i < geometry->split; i++)\n-\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry->pack[i]));\n-\t\tfor (i = geometry->split; i < geometry->pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry->pack[i]));\n+\t\tfor (i = 0; i < geometry.split; i++)\n+\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n+\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n+\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n \t\tfclose(in);\n \t}\n\n@@ -1155,9 +1156,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n \t\tmidx_included_packs(&include, &existing_nonkept_packs,\n-\t\t\t\t    &existing_kept_packs, &names, geometry);\n+\t\t\t\t    &existing_kept_packs, &names, &geometry);\n\n-\t\tret = write_midx_included_packs(&include, geometry,\n+\t\tret = write_midx_included_packs(&include, &geometry,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n\n@@ -1180,12 +1181,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n\n-\t\tif (geometry) {\n+\t\tif (geometry.split_factor) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n\n \t\t\tuint32_t i;\n-\t\t\tfor (i = 0; i < geometry->split; i++) {\n-\t\t\t\tstruct packed_git *p = geometry->pack[i];\n+\t\t\tfor (i = 0; i < geometry.split; i++) {\n+\t\t\t\tstruct packed_git *p = geometry.pack[i];\n \t\t\t\tif (string_list_has_string(&names,\n \t\t\t\t\t\t\t   hash_to_hex(p->hash)))\n \t\t\t\t\tcontinue;\n@@ -1228,7 +1229,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n-\tclear_pack_geometry(geometry);\n+\tclear_pack_geometry(&geometry);\n\n \treturn ret;\n }\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"480338","messageId":"20230808211759.GA322409@coredump.intra.peff.net","threadId":"60082","inReplyTo":"ZNKer8BfwaKEeV+W@nand.local","subject":"Re: [PATCH] repack: free geometry struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T21:17:59Z","receivedAt":"2023-08-08T21:22:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 08, 2023 at 03:59:43PM -0400, Taylor Blau wrote:\n\n> > I did actually put together the \"put it on the stack\" patch, which swaps\n> > out:\n> >\n> >   if (geometry)\n> >\n> > for:\n> >\n> >   if (geometric_factor)\n> >\n> > to get around the NULL checks. But besides being noisy for little gain,\n> > it ends up with some subtle gotchas, because we pass \"&geometry\" into\n> > some functions which don't have access to \"geometric_factor\" (so now\n> > extra call-sites have to keep track of \"is this struct valid enough to\n> > pass\").\n> \n> ...I think that storing the geometric_factor value on the pack_geometry\n> struct itself gets around most of these issues.\n> \n> This version is still somewhat noisy, but it's not too bad IMHO. I'm\n> fine with either your approach or mine :-).\n\nYeah, that is much better than what I had put together, as it means that\npack_geometry is always in a consistent state, even if we didn't call\ninit_pack_geometry(). So it is safe to clear() it, for example, without\nchecking geometric_factor again. And any sub-functions can likewise\ncheck whether it contains any useful data. So you'd need this fixup on\ntop of your patch:\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 13e4f0094e..80bffc36d4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -579,7 +579,7 @@ static void midx_included_packs(struct string_list *include,\n \t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \tfor_each_string_list_item(item, names)\n \t\tstring_list_insert(include, xstrfmt(\"pack-%s.idx\", item->string));\n-\tif (geometry) {\n+\tif (geometry->split_factor) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tuint32_t i;\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n\nor else t5326 (and I think a few others) will fail.\n\nI'd be happy to proceed that way if you want to clean it up with a\ncommit message, but I think we should do it on top of the leak-fix (I\nwanted to say that the heap-to-stack conversion was low-risk, but both\nof us introduced bugs while trying it ;) ).\n\n-Peff\n"},{"id":"480395","messageId":"6bb4c3805c8f1bdf021d1bf8005f7e944731b586.1691613149.git.me@ttaylorr.com","threadId":"60082","inReplyTo":"cover.1691613149.git.me@ttaylorr.com","subject":"[PATCH 1/2] repack: free geometry struct","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-09T20:32:39Z","receivedAt":"2023-08-09T20:32:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen the program is ending, we call clear_pack_geometry() to free any\nresources in the pack_geometry struct. But the struct itself is\nallocated on the heap, and leak-checkers will complain about the\nresulting small leak.\n\nThis one was marked by Coverity as a \"new\" leak, though it has existed\nsince 0fabafd0b9 (builtin/repack.c: add '--geometric' option,\n2021-02-22). This might be because recent unrelated changes in the file\nconfused it about what is new and what is not. But regardless, it is\nworth addressing.\n\nWe can fix it easily by free-ing the struct. We'll convert our \"clear\"\nfunction to \"free\", since the allocation happens in the matching init()\nfunction (though since there is only one call to each, and the struct is\nlocal to this file, it's mostly academic).\n\nAnother option would be to put the struct on the stack rather than the\nheap. However, this gets tricky, as we check the pointer against NULL in\nseveral places to decide whether we're in geometric mode.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex aea5ca9d44..97051479e4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -492,15 +492,13 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n \treturn NULL;\n }\n \n-static void clear_pack_geometry(struct pack_geometry *geometry)\n+static void free_pack_geometry(struct pack_geometry *geometry)\n {\n \tif (!geometry)\n \t\treturn;\n \n \tfree(geometry->pack);\n-\tgeometry->pack_nr = 0;\n-\tgeometry->pack_alloc = 0;\n-\tgeometry->split = 0;\n+\tfree(geometry);\n }\n \n struct midx_snapshot_ref_data {\n@@ -1228,7 +1226,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n-\tclear_pack_geometry(geometry);\n+\tfree_pack_geometry(geometry);\n \n \treturn ret;\n }\n-- \n2.42.0.rc0.27.g2e2a760381\n\n"},{"id":"480396","messageId":"cover.1691613149.git.me@ttaylorr.com","threadId":"60082","inReplyTo":"20230808211759.GA322409@coredump.intra.peff.net","subject":"[PATCH 0/2] repack: prevent geometry struct leak","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-09T20:32:36Z","receivedAt":"2023-08-09T20:32:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series combines a patch from Peff and a patch from me to\nprevent leaking the 'struct pack_geometry *' pointer from the repack\ncode.\n\nThe first patch free()s the heap-allocated struct, and the second\npatch moves the variable to be allocated on the stack.\n\nJeff King (1):\n  repack: free geometry struct\n\nTaylor Blau (1):\n  repack: move `pack_geometry` struct to the stack\n\n builtin/repack.c | 66 +++++++++++++++++++++++-------------------------\n 1 file changed, 32 insertions(+), 34 deletions(-)\n\n-- \n2.42.0.rc0.27.g2e2a760381\n"},{"id":"480397","messageId":"2e2a760381be1f7cdae83e2f43b17d16aa8ab161.1691613149.git.me@ttaylorr.com","threadId":"60082","inReplyTo":"cover.1691613149.git.me@ttaylorr.com","subject":"[PATCH 2/2] repack: move `pack_geometry` struct to the stack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-09T20:32:42Z","receivedAt":"2023-08-09T20:32:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `pack_geometry` struct is used to maintain and partition a list of\npackfiles into a \"frozen\" set (to be left alone), and a non-frozen set\n(to be combined into a single new pack). In the previous commit, we\nremoved a leak caused by neglecting to free() the heap allocated space\nused to store the structure itself.\n\nBut there is no need for this structure to live on the heap anyway.\nInstead, let's move it to be stack allocated, eliminating the\npossibility of a direct leak like the one addressed in the previous\npatch.\n\nThe one minor hitch is that we use the NULL-ness of the pack_geometry's\nstruct pointer to determine whether or not we are performing a geometric\nrepack with `--geometric=<d>`. But since we only initialize the\npack_geometry structure when the `geometric_factor` is non-zero, we can\nuse that variable (based on whether or not it is equal to zero) to\ndetermine whether or not we are performing a geometric repack.\n\nThere are a couple of spots that have access to a pointer to the\npack_geometry struct, but not the geometric_factor itself. Instead of\npassing in an additional variable, let's make the geometric_factor a\nfield of the pack_geometry struct.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 62 ++++++++++++++++++++++++------------------------\n 1 file changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 97051479e4..2b43a5be08 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -303,6 +303,8 @@ struct pack_geometry {\n \tstruct packed_git **pack;\n \tuint32_t pack_nr, pack_alloc;\n \tuint32_t split;\n+\n+\tint split_factor;\n };\n \n static uint32_t geometry_pack_weight(struct packed_git *p)\n@@ -324,17 +326,13 @@ static int geometry_cmp(const void *va, const void *vb)\n \treturn 0;\n }\n \n-static void init_pack_geometry(struct pack_geometry **geometry_p,\n+static void init_pack_geometry(struct pack_geometry *geometry,\n \t\t\t       struct string_list *existing_kept_packs,\n \t\t\t       const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n-\tstruct pack_geometry *geometry;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n-\tgeometry = *geometry_p;\n-\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (args->local && !p->pack_local)\n \t\t\t/*\n@@ -380,7 +378,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tstrbuf_release(&buf);\n }\n \n-static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n+static void split_pack_geometry(struct pack_geometry *geometry)\n {\n \tuint32_t i;\n \tuint32_t split;\n@@ -399,12 +397,14 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \t\tstruct packed_git *ours = geometry->pack[i];\n \t\tstruct packed_git *prev = geometry->pack[i - 1];\n \n-\t\tif (unsigned_mult_overflows(factor, geometry_pack_weight(prev)))\n+\t\tif (unsigned_mult_overflows(geometry->split_factor,\n+\t\t\t\t\t    geometry_pack_weight(prev)))\n \t\t\tdie(_(\"pack %s too large to consider in geometric \"\n \t\t\t      \"progression\"),\n \t\t\t    prev->pack_name);\n \n-\t\tif (geometry_pack_weight(ours) < factor * geometry_pack_weight(prev))\n+\t\tif (geometry_pack_weight(ours) <\n+\t\t    geometry->split_factor * geometry_pack_weight(prev))\n \t\t\tbreak;\n \t}\n \n@@ -439,10 +439,12 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \tfor (i = split; i < geometry->pack_nr; i++) {\n \t\tstruct packed_git *ours = geometry->pack[i];\n \n-\t\tif (unsigned_mult_overflows(factor, total_size))\n+\t\tif (unsigned_mult_overflows(geometry->split_factor,\n+\t\t\t\t\t    total_size))\n \t\t\tdie(_(\"pack %s too large to roll up\"), ours->pack_name);\n \n-\t\tif (geometry_pack_weight(ours) < factor * total_size) {\n+\t\tif (geometry_pack_weight(ours) <\n+\t\t    geometry->split_factor * total_size) {\n \t\t\tif (unsigned_add_overflows(total_size,\n \t\t\t\t\t\t   geometry_pack_weight(ours)))\n \t\t\t\tdie(_(\"pack %s too large to roll up\"),\n@@ -498,7 +500,6 @@ static void free_pack_geometry(struct pack_geometry *geometry)\n \t\treturn;\n \n \tfree(geometry->pack);\n-\tfree(geometry);\n }\n \n struct midx_snapshot_ref_data {\n@@ -575,7 +576,7 @@ static void midx_included_packs(struct string_list *include,\n \t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \tfor_each_string_list_item(item, names)\n \t\tstring_list_insert(include, xstrfmt(\"pack-%s.idx\", item->string));\n-\tif (geometry) {\n+\tif (geometry->split_factor) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tuint32_t i;\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n@@ -779,7 +780,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n \tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n-\tstruct pack_geometry *geometry = NULL;\n+\tstruct pack_geometry geometry = { 0 };\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n \tint i, ext, ret;\n@@ -793,7 +794,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tstruct pack_objects_args po_args = {NULL};\n \tstruct pack_objects_args cruft_po_args = {NULL};\n-\tint geometric_factor = 0;\n \tint write_midx = 0;\n \tconst char *cruft_expiration = NULL;\n \tconst char *expire_to = NULL;\n@@ -842,7 +842,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n-\t\tOPT_INTEGER('g', \"geometric\", &geometric_factor,\n+\t\tOPT_INTEGER('g', \"geometric\", &geometry.split_factor,\n \t\t\t    N_(\"find a geometric progression with factor <N>\")),\n \t\tOPT_BOOL('m', \"write-midx\", &write_midx,\n \t\t\t   N_(\"write a multi-pack index of the resulting packs\")),\n@@ -918,11 +918,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n \t\t\t       &keep_pack_list);\n \n-\tif (geometric_factor) {\n+\tif (geometry.split_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n \t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n-\t\tsplit_pack_geometry(geometry, geometric_factor);\n+\t\tsplit_pack_geometry(&geometry);\n \t}\n \n \tprepare_pack_objects(&cmd, &po_args, packtmp);\n@@ -936,7 +936,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n \t\t\t     keep_pack_list.items[i].string);\n \tstrvec_push(&cmd.args, \"--non-empty\");\n-\tif (!geometry) {\n+\tif (!geometry.split_factor) {\n \t\t/*\n \t\t * We need to grab all reachable objects, including those that\n \t\t * are reachable from reflogs and the index.\n@@ -983,7 +983,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t\t\t}\n \t\t}\n-\t} else if (geometry) {\n+\t} else if (geometry.split_factor) {\n \t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t} else {\n@@ -991,7 +991,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrvec_push(&cmd.args, \"--incremental\");\n \t}\n \n-\tif (geometry)\n+\tif (geometry.split_factor)\n \t\tcmd.in = -1;\n \telse\n \t\tcmd.no_stdin = 1;\n@@ -1000,17 +1000,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (ret)\n \t\tgoto cleanup;\n \n-\tif (geometry) {\n+\tif (geometry.split_factor) {\n \t\tFILE *in = xfdopen(cmd.in, \"w\");\n \t\t/*\n \t\t * The resulting pack should contain all objects in packs that\n \t\t * are going to be rolled up, but exclude objects in packs which\n \t\t * are being left alone.\n \t\t */\n-\t\tfor (i = 0; i < geometry->split; i++)\n-\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry->pack[i]));\n-\t\tfor (i = geometry->split; i < geometry->pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry->pack[i]));\n+\t\tfor (i = 0; i < geometry.split; i++)\n+\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n+\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n+\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n \t\tfclose(in);\n \t}\n \n@@ -1153,9 +1153,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n \t\tmidx_included_packs(&include, &existing_nonkept_packs,\n-\t\t\t\t    &existing_kept_packs, &names, geometry);\n+\t\t\t\t    &existing_kept_packs, &names, &geometry);\n \n-\t\tret = write_midx_included_packs(&include, geometry,\n+\t\tret = write_midx_included_packs(&include, &geometry,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n \n@@ -1178,12 +1178,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n \n-\t\tif (geometry) {\n+\t\tif (geometry.split_factor) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\t\tuint32_t i;\n-\t\t\tfor (i = 0; i < geometry->split; i++) {\n-\t\t\t\tstruct packed_git *p = geometry->pack[i];\n+\t\t\tfor (i = 0; i < geometry.split; i++) {\n+\t\t\t\tstruct packed_git *p = geometry.pack[i];\n \t\t\t\tif (string_list_has_string(&names,\n \t\t\t\t\t\t\t   hash_to_hex(p->hash)))\n \t\t\t\t\tcontinue;\n@@ -1226,7 +1226,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n-\tfree_pack_geometry(geometry);\n+\tfree_pack_geometry(&geometry);\n \n \treturn ret;\n }\n-- \n2.42.0.rc0.27.g2e2a760381\n"},{"id":"480414","messageId":"20230810001921.GB795985@coredump.intra.peff.net","threadId":"60082","inReplyTo":"cover.1691613149.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] repack: prevent geometry struct leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-10T00:19:21Z","receivedAt":"2023-08-10T00:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 09, 2023 at 04:32:36PM -0400, Taylor Blau wrote:\n\n> This short series combines a patch from Peff and a patch from me to\n> prevent leaking the 'struct pack_geometry *' pointer from the repack\n> code.\n> \n> The first patch free()s the heap-allocated struct, and the second\n> patch moves the variable to be allocated on the stack.\n\nThanks for cleaning up your patch. I think Junio has the first one in\n'next' already as jk/repack-leakfix, but your patch 2 can go on top.\n\nThe result looks good to me. It's a little unusual that the \"init\"\nfunction expects the caller to have already initialized-to-zero and set\nthe split_factor field, but I think that's OK for a struct that is\nlimited to this one file.\n\n-Peff\n"}]}