{"thread":{"id":"58150","subject":"[PATCH 0/6] revisions API: fix more memory leaks","startedAt":"2022-07-13T13:10:48Z","lastAt":"2022-08-05T09:02:59Z","messageCount":52,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"458954","messageId":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":null,"subject":"[PATCH 0/6] revisions API: fix more memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:29Z","receivedAt":"2022-07-13T13:10:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This short series fixes a couple of mistakes in 1-2/6 that snuck in in\nthe revisions_release() series, those compliment the already queued\njk/diff-files-cleanup-fix.\n\nThe rest of this fixes a few tricky remaining memory leaks, allowing\nus to mark the tests seen in the diffstat as passing with\nSANITIZE=leak.\n\nThe \"git show\" leak in 4/6 in particular showed up in a lot of places\nin our test suite, so fixing it really helps us to accelerate towards\nmarking more entire tests as leak-free.\n\nPassing CI for this series can be found at:\nhttps://github.com/avar/git/tree/avar/follow-up-release-revisions-fixes\n\nÆvar Arnfjörð Bjarmason (6):\n  bisect.c: add missing \"goto\" for release_revisions()\n  test-fast-rebase helper: use release_revisions() (again)\n  log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n  log: fix common \"rev.pending\" memory leak in \"git show\"\n  bisect.c: partially fix bisect_rev_setup() memory leak\n  revisions API: don't leak memory on argv elements that need free()-ing\n\n bisect.c                                      | 28 +++++++++++--------\n builtin/log.c                                 | 22 ++++++++-------\n builtin/submodule--helper.c                   |  5 +++-\n remote.c                                      |  5 +++-\n revision.c                                    |  2 ++\n revision.h                                    |  3 +-\n t/helper/test-fast-rebase.c                   |  2 --\n t/t0203-gettext-setlocale-sanity.sh           |  1 +\n t/t1020-subdirectory.sh                       |  1 +\n t/t2020-checkout-detach.sh                    |  1 +\n t/t3307-notes-man.sh                          |  1 +\n t/t3920-crlf-messages.sh                      |  2 ++\n t/t4069-remerge-diff.sh                       |  1 +\n t/t7007-show.sh                               |  1 +\n ...3-pre-commit-and-pre-merge-commit-hooks.sh |  1 +\n t/t9122-git-svn-author.sh                     |  1 -\n t/t9162-git-svn-dcommit-interactive.sh        |  1 -\n 17 files changed, 50 insertions(+), 28 deletions(-)\n\n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458955","messageId":"patch-1.6-9bea9591d17-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 1/6] bisect.c: add missing \"goto\" for release_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:30Z","receivedAt":"2022-07-13T13:10:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a missing \"goto cleanup\", this fixes a bug in\nf196c1e908d (revisions API users: use release_revisions() needing\nREV_INFO_INIT, 2022-04-13).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex b63669cc9d7..421470bfa59 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1054,7 +1054,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \t\t */\n \t\tres = error_if_skipped_commits(tried, NULL);\n \t\tif (res < 0)\n-\t\t\treturn res;\n+\t\t\tgoto cleanup;\n \t\tprintf(_(\"%s was both %s and %s\\n\"),\n \t\t       oid_to_hex(current_bad_oid),\n \t\t       term_good,\n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458956","messageId":"patch-2.6-325d05ce406-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 2/6] test-fast-rebase helper: use release_revisions() (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:31Z","receivedAt":"2022-07-13T13:10:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in 0139c58ab95 (revisions API users: add \"goto cleanup\" for\nrelease_revisions(), 2022-04-13), in that commit a release_revisions()\ncall was added to this function, but it never did anything due to this\nTODO memset() added in fe1a21d5267 (fast-rebase: demonstrate\nmerge-ort's API via new test-tool command, 2020-10-29).\n\nSimply removing the memset() will fix the \"cmdline\" which can be seen\nwhen running t5520-pull.sh.\n\nThis sort of thing could be detected automatically with a rule similar\nto the unused.cocci merged in 7fa60d2a5b6 (Merge branch\n'ab/cocci-unused' into next, 2022-07-11). The following rule on top\nwould catch the case being fixed here:\n\n\t@@\n\ttype T;\n\tidentifier I;\n\tidentifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n\tidentifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n\t@@\n\n\t- memset(\\( I \\| &I \\), 0, ...);\n\t  ... when != \\( I \\| &I \\)\n\t(\n\t  \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n\t|\n\t  \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n\t)\n\t  ... when != \\( I \\| &I \\)\n\nThat rule should arguably use only &I, not I (as we might be passed a\npointer). He distinction would matter if anyone cared about the\nside-effects of a memset() followed by release() of a pointer to a\nvariable passed into the function.\n\nAs such a pattern would be at best very confusing, and most likely\npoint to buggy code as in this case, the above rule is probably fine\nas-is.\n\nBut as this rule only found one such bug in the entire codebase let's\nnot add it to contrib/coccinelle/unused.cocci for now, we can always\ndig it up in the future if it's deemed useful.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-fast-rebase.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c\nindex 4e5553e2024..45665ec19a5 100644\n--- a/t/helper/test-fast-rebase.c\n+++ b/t/helper/test-fast-rebase.c\n@@ -184,8 +184,6 @@ int cmd__fast_rebase(int argc, const char **argv)\n \t\tlast_picked_commit = commit;\n \t\tlast_commit = create_commit(result.tree, commit, last_commit);\n \t}\n-\t/* TODO: There should be some kind of rev_info_free(&revs) call... */\n-\tmemset(&revs, 0, sizeof(revs));\n \n \tmerge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);\n \n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458957","messageId":"patch-3.6-7f3bf3f0e40-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 3/6] log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:32Z","receivedAt":"2022-07-13T13:10:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code added in 5d7eeee2ac6 (git-show: grok blobs, trees and\ntags, too, 2006-12-14) to use the \"memcpy a &blank\" idiom introduced\nin 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\nmacro, 2021-07-01).\n\nNow the types in play are guaranteed to correspond, i.e. we used \"int\"\nhere for the \"count\" before, but the corresponding \"nr\" is an\n\"unsigned int\". By using a \"blank\" object we almost entirely bypass\nthat, we'll only need to declare our own \"unsigned int i\".\n\nThere are no functional changes here aside from potential overflow\nguard rails, the structure only has these three members (\"nr\", \"alloc\"\nand \"objects\"), but now we're obviously future-proof against assuming\nthat.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/log.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 88a5e98875a..e0f40798d45 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -668,10 +668,12 @@ static void show_setup_revisions_tweak(struct rev_info *rev,\n int cmd_show(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n-\tstruct object_array_entry *objects;\n+\tstruct object_array blank = OBJECT_ARRAY_INIT;\n+\tstruct object_array cp = OBJECT_ARRAY_INIT;\n+\tunsigned int i;\n \tstruct setup_revision_opt opt;\n \tstruct pathspec match_all;\n-\tint i, count, ret = 0;\n+\tint ret = 0;\n \n \tinit_log_defaults();\n \tgit_config(git_log_config, NULL);\n@@ -698,12 +700,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \tif (!rev.no_walk)\n \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n \n-\tcount = rev.pending.nr;\n-\tobjects = rev.pending.objects;\n+\tmemcpy(&cp, &rev.pending, sizeof(rev.pending));\n \trev.diffopt.no_free = 1;\n-\tfor (i = 0; i < count && !ret; i++) {\n-\t\tstruct object *o = objects[i].item;\n-\t\tconst char *name = objects[i].name;\n+\tfor (i = 0; i < cp.nr && !ret; i++) {\n+\t\tstruct object *o = cp.objects[i].item;\n+\t\tconst char *name = cp.objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n \t\t\tret = show_blob_object(&o->oid, &rev, name);\n@@ -726,7 +727,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tif (!o)\n \t\t\t\tret = error(_(\"could not read object %s\"),\n \t\t\t\t\t    oid_to_hex(oid));\n-\t\t\tobjects[i].item = o;\n+\t\t\tcp.objects[i].item = o;\n \t\t\ti--;\n \t\t\tbreak;\n \t\t}\n@@ -743,8 +744,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n-\t\t\trev.pending.nr = rev.pending.alloc = 0;\n-\t\t\trev.pending.objects = NULL;\n+\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n \t\t\tbreak;\n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458959","messageId":"patch-4.6-9bff7b10197-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:33Z","receivedAt":"2022-07-13T13:10:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a very common memory leak introduced in 5d7eeee2ac6 (git-show: grok blobs,\ntrees and tags, too, 2006-12-14).\n\nWhen \"git show\" displays commits it needs to temporarily clobbers the\n\"rev.pending\" array, but by doing so we'll fail to\nrelease_revisions(), as we have for most other uses of \"struct\nrev_info\" since 2da81d1efb0 (Merge branch 'ab/plug-leak-in-revisions',\n2022-06-07).\n\nIn the preceding commit this code was made to use a more extendable\npattern, which we can now complete. Once we've clobbered our\n\"rev.pending\" and invoked \"cmd_log_walk_no_free()\" we need to\n\"object_array_clear()\" our newly created \"rev.pending\" to avoid\nleaking the memory related to the one member array we've created.\n\nBut more importantly we need to set \"rev.pending\" back to the original\nwe squirreled away in the \"cp\" variable, so that we'll make use of the\nrelease_revisions() added in f6bfea0ad01 (revisions API users: use\nrelease_revisions() in builtin/log.c, 2022-04-13). In f6bfea0ad01 this\nmemory leak was noted as an outstanding TODO, but it's now been fixed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/log.c                                    | 2 ++\n t/t0203-gettext-setlocale-sanity.sh              | 1 +\n t/t1020-subdirectory.sh                          | 1 +\n t/t3307-notes-man.sh                             | 1 +\n t/t3920-crlf-messages.sh                         | 2 ++\n t/t4069-remerge-diff.sh                          | 1 +\n t/t7007-show.sh                                  | 1 +\n t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 1 +\n t/t9122-git-svn-author.sh                        | 1 -\n t/t9162-git-svn-dcommit-interactive.sh           | 1 -\n 10 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e0f40798d45..77ec256a8ae 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -747,6 +747,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n+\t\t\tobject_array_clear(&rev.pending);\n+\t\t\tmemcpy(&rev.pending, &cp, sizeof(rev.pending));\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tret = error(_(\"unknown type: %d\"), o->type);\ndiff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\nindex 0ce1f22eff6..86cff324ff1 100755\n--- a/t/t0203-gettext-setlocale-sanity.sh\n+++ b/t/t0203-gettext-setlocale-sanity.sh\n@@ -5,6 +5,7 @@\n \n test_description=\"The Git C functions aren't broken by setlocale(3)\"\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-gettext.sh\n \n test_expect_success 'git show a ISO-8859-1 commit under C locale' '\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 9fdbb2af80e..45eef9457fe 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -6,6 +6,7 @@\n test_description='Try various core-level commands in subdirectory.\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/t/t3307-notes-man.sh b/t/t3307-notes-man.sh\nindex 1aa366a410e..ae316502c45 100755\n--- a/t/t3307-notes-man.sh\n+++ b/t/t3307-notes-man.sh\n@@ -4,6 +4,7 @@ test_description='Examples from the git-notes man page\n \n Make sure the manual is not full of lies.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 0276edbe3d3..4c661d4d54a 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n LIB_CRLF_BRANCHES=\"\"\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 35f94957fce..9e7cac68b1c 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -2,6 +2,7 @@\n \n test_description='remerge-diff handling'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # This test is ort-specific\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex d6cc69e0f2c..f908a4d1abc 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -2,6 +2,7 @@\n \n test_description='git show'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex ad1eb64ba0d..aa004b70a8d 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -5,6 +5,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'root commit' '\ndiff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh\nindex 527ba3d2932..0fc289ae0f0 100755\n--- a/t/t9122-git-svn-author.sh\n+++ b/t/t9122-git-svn-author.sh\n@@ -2,7 +2,6 @@\n \n test_description='git svn authorship'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'setup svn repository' '\ndiff --git a/t/t9162-git-svn-dcommit-interactive.sh b/t/t9162-git-svn-dcommit-interactive.sh\nindex e2aa8ed88a9..b3ce033a0d3 100755\n--- a/t/t9162-git-svn-dcommit-interactive.sh\n+++ b/t/t9162-git-svn-dcommit-interactive.sh\n@@ -4,7 +4,6 @@\n \n test_description='git svn dcommit --interactive series'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize repo' '\n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458958","messageId":"patch-5.6-83897fd108a-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 5/6] bisect.c: partially fix bisect_rev_setup() memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:34Z","receivedAt":"2022-07-13T13:10:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Partially fix the memory leak noted in in 8a534b61241 (bisect: use\nargv_array API, 2011-09-13), which added the \"XXX\" comment seen in the\ncontext. We can partially fix it by having the bisect_rev_setup()\nfunction take a \"struct strvec\", rather than constructing it.\n\nAs the comment notes we need to keep the construct \"rev_argv\" around\nwhile the \"struct rev_info\" is around, which as seen in the newly\nadded \"strvec_clear()\" calls here we do after \"release_revisions()\".\n\nThis \"partially\" fixes the memory leak because we're leaking the \"--\"\nadded to the \"rev_argv\" here still, which will be addressed in a\nsubsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 421470bfa59..6afb98be7a1 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -648,11 +648,11 @@ static struct commit_list *managed_skipped(struct commit_list *list,\n }\n \n static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n+\t\t\t     struct strvec *rev_argv,\n \t\t\t     const char *prefix,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n-\tstruct strvec rev_argv = STRVEC_INIT;\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -660,16 +660,16 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n \n \t/* rev_argv.argv[0] will be ignored by setup_revisions */\n-\tstrvec_push(&rev_argv, \"bisect_rev_setup\");\n-\tstrvec_pushf(&rev_argv, bad_format, oid_to_hex(current_bad_oid));\n+\tstrvec_push(rev_argv, \"bisect_rev_setup\");\n+\tstrvec_pushf(rev_argv, bad_format, oid_to_hex(current_bad_oid));\n \tfor (i = 0; i < good_revs.nr; i++)\n-\t\tstrvec_pushf(&rev_argv, good_format,\n+\t\tstrvec_pushf(rev_argv, good_format,\n \t\t\t     oid_to_hex(good_revs.oid + i));\n-\tstrvec_push(&rev_argv, \"--\");\n+\tstrvec_push(rev_argv, \"--\");\n \tif (read_paths)\n-\t\tread_bisect_paths(&rev_argv);\n+\t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n \t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n }\n \n@@ -873,10 +873,11 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int\n static int check_ancestors(struct repository *r, int rev_nr,\n \t\t\t   struct commit **rev, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs;\n \tint res;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"^%s\", \"%s\", 0);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"^%s\", \"%s\", 0);\n \n \tbisect_common(&revs);\n \tres = (revs.commits != NULL);\n@@ -885,6 +886,7 @@ static int check_ancestors(struct repository *r, int rev_nr,\n \tclear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS);\n \n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n@@ -1010,6 +1012,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n  */\n enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs = REV_INFO_INIT;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n@@ -1037,7 +1040,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tif (res)\n \t\tgoto cleanup;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"%s\", \"^%s\", 1);\n \n \trevs.first_parent_only = !!(bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY);\n \trevs.limited = 1;\n@@ -1112,6 +1115,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tres = bisect_checkout(bisect_rev, no_checkout);\n cleanup:\n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"458960","messageId":"patch-6.6-4a581a4a6ce-20220713T130511Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T13:10:35Z","receivedAt":"2022-07-13T13:11:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"free_removed_argv_elements\" member to \"struct\nsetup_revision_opt\", and use it to fix several memory leaks, e.g. the\none with a \"XXX\" comment added in 8a534b61241 (bisect: use argv_array\nAPI, 2011-09-13).\n\nWe have various memory leaks in APIs that take and munge \"const\nchar **argv\", e.g. parse_options(). Sometimes these APIs are given the\n\"argv\" we get to the \"main\" function, in which case we don't leak\nmemory, but other times we're giving it the \"v\" member of a \"struct\nstrvec\" we created.\n\nThere's several potential ways to fix those sort of leaks, we could\nadd a \"nodup\" mode to \"struct strvec\", which would work for the cases\nwhere we push constant strings to it. But that wouldn't work as soon\nas we used strvec_pushf(), or otherwise needed to duplicate or create\na string for that \"struct strvec\".\n\nLet's instead make it the responsibility of the revisions API. If it's\ngoing to clobber elements of argv it can also free() them, which it\nwill now do if instructed to do so via \"free_removed_argv_elements\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c                    | 6 ++++--\n builtin/submodule--helper.c | 5 ++++-\n remote.c                    | 5 ++++-\n revision.c                  | 2 ++\n revision.h                  | 3 ++-\n t/t2020-checkout-detach.sh  | 1 +\n 6 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 6afb98be7a1..38b3891f3a6 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -653,6 +653,9 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -669,8 +672,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tif (read_paths)\n \t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n-\t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);\n }\n \n static void bisect_common(struct rev_info *revs)\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c597df7528e..c4e47c1b15a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1114,6 +1114,9 @@ static int compute_summary_module_list(struct object_id *head_oid,\n {\n \tstruct strvec diff_args = STRVEC_INIT;\n \tstruct rev_info rev;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n \tint ret = 0;\n \n@@ -1131,7 +1134,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \tinit_revisions(&rev, info->prefix);\n \trev.abbrev = 0;\n \tprecompose_argv_prefix(diff_args.nr, diff_args.v, NULL);\n-\tsetup_revisions(diff_args.nr, diff_args.v, &rev, NULL);\n+\tsetup_revisions(diff_args.nr, diff_args.v, &rev, &opt);\n \trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = submodule_summary_callback;\n \trev.diffopt.format_callback_data = &list;\ndiff --git a/remote.c b/remote.c\nindex b19e3a2f015..f05b50b1dd1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2169,6 +2169,9 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstruct object_id oid;\n \tstruct commit *ours, *theirs;\n \tstruct rev_info revs;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct strvec argv = STRVEC_INIT;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n@@ -2203,7 +2206,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstrvec_push(&argv, \"--\");\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n-\tsetup_revisions(argv.nr, argv.v, &revs, NULL);\n+\tsetup_revisions(argv.nr, argv.v, &revs, &opt);\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \ndiff --git a/revision.c b/revision.c\nindex 211352795c5..3fa84247d33 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2748,6 +2748,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tconst char *arg = argv[i];\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n+\t\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\t\tfree((char *)argv[i]);\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..bb91e7ed914 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -375,7 +375,8 @@ struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n \tunsigned int\tassume_dashdash:1,\n-\t\t\tallow_exclude_promisor_objects:1;\n+\t\t\tallow_exclude_promisor_objects:1,\n+\t\t\tfree_removed_argv_elements:1;\n \tunsigned revarg_opt;\n };\n int setup_revisions(int argc, const char **argv, struct rev_info *revs,\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex bc46713a43e..2eab6474f8d 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -4,6 +4,7 @@ test_description='checkout into detached HEAD state'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_detached () {\n-- \n2.37.0.932.g7b7031e73bc\n\n"},{"id":"459272","messageId":"YtVzWMKRezUYuhkn@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-3.6-7f3bf3f0e40-20220713T130511Z-avarab@gmail.com","subject":"Re: [PATCH 3/6] log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T14:51:04Z","receivedAt":"2022-07-18T14:51:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2022 at 03:10:32PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 88a5e98875a..e0f40798d45 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -668,10 +668,12 @@ static void show_setup_revisions_tweak(struct rev_info *rev,\n>  int cmd_show(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct rev_info rev;\n> -\tstruct object_array_entry *objects;\n> +\tstruct object_array blank = OBJECT_ARRAY_INIT;\n> +\tstruct object_array cp = OBJECT_ARRAY_INIT;\n\nI'm not sure what \"cp\" stands for. Maybe just \"pending\" would be a more\ndescriptive name?\n\n> @@ -698,12 +700,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \tif (!rev.no_walk)\n>  \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n>  \n> -\tcount = rev.pending.nr;\n> -\tobjects = rev.pending.objects;\n> +\tmemcpy(&cp, &rev.pending, sizeof(rev.pending));\n\nOK, so now \"cp\" is a copy of the original \"rev.pending\". But that\noriginal is still in place. If I understand the intent of this code\ncorrectly, we'd never want to look at it again. The only place that\nshould do so is the call to cmd_log_walk_no_free():\n\n>  \t\tcase OBJ_COMMIT:\n> -\t\t\trev.pending.nr = rev.pending.alloc = 0;\n> -\t\t\trev.pending.objects = NULL;\n> +\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n>  \t\t\tbreak;\n\nbut both before and after your patch, we clear rev.pending before doing\nso.\n\nSo perhaps it would make the intent more clear if we fully transferred\nownership out of the rev struct? I.e., something like:\n\n  memcpy(&cp, &rev.pending, sizeof(rev.pending));\n  memcpy(&rev.pending, &blank, sizeof(rev.pending));\n  for (i = 0; i < cp.nr; i++) {\n     ...stuff...\n  }\n  object_array_clear(&cp);\n\n> @@ -726,7 +727,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (!o)\n>  \t\t\t\tret = error(_(\"could not read object %s\"),\n>  \t\t\t\t\t    oid_to_hex(oid));\n> -\t\t\tobjects[i].item = o;\n> +\t\t\tcp.objects[i].item = o;\n>  \t\t\ti--;\n>  \t\t\tbreak;\n>  \t\t}\n\nWow, this \"overwrite the current item and back up one\" strategy is truly\nhorrific. But it's neither here nor there for your series; you don't\nmake it any worse, and because \"item\" is not a free-able pointer, you\ndon't need to worry about it for leaking.\n\nI suspect the cleaner way of doing it would be to push all of this\nswitch logic into a function, and then call the function recursively\nwhen dereferencing a tag. But let's put that aside so as not to distract\nfrom your goal.\n\n-Peff\n"},{"id":"459273","messageId":"YtV0xB9d6g/XwkLk@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-4.6-9bff7b10197-20220713T130511Z-avarab@gmail.com","subject":"Re: [PATCH 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T14:57:08Z","receivedAt":"2022-07-18T14:57:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2022 at 03:10:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index e0f40798d45..77ec256a8ae 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -747,6 +747,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n> +\t\t\tobject_array_clear(&rev.pending);\n> +\t\t\tmemcpy(&rev.pending, &cp, sizeof(rev.pending));\n>  \t\t\tbreak;\n\nOK, so we clear the fake one-entry pending array we just created. That\nmake sense.\n\nAnd then we have to restore rev.pending, because we don't otherwise\nclean up cp. That makes sense in the context of your previous patch, but\nif you take my suggestion there to separate \"cp\" from rev.pending\nentirely, and clean it up explicitly, then this second memcpy() goes\naway. IMHO that leaves the resulting code much easier to follow, as the\nlifetimes are clearly distinct.\n\nTwo alternatives I briefly considered that don't work:\n\n  - you can't just leave the one-item rev.pending in place to get\n    cleaned up by release_revisions(), because we may show multiple\n    commits. We have to clean each one as we go.\n\n  - you can't just object_array_clear(&rev.pending) _before_ clearing\n    it, because in the initial state it's still a copy of \"cp\", and thus\n    would hose the array we're iterating over.\n\nThis whole \"reuse rev, but tweak its pending array\" feels a bit sketchy\nto me in general. But it has been this way since 2006, and anyway is\ncompletely out of scope for your series, so let's hold our nose and\ncontinue. ;)\n\n> diff --git a/t/t7007-show.sh b/t/t7007-show.sh\n> index d6cc69e0f2c..f908a4d1abc 100755\n> --- a/t/t7007-show.sh\n> +++ b/t/t7007-show.sh\n> @@ -2,6 +2,7 @@\n>  \n>  test_description='git show'\n>  \n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n\nLots of stuff now passes, which is good, but....\n\n> diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh\n> index 527ba3d2932..0fc289ae0f0 100755\n> --- a/t/t9122-git-svn-author.sh\n> +++ b/t/t9122-git-svn-author.sh\n> @@ -2,7 +2,6 @@\n>  \n>  test_description='git svn authorship'\n>  \n> -TEST_FAILS_SANITIZE_LEAK=true\n>  . ./lib-git-svn.sh\n\n...this one (and t9162) don't anymore? Are these hunks a mistake? If\nnot, this feels like something that needs to go into the commit message.\n\n-Peff\n"},{"id":"459274","messageId":"YtV2pImT7MBFrqWD@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-5.6-83897fd108a-20220713T130511Z-avarab@gmail.com","subject":"Re: [PATCH 5/6] bisect.c: partially fix bisect_rev_setup() memory leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T15:05:08Z","receivedAt":"2022-07-18T15:05:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2022 at 03:10:34PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Partially fix the memory leak noted in in 8a534b61241 (bisect: use\n> argv_array API, 2011-09-13), which added the \"XXX\" comment seen in the\n> context. We can partially fix it by having the bisect_rev_setup()\n> function take a \"struct strvec\", rather than constructing it.\n> \n> As the comment notes we need to keep the construct \"rev_argv\" around\n> while the \"struct rev_info\" is around, which as seen in the newly\n> added \"strvec_clear()\" calls here we do after \"release_revisions()\".\n\nThis seems like not too bad a solution on its face. The caller is\nresponsible for keeping rev_argv around as long as rev is valid.\n\nThis would be simpler if setup_revisions() made its own copy of the argv\nstrings and dropped them via release_revisions(). That has a small cost,\nbut I suspect it doesn't matter much in practice.\n\nI thought that would be too much of a pain to add, but it kind of looks\nlike you went there anyway in the next patch. I'll comment further\nthere.\n\n> This \"partially\" fixes the memory leak because we're leaking the \"--\"\n> added to the \"rev_argv\" here still, which will be addressed in a\n> subsequent commit.\n\nThis part confused me, because the \"--\" is in the strvec which gets\ncleared. I'd guess that the revisions code just overwrites it with NULL,\nand we lose access to it. But it should become clear in the next commit.\n\n> -\tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n> +\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n>  \t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n\nI wouldn't really call this a \"leak\" anymore; the caller now owns it.\nMaybe nit-picking, since this comment goes away later.\n\n-Peff\n"},{"id":"459275","messageId":"YtV4KmrTBkmcx6m3@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-6.6-4a581a4a6ce-20220713T130511Z-avarab@gmail.com","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T15:11:38Z","receivedAt":"2022-07-18T15:11:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2022 at 03:10:35PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> There's several potential ways to fix those sort of leaks, we could\n> add a \"nodup\" mode to \"struct strvec\", which would work for the cases\n> where we push constant strings to it. But that wouldn't work as soon\n> as we used strvec_pushf(), or otherwise needed to duplicate or create\n> a string for that \"struct strvec\".\n\nRight, I think this falls down when you have mixed const/non-const\nentries. You have to know which is which for strvec_clear().\n\n> Let's instead make it the responsibility of the revisions API. If it's\n> going to clobber elements of argv it can also free() them, which it\n> will now do if instructed to do so via \"free_removed_argv_elements\".\n\nOK, I think this is a reasonable and minimal fix for the \"--\" problem on\nits own.\n\nBut if you went just a little further and made the option \"rev should\nown its own argv\", then I think you can simplify life for callers even\nmore. They could construct a strvec themselves and then hand it off to\nthe rev_info, to be cleaned up when release_revisions() is called (and\nof course freeing the \"--\" when we overwrite it in the interim, as you\ndo here).\n\nThen all of the bisect callers from the previous patch could avoid\nhaving to deal with the strvec at all. They'd call bisect_rev_setup(),\nwhich would internally attach the memory to rev_info.\n\n>  bisect.c                    | 6 ++++--\n>  builtin/submodule--helper.c | 5 ++++-\n>  remote.c                    | 5 ++++-\n>  revision.c                  | 2 ++\n>  revision.h                  | 3 ++-\n>  t/t2020-checkout-detach.sh  | 1 +\n>  6 files changed, 17 insertions(+), 5 deletions(-)\n\nThe patch itself works as advertised, I think.\n\n-Peff\n"},{"id":"459276","messageId":"220718.864jzexxn6.gmgdl@evledraar.gmail.com","threadId":"58150","inReplyTo":"YtV0xB9d6g/XwkLk@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-18T15:08:08Z","receivedAt":"2022-07-18T15:12:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 18 2022, Jeff King wrote:\n\n> On Wed, Jul 13, 2022 at 03:10:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index e0f40798d45..77ec256a8ae 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -747,6 +747,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>>  \t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n>>  \t\t\tadd_object_array(o, name, &rev.pending);\n>>  \t\t\tret = cmd_log_walk_no_free(&rev);\n>> +\t\t\tobject_array_clear(&rev.pending);\n>> +\t\t\tmemcpy(&rev.pending, &cp, sizeof(rev.pending));\n>>  \t\t\tbreak;\n>\n> OK, so we clear the fake one-entry pending array we just created. That\n> make sense.\n>\n> And then we have to restore rev.pending, because we don't otherwise\n> clean up cp. That makes sense in the context of your previous patch, but\n> if you take my suggestion there to separate \"cp\" from rev.pending\n> entirely, and clean it up explicitly, then this second memcpy() goes\n> away. IMHO that leaves the resulting code much easier to follow, as the\n> lifetimes are clearly distinct.\n\nThanks, I'll mull that over, and didn't have time to do it now. FWIW I\nwas aiming to end up with the least bit of invasiveness into the private\nbits of revision.[ch], i.e. we know we can fiddle with rev.pending\nindependently of other members, but it's rather nasty to assume that. So\nrestoring the previous state and having release_revisions() handle it is\na bit less nasty.\n\nOf course we assume things about its internals here anyway, but this way\nit's only for the narrow scope of when we're clobbering it for \"show\".\n\n> Two alternatives I briefly considered that don't work:\n>\n>   - you can't just leave the one-item rev.pending in place to get\n>     cleaned up by release_revisions(), because we may show multiple\n>     commits. We have to clean each one as we go.\n>\n>   - you can't just object_array_clear(&rev.pending) _before_ clearing\n>     it, because in the initial state it's still a copy of \"cp\", and thus\n>     would hose the array we're iterating over.\n>\n> This whole \"reuse rev, but tweak its pending array\" feels a bit sketchy\n> to me in general. But it has been this way since 2006, and anyway is\n> completely out of scope for your series, so let's hold our nose and\n> continue. ;)\n\n*nod*\n\n>> diff --git a/t/t7007-show.sh b/t/t7007-show.sh\n>> index d6cc69e0f2c..f908a4d1abc 100755\n>> --- a/t/t7007-show.sh\n>> +++ b/t/t7007-show.sh\n>> @@ -2,6 +2,7 @@\n>>  \n>>  test_description='git show'\n>>  \n>> +TEST_PASSES_SANITIZE_LEAK=true\n>>  . ./test-lib.sh\n>\n> Lots of stuff now passes, which is good, but....\n>\n>> diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh\n>> index 527ba3d2932..0fc289ae0f0 100755\n>> --- a/t/t9122-git-svn-author.sh\n>> +++ b/t/t9122-git-svn-author.sh\n>> @@ -2,7 +2,6 @@\n>>  \n>>  test_description='git svn authorship'\n>>  \n>> -TEST_FAILS_SANITIZE_LEAK=true\n>>  . ./lib-git-svn.sh\n>\n> ...this one (and t9162) don't anymore? Are these hunks a mistake? If\n> not, this feels like something that needs to go into the commit message.\n\nI can add an explanation for this, this too passes, note that it's\n\"TEST_FAILS...\" not \"TEST_PASSES...\".\n\nI..e. in an earlier series I whitelisted all of lib-git-svn.sh, unless\nthat variable was specificed, which was less churn than adding\n\"TEST_PASSES_SANITIZE_LEAK=true\" to all of them (as more than 1/2 of\nthem passed already).\n\nSo there's no new leaks here, and only new passing tests.\n"},{"id":"459277","messageId":"YtV40w0n9muPnM1O@coredump.intra.peff.net","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"Re: [PATCH 0/6] revisions API: fix more memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T15:14:27Z","receivedAt":"2022-07-18T15:14:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2022 at 03:10:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Ævar Arnfjörð Bjarmason (6):\n>   bisect.c: add missing \"goto\" for release_revisions()\n>   test-fast-rebase helper: use release_revisions() (again)\n>   log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n>   log: fix common \"rev.pending\" memory leak in \"git show\"\n>   bisect.c: partially fix bisect_rev_setup() memory leak\n>   revisions API: don't leak memory on argv elements that need free()-ing\n\nThis looks correct overall to me. I left a few comments:\n\n  - the readability tweaks in 3 and 4 I think are worth doing. If you\n    agree, I'd like to see that in a re-roll.\n\n  - the bigger \"rev_info owns its argv\" option for patches 5 and 6 is a\n    good suggestion, I think, but maybe is a bigger change (though if\n    it's optional, it shouldn't need to affect any other callers). But\n    if it derails things, it may not be worth doing now. I can live\n    without it.\n\n-Peff\n"},{"id":"459278","messageId":"220718.86zgh6wiwa.gmgdl@evledraar.gmail.com","threadId":"58150","inReplyTo":"YtV4KmrTBkmcx6m3@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-18T15:13:05Z","receivedAt":"2022-07-18T15:16:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 18 2022, Jeff King wrote:\n\n> On Wed, Jul 13, 2022 at 03:10:35PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> There's several potential ways to fix those sort of leaks, we could\n>> add a \"nodup\" mode to \"struct strvec\", which would work for the cases\n>> where we push constant strings to it. But that wouldn't work as soon\n>> as we used strvec_pushf(), or otherwise needed to duplicate or create\n>> a string for that \"struct strvec\".\n>\n> Right, I think this falls down when you have mixed const/non-const\n> entries. You have to know which is which for strvec_clear().\n\nYes, that aspect of it sucks, such a caller will need to do its own\nbespoke free-ing.\n\nIn practice I haven't really found such callers, API users tend to use\none or the other. Either they're using strvec (needs free-ing) or\nmain()'s argv (no free-ing).\n\nTo the extent that that wasn't the case I figured the easiest fix for\nthose is probably to fully convert them to strvec, the copies being\ntrivial in the larger context...\n\n>> Let's instead make it the responsibility of the revisions API. If it's\n>> going to clobber elements of argv it can also free() them, which it\n>> will now do if instructed to do so via \"free_removed_argv_elements\".\n>\n> OK, I think this is a reasonable and minimal fix for the \"--\" problem on\n> its own.\n>\n> But if you went just a little further and made the option \"rev should\n> own its own argv\", then I think you can simplify life for callers even\n> more. They could construct a strvec themselves and then hand it off to\n> the rev_info, to be cleaned up when release_revisions() is called (and\n> of course freeing the \"--\" when we overwrite it in the interim, as you\n> do here).\n>\n> Then all of the bisect callers from the previous patch could avoid\n> having to deal with the strvec at all. They'd call bisect_rev_setup(),\n> which would internally attach the memory to rev_info.\n\nYes, I experimented with this, and it's a solid approach.\n\nBut it's a much larger change, particularly since we'd also want to\nupdate the API itself to take take \"const\" in the appropriate places to\ndo it properly.\n"},{"id":"459281","messageId":"YtWAMP0ROFseFs6B@coredump.intra.peff.net","threadId":"58150","inReplyTo":"220718.86zgh6wiwa.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-18T15:45:52Z","receivedAt":"2022-07-18T15:46:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2022 at 05:13:05PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > But if you went just a little further and made the option \"rev should\n> > own its own argv\", then I think you can simplify life for callers even\n> > more. They could construct a strvec themselves and then hand it off to\n> > the rev_info, to be cleaned up when release_revisions() is called (and\n> > of course freeing the \"--\" when we overwrite it in the interim, as you\n> > do here).\n> >\n> > Then all of the bisect callers from the previous patch could avoid\n> > having to deal with the strvec at all. They'd call bisect_rev_setup(),\n> > which would internally attach the memory to rev_info.\n> \n> Yes, I experimented with this, and it's a solid approach.\n> \n> But it's a much larger change, particularly since we'd also want to\n> update the API itself to take take \"const\" in the appropriate places to\n> do it properly.\n\nHmm. I was thinking that we'd just have rev_info.we_own_our_argv, and\nthen release it in release_revisions(). But actually, rev_info does not\nhold onto the argv at all! It's totally processed in setup_revisions().\n\nAnd there it either:\n\n  - becomes part of prune_data (in which case a copy is made)\n\n  - is passed to handle_revisions_opt(), etc, in which case it is parsed\n    but not held onto (it's possible that some string option holds onto\n    a pointer, but the only one I found is --filter, which makes a\n    copy).\n\n  - is passed to handle_revision_arg(), but these days\n    add_rev_cmdline(), etc, make copies. As they must, otherwise --stdin\n    would be totally buggy.\n\nSo I actually wonder if the comment in bisect_rev_setup() is simply\nwrong. It was correct in 2011 when I wrote it, but things changed in\ndf835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n2013-05-25), etc.\n\nIn that case, we could replace your patch 5 in favor of just calling\nstrvec_clear() at the end of bisect_rev_setup(). It's possible there's a\ncase I'm missing that makes this generally not-safe, but in the case of\nbisect_rev_setup() there's a very limited set of items in our argv in\nthe first place. Doing so also passes the test suite with\nSANITIZE=address, though again, this is just exercising the very limited\nbisect options here.\n\n-Peff\n"},{"id":"460171","messageId":"220729.86pmhoidsc.gmgdl@evledraar.gmail.com","threadId":"58150","inReplyTo":"YtWAMP0ROFseFs6B@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T07:07:40Z","receivedAt":"2022-07-29T07:22:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 18 2022, Jeff King wrote:\n\n> On Mon, Jul 18, 2022 at 05:13:05PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > But if you went just a little further and made the option \"rev should\n>> > own its own argv\", then I think you can simplify life for callers even\n>> > more. They could construct a strvec themselves and then hand it off to\n>> > the rev_info, to be cleaned up when release_revisions() is called (and\n>> > of course freeing the \"--\" when we overwrite it in the interim, as you\n>> > do here).\n>> >\n>> > Then all of the bisect callers from the previous patch could avoid\n>> > having to deal with the strvec at all. They'd call bisect_rev_setup(),\n>> > which would internally attach the memory to rev_info.\n>> \n>> Yes, I experimented with this, and it's a solid approach.\n>> \n>> But it's a much larger change, particularly since we'd also want to\n>> update the API itself to take take \"const\" in the appropriate places to\n>> do it properly.\n>\n> Hmm. I was thinking that we'd just have rev_info.we_own_our_argv, and\n> then release it in release_revisions(). But actually, rev_info does not\n> hold onto the argv at all! It's totally processed in setup_revisions().\n>\n> And there it either:\n>\n>   - becomes part of prune_data (in which case a copy is made)\n>\n>   - is passed to handle_revisions_opt(), etc, in which case it is parsed\n>     but not held onto (it's possible that some string option holds onto\n>     a pointer, but the only one I found is --filter, which makes a\n>     copy).\n>\n>   - is passed to handle_revision_arg(), but these days\n>     add_rev_cmdline(), etc, make copies. As they must, otherwise --stdin\n>     would be totally buggy.\n>\n> So I actually wonder if the comment in bisect_rev_setup() is simply\n> wrong. It was correct in 2011 when I wrote it, but things changed in\n> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n> 2013-05-25), etc.\n>\n> In that case, we could replace your patch 5 in favor of just calling\n> strvec_clear() at the end of bisect_rev_setup().\n\n5/6 is doing that, or rather at the end of check_ancestors() and\nbisect_next_all(), but those call bisect_rev_setup() and pass it the\nstrvec, so in terms of memory management it amounts to the same thing.\n\n> It's possible there's a\n> case I'm missing that makes this generally not-safe, but in the case of\n> bisect_rev_setup() there's a very limited set of items in our argv in\n> the first place. Doing so also passes the test suite with\n> SANITIZE=address, though again, this is just exercising the very limited\n> bisect options here.\n\nI think what you're missing is that this code is basically doing this,\nwhich is a common pattern in various parts of our codebase:\n\n\tstruct strvec sv = STRVEC_INIT;\n\tstrvec_push(&sv, \"foo\"); // sv.v[0] \n\tstrvec_push(&sv, \"bar\"); // sv.v[1] \n\tsv.v[1] = NULL; // the code in revisions.c\n\tstrvec_clear(&sv);\n\nI.e. you can't fix this by simply having revision.c having its own\nstrvec, because it would just .. proceed to do the same thing.\n\nOf course we could alter its argv-iterating state machine to not clobber\nthat \"argv\" element to NULL, and have other subsequent code know that it\nshould stop at a new lower \"argc\" etc. etc., but that's getting at the\nmuch more invasive API changes 6/6 notes as the path not taken.\n\nAnd, in the general case for things that do this to the strvec what\nwe're explicitly doing is having the caller then operate on that munged\nargv, i.e. it's important that we change *its* argv. That's not going on\nhere, but e.g. various parse_options() callers rely on that.\n\nI've experimented a bit with how to solve this more generally, the below\nis some very rough WIP code I have laying aroun to try to do that.\n\nIIRC this fails SANITIZE=address or was otherwise broken, I didn't test\nit now, but that's not the point...\n\n... I'm just including it by way of explanation. I.e. for at least\n*some* callers (IIRC the below mostly works, and I can't remember where\nit's broken) it would suffice to just keep around a list of \"here's\nstuff we should free later\".\n\nIn case below I opted to do that with a string_list, but it could be\nanother strvec or whatever.\n\n-- >8 --\nFrom: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n <avarab@gmail.com>\nDate: Fri, 10 Jun 2022 13:05:05 +0200\nSubject: [PATCH] string-list API: add utility to help free \"struct strvec\"\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nThe \"struct strvec\" has its own strvec_clear(), but unfortunately it's\na very common pattern to pass its \"v\" member to parse_options(),\nsetup_revisions() etc.\n\nThose APIs will then change the \"argv\" so that strvec_clear() can't do\nanything useful with its members, e.g. parse_options_end() will NULL\nout the dereferenced elements of the \"argv\" that's passed in to\nindicate how much of it was consumed.\n\nOne way to deal with that would be to change all of those APIs to\naccept some \"in_argv\" which they'd then copy, but that would be a very\nlarge change.\n\nThis is a less invasive way of doing it, we'll now maintain a \"struct\nstring_list\" on the side, whose \"util\" members point to the elements\nof the \"struct strvec\" that we should free() later, which we can\nhelpfully do with string_list_clear(list, 1). See the added API\ndocumentation for more details.\n\nAs a result we can mark more tests as passing with SANITIZE=leak using\n\"TEST_PASSES_SANITIZE_LEAK=true\". We'll be able to use this API for a\nlot more callers, but let's start with \"am\"'s run_apply().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/am.c                  |  3 +++\n string-list.c                 | 16 ++++++++++++++++\n string-list.h                 | 15 +++++++++++++++\n t/t0023-crlf-am.sh            |  1 +\n t/t4152-am-subjects.sh        |  2 ++\n t/t4254-am-corrupt.sh         |  2 ++\n t/t4256-am-format-flowed.sh   |  1 +\n t/t5403-post-checkout-hook.sh |  1 +\n 8 files changed, 41 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 93bec62afa9..d927fea128a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tint res, opts_left;\n \tint force_apply = 0;\n \tint options = 0;\n+\tstruct string_list to_free = STRING_LIST_INIT_NODUP;\n \n \tif (init_apply_state(&apply_state, the_repository, NULL))\n \t\tBUG(\"init_apply_state() failed\");\n@@ -1483,6 +1484,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstrvec_push(&apply_opts, \"apply\");\n \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n \n+\tstring_list_append_strvec_nodup(&to_free, &apply_opts);\n \topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n \t\t\t\t\t&apply_state, &force_apply, &options,\n \t\t\t\t\tNULL);\n@@ -1512,6 +1514,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \n \tstrvec_clear(&apply_paths);\n \tstrvec_clear(&apply_opts);\n+\tstring_list_clear(&to_free, 1);\n \tclear_apply_state(&apply_state);\n \n \tif (res)\ndiff --git a/string-list.c b/string-list.c\nindex 549fc416d68..ecbfb3065a8 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"string-list.h\"\n+#include \"strvec.h\"\n \n void string_list_init_nodup(struct string_list *list)\n {\n@@ -325,3 +326,18 @@ int string_list_split_in_place(struct string_list *list, char *string,\n \t\t}\n \t}\n }\n+\n+void string_list_append_strvec_nodup(struct string_list *list,\n+\t\t\t\t     const struct strvec *vec)\n+{\n+\tsize_t i;\n+\n+\tif (list->strdup_strings)\n+\t\tBUG(\"need .strdup_strings = 0 for string_list_append_strvec_nodup()\");\n+\n+\tfor (i = 0; i < vec->nr; i++) {\n+\t\tconst char *p = vec->v[i];\n+\n+\t\tstring_list_append_nodup(list, (char *)p)->util = (void *)p;\n+\t}\n+}\ndiff --git a/string-list.h b/string-list.h\nindex d5a744e1438..e740f3f29be 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -266,4 +266,19 @@ int string_list_split(struct string_list *list, const char *string,\n  */\n int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\t       int delim, int maxsplit);\n+\n+/**\n+ * Given a STRING_LIST_INIT_NODUP-initialized \"struct string_list\"\n+ * takes a \"struct strvec\" and \"copies\" it, the \"string\" members will\n+ * be the elements of the \"struct strvec\", and more importantly so\n+ * will the \"util\" members.\n+ *\n+ * This is intended to squirrel away pointers to \"argv\" to\n+ * string_list_clear(list, 1) them later, in cases where a \"struct\n+ * strvec\" is passed to an API like parse_options(), setup_revisions()\n+ * etc.\n+ */\n+struct strvec;\n+void string_list_append_strvec_nodup(struct string_list *list,\n+\t\t\t\t     const struct strvec *vec);\n #endif /* STRING_LIST_H */\ndiff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh\nindex f9bbb91f64e..575805513a3 100755\n--- a/t/t0023-crlf-am.sh\n+++ b/t/t0023-crlf-am.sh\n@@ -2,6 +2,7 @@\n \n test_description='Test am with auto.crlf'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >patchfile <<\\EOF\ndiff --git a/t/t4152-am-subjects.sh b/t/t4152-am-subjects.sh\nindex 4c68245acad..9f2edba1f83 100755\n--- a/t/t4152-am-subjects.sh\n+++ b/t/t4152-am-subjects.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test subject preservation with format-patch | am'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n make_patches() {\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex 54be7da1611..45f1d4f95e5 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git am with corrupt input'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n make_mbox_with_nul () {\ndiff --git a/t/t4256-am-format-flowed.sh b/t/t4256-am-format-flowed.sh\nindex 2369c4e17ad..1015273bc82 100755\n--- a/t/t4256-am-format-flowed.sh\n+++ b/t/t4256-am-format-flowed.sh\n@@ -2,6 +2,7 @@\n \n test_description='test format=flowed support of git am'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 978f240cdac..cfaae547398 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -7,6 +7,7 @@ test_description='Test the post-checkout hook.'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\n\n\n"},{"id":"460182","messageId":"patch-v2-1.6-12a4a20c59f-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 1/6] bisect.c: add missing \"goto\" for release_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:03Z","receivedAt":"2022-07-29T08:31:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a missing \"goto cleanup\", this fixes a bug in\nf196c1e908d (revisions API users: use release_revisions() needing\nREV_INFO_INIT, 2022-04-13).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex b63669cc9d7..421470bfa59 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1054,7 +1054,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \t\t */\n \t\tres = error_if_skipped_commits(tried, NULL);\n \t\tif (res < 0)\n-\t\t\treturn res;\n+\t\t\tgoto cleanup;\n \t\tprintf(_(\"%s was both %s and %s\\n\"),\n \t\t       oid_to_hex(current_bad_oid),\n \t\t       term_good,\n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460183","messageId":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com","subject":"[PATCH v2 0/6] revisions API: fix more memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:02Z","receivedAt":"2022-07-29T08:31:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A late re-roll of this set of revisions API and API user memory leak\nfixes. I think the much simpler code here in 4/6 should address the\npoints Jeff King brought up in the v1 review.\n\nOther than that I renamed the variable in 3/6 s/cp/pending/g, per\nJeff's suggestion (FWIW \"cp\" = \"copy\").\n\n1. https://lore.kernel.org/git/cover-0.6-00000000000-20220713T130511Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (6):\n  bisect.c: add missing \"goto\" for release_revisions()\n  test-fast-rebase helper: use release_revisions() (again)\n  log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n  log: fix common \"rev.pending\" memory leak in \"git show\"\n  bisect.c: partially fix bisect_rev_setup() memory leak\n  revisions API: don't leak memory on argv elements that need free()-ing\n\n bisect.c                                      | 28 +++++++++++--------\n builtin/log.c                                 | 21 +++++++-------\n builtin/submodule--helper.c                   |  5 +++-\n remote.c                                      |  5 +++-\n revision.c                                    |  2 ++\n revision.h                                    |  3 +-\n t/helper/test-fast-rebase.c                   |  2 --\n t/t0203-gettext-setlocale-sanity.sh           |  1 +\n t/t1020-subdirectory.sh                       |  1 +\n t/t2020-checkout-detach.sh                    |  1 +\n t/t3307-notes-man.sh                          |  1 +\n t/t3920-crlf-messages.sh                      |  2 ++\n t/t4069-remerge-diff.sh                       |  1 +\n t/t7007-show.sh                               |  1 +\n ...3-pre-commit-and-pre-merge-commit-hooks.sh |  1 +\n t/t9122-git-svn-author.sh                     |  1 -\n t/t9162-git-svn-dcommit-interactive.sh        |  1 -\n 17 files changed, 49 insertions(+), 28 deletions(-)\n\nRange-diff against v1:\n1:  6624eb315a5 = 1:  12a4a20c59f bisect.c: add missing \"goto\" for release_revisions()\n2:  0da0acba8b0 = 2:  bbd3a7e5ecc test-fast-rebase helper: use release_revisions() (again)\n3:  f7cc2f6a381 ! 3:  1629299f883 log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n    @@ builtin/log.c: static void show_setup_revisions_tweak(struct rev_info *rev,\n      \tstruct rev_info rev;\n     -\tstruct object_array_entry *objects;\n     +\tstruct object_array blank = OBJECT_ARRAY_INIT;\n    -+\tstruct object_array cp = OBJECT_ARRAY_INIT;\n    ++\tstruct object_array pending = OBJECT_ARRAY_INIT;\n     +\tunsigned int i;\n      \tstruct setup_revision_opt opt;\n      \tstruct pathspec match_all;\n    @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \n     -\tcount = rev.pending.nr;\n     -\tobjects = rev.pending.objects;\n    -+\tmemcpy(&cp, &rev.pending, sizeof(rev.pending));\n    ++\tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n      \trev.diffopt.no_free = 1;\n     -\tfor (i = 0; i < count && !ret; i++) {\n     -\t\tstruct object *o = objects[i].item;\n     -\t\tconst char *name = objects[i].name;\n    -+\tfor (i = 0; i < cp.nr && !ret; i++) {\n    -+\t\tstruct object *o = cp.objects[i].item;\n    -+\t\tconst char *name = cp.objects[i].name;\n    ++\tfor (i = 0; i < pending.nr && !ret; i++) {\n    ++\t\tstruct object *o = pending.objects[i].item;\n    ++\t\tconst char *name = pending.objects[i].name;\n      \t\tswitch (o->type) {\n      \t\tcase OBJ_BLOB:\n      \t\t\tret = show_blob_object(&o->oid, &rev, name);\n    @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \t\t\t\tret = error(_(\"could not read object %s\"),\n      \t\t\t\t\t    oid_to_hex(oid));\n     -\t\t\tobjects[i].item = o;\n    -+\t\t\tcp.objects[i].item = o;\n    ++\t\t\tpending.objects[i].item = o;\n      \t\t\ti--;\n      \t\t\tbreak;\n      \t\t}\n4:  2c5b41d2b24 ! 4:  54b632c1124 log: fix common \"rev.pending\" memory leak in \"git show\"\n    @@ Commit message\n         In the preceding commit this code was made to use a more extendable\n         pattern, which we can now complete. Once we've clobbered our\n         \"rev.pending\" and invoked \"cmd_log_walk_no_free()\" we need to\n    -    \"object_array_clear()\" our newly created \"rev.pending\" to avoid\n    -    leaking the memory related to the one member array we've created.\n    +    \"object_array_clear()\" our old \"rev.pending\".\n     \n    -    But more importantly we need to set \"rev.pending\" back to the original\n    -    we squirreled away in the \"cp\" variable, so that we'll make use of the\n    +    So in addition to the now-populated &pending array, to avoid leaking\n    +    the memory related to the one member array we've created. The\n    +    &rev.pending was already being object_array_clear()'d by the\n         release_revisions() added in f6bfea0ad01 (revisions API users: use\n    -    release_revisions() in builtin/log.c, 2022-04-13). In f6bfea0ad01 this\n    -    memory leak was noted as an outstanding TODO, but it's now been fixed.\n    +    release_revisions() in builtin/log.c, 2022-04-13), now we'll also\n    +    correctly free the previous data (f6bfea0ad01 noted this memory leak\n    +    as an outstanding TODO).\n    +\n    +    This way of doing it is making assumptions about the internals of\n    +    \"struct rev_info\", in particular that the \"pending\" member is\n    +    stand-alone, and that no other state is referring to it. But we've\n    +    been making those assumptions ever since 5d7eeee2ac6 (git-show: grok\n    +    blobs, trees and tags, too, 2006-12-14).\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## builtin/log.c ##\n     @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n    - \t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n    + \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n    + \n    + \tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n    ++\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n    + \trev.diffopt.no_free = 1;\n    + \tfor (i = 0; i < pending.nr && !ret; i++) {\n    + \t\tstruct object *o = pending.objects[i].item;\n    +@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n    + \t\t\trev.shown_one = 1;\n    + \t\t\tbreak;\n    + \t\tcase OBJ_COMMIT:\n    +-\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n      \t\t\tadd_object_array(o, name, &rev.pending);\n      \t\t\tret = cmd_log_walk_no_free(&rev);\n    -+\t\t\tobject_array_clear(&rev.pending);\n    -+\t\t\tmemcpy(&rev.pending, &cp, sizeof(rev.pending));\n      \t\t\tbreak;\n    - \t\tdefault:\n    +@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n    + \t\t}\n    + \t}\n    ++\tobject_array_clear(&pending);\n    + \n    + \trev.diffopt.no_free = 0;\n    + \tdiff_free(&rev.diffopt);\n     \n      ## t/t0203-gettext-setlocale-sanity.sh ##\n     @@\n5:  525e3427396 = 5:  2af2bce2d17 bisect.c: partially fix bisect_rev_setup() memory leak\n6:  291e660a2bc = 6:  3c57e126554 revisions API: don't leak memory on argv elements that need free()-ing\n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460184","messageId":"patch-v2-2.6-bbd3a7e5ecc-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 2/6] test-fast-rebase helper: use release_revisions() (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:04Z","receivedAt":"2022-07-29T08:31:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in 0139c58ab95 (revisions API users: add \"goto cleanup\" for\nrelease_revisions(), 2022-04-13), in that commit a release_revisions()\ncall was added to this function, but it never did anything due to this\nTODO memset() added in fe1a21d5267 (fast-rebase: demonstrate\nmerge-ort's API via new test-tool command, 2020-10-29).\n\nSimply removing the memset() will fix the \"cmdline\" which can be seen\nwhen running t5520-pull.sh.\n\nThis sort of thing could be detected automatically with a rule similar\nto the unused.cocci merged in 7fa60d2a5b6 (Merge branch\n'ab/cocci-unused' into next, 2022-07-11). The following rule on top\nwould catch the case being fixed here:\n\n\t@@\n\ttype T;\n\tidentifier I;\n\tidentifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n\tidentifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n\t@@\n\n\t- memset(\\( I \\| &I \\), 0, ...);\n\t  ... when != \\( I \\| &I \\)\n\t(\n\t  \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n\t|\n\t  \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n\t)\n\t  ... when != \\( I \\| &I \\)\n\nThat rule should arguably use only &I, not I (as we might be passed a\npointer). He distinction would matter if anyone cared about the\nside-effects of a memset() followed by release() of a pointer to a\nvariable passed into the function.\n\nAs such a pattern would be at best very confusing, and most likely\npoint to buggy code as in this case, the above rule is probably fine\nas-is.\n\nBut as this rule only found one such bug in the entire codebase let's\nnot add it to contrib/coccinelle/unused.cocci for now, we can always\ndig it up in the future if it's deemed useful.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-fast-rebase.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c\nindex 4e5553e2024..45665ec19a5 100644\n--- a/t/helper/test-fast-rebase.c\n+++ b/t/helper/test-fast-rebase.c\n@@ -184,8 +184,6 @@ int cmd__fast_rebase(int argc, const char **argv)\n \t\tlast_picked_commit = commit;\n \t\tlast_commit = create_commit(result.tree, commit, last_commit);\n \t}\n-\t/* TODO: There should be some kind of rev_info_free(&revs) call... */\n-\tmemset(&revs, 0, sizeof(revs));\n \n \tmerge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);\n \n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460185","messageId":"patch-v2-3.6-1629299f883-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 3/6] log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:05Z","receivedAt":"2022-07-29T08:31:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code added in 5d7eeee2ac6 (git-show: grok blobs, trees and\ntags, too, 2006-12-14) to use the \"memcpy a &blank\" idiom introduced\nin 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\nmacro, 2021-07-01).\n\nNow the types in play are guaranteed to correspond, i.e. we used \"int\"\nhere for the \"count\" before, but the corresponding \"nr\" is an\n\"unsigned int\". By using a \"blank\" object we almost entirely bypass\nthat, we'll only need to declare our own \"unsigned int i\".\n\nThere are no functional changes here aside from potential overflow\nguard rails, the structure only has these three members (\"nr\", \"alloc\"\nand \"objects\"), but now we're obviously future-proof against assuming\nthat.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/log.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 88a5e98875a..6135f8191a9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -668,10 +668,12 @@ static void show_setup_revisions_tweak(struct rev_info *rev,\n int cmd_show(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n-\tstruct object_array_entry *objects;\n+\tstruct object_array blank = OBJECT_ARRAY_INIT;\n+\tstruct object_array pending = OBJECT_ARRAY_INIT;\n+\tunsigned int i;\n \tstruct setup_revision_opt opt;\n \tstruct pathspec match_all;\n-\tint i, count, ret = 0;\n+\tint ret = 0;\n \n \tinit_log_defaults();\n \tgit_config(git_log_config, NULL);\n@@ -698,12 +700,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \tif (!rev.no_walk)\n \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n \n-\tcount = rev.pending.nr;\n-\tobjects = rev.pending.objects;\n+\tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n \trev.diffopt.no_free = 1;\n-\tfor (i = 0; i < count && !ret; i++) {\n-\t\tstruct object *o = objects[i].item;\n-\t\tconst char *name = objects[i].name;\n+\tfor (i = 0; i < pending.nr && !ret; i++) {\n+\t\tstruct object *o = pending.objects[i].item;\n+\t\tconst char *name = pending.objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n \t\t\tret = show_blob_object(&o->oid, &rev, name);\n@@ -726,7 +727,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tif (!o)\n \t\t\t\tret = error(_(\"could not read object %s\"),\n \t\t\t\t\t    oid_to_hex(oid));\n-\t\t\tobjects[i].item = o;\n+\t\t\tpending.objects[i].item = o;\n \t\t\ti--;\n \t\t\tbreak;\n \t\t}\n@@ -743,8 +744,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n-\t\t\trev.pending.nr = rev.pending.alloc = 0;\n-\t\t\trev.pending.objects = NULL;\n+\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n \t\t\tbreak;\n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460186","messageId":"patch-v2-4.6-54b632c1124-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:06Z","receivedAt":"2022-07-29T08:31:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a very common memory leak introduced in 5d7eeee2ac6 (git-show: grok blobs,\ntrees and tags, too, 2006-12-14).\n\nWhen \"git show\" displays commits it needs to temporarily clobbers the\n\"rev.pending\" array, but by doing so we'll fail to\nrelease_revisions(), as we have for most other uses of \"struct\nrev_info\" since 2da81d1efb0 (Merge branch 'ab/plug-leak-in-revisions',\n2022-06-07).\n\nIn the preceding commit this code was made to use a more extendable\npattern, which we can now complete. Once we've clobbered our\n\"rev.pending\" and invoked \"cmd_log_walk_no_free()\" we need to\n\"object_array_clear()\" our old \"rev.pending\".\n\nSo in addition to the now-populated &pending array, to avoid leaking\nthe memory related to the one member array we've created. The\n&rev.pending was already being object_array_clear()'d by the\nrelease_revisions() added in f6bfea0ad01 (revisions API users: use\nrelease_revisions() in builtin/log.c, 2022-04-13), now we'll also\ncorrectly free the previous data (f6bfea0ad01 noted this memory leak\nas an outstanding TODO).\n\nThis way of doing it is making assumptions about the internals of\n\"struct rev_info\", in particular that the \"pending\" member is\nstand-alone, and that no other state is referring to it. But we've\nbeen making those assumptions ever since 5d7eeee2ac6 (git-show: grok\nblobs, trees and tags, too, 2006-12-14).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/log.c                                    | 3 ++-\n t/t0203-gettext-setlocale-sanity.sh              | 1 +\n t/t1020-subdirectory.sh                          | 1 +\n t/t3307-notes-man.sh                             | 1 +\n t/t3920-crlf-messages.sh                         | 2 ++\n t/t4069-remerge-diff.sh                          | 1 +\n t/t7007-show.sh                                  | 1 +\n t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 1 +\n t/t9122-git-svn-author.sh                        | 1 -\n t/t9162-git-svn-dcommit-interactive.sh           | 1 -\n 10 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6135f8191a9..5c7e254e994 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -701,6 +701,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n \n \tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n+\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n \trev.diffopt.no_free = 1;\n \tfor (i = 0; i < pending.nr && !ret; i++) {\n \t\tstruct object *o = pending.objects[i].item;\n@@ -744,7 +745,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n-\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n \t\t\tbreak;\n@@ -752,6 +752,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n \t\t}\n \t}\n+\tobject_array_clear(&pending);\n \n \trev.diffopt.no_free = 0;\n \tdiff_free(&rev.diffopt);\ndiff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\nindex 0ce1f22eff6..86cff324ff1 100755\n--- a/t/t0203-gettext-setlocale-sanity.sh\n+++ b/t/t0203-gettext-setlocale-sanity.sh\n@@ -5,6 +5,7 @@\n \n test_description=\"The Git C functions aren't broken by setlocale(3)\"\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-gettext.sh\n \n test_expect_success 'git show a ISO-8859-1 commit under C locale' '\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 9fdbb2af80e..45eef9457fe 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -6,6 +6,7 @@\n test_description='Try various core-level commands in subdirectory.\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/t/t3307-notes-man.sh b/t/t3307-notes-man.sh\nindex 1aa366a410e..ae316502c45 100755\n--- a/t/t3307-notes-man.sh\n+++ b/t/t3307-notes-man.sh\n@@ -4,6 +4,7 @@ test_description='Examples from the git-notes man page\n \n Make sure the manual is not full of lies.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 0276edbe3d3..4c661d4d54a 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n LIB_CRLF_BRANCHES=\"\"\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 35f94957fce..9e7cac68b1c 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -2,6 +2,7 @@\n \n test_description='remerge-diff handling'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # This test is ort-specific\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex d6cc69e0f2c..f908a4d1abc 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -2,6 +2,7 @@\n \n test_description='git show'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex ad1eb64ba0d..aa004b70a8d 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -5,6 +5,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'root commit' '\ndiff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh\nindex 527ba3d2932..0fc289ae0f0 100755\n--- a/t/t9122-git-svn-author.sh\n+++ b/t/t9122-git-svn-author.sh\n@@ -2,7 +2,6 @@\n \n test_description='git svn authorship'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'setup svn repository' '\ndiff --git a/t/t9162-git-svn-dcommit-interactive.sh b/t/t9162-git-svn-dcommit-interactive.sh\nindex e2aa8ed88a9..b3ce033a0d3 100755\n--- a/t/t9162-git-svn-dcommit-interactive.sh\n+++ b/t/t9162-git-svn-dcommit-interactive.sh\n@@ -4,7 +4,6 @@\n \n test_description='git svn dcommit --interactive series'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize repo' '\n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460187","messageId":"patch-v2-5.6-2af2bce2d17-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 5/6] bisect.c: partially fix bisect_rev_setup() memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:07Z","receivedAt":"2022-07-29T08:31:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Partially fix the memory leak noted in in 8a534b61241 (bisect: use\nargv_array API, 2011-09-13), which added the \"XXX\" comment seen in the\ncontext. We can partially fix it by having the bisect_rev_setup()\nfunction take a \"struct strvec\", rather than constructing it.\n\nAs the comment notes we need to keep the construct \"rev_argv\" around\nwhile the \"struct rev_info\" is around, which as seen in the newly\nadded \"strvec_clear()\" calls here we do after \"release_revisions()\".\n\nThis \"partially\" fixes the memory leak because we're leaking the \"--\"\nadded to the \"rev_argv\" here still, which will be addressed in a\nsubsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 421470bfa59..6afb98be7a1 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -648,11 +648,11 @@ static struct commit_list *managed_skipped(struct commit_list *list,\n }\n \n static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n+\t\t\t     struct strvec *rev_argv,\n \t\t\t     const char *prefix,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n-\tstruct strvec rev_argv = STRVEC_INIT;\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -660,16 +660,16 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n \n \t/* rev_argv.argv[0] will be ignored by setup_revisions */\n-\tstrvec_push(&rev_argv, \"bisect_rev_setup\");\n-\tstrvec_pushf(&rev_argv, bad_format, oid_to_hex(current_bad_oid));\n+\tstrvec_push(rev_argv, \"bisect_rev_setup\");\n+\tstrvec_pushf(rev_argv, bad_format, oid_to_hex(current_bad_oid));\n \tfor (i = 0; i < good_revs.nr; i++)\n-\t\tstrvec_pushf(&rev_argv, good_format,\n+\t\tstrvec_pushf(rev_argv, good_format,\n \t\t\t     oid_to_hex(good_revs.oid + i));\n-\tstrvec_push(&rev_argv, \"--\");\n+\tstrvec_push(rev_argv, \"--\");\n \tif (read_paths)\n-\t\tread_bisect_paths(&rev_argv);\n+\t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n \t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n }\n \n@@ -873,10 +873,11 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int\n static int check_ancestors(struct repository *r, int rev_nr,\n \t\t\t   struct commit **rev, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs;\n \tint res;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"^%s\", \"%s\", 0);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"^%s\", \"%s\", 0);\n \n \tbisect_common(&revs);\n \tres = (revs.commits != NULL);\n@@ -885,6 +886,7 @@ static int check_ancestors(struct repository *r, int rev_nr,\n \tclear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS);\n \n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n@@ -1010,6 +1012,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n  */\n enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs = REV_INFO_INIT;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n@@ -1037,7 +1040,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tif (res)\n \t\tgoto cleanup;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"%s\", \"^%s\", 1);\n \n \trevs.first_parent_only = !!(bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY);\n \trevs.limited = 1;\n@@ -1112,6 +1115,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tres = bisect_checkout(bisect_rev, no_checkout);\n cleanup:\n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460188","messageId":"patch-v2-6.6-3c57e126554-20220729T082919Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v2 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-29T08:31:08Z","receivedAt":"2022-07-29T08:31:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"free_removed_argv_elements\" member to \"struct\nsetup_revision_opt\", and use it to fix several memory leaks, e.g. the\none with a \"XXX\" comment added in 8a534b61241 (bisect: use argv_array\nAPI, 2011-09-13).\n\nWe have various memory leaks in APIs that take and munge \"const\nchar **argv\", e.g. parse_options(). Sometimes these APIs are given the\n\"argv\" we get to the \"main\" function, in which case we don't leak\nmemory, but other times we're giving it the \"v\" member of a \"struct\nstrvec\" we created.\n\nThere's several potential ways to fix those sort of leaks, we could\nadd a \"nodup\" mode to \"struct strvec\", which would work for the cases\nwhere we push constant strings to it. But that wouldn't work as soon\nas we used strvec_pushf(), or otherwise needed to duplicate or create\na string for that \"struct strvec\".\n\nLet's instead make it the responsibility of the revisions API. If it's\ngoing to clobber elements of argv it can also free() them, which it\nwill now do if instructed to do so via \"free_removed_argv_elements\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c                    | 6 ++++--\n builtin/submodule--helper.c | 5 ++++-\n remote.c                    | 5 ++++-\n revision.c                  | 2 ++\n revision.h                  | 3 ++-\n t/t2020-checkout-detach.sh  | 1 +\n 6 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 6afb98be7a1..38b3891f3a6 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -653,6 +653,9 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -669,8 +672,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tif (read_paths)\n \t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n-\t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);\n }\n \n static void bisect_common(struct rev_info *revs)\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fac52ade5e1..b63f420ecef 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1104,6 +1104,9 @@ static int compute_summary_module_list(struct object_id *head_oid,\n {\n \tstruct strvec diff_args = STRVEC_INIT;\n \tstruct rev_info rev;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n \tint ret = 0;\n \n@@ -1121,7 +1124,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \tinit_revisions(&rev, info->prefix);\n \trev.abbrev = 0;\n \tprecompose_argv_prefix(diff_args.nr, diff_args.v, NULL);\n-\tsetup_revisions(diff_args.nr, diff_args.v, &rev, NULL);\n+\tsetup_revisions(diff_args.nr, diff_args.v, &rev, &opt);\n \trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = submodule_summary_callback;\n \trev.diffopt.format_callback_data = &list;\ndiff --git a/remote.c b/remote.c\nindex 1ee2b145d07..d66064118cb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2169,6 +2169,9 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstruct object_id oid;\n \tstruct commit *ours, *theirs;\n \tstruct rev_info revs;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct strvec argv = STRVEC_INIT;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n@@ -2203,7 +2206,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstrvec_push(&argv, \"--\");\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n-\tsetup_revisions(argv.nr, argv.v, &revs, NULL);\n+\tsetup_revisions(argv.nr, argv.v, &revs, &opt);\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \ndiff --git a/revision.c b/revision.c\nindex 0c6e26cd9c8..35d24e4fd3e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2784,6 +2784,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tconst char *arg = argv[i];\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n+\t\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\t\tfree((char *)argv[i]);\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..bb91e7ed914 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -375,7 +375,8 @@ struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n \tunsigned int\tassume_dashdash:1,\n-\t\t\tallow_exclude_promisor_objects:1;\n+\t\t\tallow_exclude_promisor_objects:1,\n+\t\t\tfree_removed_argv_elements:1;\n \tunsigned revarg_opt;\n };\n int setup_revisions(int argc, const char **argv, struct rev_info *revs,\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex bc46713a43e..2eab6474f8d 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -4,6 +4,7 @@ test_description='checkout into detached HEAD state'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_detached () {\n-- \n2.37.1.1196.g8af3636bc64\n\n"},{"id":"460242","messageId":"YuQg5M/cSLtqOgdw@coredump.intra.peff.net","threadId":"58150","inReplyTo":"220729.86pmhoidsc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T18:03:16Z","receivedAt":"2022-07-29T18:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 09:07:40AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > In that case, we could replace your patch 5 in favor of just calling\n> > strvec_clear() at the end of bisect_rev_setup().\n> \n> 5/6 is doing that, or rather at the end of check_ancestors() and\n> bisect_next_all(), but those call bisect_rev_setup() and pass it the\n> strvec, so in terms of memory management it amounts to the same thing.\n\nRight, but my point is that we do not need to complicate the interface\nand ownership rules for bisect_rev_setup() by passing around the extra\nvariable.\n\n> > It's possible there's a\n> > case I'm missing that makes this generally not-safe, but in the case of\n> > bisect_rev_setup() there's a very limited set of items in our argv in\n> > the first place. Doing so also passes the test suite with\n> > SANITIZE=address, though again, this is just exercising the very limited\n> > bisect options here.\n> \n> I think what you're missing is that this code is basically doing this,\n> which is a common pattern in various parts of our codebase:\n> \n> \tstruct strvec sv = STRVEC_INIT;\n> \tstrvec_push(&sv, \"foo\"); // sv.v[0] \n> \tstrvec_push(&sv, \"bar\"); // sv.v[1] \n> \tsv.v[1] = NULL; // the code in revisions.c\n> \tstrvec_clear(&sv);\n> \n> I.e. you can't fix this by simply having revision.c having its own\n> strvec, because it would just .. proceed to do the same thing.\n\nRight, none of what I was suggesting above gets rid of the flag to tell\nrevisions.c that it should free elements it removes from argv. We still\nhave to do that. My point was just that if we can assume that\nsetup_revisions() does not need for the passed-in argv to exist after it\nexits (which _used_ to not be true, but I think is these days), then\nthat can simplify a few of the callers.\n\n> Of course we could alter its argv-iterating state machine to not clobber\n> that \"argv\" element to NULL, and have other subsequent code know that it\n> should stop at a new lower \"argc\" etc. etc., but that's getting at the\n> much more invasive API changes 6/6 notes as the path not taken.\n\nYeah, I don't think that's at all worth it. If anything, it could\nperhaps hold on to the \"removed\" pointer and restore it at the end, but\nI wouldn't be surprised if that gets ugly, too.\n\n> And, in the general case for things that do this to the strvec what\n> we're explicitly doing is having the caller then operate on that munged\n> argv, i.e. it's important that we change *its* argv. That's not going on\n> here, but e.g. various parse_options() callers rely on that.\n\nRight, agreed.\n\n> IIRC this fails SANITIZE=address or was otherwise broken, I didn't test\n> it now, but that's not the point...\n> \n> ... I'm just including it by way of explanation. I.e. for at least\n> *some* callers (IIRC the below mostly works, and I can't remember where\n> it's broken) it would suffice to just keep around a list of \"here's\n> stuff we should free later\".\n> \n> In case below I opted to do that with a string_list, but it could be\n> another strvec or whatever.\n\nFWIW, I don't really like this direction. It feels like a huge band-aid,\nand the right solution is either having functions _not_ munge strvecs\ntoo much, or be told that they can take ownership of removed elements\nand free them (i.e., your current patch 6).\n\n-Peff\n"},{"id":"460245","messageId":"YuQpBwUt8rAE9oym@coredump.intra.peff.net","threadId":"58150","inReplyTo":"YuQg5M/cSLtqOgdw@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T18:37:59Z","receivedAt":"2022-07-29T18:38:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 02:03:16PM -0400, Jeff King wrote:\n\n> On Fri, Jul 29, 2022 at 09:07:40AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > > In that case, we could replace your patch 5 in favor of just calling\n> > > strvec_clear() at the end of bisect_rev_setup().\n> > \n> > 5/6 is doing that, or rather at the end of check_ancestors() and\n> > bisect_next_all(), but those call bisect_rev_setup() and pass it the\n> > strvec, so in terms of memory management it amounts to the same thing.\n> \n> Right, but my point is that we do not need to complicate the interface\n> and ownership rules for bisect_rev_setup() by passing around the extra\n> variable.\n\nJust to be clear, what I'm proposing is to replace your 5/6 with the\npatch below.\n\nIt passes the leak-checker, but I don't think that should be a surprise.\nThe end result should be the same as your patch, and anyway I don't\nthink your series actually marks any scripts as PASSING_SANITIZE_LEAK\nas a result of fixing this (presumably because there are other leaks in\nthose scripts, too).\n\nThe more interesting question is whether it causes any use-after-free\nbugs. I don't think it does, and certainly SANITIZE=address agrees.\n\n-- >8 --\nSubject: [PATCH] bisect: stop leaking strvec in bisect_rev_setup()\n\nBack when 8a534b6124 (bisect: use argv_array API, 2011-09-13) was\nwritten, it was not safe to free the argv we had passed to\nsetup_revisions() until the actual traversal was done.\n\nBut since then, we've had many cleanups that makes this safe; e.g.,\ndf835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n2013-05-25) and 31faeb2088 (object_array_entry: fix memory handling of\nthe name field, 2013-05-25). The comment here is now out-dated; we can\njust clear the strvec rather than leaking.\n\nNote that there's still a small leak because of the way\nsetup_revisions() handles removed elements internally. That will be\nfixed in a subsequent patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 421470bfa5..9cce23e929 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -670,7 +670,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \t\tread_bisect_paths(&rev_argv);\n \n \tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n-\t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n+\tstrvec_clear(&rev_argv);\n }\n \n static void bisect_common(struct rev_info *revs)\n-- \n2.37.1.804.g1775fa20e0\n\n"},{"id":"460246","messageId":"xmqqwnbv69n8.fsf@gitster.g","threadId":"58150","inReplyTo":"patch-v2-3.6-1629299f883-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/6] log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-29T18:44:59Z","receivedAt":"2022-07-29T18:45:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Adjust code added in 5d7eeee2ac6 (git-show: grok blobs, trees and\n> tags, too, 2006-12-14) to use the \"memcpy a &blank\" idiom introduced\n> in 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\n> macro, 2021-07-01).\n\nOnce we start using the \"copy blank object to clear\" pattern, we may\nwant to go all the way to use structure assignment instead of\nmemcpy().\n\n"},{"id":"460247","messageId":"xmqqk07v69bh.fsf@gitster.g","threadId":"58150","inReplyTo":"YuQpBwUt8rAE9oym@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-29T18:52:02Z","receivedAt":"2022-07-29T18:52:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The more interesting question is whether it causes any use-after-free\n> bugs.\n\nThanks for mentioning this.  All the \"plug more leaks\" patches make\nme worried for exactly that.  Another is a potential subtle breakage\nhidden by use of FREE_AND_NULL() and friends, which the sanitizers\nwould probably not see, but can appear as behaviour change.\n\n> I don't think it does, and certainly SANITIZE=address agrees.\n\n;-)\n\n> -- >8 --\n> Subject: [PATCH] bisect: stop leaking strvec in bisect_rev_setup()\n>\n> Back when 8a534b6124 (bisect: use argv_array API, 2011-09-13) was\n> written, it was not safe to free the argv we had passed to\n> setup_revisions() until the actual traversal was done.\n>\n> But since then, we've had many cleanups that makes this safe; e.g.,\n> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n> 2013-05-25) and 31faeb2088 (object_array_entry: fix memory handling of\n> the name field, 2013-05-25). The comment here is now out-dated; we can\n> just clear the strvec rather than leaking.\n>\n> Note that there's still a small leak because of the way\n> setup_revisions() handles removed elements internally. That will be\n> fixed in a subsequent patch.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  bisect.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 421470bfa5..9cce23e929 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -670,7 +670,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n>  \t\tread_bisect_paths(&rev_argv);\n>  \n>  \tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n> -\t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n> +\tstrvec_clear(&rev_argv);\n>  }\n>  \n>  static void bisect_common(struct rev_info *revs)\n"},{"id":"460248","messageId":"YuQtJ2DxNKX/y70N@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-v2-4.6-54b632c1124-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T18:55:35Z","receivedAt":"2022-07-29T18:55:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 10:31:06AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> @@ -701,6 +701,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n>  \n>  \tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n> +\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n>  \trev.diffopt.no_free = 1;\n>  \tfor (i = 0; i < pending.nr && !ret; i++) {\n>  \t\tstruct object *o = pending.objects[i].item;\n\nOK, so now \"pending\" is completely separate from what's in \"rev\". And\nthen in the later hunk we clear it:\n\n> @@ -752,6 +752,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n>  \t\t}\n>  \t}\n> +\tobject_array_clear(&pending);\n\nGood. But in the middle hunk...\n\n> @@ -744,7 +745,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\trev.shown_one = 1;\n>  \t\t\tbreak;\n>  \t\tcase OBJ_COMMIT:\n> -\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n>  \t\t\tbreak;\n\nWe now do not do anything to clean up rev.pending. On the first pass,\nwe'd see our blank pending array and add to it. But on a subsequent pass\n(i.e., because we are showing two or more commits), what will we see?\n\nMy initial assumption was we'd have the last pass's commit in \"pending\"\nhere, so we'd be leaking it. But I think in practice it's OK. We end up\nin prepare_revision_walk(), which blanks the list again, and then\nprocesses each element. Non-commits _do_ end up back in the pending\nlist, which would be a leak. But we know that this code triggers only\nfor commits, which are placed only in the \"commits\" list (and that's\ncleaned up as we walk it via get_revision()).\n\nThat might be worth mentioning in the commit message, or even including\na comment like:\n\n  /*\n   * No need to clean up rev.pending here; commits are removed from it\n   * as part of prepare_revision_walk().\n   */\n\n-Peff\n"},{"id":"460249","messageId":"YuQt9rKMJek0rEDx@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-v2-4.6-54b632c1124-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T18:59:02Z","receivedAt":"2022-07-29T18:59:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 10:31:06AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> So in addition to the now-populated &pending array, to avoid leaking\n> the memory related to the one member array we've created. The\n> &rev.pending was already being object_array_clear()'d by the\n> release_revisions() added in f6bfea0ad01 (revisions API users: use\n> release_revisions() in builtin/log.c, 2022-04-13), now we'll also\n> correctly free the previous data (f6bfea0ad01 noted this memory leak\n> as an outstanding TODO).\n\nI couldn't quite parse the first sentence. But I think this explanation\nis proceeding along a wrong assumption (which is the same one I had when\nreviewing v1) that we are leaking the one-member array. I think this\nparagraph can be replaced with something along the lines of what I\nmentioned in my other reply.\n\n-Peff\n"},{"id":"460250","messageId":"YuQuWPHK1iwuJvd4@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-v2-6.6-3c57e126554-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T19:00:40Z","receivedAt":"2022-07-29T19:00:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 10:31:08AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Add a \"free_removed_argv_elements\" member to \"struct\n> setup_revision_opt\", and use it to fix several memory leaks, e.g. the\n> one with a \"XXX\" comment added in 8a534b61241 (bisect: use argv_array\n> API, 2011-09-13).\n\nI think the mention of this \"XXX\" comment is a little misleading. That\ncomment is not talking about leaking the \"--\" entry. It is talking about\nleaking the _entire_ strvec, because of memory-lifetime issues. And we\nfixed those already in 5.\n\nIt's a small point, and the code here is fine. But if you take my\nsuggested replacement for 5, then it becomes even more nonsensical. ;)\n\n-Peff\n"},{"id":"460251","messageId":"YuQuwVRNrgfWB7SY@coredump.intra.peff.net","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/6] revisions API: fix more memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T19:02:25Z","receivedAt":"2022-07-29T19:02:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 10:31:02AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> A late re-roll of this set of revisions API and API user memory leak\n> fixes. I think the much simpler code here in 4/6 should address the\n> points Jeff King brought up in the v1 review.\n> \n> Other than that I renamed the variable in 3/6 s/cp/pending/g, per\n> Jeff's suggestion (FWIW \"cp\" = \"copy\").\n\nThanks (and for the explanation regarding t91xx; I did totally miss in\nthe v1 review that those were removing \"FAIL\" lines, not \"PASS\" ones).\n\nI don't think there's anything incorrect here, but I did make a few\ncosmetic / explanation suggestions.\n\n-Peff\n"},{"id":"460252","messageId":"YuQvUDlGPTHu2sOm@coredump.intra.peff.net","threadId":"58150","inReplyTo":"xmqqk07v69bh.fsf@gitster.g","subject":"Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T19:04:48Z","receivedAt":"2022-07-29T19:06:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 11:52:02AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The more interesting question is whether it causes any use-after-free\n> > bugs.\n> \n> Thanks for mentioning this.  All the \"plug more leaks\" patches make\n> me worried for exactly that.  Another is a potential subtle breakage\n> hidden by use of FREE_AND_NULL() and friends, which the sanitizers\n> would probably not see, but can appear as behaviour change.\n\nYeah, agreed that is a potential pitfall.\n\n> > I don't think it does, and certainly SANITIZE=address agrees.\n> \n> ;-)\n\nJust to expound a bit: I'm quite sure _this_ call site is fine, because\nwe are passing pretty vanilla options to setup_revisions(), and I looked\nat how we handle the strings there. I'd worry a little more that there's\nsome dark corner of setup_revisions() which relies on memory lifetimes,\nand so this isn't a safe thing to do in the general case.\n\nBut IMHO we should assume that setup_revisions() makes copies of strings\nit needs, and if there is a dark corner where it doesn't, we should find\nand fix that.\n\n-Peff\n"},{"id":"460253","messageId":"YuQv7T11VSj0EDSA@coredump.intra.peff.net","threadId":"58150","inReplyTo":"YuQtJ2DxNKX/y70N@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/6] log: fix common \"rev.pending\" memory leak in \"git show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-29T19:07:25Z","receivedAt":"2022-07-29T19:08:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2022 at 02:55:35PM -0400, Jeff King wrote:\n\n> > @@ -744,7 +745,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> >  \t\t\trev.shown_one = 1;\n> >  \t\t\tbreak;\n> >  \t\tcase OBJ_COMMIT:\n> > -\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n> >  \t\t\tadd_object_array(o, name, &rev.pending);\n> >  \t\t\tret = cmd_log_walk_no_free(&rev);\n> >  \t\t\tbreak;\n> \n> We now do not do anything to clean up rev.pending. On the first pass,\n> we'd see our blank pending array and add to it. But on a subsequent pass\n> (i.e., because we are showing two or more commits), what will we see?\n> \n> My initial assumption was we'd have the last pass's commit in \"pending\"\n> here, so we'd be leaking it. But I think in practice it's OK. We end up\n> in prepare_revision_walk(), which blanks the list again, and then\n> processes each element. Non-commits _do_ end up back in the pending\n> list, which would be a leak. But we know that this code triggers only\n> for commits, which are placed only in the \"commits\" list (and that's\n> cleaned up as we walk it via get_revision()).\n\nSorry, just one more clarification here.\n\nThe \"so we'd be leaking it\" in the second paragraph is really: so before\nthis patch, we'd have been leaking it writing over it with blank. After\nthis patch, we'd be building up an ever-growing list of pending objects,\nand showing a quadratic number of entries. ;)\n\n-Peff\n"},{"id":"460299","messageId":"CAPig+cRF23qR2TF6TpRnMkj+FYQJBoEJUfCzSbF8EGWzANgXeg@mail.gmail.com","threadId":"58150","inReplyTo":"patch-v2-2.6-bbd3a7e5ecc-20220729T082919Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/6] test-fast-rebase helper: use release_revisions() (again)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-30T05:22:12Z","receivedAt":"2022-07-30T05:22:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 29, 2022 at 4:33 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> Fix a bug in 0139c58ab95 (revisions API users: add \"goto cleanup\" for\n> release_revisions(), 2022-04-13), in that commit a release_revisions()\n> call was added to this function, but it never did anything due to this\n> TODO memset() added in fe1a21d5267 (fast-rebase: demonstrate\n> merge-ort's API via new test-tool command, 2020-10-29).\n>\n> Simply removing the memset() will fix the \"cmdline\" which can be seen\n> when running t5520-pull.sh.\n>\n> This sort of thing could be detected automatically with a rule similar\n> to the unused.cocci merged in 7fa60d2a5b6 (Merge branch\n> 'ab/cocci-unused' into next, 2022-07-11). The following rule on top\n> would catch the case being fixed here:\n>\n>         @@\n>         type T;\n>         identifier I;\n>         identifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n>         identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n>         @@\n>\n>         - memset(\\( I \\| &I \\), 0, ...);\n>           ... when != \\( I \\| &I \\)\n>         (\n>           \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n>         |\n>           \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n>         )\n>           ... when != \\( I \\| &I \\)\n>\n> That rule should arguably use only &I, not I (as we might be passed a\n> pointer). He distinction would matter if anyone cared about the\n\ns/He/The/\n\n> side-effects of a memset() followed by release() of a pointer to a\n> variable passed into the function.\n>\n> As such a pattern would be at best very confusing, and most likely\n> point to buggy code as in this case, the above rule is probably fine\n> as-is.\n>\n> But as this rule only found one such bug in the entire codebase let's\n> not add it to contrib/coccinelle/unused.cocci for now, we can always\n> dig it up in the future if it's deemed useful.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n"},{"id":"460435","messageId":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v2-0.6-00000000000-20220729T082919Z-avarab@gmail.com","subject":"[PATCH v3 0/6] revisions API: fix more memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:10Z","receivedAt":"2022-08-02T15:33:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes various leaking users of the revisions API.\n\nChanges since v2:\n\n* I think the rewrite here of fixing the leak should address Jeff\n  King's concerns. I was somewhat mentally stuck on trying to change\n  the code to make the leak fix easier, and then having the leak fix.\n\n  But I think as the new 3/6 shows fixing the leak first is much\n  easier, and more straightforward to explain.\n\n* A new 4/6 then follows-up and rewrites the variable clobbering that\n  was needed for the pre-image of 3/6.\n\n* Don't reference the \"XXX\" comment in 6/6.\n\n* A typo fix, pointed out by Eric Sunshine.\n\nThanks all for the review on the v2!\n\nÆvar Arnfjörð Bjarmason (6):\n  bisect.c: add missing \"goto\" for release_revisions()\n  test-fast-rebase helper: use release_revisions() (again)\n  log: fix a memory leak in \"git show <revision>...\"\n  log: refactor \"rev.pending\" code in cmd_show()\n  bisect.c: partially fix bisect_rev_setup() memory leak\n  revisions API: don't leak memory on argv elements that need free()-ing\n\n bisect.c                                      | 28 ++++++++++-------\n builtin/log.c                                 | 31 +++++++++++++------\n builtin/submodule--helper.c                   |  5 ++-\n remote.c                                      |  5 ++-\n revision.c                                    |  2 ++\n revision.h                                    |  3 +-\n t/helper/test-fast-rebase.c                   |  2 --\n t/t0203-gettext-setlocale-sanity.sh           |  1 +\n t/t1020-subdirectory.sh                       |  1 +\n t/t2020-checkout-detach.sh                    |  1 +\n t/t3307-notes-man.sh                          |  1 +\n t/t3920-crlf-messages.sh                      |  2 ++\n t/t4069-remerge-diff.sh                       |  1 +\n t/t7007-show.sh                               |  1 +\n ...3-pre-commit-and-pre-merge-commit-hooks.sh |  1 +\n t/t9122-git-svn-author.sh                     |  1 -\n t/t9162-git-svn-dcommit-interactive.sh        |  1 -\n 17 files changed, 59 insertions(+), 28 deletions(-)\n\nRange-diff against v2:\n1:  12a4a20c59f = 1:  12a4a20c59f bisect.c: add missing \"goto\" for release_revisions()\n2:  bbd3a7e5ecc ! 2:  a04fade9d9d test-fast-rebase helper: use release_revisions() (again)\n    @@ Commit message\n                   ... when != \\( I \\| &I \\)\n     \n         That rule should arguably use only &I, not I (as we might be passed a\n    -    pointer). He distinction would matter if anyone cared about the\n    +    pointer). The distinction would matter if anyone cared about the\n         side-effects of a memset() followed by release() of a pointer to a\n         variable passed into the function.\n     \n4:  54b632c1124 ! 3:  83fc1835fe5 log: fix common \"rev.pending\" memory leak in \"git show\"\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    log: fix common \"rev.pending\" memory leak in \"git show\"\n    -\n    -    Fix a very common memory leak introduced in 5d7eeee2ac6 (git-show: grok blobs,\n    -    trees and tags, too, 2006-12-14).\n    -\n    -    When \"git show\" displays commits it needs to temporarily clobbers the\n    -    \"rev.pending\" array, but by doing so we'll fail to\n    -    release_revisions(), as we have for most other uses of \"struct\n    -    rev_info\" since 2da81d1efb0 (Merge branch 'ab/plug-leak-in-revisions',\n    -    2022-06-07).\n    -\n    -    In the preceding commit this code was made to use a more extendable\n    -    pattern, which we can now complete. Once we've clobbered our\n    -    \"rev.pending\" and invoked \"cmd_log_walk_no_free()\" we need to\n    -    \"object_array_clear()\" our old \"rev.pending\".\n    -\n    -    So in addition to the now-populated &pending array, to avoid leaking\n    -    the memory related to the one member array we've created. The\n    -    &rev.pending was already being object_array_clear()'d by the\n    -    release_revisions() added in f6bfea0ad01 (revisions API users: use\n    -    release_revisions() in builtin/log.c, 2022-04-13), now we'll also\n    -    correctly free the previous data (f6bfea0ad01 noted this memory leak\n    -    as an outstanding TODO).\n    -\n    -    This way of doing it is making assumptions about the internals of\n    -    \"struct rev_info\", in particular that the \"pending\" member is\n    -    stand-alone, and that no other state is referring to it. But we've\n    -    been making those assumptions ever since 5d7eeee2ac6 (git-show: grok\n    -    blobs, trees and tags, too, 2006-12-14).\n    +    log: fix a memory leak in \"git show <revision>...\"\n    +\n    +    Fix a memory leak in code added in 5d7eeee2ac6 (git-show: grok blobs,\n    +    trees and tags, too, 2006-12-14). As we iterate over a \"<revision>...\"\n    +    command-line and encounter ad OBJ_COMMIT we want to use our \"struct\n    +    rev_info\", but with a \"pending\" array of one element: the one commit\n    +    we're showing in the loop.\n    +\n    +    To do this 5d7eeee2ac6 saved away a pointer to rev.pending.objects and\n    +    rev.pending.nr for its iteration. We'd then clobber those (and alloc)\n    +    when we needed to show an OBJ_COMMIT.\n    +\n    +    We'd therefore leak the \"rev.pending\" we started out with, and only\n    +    free the new \"rev.pending\" in the \"OBJ_COMMIT\" case arm as\n    +    prepare_revision_walk() would draw it down.\n    +\n    +    Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we\n    +    save away the \"rev.pending\" before clearing it. We then add a single\n    +    commit to it, which our indirect invocation of prepare_revision_walk()\n    +    will remove. After that we restore the \"rev.pending\".\n    +\n    +    Our \"rev.pending\" will then get free'd by the release_revisions()\n    +    added in f6bfea0ad01 (revisions API users: use release_revisions() in\n    +    builtin/log.c, 2022-04-13)\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## builtin/log.c ##\n    -@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n    - \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n    - \n    - \tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n    -+\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n    - \trev.diffopt.no_free = 1;\n    - \tfor (i = 0; i < pending.nr && !ret; i++) {\n    - \t\tstruct object *o = pending.objects[i].item;\n     @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \t\t\trev.shown_one = 1;\n      \t\t\tbreak;\n      \t\tcase OBJ_COMMIT:\n    --\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n    ++\t\t{\n    ++\t\t\tstruct object_array old;\n    ++\n    ++\t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n    + \t\t\trev.pending.nr = rev.pending.alloc = 0;\n    + \t\t\trev.pending.objects = NULL;\n      \t\t\tadd_object_array(o, name, &rev.pending);\n      \t\t\tret = cmd_log_walk_no_free(&rev);\n    ++\t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n      \t\t\tbreak;\n    -@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n    ++\t\t}\n    + \t\tdefault:\n      \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n      \t\t}\n    - \t}\n    -+\tobject_array_clear(&pending);\n    - \n    - \trev.diffopt.no_free = 0;\n    - \tdiff_free(&rev.diffopt);\n     \n      ## t/t0203-gettext-setlocale-sanity.sh ##\n     @@\n3:  1629299f883 ! 4:  fd474666e7c log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    log: make the intent of cmd_show()'s \"rev.pending\" juggling clearer\n    +    log: refactor \"rev.pending\" code in cmd_show()\n     \n    -    Adjust code added in 5d7eeee2ac6 (git-show: grok blobs, trees and\n    -    tags, too, 2006-12-14) to use the \"memcpy a &blank\" idiom introduced\n    -    in 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\n    -    macro, 2021-07-01).\n    +    Refactor the juggling of \"rev.pending\" and our replacement for it\n    +    amended in the preceding commit so that:\n     \n    -    Now the types in play are guaranteed to correspond, i.e. we used \"int\"\n    -    here for the \"count\" before, but the corresponding \"nr\" is an\n    -    \"unsigned int\". By using a \"blank\" object we almost entirely bypass\n    -    that, we'll only need to declare our own \"unsigned int i\".\n    +     * We use an \"unsigned int\" instead of an \"int\" for \"i\", this matches\n    +       the types of \"struct rev_info\" itself.\n     \n    -    There are no functional changes here aside from potential overflow\n    -    guard rails, the structure only has these three members (\"nr\", \"alloc\"\n    -    and \"objects\"), but now we're obviously future-proof against assuming\n    -    that.\n    +     * We don't need the \"count\" and \"objects\" variables introduced in\n    +       5d7eeee2ac6 (git-show: grok blobs, trees and tags, too, 2006-12-14).\n    +\n    +       They were originally added since we'd clobber rev.pending in the\n    +       loop without restoring it. Since the preceding commit we are\n    +       restoring it when we handle OBJ_COMMIT, so the main for-loop can\n    +       refer to \"rev.pending\" didrectly.\n    +\n    +     * We use the \"memcpy a &blank\" idiom introduced in\n    +       5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\n    +       macro, 2021-07-01).\n    +\n    +       This is more obvious than relying on us enumerating all of the\n    +       relevant members of the \"struct object_array\" that we need to\n    +       clear.\n    +\n    +     * We comment on why we don't need an object_array_clear() here, see\n    +       the analysis in [1].\n    +\n    +    1. https://lore.kernel.org/git/YuQtJ2DxNKX%2Fy70N@coredump.intra.peff.net/\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    +    Helped-by: Jeff King <peff@peff.net>\n     \n      ## builtin/log.c ##\n     @@ builtin/log.c: static void show_setup_revisions_tweak(struct rev_info *rev,\n    @@ builtin/log.c: static void show_setup_revisions_tweak(struct rev_info *rev,\n      {\n      \tstruct rev_info rev;\n     -\tstruct object_array_entry *objects;\n    -+\tstruct object_array blank = OBJECT_ARRAY_INIT;\n    -+\tstruct object_array pending = OBJECT_ARRAY_INIT;\n     +\tunsigned int i;\n      \tstruct setup_revision_opt opt;\n      \tstruct pathspec match_all;\n    @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \n     -\tcount = rev.pending.nr;\n     -\tobjects = rev.pending.objects;\n    -+\tmemcpy(&pending, &rev.pending, sizeof(rev.pending));\n      \trev.diffopt.no_free = 1;\n     -\tfor (i = 0; i < count && !ret; i++) {\n     -\t\tstruct object *o = objects[i].item;\n     -\t\tconst char *name = objects[i].name;\n    -+\tfor (i = 0; i < pending.nr && !ret; i++) {\n    -+\t\tstruct object *o = pending.objects[i].item;\n    -+\t\tconst char *name = pending.objects[i].name;\n    ++\tfor (i = 0; i < rev.pending.nr && !ret; i++) {\n    ++\t\tstruct object *o = rev.pending.objects[i].item;\n    ++\t\tconst char *name = rev.pending.objects[i].name;\n      \t\tswitch (o->type) {\n      \t\tcase OBJ_BLOB:\n      \t\t\tret = show_blob_object(&o->oid, &rev, name);\n    @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n      \t\t\t\tret = error(_(\"could not read object %s\"),\n      \t\t\t\t\t    oid_to_hex(oid));\n     -\t\t\tobjects[i].item = o;\n    -+\t\t\tpending.objects[i].item = o;\n    ++\t\t\trev.pending.objects[i].item = o;\n      \t\t\ti--;\n      \t\t\tbreak;\n      \t\t}\n     @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix)\n    - \t\t\trev.shown_one = 1;\n    - \t\t\tbreak;\n      \t\tcase OBJ_COMMIT:\n    + \t\t{\n    + \t\t\tstruct object_array old;\n    ++\t\t\tstruct object_array blank = OBJECT_ARRAY_INIT;\n    + \n    + \t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n     -\t\t\trev.pending.nr = rev.pending.alloc = 0;\n     -\t\t\trev.pending.objects = NULL;\n     +\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n    ++\n      \t\t\tadd_object_array(o, name, &rev.pending);\n      \t\t\tret = cmd_log_walk_no_free(&rev);\n    ++\n    ++\t\t\t/*\n    ++\t\t\t * No need for\n    ++\t\t\t * object_array_clear(&pending). It was\n    ++\t\t\t * cleared already in prepare_revision_walk()\n    ++\t\t\t */\n    + \t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n      \t\t\tbreak;\n    + \t\t}\n5:  2af2bce2d17 = 5:  dae5872e722 bisect.c: partially fix bisect_rev_setup() memory leak\n6:  3c57e126554 ! 6:  5e637f55ff1 revisions API: don't leak memory on argv elements that need free()-ing\n    @@ Commit message\n         revisions API: don't leak memory on argv elements that need free()-ing\n     \n         Add a \"free_removed_argv_elements\" member to \"struct\n    -    setup_revision_opt\", and use it to fix several memory leaks, e.g. the\n    -    one with a \"XXX\" comment added in 8a534b61241 (bisect: use argv_array\n    -    API, 2011-09-13).\n    +    setup_revision_opt\", and use it to fix several memory leaks.\n     \n         We have various memory leaks in APIs that take and munge \"const\n         char **argv\", e.g. parse_options(). Sometimes these APIs are given the\n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460436","messageId":"patch-v3-1.6-12a4a20c59f-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 1/6] bisect.c: add missing \"goto\" for release_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:11Z","receivedAt":"2022-08-02T15:33:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a missing \"goto cleanup\", this fixes a bug in\nf196c1e908d (revisions API users: use release_revisions() needing\nREV_INFO_INIT, 2022-04-13).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex b63669cc9d7..421470bfa59 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1054,7 +1054,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \t\t */\n \t\tres = error_if_skipped_commits(tried, NULL);\n \t\tif (res < 0)\n-\t\t\treturn res;\n+\t\t\tgoto cleanup;\n \t\tprintf(_(\"%s was both %s and %s\\n\"),\n \t\t       oid_to_hex(current_bad_oid),\n \t\t       term_good,\n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460437","messageId":"patch-v3-2.6-a04fade9d9d-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 2/6] test-fast-rebase helper: use release_revisions() (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:12Z","receivedAt":"2022-08-02T15:33:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in 0139c58ab95 (revisions API users: add \"goto cleanup\" for\nrelease_revisions(), 2022-04-13), in that commit a release_revisions()\ncall was added to this function, but it never did anything due to this\nTODO memset() added in fe1a21d5267 (fast-rebase: demonstrate\nmerge-ort's API via new test-tool command, 2020-10-29).\n\nSimply removing the memset() will fix the \"cmdline\" which can be seen\nwhen running t5520-pull.sh.\n\nThis sort of thing could be detected automatically with a rule similar\nto the unused.cocci merged in 7fa60d2a5b6 (Merge branch\n'ab/cocci-unused' into next, 2022-07-11). The following rule on top\nwould catch the case being fixed here:\n\n\t@@\n\ttype T;\n\tidentifier I;\n\tidentifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n\tidentifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n\t@@\n\n\t- memset(\\( I \\| &I \\), 0, ...);\n\t  ... when != \\( I \\| &I \\)\n\t(\n\t  \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n\t|\n\t  \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n\t)\n\t  ... when != \\( I \\| &I \\)\n\nThat rule should arguably use only &I, not I (as we might be passed a\npointer). The distinction would matter if anyone cared about the\nside-effects of a memset() followed by release() of a pointer to a\nvariable passed into the function.\n\nAs such a pattern would be at best very confusing, and most likely\npoint to buggy code as in this case, the above rule is probably fine\nas-is.\n\nBut as this rule only found one such bug in the entire codebase let's\nnot add it to contrib/coccinelle/unused.cocci for now, we can always\ndig it up in the future if it's deemed useful.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-fast-rebase.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c\nindex 4e5553e2024..45665ec19a5 100644\n--- a/t/helper/test-fast-rebase.c\n+++ b/t/helper/test-fast-rebase.c\n@@ -184,8 +184,6 @@ int cmd__fast_rebase(int argc, const char **argv)\n \t\tlast_picked_commit = commit;\n \t\tlast_commit = create_commit(result.tree, commit, last_commit);\n \t}\n-\t/* TODO: There should be some kind of rev_info_free(&revs) call... */\n-\tmemset(&revs, 0, sizeof(revs));\n \n \tmerge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);\n \n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460438","messageId":"patch-v3-3.6-83fc1835fe5-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 3/6] log: fix a memory leak in \"git show <revision>...\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:13Z","receivedAt":"2022-08-02T15:33:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in code added in 5d7eeee2ac6 (git-show: grok blobs,\ntrees and tags, too, 2006-12-14). As we iterate over a \"<revision>...\"\ncommand-line and encounter ad OBJ_COMMIT we want to use our \"struct\nrev_info\", but with a \"pending\" array of one element: the one commit\nwe're showing in the loop.\n\nTo do this 5d7eeee2ac6 saved away a pointer to rev.pending.objects and\nrev.pending.nr for its iteration. We'd then clobber those (and alloc)\nwhen we needed to show an OBJ_COMMIT.\n\nWe'd therefore leak the \"rev.pending\" we started out with, and only\nfree the new \"rev.pending\" in the \"OBJ_COMMIT\" case arm as\nprepare_revision_walk() would draw it down.\n\nLet's fix this memory leak. Now when we encounter an OBJ_COMMIT we\nsave away the \"rev.pending\" before clearing it. We then add a single\ncommit to it, which our indirect invocation of prepare_revision_walk()\nwill remove. After that we restore the \"rev.pending\".\n\nOur \"rev.pending\" will then get free'd by the release_revisions()\nadded in f6bfea0ad01 (revisions API users: use release_revisions() in\nbuiltin/log.c, 2022-04-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/log.c                                    | 6 ++++++\n t/t0203-gettext-setlocale-sanity.sh              | 1 +\n t/t1020-subdirectory.sh                          | 1 +\n t/t3307-notes-man.sh                             | 1 +\n t/t3920-crlf-messages.sh                         | 2 ++\n t/t4069-remerge-diff.sh                          | 1 +\n t/t7007-show.sh                                  | 1 +\n t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 1 +\n t/t9122-git-svn-author.sh                        | 1 -\n t/t9162-git-svn-dcommit-interactive.sh           | 1 -\n 10 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 88a5e98875a..b4b1d974617 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n+\t\t{\n+\t\t\tstruct object_array old;\n+\n+\t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n \t\t\trev.pending.nr = rev.pending.alloc = 0;\n \t\t\trev.pending.objects = NULL;\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n+\t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n \t\t\tbreak;\n+\t\t}\n \t\tdefault:\n \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n \t\t}\ndiff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\nindex 0ce1f22eff6..86cff324ff1 100755\n--- a/t/t0203-gettext-setlocale-sanity.sh\n+++ b/t/t0203-gettext-setlocale-sanity.sh\n@@ -5,6 +5,7 @@\n \n test_description=\"The Git C functions aren't broken by setlocale(3)\"\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-gettext.sh\n \n test_expect_success 'git show a ISO-8859-1 commit under C locale' '\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 9fdbb2af80e..45eef9457fe 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -6,6 +6,7 @@\n test_description='Try various core-level commands in subdirectory.\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/t/t3307-notes-man.sh b/t/t3307-notes-man.sh\nindex 1aa366a410e..ae316502c45 100755\n--- a/t/t3307-notes-man.sh\n+++ b/t/t3307-notes-man.sh\n@@ -4,6 +4,7 @@ test_description='Examples from the git-notes man page\n \n Make sure the manual is not full of lies.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 0276edbe3d3..4c661d4d54a 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n LIB_CRLF_BRANCHES=\"\"\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 35f94957fce..9e7cac68b1c 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -2,6 +2,7 @@\n \n test_description='remerge-diff handling'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # This test is ort-specific\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex d6cc69e0f2c..f908a4d1abc 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -2,6 +2,7 @@\n \n test_description='git show'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex ad1eb64ba0d..aa004b70a8d 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -5,6 +5,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'root commit' '\ndiff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh\nindex 527ba3d2932..0fc289ae0f0 100755\n--- a/t/t9122-git-svn-author.sh\n+++ b/t/t9122-git-svn-author.sh\n@@ -2,7 +2,6 @@\n \n test_description='git svn authorship'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'setup svn repository' '\ndiff --git a/t/t9162-git-svn-dcommit-interactive.sh b/t/t9162-git-svn-dcommit-interactive.sh\nindex e2aa8ed88a9..b3ce033a0d3 100755\n--- a/t/t9162-git-svn-dcommit-interactive.sh\n+++ b/t/t9162-git-svn-dcommit-interactive.sh\n@@ -4,7 +4,6 @@\n \n test_description='git svn dcommit --interactive series'\n \n-TEST_FAILS_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize repo' '\n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460439","messageId":"patch-v3-5.6-dae5872e722-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 5/6] bisect.c: partially fix bisect_rev_setup() memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:15Z","receivedAt":"2022-08-02T15:33:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Partially fix the memory leak noted in in 8a534b61241 (bisect: use\nargv_array API, 2011-09-13), which added the \"XXX\" comment seen in the\ncontext. We can partially fix it by having the bisect_rev_setup()\nfunction take a \"struct strvec\", rather than constructing it.\n\nAs the comment notes we need to keep the construct \"rev_argv\" around\nwhile the \"struct rev_info\" is around, which as seen in the newly\nadded \"strvec_clear()\" calls here we do after \"release_revisions()\".\n\nThis \"partially\" fixes the memory leak because we're leaking the \"--\"\nadded to the \"rev_argv\" here still, which will be addressed in a\nsubsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 421470bfa59..6afb98be7a1 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -648,11 +648,11 @@ static struct commit_list *managed_skipped(struct commit_list *list,\n }\n \n static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n+\t\t\t     struct strvec *rev_argv,\n \t\t\t     const char *prefix,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n-\tstruct strvec rev_argv = STRVEC_INIT;\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -660,16 +660,16 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n \n \t/* rev_argv.argv[0] will be ignored by setup_revisions */\n-\tstrvec_push(&rev_argv, \"bisect_rev_setup\");\n-\tstrvec_pushf(&rev_argv, bad_format, oid_to_hex(current_bad_oid));\n+\tstrvec_push(rev_argv, \"bisect_rev_setup\");\n+\tstrvec_pushf(rev_argv, bad_format, oid_to_hex(current_bad_oid));\n \tfor (i = 0; i < good_revs.nr; i++)\n-\t\tstrvec_pushf(&rev_argv, good_format,\n+\t\tstrvec_pushf(rev_argv, good_format,\n \t\t\t     oid_to_hex(good_revs.oid + i));\n-\tstrvec_push(&rev_argv, \"--\");\n+\tstrvec_push(rev_argv, \"--\");\n \tif (read_paths)\n-\t\tread_bisect_paths(&rev_argv);\n+\t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n \t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n }\n \n@@ -873,10 +873,11 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int\n static int check_ancestors(struct repository *r, int rev_nr,\n \t\t\t   struct commit **rev, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs;\n \tint res;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"^%s\", \"%s\", 0);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"^%s\", \"%s\", 0);\n \n \tbisect_common(&revs);\n \tres = (revs.commits != NULL);\n@@ -885,6 +886,7 @@ static int check_ancestors(struct repository *r, int rev_nr,\n \tclear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS);\n \n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n@@ -1010,6 +1012,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n  */\n enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n {\n+\tstruct strvec rev_argv = STRVEC_INIT;\n \tstruct rev_info revs = REV_INFO_INIT;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n@@ -1037,7 +1040,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tif (res)\n \t\tgoto cleanup;\n \n-\tbisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n+\tbisect_rev_setup(r, &revs, &rev_argv, prefix, \"%s\", \"^%s\", 1);\n \n \trevs.first_parent_only = !!(bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY);\n \trevs.limited = 1;\n@@ -1112,6 +1115,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tres = bisect_checkout(bisect_rev, no_checkout);\n cleanup:\n \trelease_revisions(&revs);\n+\tstrvec_clear(&rev_argv);\n \treturn res;\n }\n \n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460440","messageId":"patch-v3-4.6-fd474666e7c-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:14Z","receivedAt":"2022-08-02T15:33:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the juggling of \"rev.pending\" and our replacement for it\namended in the preceding commit so that:\n\n * We use an \"unsigned int\" instead of an \"int\" for \"i\", this matches\n   the types of \"struct rev_info\" itself.\n\n * We don't need the \"count\" and \"objects\" variables introduced in\n   5d7eeee2ac6 (git-show: grok blobs, trees and tags, too, 2006-12-14).\n\n   They were originally added since we'd clobber rev.pending in the\n   loop without restoring it. Since the preceding commit we are\n   restoring it when we handle OBJ_COMMIT, so the main for-loop can\n   refer to \"rev.pending\" didrectly.\n\n * We use the \"memcpy a &blank\" idiom introduced in\n   5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT\n   macro, 2021-07-01).\n\n   This is more obvious than relying on us enumerating all of the\n   relevant members of the \"struct object_array\" that we need to\n   clear.\n\n * We comment on why we don't need an object_array_clear() here, see\n   the analysis in [1].\n\n1. https://lore.kernel.org/git/YuQtJ2DxNKX%2Fy70N@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b4b1d974617..9b937d59b83 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -668,10 +668,10 @@ static void show_setup_revisions_tweak(struct rev_info *rev,\n int cmd_show(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n-\tstruct object_array_entry *objects;\n+\tunsigned int i;\n \tstruct setup_revision_opt opt;\n \tstruct pathspec match_all;\n-\tint i, count, ret = 0;\n+\tint ret = 0;\n \n \tinit_log_defaults();\n \tgit_config(git_log_config, NULL);\n@@ -698,12 +698,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \tif (!rev.no_walk)\n \t\treturn cmd_log_deinit(cmd_log_walk(&rev), &rev);\n \n-\tcount = rev.pending.nr;\n-\tobjects = rev.pending.objects;\n \trev.diffopt.no_free = 1;\n-\tfor (i = 0; i < count && !ret; i++) {\n-\t\tstruct object *o = objects[i].item;\n-\t\tconst char *name = objects[i].name;\n+\tfor (i = 0; i < rev.pending.nr && !ret; i++) {\n+\t\tstruct object *o = rev.pending.objects[i].item;\n+\t\tconst char *name = rev.pending.objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n \t\t\tret = show_blob_object(&o->oid, &rev, name);\n@@ -726,7 +724,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tif (!o)\n \t\t\t\tret = error(_(\"could not read object %s\"),\n \t\t\t\t\t    oid_to_hex(oid));\n-\t\t\tobjects[i].item = o;\n+\t\t\trev.pending.objects[i].item = o;\n \t\t\ti--;\n \t\t\tbreak;\n \t\t}\n@@ -745,12 +743,19 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\tcase OBJ_COMMIT:\n \t\t{\n \t\t\tstruct object_array old;\n+\t\t\tstruct object_array blank = OBJECT_ARRAY_INIT;\n \n \t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n-\t\t\trev.pending.nr = rev.pending.alloc = 0;\n-\t\t\trev.pending.objects = NULL;\n+\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n+\n \t\t\tadd_object_array(o, name, &rev.pending);\n \t\t\tret = cmd_log_walk_no_free(&rev);\n+\n+\t\t\t/*\n+\t\t\t * No need for\n+\t\t\t * object_array_clear(&pending). It was\n+\t\t\t * cleared already in prepare_revision_walk()\n+\t\t\t */\n \t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n \t\t\tbreak;\n \t\t}\n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460441","messageId":"patch-v3-6.6-5e637f55ff1-20220802T152925Z-avarab@gmail.com","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"[PATCH v3 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-02T15:33:16Z","receivedAt":"2022-08-02T15:34:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"free_removed_argv_elements\" member to \"struct\nsetup_revision_opt\", and use it to fix several memory leaks.\n\nWe have various memory leaks in APIs that take and munge \"const\nchar **argv\", e.g. parse_options(). Sometimes these APIs are given the\n\"argv\" we get to the \"main\" function, in which case we don't leak\nmemory, but other times we're giving it the \"v\" member of a \"struct\nstrvec\" we created.\n\nThere's several potential ways to fix those sort of leaks, we could\nadd a \"nodup\" mode to \"struct strvec\", which would work for the cases\nwhere we push constant strings to it. But that wouldn't work as soon\nas we used strvec_pushf(), or otherwise needed to duplicate or create\na string for that \"struct strvec\".\n\nLet's instead make it the responsibility of the revisions API. If it's\ngoing to clobber elements of argv it can also free() them, which it\nwill now do if instructed to do so via \"free_removed_argv_elements\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c                    | 6 ++++--\n builtin/submodule--helper.c | 5 ++++-\n remote.c                    | 5 ++++-\n revision.c                  | 2 ++\n revision.h                  | 3 ++-\n t/t2020-checkout-detach.sh  | 1 +\n 6 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 6afb98be7a1..38b3891f3a6 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -653,6 +653,9 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \t\t\t     const char *bad_format, const char *good_format,\n \t\t\t     int read_paths)\n {\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -669,8 +672,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tif (read_paths)\n \t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);\n-\t/* XXX leak rev_argv, as \"revs\" may still be pointing to it */\n+\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);\n }\n \n static void bisect_common(struct rev_info *revs)\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fac52ade5e1..b63f420ecef 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1104,6 +1104,9 @@ static int compute_summary_module_list(struct object_id *head_oid,\n {\n \tstruct strvec diff_args = STRVEC_INIT;\n \tstruct rev_info rev;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n \tint ret = 0;\n \n@@ -1121,7 +1124,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \tinit_revisions(&rev, info->prefix);\n \trev.abbrev = 0;\n \tprecompose_argv_prefix(diff_args.nr, diff_args.v, NULL);\n-\tsetup_revisions(diff_args.nr, diff_args.v, &rev, NULL);\n+\tsetup_revisions(diff_args.nr, diff_args.v, &rev, &opt);\n \trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = submodule_summary_callback;\n \trev.diffopt.format_callback_data = &list;\ndiff --git a/remote.c b/remote.c\nindex 1ee2b145d07..d66064118cb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2169,6 +2169,9 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstruct object_id oid;\n \tstruct commit *ours, *theirs;\n \tstruct rev_info revs;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \tstruct strvec argv = STRVEC_INIT;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n@@ -2203,7 +2206,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstrvec_push(&argv, \"--\");\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n-\tsetup_revisions(argv.nr, argv.v, &revs, NULL);\n+\tsetup_revisions(argv.nr, argv.v, &revs, &opt);\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \ndiff --git a/revision.c b/revision.c\nindex 0c6e26cd9c8..35d24e4fd3e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2784,6 +2784,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tconst char *arg = argv[i];\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n+\t\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\t\tfree((char *)argv[i]);\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..bb91e7ed914 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -375,7 +375,8 @@ struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n \tunsigned int\tassume_dashdash:1,\n-\t\t\tallow_exclude_promisor_objects:1;\n+\t\t\tallow_exclude_promisor_objects:1,\n+\t\t\tfree_removed_argv_elements:1;\n \tunsigned revarg_opt;\n };\n int setup_revisions(int argc, const char **argv, struct rev_info *revs,\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex bc46713a43e..2eab6474f8d 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -4,6 +4,7 @@ test_description='checkout into detached HEAD state'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_detached () {\n-- \n2.37.1.1233.ge8b09efaedc\n\n"},{"id":"460561","messageId":"xmqqedxxb683.fsf@gitster.g","threadId":"58150","inReplyTo":"patch-v3-1.6-12a4a20c59f-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/6] bisect.c: add missing \"goto\" for release_revisions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T17:13:32Z","receivedAt":"2022-08-03T17:13:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Add a missing \"goto cleanup\", this fixes a bug in\n> f196c1e908d (revisions API users: use release_revisions() needing\n> REV_INFO_INIT, 2022-04-13).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  bisect.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index b63669cc9d7..421470bfa59 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -1054,7 +1054,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n>  \t\t */\n>  \t\tres = error_if_skipped_commits(tried, NULL);\n>  \t\tif (res < 0)\n> -\t\t\treturn res;\n> +\t\t\tgoto cleanup;\n>  \t\tprintf(_(\"%s was both %s and %s\\n\"),\n>  \t\t       oid_to_hex(current_bad_oid),\n>  \t\t       term_good,\n\nOK, from the cleanup: label we call release_revisions(&rev) which\nwas used in the \"failed\" attempt to find bisection.  Looks good.\n\n"},{"id":"460563","messageId":"YuqwCEqL4xZkxjL8@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-v3-3.6-83fc1835fe5-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/6] log: fix a memory leak in \"git show <revision>...\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T17:27:36Z","receivedAt":"2022-08-03T17:27:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2022 at 05:33:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we\n> save away the \"rev.pending\" before clearing it. We then add a single\n> commit to it, which our indirect invocation of prepare_revision_walk()\n> will remove. After that we restore the \"rev.pending\".\n> \n> Our \"rev.pending\" will then get free'd by the release_revisions()\n> added in f6bfea0ad01 (revisions API users: use release_revisions() in\n> builtin/log.c, 2022-04-13)\n\nOK, I agree this is doing the correct thing. I actually found the\nearlier \"let's dissociate rev.pending from rev entirely\" approach easier\nto reason about, though.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 88a5e98875a..b4b1d974617 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\trev.shown_one = 1;\n>  \t\t\tbreak;\n>  \t\tcase OBJ_COMMIT:\n> +\t\t{\n> +\t\t\tstruct object_array old;\n> +\n> +\t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n>  \t\t\trev.pending.nr = rev.pending.alloc = 0;\n>  \t\t\trev.pending.objects = NULL;\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n> +\t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n>  \t\t\tbreak;\n> +\t\t}\n\nHere we overwrite the one-item rev.pending without freeing it, but just\nimmediately after instead of before. It's a little subtle, but your\ncomment in the commit message:\n\n  [...] and only free the new \"rev.pending\" in the \"OBJ_COMMIT\" case arm\n  as prepare_revision_walk() would draw it down.\n\ncovers that. IMHO that could be spelled out a bit more (particularly\nthat this only works for OBJ_COMMIT, but that's OK because that's the\nonly type we're adding), but I can live with it.\n\n-Peff\n"},{"id":"460564","messageId":"Yuqwa6NSG0Ee+7ev@coredump.intra.peff.net","threadId":"58150","inReplyTo":"YuqwCEqL4xZkxjL8@coredump.intra.peff.net","subject":"Re: [PATCH v3 3/6] log: fix a memory leak in \"git show <revision>...\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T17:29:15Z","receivedAt":"2022-08-03T17:29:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 03, 2022 at 01:27:36PM -0400, Jeff King wrote:\n\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > index 88a5e98875a..b4b1d974617 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> >  \t\t\trev.shown_one = 1;\n> >  \t\t\tbreak;\n> >  \t\tcase OBJ_COMMIT:\n> > +\t\t{\n> > +\t\t\tstruct object_array old;\n> > +\n> > +\t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n> >  \t\t\trev.pending.nr = rev.pending.alloc = 0;\n> >  \t\t\trev.pending.objects = NULL;\n> >  \t\t\tadd_object_array(o, name, &rev.pending);\n> >  \t\t\tret = cmd_log_walk_no_free(&rev);\n> > +\t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n> >  \t\t\tbreak;\n> > +\t\t}\n> \n> Here we overwrite the one-item rev.pending without freeing it, but just\n> immediately after instead of before. It's a little subtle, but your\n> comment in the commit message:\n> \n>   [...] and only free the new \"rev.pending\" in the \"OBJ_COMMIT\" case arm\n>   as prepare_revision_walk() would draw it down.\n> \n> covers that. IMHO that could be spelled out a bit more (particularly\n> that this only works for OBJ_COMMIT, but that's OK because that's the\n> only type we're adding), but I can live with it.\n\nAh, I see that you did add a comment in the next commit. That's\nsufficient, though I really think it makes more sense here, where we're\nactually dealing with leaks.\n\n-Peff\n"},{"id":"460565","messageId":"YuqxIFkw64zcv+3c@coredump.intra.peff.net","threadId":"58150","inReplyTo":"patch-v3-4.6-fd474666e7c-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T17:32:16Z","receivedAt":"2022-08-03T17:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2022 at 05:33:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n>  * We don't need the \"count\" and \"objects\" variables introduced in\n>    5d7eeee2ac6 (git-show: grok blobs, trees and tags, too, 2006-12-14).\n> \n>    They were originally added since we'd clobber rev.pending in the\n>    loop without restoring it. Since the preceding commit we are\n>    restoring it when we handle OBJ_COMMIT, so the main for-loop can\n>    refer to \"rev.pending\" didrectly.\n\nI think this is accurate, though it does feel a bit weird that we are\niterating over rev.pending, and we clobber and restore it mid-loop.\n\nIt's correct because of the restore, but I think that's why my gut\nfeeling favored the earlier approach to completely dissociate the\niteration variables from \"rev.pending\" before the loop even starts.\n\nThat said, it seems like we're spending a lot more time going back and\nforth on this topic than it is really worth, so I can live with any of\nthe versions.\n\n-Peff\n"},{"id":"460566","messageId":"YuqxZTKR65pVFqBp@coredump.intra.peff.net","threadId":"58150","inReplyTo":"cover-v3-0.6-00000000000-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 0/6] revisions API: fix more memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T17:33:25Z","receivedAt":"2022-08-03T17:33:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2022 at 05:33:10PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Changes since v2:\n> \n> * I think the rewrite here of fixing the leak should address Jeff\n>   King's concerns. I was somewhat mentally stuck on trying to change\n>   the code to make the leak fix easier, and then having the leak fix.\n> \n>   But I think as the new 3/6 shows fixing the leak first is much\n>   easier, and more straightforward to explain.\n\nYeah, I think fixing the leak first is fine. I did prefer the end result\nof the earlier round, but I can live with it either way.\n\n> * A new 4/6 then follows-up and rewrites the variable clobbering that\n>   was needed for the pre-image of 3/6.\n> \n> * Don't reference the \"XXX\" comment in 6/6.\n\nI'd hoped you'd take my replacement 5/6, but again, I can live with it\neither way.\n\n-Peff\n"},{"id":"460567","messageId":"xmqqv8r99pry.fsf@gitster.g","threadId":"58150","inReplyTo":"patch-v3-3.6-83fc1835fe5-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/6] log: fix a memory leak in \"git show <revision>...\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T17:54:09Z","receivedAt":"2022-08-03T17:54:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a memory leak in code added in 5d7eeee2ac6 (git-show: grok blobs,\n> trees and tags, too, 2006-12-14). As we iterate over a \"<revision>...\"\n> command-line and encounter ad OBJ_COMMIT we want to use our \"struct\n\n\"... encounter an OBJ_COMMIT, we want to ...\"?\n\n> rev_info\", but with a \"pending\" array of one element: the one commit\n> we're showing in the loop.\n>\n> To do this 5d7eeee2ac6 saved away a pointer to rev.pending.objects and\n> rev.pending.nr for its iteration. We'd then clobber those (and alloc)\n> when we needed to show an OBJ_COMMIT.\n>\n> We'd therefore leak the \"rev.pending\" we started out with, and only\n> free the new \"rev.pending\" in the \"OBJ_COMMIT\" case arm as\n> prepare_revision_walk() would draw it down.\n>\n> Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we\n> save away the \"rev.pending\" before clearing it. We then add a single\n> commit to it, which our indirect invocation of prepare_revision_walk()\n> will remove. After that we restore the \"rev.pending\".\n>\n> Our \"rev.pending\" will then get free'd by the release_revisions()\n> added in f6bfea0ad01 (revisions API users: use release_revisions() in\n> builtin/log.c, 2022-04-13)\n\nHmph, this gives me a strange sense of deja-vu that I saw a better\nsolution in a separate patch from you, strange because I do not see\nanything at the tip of 'seen' like what I thought you did elsewhere.\n\nWe do need to reuse \"rev_info\" we got from outside the loop so that\nwe will have to consistently apply diffopt and other things we got\nin there from the configuration and the command line.  But after we\ndecide to go to \"each object is shown individually\" mode, the\ncontents of the pending array (rather, what we got from the command\nline in cmdline member) is only relevant to enumerate which\nindividual objects are shown in the loop, and should not even be\nvisible to the code that handles each individual object, e.g. even\nwe pass &rev to this code when we see a blob:\n\n\t\tswitch (o->type) {\n\t\tcase OBJ_BLOB:\n\t\t\tret = show_blob_object(&o->oid, &rev, name);\n\t\t\tbreak;\n\nthere is no point in exposing the rev.pending to show_blob_object()\nat al.  The same is true for codepaths used to show a tag or a tree.\nWhen showing a commit, we do not even want the codepath that shows a\nsingle-commit range to touch and destroy the original rev.pending;\nwe instead want to give a single-element pending array.\n\nSo instead of keeping rev.pending when we enter the loop and saving\nit away and restoring it, it feels a lot cleaner to invent/use an\ninterface to \"clone\" the settings in an existing rev_info by:\n\n * Grab rev.pending into a separate \"struct object_array\" that is a\n   local variable in cmd_show() and clear rev.pending, immediately\n   after we decide to go to \"show individually\" mode.\n\n * Iterate over the objects in that local variable.  For each object\n   to be shown, prepare a rev_info, clone the setting from the\n   original rev_info, put that single object to the pending member\n   of the clone, use that cloned rev_info, and release the resources\n   held by the cloned rev_info once we are done.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 88a5e98875a..b4b1d974617 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\t\trev.shown_one = 1;\n>  \t\t\tbreak;\n>  \t\tcase OBJ_COMMIT:\n> +\t\t{\n> +\t\t\tstruct object_array old;\n> +\n> +\t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n>  \t\t\trev.pending.nr = rev.pending.alloc = 0;\n>  \t\t\trev.pending.objects = NULL;\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n> +\t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n>  \t\t\tbreak;\n> +\t\t}\n\nThe reason why I find this approach a bit disturbing is that we\npretend to know for certain that pending is the only thing we need\nto protect across multiple calls to the log_walk(), but I suspect\nthat such an overconfidence will come back and bite us.  We are not\nre-initializing other states reachable from the rev_info (e.g. the\ndiff_options struct has various members that records what happened\nin an invocation, like needed_rename_limit, found_follow, and\nfound_changes, that would want to be reinitialized if we are\nstarting a new and totally independent traversal after we are done\none traversal).\n\nBut I do not mind at all to leave such a fundamental clean-up to a\ntotally separate topic, and keep this patch only about \"we are\nclobbering and leaking rev.pending, let's plug the leak\".  In fact,\nI would prefer it that way.  So take all of the above as material\nfor possible NEEDSWORK comment, food for further thought.\n\n"},{"id":"460568","messageId":"xmqqo7x19pif.fsf@gitster.g","threadId":"58150","inReplyTo":"patch-v3-4.6-fd474666e7c-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T17:59:52Z","receivedAt":"2022-08-03T17:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Refactor the juggling of \"rev.pending\" and our replacement for it\n> amended in the preceding commit so that:\n> ...\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Helped-by: Jeff King <peff@peff.net>\n\nList trailer lines chronologically, please.\n\nThis does not look wrong per-se, but given that we might want to\nrethink the way the original rev_info is reused inside the loop,\nas I mentioned in my review of the previous step, this change may be\nirrelevant in the longer term.\n\nI may have said this earlier, but once we start using the \"prepare a\nblank one, copy it to clear another\" pattern, we should stop using\nmemcpy() and use structure assignment, especially if we are trying to\nmake our intent clear.\n\n> @@ -745,12 +743,19 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \t\tcase OBJ_COMMIT:\n>  \t\t{\n>  \t\t\tstruct object_array old;\n> +\t\t\tstruct object_array blank = OBJECT_ARRAY_INIT;\n>  \n>  \t\t\tmemcpy(&old, &rev.pending, sizeof(old));\n> -\t\t\trev.pending.nr = rev.pending.alloc = 0;\n> -\t\t\trev.pending.objects = NULL;\n> +\t\t\tmemcpy(&rev.pending, &blank, sizeof(rev.pending));\n> +\n>  \t\t\tadd_object_array(o, name, &rev.pending);\n>  \t\t\tret = cmd_log_walk_no_free(&rev);\n> +\n> +\t\t\t/*\n> +\t\t\t * No need for\n> +\t\t\t * object_array_clear(&pending). It was\n> +\t\t\t * cleared already in prepare_revision_walk()\n> +\t\t\t */\n>  \t\t\tmemcpy(&rev.pending, &old, sizeof(rev.pending));\n>  \t\t\tbreak;\n>  \t\t}\n"},{"id":"460569","messageId":"xmqq8ro59o3d.fsf@gitster.g","threadId":"58150","inReplyTo":"patch-v3-6.6-5e637f55ff1-20220802T152925Z-avarab@gmail.com","subject":"Re: [PATCH v3 6/6] revisions API: don't leak memory on argv elements that need free()-ing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T18:30:30Z","receivedAt":"2022-08-03T18:30:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> @@ -2784,6 +2784,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\tconst char *arg = argv[i];\n>  \t\t\tif (strcmp(arg, \"--\"))\n>  \t\t\t\tcontinue;\n> +\t\t\tif (opt && opt->free_removed_argv_elements)\n> +\t\t\t\tfree((char *)argv[i]);\n\nWe have \"arg\", let's free that instead.\n\n>  \t\t\targv[i] = NULL;\n>  \t\t\targc = i;\n\n>  \tunsigned int\tassume_dashdash:1,\n> -\t\t\tallow_exclude_promisor_objects:1;\n> +\t\t\tallow_exclude_promisor_objects:1,\n> +\t\t\tfree_removed_argv_elements:1;\n\nIt would be nicer to pick a name that explains why we want to \"free\nremoved argv[] elements\" than \"if you remove argv[] elements, then\nplease free them\", because the explanation on the reason why we\nrequest the API to free would help future developers to decide if\nthey need to free in some situations where they do not \"remove\" but\ndo something else in their updates to the revisions API.\n\nIf we cannot come up with a better name, at least we should add a\ncomment that says that the caller owns the **argv strings, and it\nasks the API to free those if reference to them are lost in any way.\n\nThanks.  Overall the series was a pleasant read.\n\n"},{"id":"460607","messageId":"220804.865yj8e93t.gmgdl@evledraar.gmail.com","threadId":"58150","inReplyTo":"xmqqo7x19pif.fsf@gitster.g","subject":"Re: [PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-04T07:51:41Z","receivedAt":"2022-08-04T07:55:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 03 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Refactor the juggling of \"rev.pending\" and our replacement for it\n>> amended in the preceding commit so that:\n>> ...\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> Helped-by: Jeff King <peff@peff.net>\n>\n> List trailer lines chronologically, please.\n\nWilldo.\n\n> I may have said this earlier, but once we start using the \"prepare a\n> blank one, copy it to clear another\" pattern, we should stop using\n> memcpy() and use structure assignment, especially if we are trying to\n> make our intent clear.\n\nYeah, I saw that. I took it as we should consider changing this more\ngenerally (e.g. with coccicheck etc.).\n\nThis was mentioned in one of the original threads about the memcpy()\nidiom, but IIRC there was some reason to think that it wasn't as widely\nsupported, or in any case we'd want to re-rest that the compilers we\ncare about similarly optimize it.\n\nSo I think it's best to just use the more widely used pattern for now,\nand if we'd like change them all in some follow-up change...\n\n"},{"id":"460625","messageId":"xmqqy1w46luv.fsf@gitster.g","threadId":"58150","inReplyTo":"220804.865yj8e93t.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T15:59:20Z","receivedAt":"2022-08-04T15:59:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Yeah, I saw that. I took it as we should consider changing this more\n> generally (e.g. with coccicheck etc.).\n\nTo make things easier in the future, for the record, I in general do\nnot suggest such a bulk rewrite for the sake of rewrite, whether\ndriven with Coccinelle or something else, and I did not in this\ncase.\n\n> This was mentioned in one of the original threads about the memcpy()\n> idiom, but IIRC there was some reason to think that it wasn't as widely\n> supported, ...\n\nI somehow thought that we had that stage too long ago; I recall we\nspotted struct assignment in a patch post release and left it there\nwithout reverting.\n\n> ... or in any case we'd want to re-rest that the compilers we\n> care about similarly optimize it.\n\nPerhaps.  Using struct assignment only when we feel an urge to use\nmemcpy() in a new code (or in the postimage of a newly rewritten\ncode), instead of doing a bulk update, would give us a chance to\nstart small and vet the result with compilers of such a small scale\nrewrite carefully to build confidence, hopefully?\n\nThanks.\n"},{"id":"460704","messageId":"220805.868ro3rrkf.gmgdl@evledraar.gmail.com","threadId":"58150","inReplyTo":"xmqqy1w46luv.fsf@gitster.g","subject":"Re: [PATCH v3 4/6] log: refactor \"rev.pending\" code in cmd_show()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-05T09:01:58Z","receivedAt":"2022-08-05T09:02:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 04 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> [...]\n>> ... or in any case we'd want to re-rest that the compilers we\n>> care about similarly optimize it.\n>\n> Perhaps.  Using struct assignment only when we feel an urge to use\n> memcpy() in a new code (or in the postimage of a newly rewritten\n> code), instead of doing a bulk update, would give us a chance to\n> start small and vet the result with compilers of such a small scale\n> rewrite carefully to build confidence, hopefully?\n\nI think it would make sense to set out a baloon for it, but as some of\nit (and I'll note this in a re-roll) involves some new C99 support I'd\nprefer not to conflate that with this series.\n"}]}