{"thread":{"id":"56790","subject":"[PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'","startedAt":"2021-10-26T21:01:05Z","lastAt":"2021-10-28T20:25:52Z","messageCount":13,"participants":["Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":9},"messages":[{"id":"439673","messageId":"cover.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":null,"subject":"[PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:00Z","receivedAt":"2021-10-26T21:01:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small-ish update to my series which makes t5319 leak-free (i.e., it\npasses even when Git is compiled with SANITIZE=leak). It is based on\nab/only-single-progress-at-once (so I dropped the cherry-picked patch towards\nthe end from Ævar).\n\nMost of the fixes are straightforward from review, but the biggest change is in\n7/10, which converts get_midx_filename() to write its result to a strbuf per\nJunio's helpful suggestion.\n\nThinking ahead, there are a few things I noted while reading through replies for\nfuture improvements to the MIDX and pack bitmap code. From my ~/notes, they are:\n\n  - load_multi_pack_index() should not die when missing the packfile names chunk,\n    instead should pretend that no MIDX exists (maybe warn?)\n  - double-close when reaching the cleanup_fail codepath after xmmap returns\n  - static `verify_midx_error` is ugly\n  - load_bitmap() does not clean up after itself very well (e.g.,\n    bitmap_git->trees and others)\n\nI'm pretty happy with the state of this topic, and I'll plan on taking on the\nabove in separate series in the future.\n\nTaylor Blau (9):\n  midx.c: clean up chunkfile after reading the MIDX\n  midx.c: don't leak MIDX from verify_midx_file\n  t/helper/test-read-midx.c: free MIDX within read_midx_file()\n  builtin/pack-objects.c: don't leak memory via arguments\n  builtin/repack.c: avoid leaking child arguments\n  builtin/multi-pack-index.c: don't leak concatenated options\n  midx.c: write MIDX filenames to strbuf\n  pack-bitmap.c: don't leak type-level bitmaps\n  pack-bitmap.c: more aggressively free in free_bitmap_index()\n\n builtin/multi-pack-index.c |  4 +++\n builtin/pack-objects.c     | 11 ++++---\n builtin/repack.c           | 23 +++++++++----\n midx.c                     | 66 ++++++++++++++++++++++----------------\n midx.h                     |  4 +--\n pack-bitmap.c              | 29 ++++++++++++++---\n pack-revindex.c            |  8 ++---\n t/helper/test-read-midx.c  |  3 +-\n 8 files changed, 97 insertions(+), 51 deletions(-)\n\nRange-diff against v1:\n 1:  30f6f23daf =  1:  dcc5998072 midx.c: clean up chunkfile after reading the MIDX\n 2:  b0c79904ab !  2:  258a9e2e57 midx.c: don't leak MIDX from verify_midx_file\n    @@ Metadata\n      ## Commit message ##\n         midx.c: don't leak MIDX from verify_midx_file\n     \n    -    The function midx.c:verify_midx_file() allocate a MIDX struct by calling\n    -    load_multi_pack_index(). But when cleaning up, it calls free() without\n    -    freeing any resources associated with the MIDX.\n    +    The function midx.c:verify_midx_file() allocates a MIDX struct by\n    +    calling load_multi_pack_index(). But when cleaning up, it calls free()\n    +    without freeing any resources associated with the MIDX.\n     \n         Call the more appropriate close_midx() which does free those resources,\n         which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n 3:  5157edb41e =  3:  84859d5b53 t/helper/test-read-midx.c: free MIDX within read_midx_file()\n 4:  dd3b9a949e =  4:  aedb1713b4 builtin/pack-objects.c: don't leak memory via arguments\n 5:  a68c77c006 !  5:  bcd12ecab8 builtin/repack.c: avoid leaking child arguments\n    @@ Commit message\n         In none of these cases do we bother to call child_process_clear(), which\n         frees the memory associated with each child's arguments and environment.\n     \n    -    In order to do so, tweak each function that spawns a child process to\n    -    have a `cleanup` label that we always visit before returning from each\n    -    function. Then, make sure that we call child_process_clear() as a part\n    -    of that label.\n    +    Make sure that we call child_process_clear() in any functions which\n    +    initialize a struct child_process before returning along a path which\n    +    did not call finish_command().\n    +\n    +    In cmd_repack(), take a slightly different approach to use a cleanup\n    +    label to clear the child_process, unless finish_command() was called.\n    +    This allows us to free other memory allocated during the lifetime of\n    +    that function. But it avoids calling child_process_clear() twice (the\n    +    other call coming from inside of finish_command()) to avoid assuming the\n    +    function's implementation is idempotent.\n     \n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## builtin/repack.c ##\n     @@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,\n    - \tstruct child_process cmd = CHILD_PROCESS_INIT;\n    - \tFILE *out;\n    - \tstruct strbuf line = STRBUF_INIT;\n    -+\tint ret = 0;\n    + \tfor_each_packed_object(write_oid, &cmd,\n    + \t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n      \n    - \tprepare_pack_objects(&cmd, args);\n    - \tcmd.in = -1;\n    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,\n    - \n    - \tif (cmd.in == -1)\n    +-\tif (cmd.in == -1)\n    ++\tif (cmd.in == -1) {\n      \t\t/* No packed objects; cmd was never started */\n    --\t\treturn;\n    -+\t\tgoto cleanup;\n    ++\t\tchild_process_clear(&cmd);\n    + \t\treturn;\n    ++\t}\n      \n      \tclose(cmd.in);\n      \n    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,\n    - \t\tfree(promisor_name);\n    - \t}\n    - \tfclose(out);\n    --\tif (finish_command(&cmd))\n    -+\tret = finish_command(&cmd);\n    -+\n    -+cleanup:\n    -+\tchild_process_clear(&cmd);\n    -+\n    -+\tif (ret)\n    - \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n    - }\n    - \n    -@@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,\n    - \tstruct string_list_item *item;\n    - \tstruct packed_git *largest = get_largest_active_pack(geometry);\n    - \tFILE *in;\n    --\tint ret;\n    -+\tint ret = 0;\n    - \n    - \tif (!include->nr)\n    --\t\treturn 0;\n    -+\t\tgoto cleanup;\n    - \n    - \tcmd.in = -1;\n    - \tcmd.git_cmd = 1;\n     @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,\n    + \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n      \n      \tret = start_command(&cmd);\n    - \tif (ret)\n    --\t\treturn ret;\n    -+\t\tgoto cleanup;\n    +-\tif (ret)\n    ++\tif (ret) {\n    ++\t\tchild_process_clear(&cmd);\n    + \t\treturn ret;\n    ++\t}\n      \n      \tin = xfdopen(cmd.in, \"w\");\n      \tfor_each_string_list_item(item, include)\n    - \t\tfprintf(in, \"%s\\n\", item->string);\n    - \tfclose(in);\n    - \n    --\treturn finish_command(&cmd);\n    -+\tret = finish_command(&cmd);\n    -+\n    -+cleanup:\n    -+\tchild_process_clear(&cmd);\n    -+\n    -+\treturn ret;\n    - }\n    - \n    - int cmd_repack(int argc, const char **argv, const char *prefix)\n     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n      \tstruct pack_geometry *geometry = NULL;\n      \tstruct strbuf line = STRBUF_INIT;\n    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n     +\tint i, ext, ret = 0;\n      \tFILE *out;\n      \tint show_progress = isatty(2);\n    ++\tint cmd_cleared = 0;\n      \n    + \t/* variables to be filled by option parsing */\n    + \tint pack_everything = 0;\n     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n      \n      \tret = start_command(&cmd);\n    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n      \tif (geometry) {\n      \t\tFILE *in = xfdopen(cmd.in, \"w\");\n     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n    + \t}\n      \tfclose(out);\n      \tret = finish_command(&cmd);\n    ++\tcmd_cleared = 1;\n      \tif (ret)\n     -\t\treturn ret;\n     +\t\tgoto cleanup;\n    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n      \tstring_list_clear(&existing_kept_packs, 0);\n      \tclear_pack_geometry(geometry);\n      \tstrbuf_release(&line);\n    -+\tchild_process_clear(&cmd);\n    ++\tif (!cmd_cleared)\n    ++\t\tchild_process_clear(&cmd);\n      \n     -\treturn 0;\n     +\treturn ret;\n 6:  ffded80c7d =  6:  0d252cd323 builtin/multi-pack-index.c: don't leak concatenated options\n 7:  f3897c3afc <  -:  ---------- pack-bitmap.c: avoid leaking via midx_bitmap_filename()\n -:  ---------- >  7:  0f293ab638 midx.c: write MIDX filenames to strbuf\n 8:  29920e7735 =  8:  77a4454632 pack-bitmap.c: don't leak type-level bitmaps\n 9:  e65ac7deb5 !  9:  c1e7e6cc92 pack-bitmap.c: more aggressively free in free_bitmap_index()\n    @@ Commit message\n         The function free_bitmap_index() is somewhat lax in what it frees. There\n         are two notable examples:\n     \n    -      - While it does call kh_destroy_oid_map on the \"bitmaps\" map (which\n    -        maps) commit OIDs to their corresponding bitmaps, the bitmaps\n    +      - While it does call kh_destroy_oid_map on the \"bitmaps\" map, which\n    +        maps commit OIDs to their corresponding bitmaps, the bitmaps\n             themselves are not freed. Note here that we recycle already-freed\n             ewah_bitmaps into a pool, but these are handled correctly by\n             ewah_pool_free().\n10:  cb30aa67c0 <  -:  ---------- pack-bitmap-write.c: don't return without stop_progress()\n11:  f1bb8b73ff <  -:  ---------- t5319: UNLEAK() the remaining leaks\n-- \n2.33.0.96.g73915697e6\n"},{"id":"439674","messageId":"dcc599807292cbd03a0a60d5ff0f73f49ea45234.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 1/9] midx.c: clean up chunkfile after reading the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:05Z","receivedAt":"2021-10-26T21:01:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In order to read the contents of a MIDX, we initialize a chunkfile\nstructure which can read the table of contents and assign pointers into\ndifferent sections of the file for us.\n\nWe do call free(), since the chunkfile struct is heap allocated, but not\nthe more appropriate free_chunkfile(), which also frees memory that the\nstructure itself owns.\n\nCall that instead to avoid leaking memory in this function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8433086ac1..36e4754767 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_packs\", m->num_packs);\n \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_objects\", m->num_objects);\n \n+\tfree_chunkfile(cf);\n \treturn m;\n \n cleanup_fail:\n \tfree(m);\n \tfree(midx_name);\n-\tfree(cf);\n+\tfree_chunkfile(cf);\n \tif (midx_map)\n \t\tmunmap(midx_map, midx_size);\n \tif (0 <= fd)\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439675","messageId":"258a9e2e57eaa81c906a05f1d7f8c193d7efebae.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 2/9] midx.c: don't leak MIDX from verify_midx_file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:08Z","receivedAt":"2021-10-26T21:01:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function midx.c:verify_midx_file() allocates a MIDX struct by\ncalling load_multi_pack_index(). But when cleaning up, it calls free()\nwithout freeing any resources associated with the MIDX.\n\nCall the more appropriate close_midx() which does free those resources,\nwhich causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 36e4754767..ad60e48468 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t * Remaining tests assume that we have objects, so we can\n \t\t * return here.\n \t\t */\n-\t\treturn verify_midx_error;\n+\t\tgoto cleanup;\n \t}\n \n \tif (flags & MIDX_PROGRESS)\n@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t}\n \tstop_progress(&progress);\n \n+cleanup:\n \tfree(pairs);\n+\tclose_midx(m);\n \n \treturn verify_midx_error;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439676","messageId":"84859d5b534606ab243b82afa119514f71ca1c03.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 3/9] t/helper/test-read-midx.c: free MIDX within read_midx_file()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:11Z","receivedAt":"2021-10-26T21:01:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When calling `read_midx_file()` to show information about a MIDX or list\nthe objects contained within it we fail to call `close_midx()`, leaking\nthe memory allocated to store that MIDX.\n\nFix this by calling `close_midx()` before exiting the function. We can\ndrop the \"early\" return when `show_objects` is non-zero, since the next\ninstruction is also a return.\n\n(We could just as easily put a `cleanup` label here as with previous\npatches. But the only other time we terminate the function early is\nwhen we fail to load a MIDX in the first place. `close_midx()` does\nhandle a NULL argument, but the extra complexity is likely not\nwarranted).\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/helper/test-read-midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex 9d6fa7a377..27072ba94d 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -55,9 +55,10 @@ static int read_midx_file(const char *object_dir, int show_objects)\n \t\t\tprintf(\"%s %\"PRIu64\"\\t%s\\n\",\n \t\t\t       oid_to_hex(&oid), e.offset, e.p->pack_name);\n \t\t}\n-\t\treturn 0;\n \t}\n \n+\tclose_midx(m);\n+\n \treturn 0;\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439677","messageId":"aedb1713b40f5e0e7c316dd6f5c9b9a1cc0df39c.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 4/9] builtin/pack-objects.c: don't leak memory via arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:13Z","receivedAt":"2021-10-26T21:01:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When constructing arguments to pass to setup_revision(), pack-objects\nonly frees the memory used by that array after calling\nget_object_list().\n\nEnsure that we call strvec_clear() whether or not we use the arguments\narray by cleaning up whenever we exit the function (and rewriting one\nearly return to jump to a label which frees the memory and then\nreturns).\n\nWe could avoid setting this array up altogether unless we are in the\nif-else block that calls get_object_list(), but setting up the argument\narray is intermingled with lots of other side-effects, e.g.:\n\n    if (exclude_promisor_objects) {\n      use_internal_rev_list = 1;\n      fetch_if_missing = 0;\n      strvec_push(&rp, \"--exclude-promisor-objects\");\n    }\n\nSo it would be awkward to check exclude_promisor_objects twice: first to\nset use_internal_rev_list and fetch_if_missing, and then again above\nget_object_list() to push the relevant argument onto the array.\n\nInstead, leave the array's construction alone and make sure to free it\nunconditionally.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1a3dd445f8..857be7826f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4148,11 +4148,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_packs_list_from_stdin();\n \t\tif (rev_list_unpacked)\n \t\t\tadd_unreachable_loose_objects();\n-\t} else if (!use_internal_rev_list)\n+\t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\telse {\n+\t} else {\n \t\tget_object_list(rp.nr, rp.v);\n-\t\tstrvec_clear(&rp);\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n@@ -4162,7 +4161,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t    the_repository);\n \n \tif (non_empty && !nr_result)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \tif (nr_result) {\n \t\ttrace2_region_enter(\"pack-objects\", \"prepare-pack\",\n \t\t\t\t    the_repository);\n@@ -4183,5 +4182,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t     \" pack-reused %\"PRIu32),\n \t\t\t   written, written_delta, reused, reused_delta,\n \t\t\t   reuse_packfile_objects);\n+\n+cleanup:\n+\tstrvec_clear(&rp);\n+\n \treturn 0;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439678","messageId":"bcd12ecab81029be825a646348cb7ae69970a819.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:16Z","receivedAt":"2021-10-26T21:01:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`git repack` invokes a handful of child processes: one to write the\nactual pack, and optionally ones to repack promisor objects and update\nthe MIDX.\n\nIn none of these cases do we bother to call child_process_clear(), which\nfrees the memory associated with each child's arguments and environment.\n\nMake sure that we call child_process_clear() in any functions which\ninitialize a struct child_process before returning along a path which\ndid not call finish_command().\n\nIn cmd_repack(), take a slightly different approach to use a cleanup\nlabel to clear the child_process, unless finish_command() was called.\nThis allows us to free other memory allocated during the lifetime of\nthat function. But it avoids calling child_process_clear() twice (the\nother call coming from inside of finish_command()) to avoid assuming the\nfunction's implementation is idempotent.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0b2d1e5d82..b82f6b485c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tfor_each_packed_object(write_oid, &cmd,\n \t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n \n-\tif (cmd.in == -1)\n+\tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n+\t\tchild_process_clear(&cmd);\n \t\treturn;\n+\t}\n \n \tclose(cmd.in);\n \n@@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n \n \tret = start_command(&cmd);\n-\tif (ret)\n+\tif (ret) {\n+\t\tchild_process_clear(&cmd);\n \t\treturn ret;\n+\t}\n \n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, include)\n@@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct pack_geometry *geometry = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n-\tint i, ext, ret;\n+\tint i, ext, ret = 0;\n \tFILE *out;\n \tint show_progress = isatty(2);\n+\tint cmd_cleared = 0;\n \n \t/* variables to be filled by option parsing */\n \tint pack_everything = 0;\n@@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tret = start_command(&cmd);\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \n \tif (geometry) {\n \t\tFILE *in = xfdopen(cmd.in, \"w\");\n@@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\n+\tcmd_cleared = 1;\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \n \tif (!names.nr && !po_args.quiet)\n \t\tprintf_ln(_(\"Nothing new to pack.\"));\n@@ -893,7 +899,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&include, 0);\n \n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto cleanup;\n \t}\n \n \treprepare_packed_git(the_repository);\n@@ -946,12 +952,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n \t}\n \n+cleanup:\n \tstring_list_clear(&names, 0);\n \tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\n \tstrbuf_release(&line);\n+\tif (!cmd_cleared)\n+\t\tchild_process_clear(&cmd);\n \n-\treturn 0;\n+\treturn ret;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439679","messageId":"0d252cd3239b9dca43fda37f7471b924440f6336.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 6/9] builtin/multi-pack-index.c: don't leak concatenated options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:18Z","receivedAt":"2021-10-26T21:01:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `multi-pack-index` builtin dynamically allocates an array of\ncommand-line option for each of its separate modes by calling\nadd_common_options() to concatante the common options with sub-command\nspecific ones.\n\nBecause this operation allocates a new array, we have to be careful to\nremember to free it. We already do this in the repack and write\nsub-commands, but verify and expire don't. Rectify this by calling\nFREE_AND_NULL as the other modes do.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/multi-pack-index.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 075d15d706..4480ba3982 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -167,6 +167,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n \t\t\t\t   options);\n \n+\tFREE_AND_NULL(options);\n+\n \treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n }\n \n@@ -191,6 +193,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n \t\t\t\t   options);\n \n+\tFREE_AND_NULL(options);\n+\n \treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439680","messageId":"0f293ab63825e767043876d951ef6c742a540096.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 7/9] midx.c: write MIDX filenames to strbuf","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:21Z","receivedAt":"2021-10-26T21:01:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To ask for the name of a MIDX and its corresponding .rev file, callers\ninvoke get_midx_filename() and get_midx_rev_filename(), respectively.\nThese both invoke xstrfmt(), allocating a chunk of memory which must be\nfreed later on.\n\nThis makes callers in pack-bitmap.c somewhat awkward. Specifically,\nmidx_bitmap_filename(), which is implemented like:\n\n    return xstrfmt(\"%s-%s.bitmap\",\n                   get_midx_filename(midx->object_dir),\n                   hash_to_hex(get_midx_checksum(midx)));\n\nthis leaks the second argument to xstrfmt(), which itself was allocated\nwith xstrfmt(). This caller could assign both the result of\nget_midx_filename() and the outer xstrfmt() to a temporary variable,\nremembering to free() the former before returning. But that involves a\nwasteful copy.\n\nInstead, get_midx_filename() and get_midx_rev_filename() take a strbuf\nas an output parameter. This way midx_bitmap_filename() can manipulate\nand pass around a temporary buffer which it detaches back to its caller.\n\nThat allows us to implement the function without copying or open-coding\nget_midx_filename() in a way that doesn't leak.\n\nUpdate the other callers of get_midx_filename() and\nget_midx_rev_filename() accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c          | 59 +++++++++++++++++++++++++++----------------------\n midx.h          |  4 ++--\n pack-bitmap.c   | 15 ++++++++-----\n pack-revindex.c |  8 +++----\n 4 files changed, 49 insertions(+), 37 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex ad60e48468..837b46b2af 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -57,15 +57,15 @@ const unsigned char *get_midx_checksum(struct multi_pack_index *m)\n \treturn m->data + m->data_len - the_hash_algo->rawsz;\n }\n \n-char *get_midx_filename(const char *object_dir)\n+void get_midx_filename(struct strbuf *out, const char *object_dir)\n {\n-\treturn xstrfmt(\"%s/pack/multi-pack-index\", object_dir);\n+\tstrbuf_addf(out, \"%s/pack/multi-pack-index\", object_dir);\n }\n \n-char *get_midx_rev_filename(struct multi_pack_index *m)\n+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n {\n-\treturn xstrfmt(\"%s/pack/multi-pack-index-%s.rev\",\n-\t\t       m->object_dir, hash_to_hex(get_midx_checksum(m)));\n+\tget_midx_filename(out, m->object_dir);\n+\tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n }\n \n static int midx_read_oid_fanout(const unsigned char *chunk_start,\n@@ -89,28 +89,30 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tsize_t midx_size;\n \tvoid *midx_map = NULL;\n \tuint32_t hash_version;\n-\tchar *midx_name = get_midx_filename(object_dir);\n+\tstruct strbuf midx_name = STRBUF_INIT;\n \tuint32_t i;\n \tconst char *cur_pack_name;\n \tstruct chunkfile *cf = NULL;\n \n-\tfd = git_open(midx_name);\n+\tget_midx_filename(&midx_name, object_dir);\n+\n+\tfd = git_open(midx_name.buf);\n \n \tif (fd < 0)\n \t\tgoto cleanup_fail;\n \tif (fstat(fd, &st)) {\n-\t\terror_errno(_(\"failed to read %s\"), midx_name);\n+\t\terror_errno(_(\"failed to read %s\"), midx_name.buf);\n \t\tgoto cleanup_fail;\n \t}\n \n \tmidx_size = xsize_t(st.st_size);\n \n \tif (midx_size < MIDX_MIN_SIZE) {\n-\t\terror(_(\"multi-pack-index file %s is too small\"), midx_name);\n+\t\terror(_(\"multi-pack-index file %s is too small\"), midx_name.buf);\n \t\tgoto cleanup_fail;\n \t}\n \n-\tFREE_AND_NULL(midx_name);\n+\tstrbuf_release(&midx_name);\n \n \tmidx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n@@ -184,7 +186,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \n cleanup_fail:\n \tfree(m);\n-\tfree(midx_name);\n+\tstrbuf_release(&midx_name);\n \tfree_chunkfile(cf);\n \tif (midx_map)\n \t\tmunmap(midx_map, midx_size);\n@@ -1131,7 +1133,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t\t       const char *refs_snapshot,\n \t\t\t       unsigned flags)\n {\n-\tchar *midx_name;\n+\tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n \tuint32_t i;\n \tstruct hashfile *f = NULL;\n@@ -1142,10 +1144,10 @@ static int write_midx_internal(const char *object_dir,\n \tint result = 0;\n \tstruct chunkfile *cf;\n \n-\tmidx_name = get_midx_filename(object_dir);\n-\tif (safe_create_leading_directories(midx_name))\n+\tget_midx_filename(&midx_name, object_dir);\n+\tif (safe_create_leading_directories(midx_name.buf))\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n-\t\t\t  midx_name);\n+\t\t\t  midx_name.buf);\n \n \tif (!packs_to_include) {\n \t\t/*\n@@ -1374,7 +1376,7 @@ static int write_midx_internal(const char *object_dir,\n \t\tpack_name_concat_len += MIDX_CHUNK_ALIGNMENT -\n \t\t\t\t\t(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);\n \n-\thold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lk, midx_name.buf, LOCK_DIE_ON_ERROR);\n \tf = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));\n \n \tif (ctx.nr - dropped_packs == 0) {\n@@ -1411,9 +1413,9 @@ static int write_midx_internal(const char *object_dir,\n \t\tctx.pack_order = midx_pack_order(&ctx);\n \n \tif (flags & MIDX_WRITE_REV_INDEX)\n-\t\twrite_midx_reverse_index(midx_name, midx_hash, &ctx);\n+\t\twrite_midx_reverse_index(midx_name.buf, midx_hash, &ctx);\n \tif (flags & MIDX_WRITE_BITMAP) {\n-\t\tif (write_midx_bitmap(midx_name, midx_hash, &ctx,\n+\t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,\n \t\t\t\t      refs_snapshot, flags) < 0) {\n \t\t\terror(_(\"could not write multi-pack bitmap\"));\n \t\t\tresult = 1;\n@@ -1443,7 +1445,7 @@ static int write_midx_internal(const char *object_dir,\n \tfree(ctx.entries);\n \tfree(ctx.pack_perm);\n \tfree(ctx.pack_order);\n-\tfree(midx_name);\n+\tstrbuf_release(&midx_name);\n \n \treturn result;\n }\n@@ -1507,20 +1509,22 @@ static void clear_midx_files_ext(const char *object_dir, const char *ext,\n \n void clear_midx_file(struct repository *r)\n {\n-\tchar *midx = get_midx_filename(r->objects->odb->path);\n+\tstruct strbuf midx = STRBUF_INIT;\n+\n+\tget_midx_filename(&midx, r->objects->odb->path);\n \n \tif (r->objects && r->objects->multi_pack_index) {\n \t\tclose_midx(r->objects->multi_pack_index);\n \t\tr->objects->multi_pack_index = NULL;\n \t}\n \n-\tif (remove_path(midx))\n-\t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n+\tif (remove_path(midx.buf))\n+\t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx.buf);\n \n \tclear_midx_files_ext(r->objects->odb->path, \".bitmap\", NULL);\n \tclear_midx_files_ext(r->objects->odb->path, \".rev\", NULL);\n \n-\tfree(midx);\n+\tstrbuf_release(&midx);\n }\n \n static int verify_midx_error;\n@@ -1573,12 +1577,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \tif (!m) {\n \t\tint result = 0;\n \t\tstruct stat sb;\n-\t\tchar *filename = get_midx_filename(object_dir);\n-\t\tif (!stat(filename, &sb)) {\n+\t\tstruct strbuf filename = STRBUF_INIT;\n+\n+\t\tget_midx_filename(&filename, object_dir);\n+\n+\t\tif (!stat(filename.buf, &sb)) {\n \t\t\terror(_(\"multi-pack-index file exists, but failed to parse\"));\n \t\t\tresult = 1;\n \t\t}\n-\t\tfree(filename);\n+\t\tstrbuf_release(&filename);\n \t\treturn result;\n \t}\n \ndiff --git a/midx.h b/midx.h\nindex 6e32297fa3..b7d79a515c 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -48,8 +48,8 @@ struct multi_pack_index {\n #define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n \n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n-char *get_midx_filename(const char *object_dir);\n-char *get_midx_rev_filename(struct multi_pack_index *m);\n+void get_midx_filename(struct strbuf *out, const char *object_dir);\n+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m);\n \n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\n int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex f47a0a7db4..3f603425c9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -292,9 +292,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \n char *midx_bitmap_filename(struct multi_pack_index *midx)\n {\n-\treturn xstrfmt(\"%s-%s.bitmap\",\n-\t\t       get_midx_filename(midx->object_dir),\n-\t\t       hash_to_hex(get_midx_checksum(midx)));\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tget_midx_filename(&buf, midx->object_dir);\n+\tstrbuf_addf(&buf, \"-%s.bitmap\", hash_to_hex(get_midx_checksum(midx)));\n+\n+\treturn strbuf_detach(&buf, NULL);\n }\n \n char *pack_bitmap_filename(struct packed_git *p)\n@@ -324,10 +327,12 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tget_midx_filename(&buf, midx->object_dir);\n \t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(\"ignoring extra bitmap file: %s\",\n-\t\t\tget_midx_filename(midx->object_dir));\n+\t\twarning(\"ignoring extra bitmap file: %s\", buf.buf);\n \t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n \t\treturn -1;\n \t}\n \ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 0e4a31d9db..70d0fbafcb 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -296,14 +296,14 @@ int load_pack_revindex(struct packed_git *p)\n \n int load_midx_revindex(struct multi_pack_index *m)\n {\n-\tchar *revindex_name;\n+\tstruct strbuf revindex_name = STRBUF_INIT;\n \tint ret;\n \tif (m->revindex_data)\n \t\treturn 0;\n \n-\trevindex_name = get_midx_rev_filename(m);\n+\tget_midx_rev_filename(&revindex_name, m);\n \n-\tret = load_revindex_from_disk(revindex_name,\n+\tret = load_revindex_from_disk(revindex_name.buf,\n \t\t\t\t      m->num_objects,\n \t\t\t\t      &m->revindex_map,\n \t\t\t\t      &m->revindex_len);\n@@ -313,7 +313,7 @@ int load_midx_revindex(struct multi_pack_index *m)\n \tm->revindex_data = (const uint32_t *)((const char *)m->revindex_map + RIDX_HEADER_SIZE);\n \n cleanup:\n-\tfree(revindex_name);\n+\tstrbuf_release(&revindex_name);\n \treturn ret;\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439681","messageId":"77a445463298cac69f00a0b9c5b5951bae80b2dd.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 8/9] pack-bitmap.c: don't leak type-level bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:23Z","receivedAt":"2021-10-26T21:01:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,\nwhich compares the result of the on-disk bitmaps with ones generated\non-the-fly during a revision walk.\n\nIn fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type\nbitmaps, 2021-08-24), we hardened those tests to also check the four\nspecial type-level bitmaps, but never freed those bitmaps. We should\nhave, since each required an allocation when we EWAH-decompressed them.\n\nFree those, plugging that leak, and also free the base (the scratch-pad\nbitmap), too.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3f603425c9..3d81425c29 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1726,6 +1726,12 @@ void test_bitmap_walk(struct rev_info *revs)\n \telse\n \t\tdie(\"mismatch in bitmap results\");\n \n+\tbitmap_free(result);\n+\tbitmap_free(tdata.base);\n+\tbitmap_free(tdata.commits);\n+\tbitmap_free(tdata.trees);\n+\tbitmap_free(tdata.blobs);\n+\tbitmap_free(tdata.tags);\n \tfree_bitmap_index(bitmap_git);\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439682","messageId":"c1e7e6cc924f61d237e5cc7045ad9698fbe4c3b2.1635282024.git.me@ttaylorr.com","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"[PATCH v2 9/9] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:01:26Z","receivedAt":"2021-10-26T21:01:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function free_bitmap_index() is somewhat lax in what it frees. There\nare two notable examples:\n\n  - While it does call kh_destroy_oid_map on the \"bitmaps\" map, which\n    maps commit OIDs to their corresponding bitmaps, the bitmaps\n    themselves are not freed. Note here that we recycle already-freed\n    ewah_bitmaps into a pool, but these are handled correctly by\n    ewah_pool_free().\n\n  - We never bother to free the extended index's \"positions\" map, which\n    we always allocate in load_bitmap().\n\nFix both of these.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3d81425c29..a56ceb9441 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1859,9 +1859,17 @@ void free_bitmap_index(struct bitmap_index *b)\n \tewah_pool_free(b->trees);\n \tewah_pool_free(b->blobs);\n \tewah_pool_free(b->tags);\n+\tif (b->bitmaps) {\n+\t\tstruct stored_bitmap *sb;\n+\t\tkh_foreach_value(b->bitmaps, sb, {\n+\t\t\tewah_pool_free(sb->root);\n+\t\t\tfree(sb);\n+\t\t});\n+\t}\n \tkh_destroy_oid_map(b->bitmaps);\n \tfree(b->ext_index.objects);\n \tfree(b->ext_index.hashes);\n+\tkh_destroy_oid_pos(b->ext_index.positions);\n \tbitmap_free(b->result);\n \tbitmap_free(b->haves);\n \tif (bitmap_is_midx(b)) {\n-- \n2.33.0.96.g73915697e6\n"},{"id":"439846","messageId":"xmqqzgqut4lr.fsf@gitster.g","threadId":"56790","inReplyTo":"bcd12ecab81029be825a646348cb7ae69970a819.1635282024.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T23:44:48Z","receivedAt":"2021-10-27T23:44:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,\n>  \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n>  \n>  \tret = start_command(&cmd);\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\tchild_process_clear(&cmd);\n>  \t\treturn ret;\n> +\t}\n\nThis happens only when start_command() returns an error.  But the\nfunction always calls child_process_clear() before doing so.\n\nSo I am not sure if this hunk is needed.  It didn't exist in v1, if\nI recall correctly.  Am I missing something obvious?\n\n> @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tstruct pack_geometry *geometry = NULL;\n>  \tstruct strbuf line = STRBUF_INIT;\n>  \tstruct tempfile *refs_snapshot = NULL;\n> -\tint i, ext, ret;\n> +\tint i, ext, ret = 0;\n>  \tFILE *out;\n>  \tint show_progress = isatty(2);\n> +\tint cmd_cleared = 0;\n>  \n>  \t/* variables to be filled by option parsing */\n>  \tint pack_everything = 0;\n> @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \tret = start_command(&cmd);\n>  \tif (ret)\n> -\t\treturn ret;\n> +\t\tgoto cleanup;\n\nLikewise, at this point, start_command() should have already cleared\ncmd before it returned an error, no?\n\n> @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \tfclose(out);\n>  \tret = finish_command(&cmd);\n> +\tcmd_cleared = 1;\n>  \tif (ret)\n> -\t\treturn ret;\n> +\t\tgoto cleanup;\n\nAnd cmd is also cleared after we pass this point.  So perhaps after\nthe cleanup label, there is no need to call child_process_clear() at\nall, no?\n"},{"id":"439847","messageId":"xmqqv91it4de.fsf@gitster.g","threadId":"56790","inReplyTo":"cover.1635282024.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T23:49:49Z","receivedAt":"2021-10-27T23:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Here is a small-ish update to my series which makes t5319 leak-free (i.e., it\n> passes even when Git is compiled with SANITIZE=leak). It is based on\n> ab/only-single-progress-at-once (so I dropped the cherry-picked patch towards\n> the end from Ævar).\n\nThe range-diff was almost unreadable, so I looked at the difference\nrelative to the previous tip (i.e. \"git diff @{1}\").  I spotted two\nclasses of changes, one is where and how \"struct child_process\" gets\ncleared, the other is how midx-related filenames are released.  I\nthink the latter changes are improvements, and among the former, the\n\"write_oid() was never called, so cmd was left unused\" change does\nlook an improvement, but I do not know about the others.\n\nThanks.\n"},{"id":"439934","messageId":"YXsHTKGBYYeDsdhh@nand.local","threadId":"56790","inReplyTo":"xmqqzgqut4lr.fsf@gitster.g","subject":"Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-28T20:25:48Z","receivedAt":"2021-10-28T20:25:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,\n> >  \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\n> >\n> >  \tret = start_command(&cmd);\n> > -\tif (ret)\n> > +\tif (ret) {\n> > +\t\tchild_process_clear(&cmd);\n> >  \t\treturn ret;\n> > +\t}\n>\n> This happens only when start_command() returns an error.  But the\n> function always calls child_process_clear() before doing so.\n>\n> So I am not sure if this hunk is needed.  It didn't exist in v1, if\n> I recall correctly.  Am I missing something obvious?\n\nNo, it was the person replying to you missing something obvious ;).\n\nAny hunks like this that call child_process_clear() after\nstart_command() returns a non-zero value are unnecessary. But the one in\nrepack_promisor_objects() is good, and does prevent the leak that had\nled me in this direction in the first place.\n\nHere is a suitable replacement for this patch (I believe that everything\nelse in this version is fine as-is):\n\n--- >8 ---\n\nSubject: [PATCH] builtin/repack.c: avoid leaking child arguments\n\n`git repack` invokes a handful of child processes: one to write the\nactual pack, and optionally ones to repack promisor objects and update\nthe MIDX.\n\nMost of these are freed automatically by calling `start_command()` (which\ninvokes it on error) and `finish_command()` which calls it\nautomatically.\n\nBut repack_promisor_objects() can initialize a child_process, populate\nits array of arguments, and then return from the function before even\ncalling start_command().\n\nMake sure that the prepared list of arguments is freed by calling\nchild_process_clear() ourselves to avoid leaking memory along this path.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0b2d1e5d82..9b74e0d468 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tfor_each_packed_object(write_oid, &cmd,\n \t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n\n-\tif (cmd.in == -1)\n+\tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n+\t\tchild_process_clear(&cmd);\n \t\treturn;\n+\t}\n\n \tclose(cmd.in);\n\n--\n2.33.0.96.g73915697e6\n\n"}]}