{"thread":{"id":"56531","subject":"[PATCH 0/2] Squash leaks in t0000","startedAt":"2021-09-18T13:49:43Z","lastAt":"2021-09-21T23:06:34Z","messageCount":15,"participants":["Andrzej Hunt via GitGitGadget","Carlo Arenas","Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","Andrzej Hunt","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"436305","messageId":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","threadId":"56531","inReplyTo":null,"subject":"[PATCH 0/2] Squash leaks in t0000","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-18T13:49:36Z","receivedAt":"2021-09-18T13:49:43Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"Carlo points out that t0000 currently doesn't pass with leak-checking\nenabled in:\nhttps://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71\n\nHere's a series that I've sat on for a while, which adds some UNLEAK's to\n\"fix\" this situation - see the individual patches for a justification of why\nan UNLEAK seems appropriate.\n\nATB, Andrzej\n\nAndrzej Hunt (2):\n log: UNLEAK rev to silence a large number of leaks\n log: UNLEAK original pending objects\n\n builtin/log.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\n\nbase-commit: 186eaaae567db501179c0af0bf89b34cbea02c26\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1092%2Fahunt%2Fleaks-t0000-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1092/ahunt/leaks-t0000-v1\nPull-Request: https://github.com/git/git/pull/1092\n-- \ngitgitgadget\n"},{"id":"436306","messageId":"6d54bc264e2f9ce519f32c0673167a00bab55573.1631972978.git.gitgitgadget@gmail.com","threadId":"56531","inReplyTo":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","subject":"[PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-18T13:49:37Z","receivedAt":"2021-09-18T13:49:43Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncmd_show puts a lot of data into rev, and doesn't clean it up before\nreturning. That's reasonable - we use most if not all of rev up until\ncmd_show is finished - there's not much value in doing a proper cleanup.\nTherefore we take the easy way out and UNLEAK rev.\n\nThe UNLEAK has to be performed early on, as cmd_show might return via\ncmd_log_walk() in the next few lines, or it might continue to the\nno-walk implementation below.\n\nThis patch silences the following leaks which were found when running\nt0000 against LSAN:\n\nDirect leak of 41 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14\n #2 0x83cced in add_object_array_with_path /home/ahunt/oss-fuzz/git/object.c:349:17\n #3 0x8f4f5a in add_pending_object_with_path /home/ahunt/oss-fuzz/git/revision.c:329:2\n #4 0x8eb2b6 in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2082:2\n #5 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12\n #6 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7\n #7 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9\n #8 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2\n #9 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2\n #10 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11\n #11 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3\n #12 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4\n #13 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19\n #14 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11\n #15 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 32 byte(s) in 1 object(s) allocated from:\n #0 0x49a9d2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0x9ab4c2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8\n #2 0x59c269 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:233:18\n #3 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2\n #4 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2\n #5 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11\n #6 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3\n #7 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4\n #8 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19\n #9 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11\n #10 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 41 byte(s) in 1 object(s) allocated from:\n #0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14\n #2 0x8f5e30 in add_rev_cmdline /home/ahunt/oss-fuzz/git/revision.c:1482:23\n #3 0x8eb26d in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2081:2\n #4 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12\n #5 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7\n #6 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9\n #7 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2\n #8 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2\n #9 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11\n #10 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3\n #11 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4\n #12 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19\n #13 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11\n #14 0x7fc4b3f06349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/log.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7f..6faaddf17a6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -644,6 +644,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \topt.def = \"HEAD\";\n \topt.tweak = show_setup_revisions_tweak;\n \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n+\tUNLEAK(rev);\n \n \tif (!rev.no_walk)\n \t\treturn cmd_log_walk(&rev);\n-- \ngitgitgadget\n\n"},{"id":"436307","messageId":"aad3fe7381ced5eeff9c8d57ce90911bc59e3923.1631972978.git.gitgitgadget@gmail.com","threadId":"56531","inReplyTo":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","subject":"[PATCH 2/2] log: UNLEAK original pending objects","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-18T13:49:38Z","receivedAt":"2021-09-18T13:49:46Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncmd_show() uses objects to point to rev.pending's objects. Later, it might\ndetach rev.pending's objects: when handling an OBJ_COMMIT it will reset\nrev.pending (without freeing its objects). Detaching (as opposed to freeing)\nis necessary because cmd_show() continues iterating over the original objects\narray.\n\nWe choose to UNLEAK because there's no real advantage to cleaning up\nproperly (cmd_show() exits immediately after looping over these\nobjects). A number of alternatives exist, but are all significantly more\ncomplex for no gain:\n\nAlternative 1:\n Convert objects into an object_array, and memcpy rev.pending into it\n (followed by detaching rev.pending immediately - making objects the\n owner of what used to be rev.pending). Then we could safely\n objects_array_clear() at the end of cmd_show(). And we can rely on\n a preexisting UNLEAK(rev) to avoid having to clean up rev.pending.\n This is a more complex and riskier approach vs a simple UNLEAK,\n and doesn't add any user-visible value.\n\nAlternative 2:\n A variation on alternative 1. We make objects own the object_array as\n before. Once we're done, we free the new rev.pending array (which\n might be empty), and we memcpy objects back into rev.pending, relying\n on the existin UNLEAK(rev) to avoid having to free rev.pending.\n\nASAN output from t0000:\n\nDirect leak of 41 byte(s) in 1 object(s) allocated from:\n #0 0x487504 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:437:3\n #1 0x9e4ef8 in xstrdup wrapper.c:29:14\n #2 0x86395d in add_object_array_with_path object.c:366:17\n #3 0x9264fc in add_pending_object_with_path revision.c:330:2\n #4 0x91c4e0 in handle_revision_arg_1 revision.c:2086:2\n #5 0x91bfcd in handle_revision_arg revision.c:2093:12\n #6 0x91ff5a in setup_revisions revision.c:2780:7\n #7 0x5a7678 in cmd_log_init_finish builtin/log.c:206:9\n #8 0x5a4f18 in cmd_log_init builtin/log.c:278:2\n #9 0x5a55d1 in cmd_show builtin/log.c:646:2\n #10 0x4cff30 in run_builtin git.c:461:11\n #11 0x4cdb00 in handle_builtin git.c:713:3\n #12 0x4cf527 in run_argv git.c:780:4\n #13 0x4cd426 in cmd_main git.c:911:19\n #14 0x6b2eb5 in main common-main.c:52:11\n #15 0x7f74fc9bd349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 41 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6faaddf17a6..769ee6a9258 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -702,7 +702,8 @@ 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-\tfree(objects);\n+\tUNLEAK(objects);\n+\n \treturn ret;\n }\n \n-- \ngitgitgadget\n"},{"id":"436316","messageId":"CAPUEspjjBAr07VB7XqGFVXDcKWkkX4OUTCxUf+VJaEAX8KTAyw@mail.gmail.com","threadId":"56531","inReplyTo":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Squash leaks in t0000","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-18T17:28:17Z","receivedAt":"2021-09-18T17:28:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Sep 18, 2021 at 6:49 AM Andrzej Hunt via GitGitGadget\n wrote:\n> Carlo points out that t0000 currently doesn't pass with leak-checking\n> enabled in:\n> https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71\n\nDid you figure out why it doesn't trigger on maint even if the code\nseems to be mostly the same?\nAt least seems to trigger consistently in master, next and seen.\n\n> Here's a series that I've sat on for a while, which adds some UNLEAK's to\n> \"fix\" this situation - see the individual patches for a justification of why\n> an UNLEAK seems appropriate.\n\nWhile I see that UNLEAK in this specific case, might be an ok \"fix\", I\nhave to admit that not finding a repo_clear_revisions() (or equivalent\nfunction) that could be used to clear revs seems like a problem worth\nfixing as well for the future.\n\nWill reply with my WIP so we can see if it could work either as an\nalternative to this, or at least lay some foundations so that a long\nrunning process that needs to use a `struct revision` or some of this\nlogic can in the future without having to deal with leaks.\n\nThanks and \"Reviewed-by: Carlo Marcelo Arenas Belón\n\" if needed.\n\nCarlo\n"},{"id":"436320","messageId":"YUZG0D5ayEWd7MLP@carlos-mbp.lan","threadId":"56531","inReplyTo":"6d54bc264e2f9ce519f32c0673167a00bab55573.1631972978.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-18T20:06:40Z","receivedAt":"2021-09-18T20:06:48Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"My equivalent version for these fixes is obviously more verbose but IMHO\nnot that ugly (and as safe)\n\nIt avoids the need to UNLEAK early by changing the program flow also for\nthe early return so the cleanup could be centralized in one single\nfunction.\n\nBoth, the cmdline and mailmap arrays (and the objects they accumulate)\nare cleaned in a \"reusable\" way.\n\nNote that the cleaning of the \"name\" in the cmdline item throws a warning\nas shown below which I intentionally didn't fix, as it would seem that\neither the use of const there or the need to strdup is wrong. So hope\nsomeone that knows this code better could chime in.\n\nCarlo\n------ >8 ------\nSubject: [PATCH] builtin/log: leaks from `git show` in t0000\n\nobviously not ready, since the following will need to be corrected:\n\n revision.c:1496:8: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]\n free(info->rev[i].name);\n ^~~~~~~~~~~~~~~~~\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/log.c | 8 ++++++--\n revision.c | 20 ++++++++++++++++++++\n revision.h | 5 +++++\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7..1b1c1f53f4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -645,8 +645,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \topt.tweak = show_setup_revisions_tweak;\n \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n \n-\tif (!rev.no_walk)\n-\t\treturn cmd_log_walk(&rev);\n+\tif (!rev.no_walk) {\n+\t\tret = cmd_log_walk(&rev);\n+\t\tgoto done;\n+\t}\n \n \tcount = rev.pending.nr;\n \tobjects = rev.pending.objects;\n@@ -702,6 +704,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \tfree(objects);\n+done:\n+\trepo_clear_revisions(&rev);\n \treturn ret;\n }\n \ndiff --git a/revision.c b/revision.c\nindex 0dabb5a0bc..ce62192dd8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1487,6 +1487,18 @@ static void add_rev_cmdline(struct rev_info *revs,\n \tinfo->nr++;\n }\n \n+static void clear_rev_cmdline(struct rev_info *revs)\n+{\n+\tstruct rev_cmdline_info *info = &revs->cmdline;\n+\tsize_t i, nr = info->nr;\n+\n+\tfor (i = 0; i < nr; i++)\n+\t\tfree(info->rev[i].name);\n+\n+\tFREE_AND_NULL(info->rev);\n+\tinfo->nr = info->alloc = 0;\n+}\n+\n static void add_rev_cmdline_list(struct rev_info *revs,\n \t\t\t\t struct commit_list *commit_list,\n \t\t\t\t int whence,\n@@ -1845,6 +1857,14 @@ void repo_init_revisions(struct repository *r,\n \tinit_display_notes(&revs->notes_opt);\n }\n \n+void repo_clear_revisions(struct rev_info *revs)\n+{\n+\tif (revs->mailmap)\n+\t\tclear_mailmap(revs->mailmap);\n+\tFREE_AND_NULL(revs->mailmap);\n+\tclear_rev_cmdline(revs);\n+}\n+\n static void add_pending_commit_list(struct rev_info *revs,\n \t\t\t\t struct commit_list *commit_list,\n \t\t\t\t unsigned int flags)\ndiff --git a/revision.h b/revision.h\nindex 0c65a760ee..f695c41cee 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -358,6 +358,11 @@ void repo_init_revisions(struct repository *r,\n \t\t\t struct rev_info *revs,\n \t\t\t const char *prefix);\n \n+/*\n+ * Free all structures dynamically allocated for the provided rev_info\n+ */\n+void repo_clear_revisions(struct rev_info *revs);\n+\n /**\n * Parse revision information, filling in the `rev_info` structure, and\n * removing the used arguments from the argument list. Returns the number\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436366","messageId":"87a6k8daeu.fsf@evledraar.gmail.com","threadId":"56531","inReplyTo":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Squash leaks in t0000","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T10:58:16Z","receivedAt":"2021-09-19T12:20:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 18 2021, Andrzej Hunt via GitGitGadget wrote:\n\n> Carlo points out that t0000 currently doesn't pass with leak-checking\n> enabled in:\n> https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71\n>\n> Here's a series that I've sat on for a while, which adds some UNLEAK's to\n> \"fix\" this situation - see the individual patches for a justification of why\n> an UNLEAK seems appropriate.\n>\n> ATB, Andrzej\n>\n> Andrzej Hunt (2):\n> log: UNLEAK rev to silence a large number of leaks\n> log: UNLEAK original pending objects\n>\n> builtin/log.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n\nI sent a re-roll of that series[1] that bypasses the issue by no longer\nrunning t0000-basic.sh, so there won't be an immediate need for a fixup\nseries like this.\n\nAs for these patches & approach, I think that these unleak fixes that\njust narrowly squash some failure in a specific test aren't worth doing,\nand are actually counter-productive.\n\nWe should instead eventually fix the leaks more generally and make the\nbuilt-ins use those APIs.\n\nMaybe our differing approaches there are because we've got different\nend-goals in mind. My end-goal is three-fold:\n\n A. Make git's core APIs nicer, in most cases that we're not freeing\n memory is a result of a rather messy API that's not quite sure who\n should be managing its memory. This usually makes using it correctly\n harder in other ways.\n\n B. Make those APIs not leak memory, so we can use them as libraries.\n\n C. Have regression tests testing [*B*]\n\nGiven that, I think that fixing memory leaks in built-in when we're\nabout to exit is completely pointless as a goal in itself. We're about\nto exit anyway, why care that we're leaking memory?\n\nThe only reason, I think, is that we're doing it as a proxy to get to a\ncombination of [*A*] and [*B*] above. Once we know that we can run \"git\nlog\" in various modes without it leaking, it's likely that most or all\nof the revisions walking API, refname resolution, object lookup\netc. isn't leaking.\n\nI have a WIP branch that would obsolete this[2], see the commit at its\ntip. As shown there you're fixing a leak in cmd_show(), but omit the\nsame leak in its sister functions.\n\nAt that point we won't need these UNLEAK(), and as a follow-up any\nconcerns about spending too much time in a built-in just to clean up\ncould rather easily be done with something like a GIT_DESTRUCT_LEVEL[3],\ni.e. we'd conditionally skip the freeing in some cases.\n\nI'm not saying that there's no point in adding UNLEAK() somewhere, but I\nreally don't see it in this case. We didn't *need* to mark\nt0000-basic.sh as leak-free right away, I just did so because it was the\nfirst test, and I naïvely thought it would stay that way while my series\ncooked.\n\nI'd think that when building on top of my SANITIZE=leak series you'd\nwant instead of UNLEAK() to instead label the test as\nTEST_PASSES_SANITIZE_LEAK=true, but just omit some specific breakages\nwith a use of the \"SANITIZE_LEAK\" prerequisite.\n\nMaybe there's cases where you'd want to use\nTEST_PASSES_SANITIZE_LEAK=true, but the leak is so deep in the guts of\nsome API that a transitional UNLEAK() is worth it, *and* you can't just\nmark some other test that mostly tests the command you're interested in\nwith TEST_PASSES_SANITIZE_LEAK=true.\n\nBut so far I haven't seen such cases, e.g. there's cases where \"git tag\"\nleaks in obscure cases, but not in some common cases with some of my\npreliminary fixes. In that state I can usually find a test that uses\n\"git tag\" in some way and mark that as TEST_PASSES_SANITIZE_LEAK=true,\ninstead of sprinkling UNLEAK() in builtin/tag.c just so I can mark the\nmain \"git tag\" test as passing.\n\n1. 62833https://lore.kernel.org/git/cover-v7-0.2-00000000000-20210919T075619Z-avarab@gmail.com/\n2. https://github.com/git/git/compare/master...avar:avar/tests-post-add-sanitize-leak-test-mode-fix-leaks\n3. https://lore.kernel.org/git/87y2bi0vvl.fsf@evledraar.gmail.com/\n"},{"id":"436372","messageId":"05754f9c-cd58-30f5-e2d3-58b9221d2770@ahunt.org","threadId":"56531","inReplyTo":"CAPUEspjjBAr07VB7XqGFVXDcKWkkX4OUTCxUf+VJaEAX8KTAyw@mail.gmail.com","subject":"Re: [PATCH 0/2] Squash leaks in t0000","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-09-19T15:38:43Z","receivedAt":"2021-09-19T15:47:56Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 18/09/2021 19:28, Carlo Arenas wrote:\n> \n>> Here's a series that I've sat on for a while, which adds some UNLEAK's to\n>> \"fix\" this situation - see the individual patches for a justification of why\n>> an UNLEAK seems appropriate.\n> \n> While I see that UNLEAK in this specific case, might be an ok \"fix\", I\n> have to admit that not finding a repo_clear_revisions() (or equivalent\n> function) that could be used to clear revs seems like a problem worth\n> fixing as well for the future.\n> \n> Will reply with my WIP so we can see if it could work either as an\n> alternative to this, or at least lay some foundations so that a long\n> running process that needs to use a `struct revision` or some of this\n> logic can in the future without having to deal with leaks.\n\nGreat - in this case I think it's best to ignore my patches since \nactually fixing the leaks is obviously a better solution :) !\n\nATB,\n\nAndrzej\n\n"},{"id":"436373","messageId":"fd65fe57-819e-88d5-8ba1-99bb59a980bb@ahunt.org","threadId":"56531","inReplyTo":"YUZG0D5ayEWd7MLP@carlos-mbp.lan","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-09-19T15:51:38Z","receivedAt":"2021-09-19T15:51:48Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 18/09/2021 22:06, Carlo Marcelo Arenas Belón wrote:\n> My equivalent version for these fixes is obviously more verbose but IMHO\n> not that ugly (and as safe)\n> \n> It avoids the need to UNLEAK early by changing the program flow also for\n> the early return so the cleanup could be centralized in one single\n> function.\n> \n> Both, the cmdline and mailmap arrays (and the objects they accumulate)\n> are cleaned in a \"reusable\" way.\n> \n> Note that the cleaning of the \"name\" in the cmdline item throws a warning\n> as shown below which I intentionally didn't fix, as it would seem that\n> either the use of const there or the need to strdup is wrong. So hope\n> someone that knows this code better could chime in.\n> \n> Carlo\n> ------ >8 ------\n> Subject: [PATCH] builtin/log: leaks from `git show` in t0000\n> \n> obviously not ready, since the following will need to be corrected:\n> \n> revision.c:1496:8: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]\n> free(info->rev[i].name);\n> ^~~~~~~~~~~~~~~~~\n> \n\nCasting the pointer a la \"free((void *) ...)\" seems to be a common \npattern in git, and seems like a reasonable option here. AFAIUI the \nconst is still needed because clients of rev_cmdline_info shouldn't be \nchanging name. But since we own and created rev_cmdline_info, we also \nknow it's safe to clean it up. For comparison, here's an example of \nsubmodule_entry being cleaned up - all members end up needing a cast:\n\nstatic void free_one_config(struct submodule_entry *entry)\n{\n\tfree((void *) entry->config->path);\n\tfree((void *) entry->config->name);\n\tfree((void *) entry->config->branch);\n\tfree((void *) entry->config->update_strategy.command);\n\tfree(entry->config);\n}\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> builtin/log.c | 8 ++++++--\n> revision.c | 20 ++++++++++++++++++++\n> revision.h | 5 +++++\n> 3 files changed, 31 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index f75d87e8d7..1b1c1f53f4 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -645,8 +645,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \topt.tweak = show_setup_revisions_tweak;\n> \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n> \n> -\tif (!rev.no_walk)\n> -\t\treturn cmd_log_walk(&rev);\n> +\tif (!rev.no_walk) {\n> +\t\tret = cmd_log_walk(&rev);\n> +\t\tgoto done;\n> +\t}\n> \n> \tcount = rev.pending.nr;\n> \tobjects = rev.pending.objects;\n> @@ -702,6 +704,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \tfree(objects);\n> +done:\n> +\trepo_clear_revisions(&rev);\n> \treturn ret;\n> }\n> \n> diff --git a/revision.c b/revision.c\n> index 0dabb5a0bc..ce62192dd8 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1487,6 +1487,18 @@ static void add_rev_cmdline(struct rev_info *revs,\n> \tinfo->nr++;\n> }\n> \n> +static void clear_rev_cmdline(struct rev_info *revs)\n> +{\n> +\tstruct rev_cmdline_info *info = &revs->cmdline;\n> +\tsize_t i, nr = info->nr;\n> +\n> +\tfor (i = 0; i < nr; i++)\n> +\t\tfree(info->rev[i].name);\n> +\n> +\tFREE_AND_NULL(info->rev);\n> +\tinfo->nr = info->alloc = 0;\n> +}\n> +\n> static void add_rev_cmdline_list(struct rev_info *revs,\n> \t\t\t\t struct commit_list *commit_list,\n> \t\t\t\t int whence,\n> @@ -1845,6 +1857,14 @@ void repo_init_revisions(struct repository *r,\n> \tinit_display_notes(&revs->notes_opt);\n> }\n> \n> +void repo_clear_revisions(struct rev_info *revs)\n> +{\n> +\tif (revs->mailmap)\n> +\t\tclear_mailmap(revs->mailmap);\n> +\tFREE_AND_NULL(revs->mailmap);\n> +\tclear_rev_cmdline(revs);\n> +}\n> +\n> static void add_pending_commit_list(struct rev_info *revs,\n> \t\t\t\t struct commit_list *commit_list,\n> \t\t\t\t unsigned int flags)\n> diff --git a/revision.h b/revision.h\n> index 0c65a760ee..f695c41cee 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -358,6 +358,11 @@ void repo_init_revisions(struct repository *r,\n> \t\t\t struct rev_info *revs,\n> \t\t\t const char *prefix);\n> \n> +/*\n> + * Free all structures dynamically allocated for the provided rev_info\n> + */\n> +void repo_clear_revisions(struct rev_info *revs);\n> +\n> /**\n> * Parse revision information, filling in the `rev_info` structure, and\n> * removing the used arguments from the argument list. Returns the number\n> \n\nThis patch looks good to me (modulo adding the cast as discussed above), \nand is obviously much better than my approach of using an UNLEAK!\n\nATB,\n\nAndrzej\n"},{"id":"436374","messageId":"87o88obkb1.fsf@evledraar.gmail.com","threadId":"56531","inReplyTo":"YUZG0D5ayEWd7MLP@carlos-mbp.lan","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T16:13:43Z","receivedAt":"2021-09-19T16:29:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 18 2021, Carlo Marcelo Arenas Belón wrote:\n\n> My equivalent version for these fixes is obviously more verbose but IMHO\n> not that ugly (and as safe)\n>\n> It avoids the need to UNLEAK early by changing the program flow also for\n> the early return so the cleanup could be centralized in one single\n> function.\n>\n> Both, the cmdline and mailmap arrays (and the objects they accumulate)\n> are cleaned in a \"reusable\" way.\n>\n> Note that the cleaning of the \"name\" in the cmdline item throws a warning\n> as shown below which I intentionally didn't fix, as it would seem that\n> either the use of const there or the need to strdup is wrong. So hope\n> someone that knows this code better could chime in.\n\nIt should just be a \"char *\", I got that wrong in my version posted in\nthe side-thread[1] & mentioned in the side-reply[2].\n\n(I think I got it right in some earlier version days ago, it should be a\n'char *' like anyting we malloc, but brainfart when re-doing/re-basing\nthose changes).\n\nYours here below has a bug where you free() rev_cmdline_info items, you\nneed to use release_revisions_cmdline_rev().\n\nI should have said in [2], but thanks a lot to you and Andrzej for\nfollowing up on the mess in t0000-basic.sh addressed by my v7\nre-roll. It'll be really nice to get some of these leaks fixed & tested\nfor.\n\nI think I was rather curt in [2] after a long debugging session, just\nsaying I appreciate it. Hopefully we can figure out some plan for mostly\npulling in the same direction with regards to the way forward.\n\n1. https://github.com/git/git/commit/06380cd4f56f4c542685eb7aa79e28285fe02c55\n2. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/\n\n> Carlo\n> ------ >8 ------\n> Subject: [PATCH] builtin/log: leaks from `git show` in t0000\n>\n> obviously not ready, since the following will need to be corrected:\n>\n> revision.c:1496:8: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]\n> free(info->rev[i].name);\n> ^~~~~~~~~~~~~~~~~\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> builtin/log.c | 8 ++++++--\n> revision.c | 20 ++++++++++++++++++++\n> revision.h | 5 +++++\n> 3 files changed, 31 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index f75d87e8d7..1b1c1f53f4 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -645,8 +645,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \topt.tweak = show_setup_revisions_tweak;\n> \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n> \n> -\tif (!rev.no_walk)\n> -\t\treturn cmd_log_walk(&rev);\n> +\tif (!rev.no_walk) {\n> +\t\tret = cmd_log_walk(&rev);\n> +\t\tgoto done;\n> +\t}\n> \n> \tcount = rev.pending.nr;\n> \tobjects = rev.pending.objects;\n> @@ -702,6 +704,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \tfree(objects);\n> +done:\n> +\trepo_clear_revisions(&rev);\n> \treturn ret;\n> }\n> \n> diff --git a/revision.c b/revision.c\n> index 0dabb5a0bc..ce62192dd8 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1487,6 +1487,18 @@ static void add_rev_cmdline(struct rev_info *revs,\n> \tinfo->nr++;\n> }\n> \n> +static void clear_rev_cmdline(struct rev_info *revs)\n> +{\n> +\tstruct rev_cmdline_info *info = &revs->cmdline;\n> +\tsize_t i, nr = info->nr;\n> +\n> +\tfor (i = 0; i < nr; i++)\n> +\t\tfree(info->rev[i].name);\n> +\n> +\tFREE_AND_NULL(info->rev);\n> +\tinfo->nr = info->alloc = 0;\n> +}\n> +\n> static void add_rev_cmdline_list(struct rev_info *revs,\n> \t\t\t\t struct commit_list *commit_list,\n> \t\t\t\t int whence,\n> @@ -1845,6 +1857,14 @@ void repo_init_revisions(struct repository *r,\n> \tinit_display_notes(&revs->notes_opt);\n> }\n> \n> +void repo_clear_revisions(struct rev_info *revs)\n> +{\n> +\tif (revs->mailmap)\n> +\t\tclear_mailmap(revs->mailmap);\n> +\tFREE_AND_NULL(revs->mailmap);\n> +\tclear_rev_cmdline(revs);\n> +}\n> +\n> static void add_pending_commit_list(struct rev_info *revs,\n> \t\t\t\t struct commit_list *commit_list,\n> \t\t\t\t unsigned int flags)\n> diff --git a/revision.h b/revision.h\n> index 0c65a760ee..f695c41cee 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -358,6 +358,11 @@ void repo_init_revisions(struct repository *r,\n> \t\t\t struct rev_info *revs,\n> \t\t\t const char *prefix);\n> \n> +/*\n> + * Free all structures dynamically allocated for the provided rev_info\n> + */\n> +void repo_clear_revisions(struct rev_info *revs);\n> +\n> /**\n> * Parse revision information, filling in the `rev_info` structure, and\n> * removing the used arguments from the argument list. Returns the number\n\n"},{"id":"436386","messageId":"YUes7yxKHKW7cXcl@carlos-mbp.lan","threadId":"56531","inReplyTo":"87o88obkb1.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-19T21:34:39Z","receivedAt":"2021-09-19T21:34:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Sep 19, 2021 at 06:13:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sat, Sep 18 2021, Carlo Marcelo Arenas Belón wrote:\n> \n> > Note that the cleaning of the \"name\" in the cmdline item throws a warning\n> > as shown below which I intentionally didn't fix, as it would seem that\n> > either the use of const there or the need to strdup is wrong. So hope\n> > someone that knows this code better could chime in.\n> \n> It should just be a \"char *\", I got that wrong in my version posted in\n> the side-thread[1] & mentioned in the side-reply[2].\n\nI was instead leaning towards keeping it as a \"const char *\" and removing\nthe strdup as shown in the patch below (obviously the last hunk not relevant\nto your series).\n\nThis object doesn't hold or even manipulate, the objects it contains, so\nit might be also a cleaner API to ensure it only keeps references and\ndoesn't own any in the more CS sense (note I am not a CS guy, so maybe I\nget the concept here wrong).\n\nIronically the original patch that added the strdup was because of leak\nrelated work, but I think that in this case might had gotten it backwards.\n\nEven if we start holding pointers to names that are not static, I would\nexpect whoever created those buffers to own the data anyway.\n\nCarlo\n\nCC Michael for advise as the original author\n------ >8 ------\nSubject: [PATCH] revision: remove dup() of name in add_rev_cmdline()\n\ndf835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n2013-05-25) adds it, probably introducing a leak.\n\nAll names we will ever get will either come from the commandline\nor be pointers to a static buffer in hex.c, so it is safe not to\nxstrdup and clean them up (just like the struct object *item).\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n revision.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex ce62192dd8..b20bc58ccd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1468,7 +1468,6 @@ static int limit_list(struct rev_info *revs)\n \n /*\n * Add an entry to refs->cmdline with the specified information.\n- * *name is copied.\n */\n static void add_rev_cmdline(struct rev_info *revs,\n \t\t\t struct object *item,\n@@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n \n \tALLOC_GROW(info->rev, nr + 1, info->alloc);\n \tinfo->rev[nr].item = item;\n-\tinfo->rev[nr].name = xstrdup(name);\n+\tinfo->rev[nr].name = name;\n \tinfo->rev[nr].whence = whence;\n \tinfo->rev[nr].flags = flags;\n \tinfo->nr++;\n@@ -1490,10 +1489,6 @@ static void add_rev_cmdline(struct rev_info *revs,\n static void clear_rev_cmdline(struct rev_info *revs)\n {\n \tstruct rev_cmdline_info *info = &revs->cmdline;\n-\tsize_t i, nr = info->nr;\n-\n-\tfor (i = 0; i < nr; i++)\n-\t\tfree(info->rev[i].name);\n \n \tFREE_AND_NULL(info->rev);\n \tinfo->nr = info->alloc = 0;\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436395","messageId":"CAPig+cT-ajKsoj19ChPnkNByf-6P-vX=SG0NmgYt8CXyNH8y-w@mail.gmail.com","threadId":"56531","inReplyTo":"YUes7yxKHKW7cXcl@carlos-mbp.lan","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-20T06:06:01Z","receivedAt":"2021-09-20T06:06:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón\n wrote:\n> Subject: [PATCH] revision: remove dup() of name in add_rev_cmdline()\n>\n> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n> 2013-05-25) adds it, probably introducing a leak.\n>\n> All names we will ever get will either come from the commandline\n> or be pointers to a static buffer in hex.c, so it is safe not to\n> xstrdup and clean them up (just like the struct object *item).\n\nI haven't been following this thread closely, but the mention of the\nstatic buffer in hex.c invalidates the premise of this patch, as far\nas I can tell. The \"static buffer\" is actually a ring of four buffers\nwhich oid_to_hex() uses, one after another, into which it formats an\nOID as hex. This allows a caller to format up to -- and only up to --\nfour OIDs without worrying about allocating its own memory for the hex\nresult. Beyond four, the caller can't use oid_to_hex() without doing\nsome sort of memory management itself, whether that be duplicating the\nresult of oid_to_hex() or by allocating its own buffers and calling\noid_to_hex_r() instead.\n\nIn this particular case, one of the callers of add_rev_cmdline() is\nadd_rev_cmdline_list(), which does this:\n\n while (commit_list) {\n ...\n add_rev_cmdline(..., oid_to_hex(...), ...);\n ...\n }\n\nwhich may call add_rev_cmdline() any number of times, quite possibly\nmore than four.\n\nTherefore (if I'm reading this correctly), it is absolutely correct\nfor add_rev_cmdline() to be duplicating that string to ensure that the\nhexified OID value remains valid, and incorrect for this patch to be\nremoving the call to xstrdup().\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> diff --git a/revision.c b/revision.c\n> @@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n> info->rev[nr].item = item;\n> - info->rev[nr].name = xstrdup(name);\n> + info->rev[nr].name = name;\n> info->rev[nr].whence = whence;\n> @@ -1490,10 +1489,6 @@ static void add_rev_cmdline(struct rev_info *revs,\n> static void clear_rev_cmdline(struct rev_info *revs)\n> {\n> struct rev_cmdline_info *info = &revs->cmdline;\n> - size_t i, nr = info->nr;\n> -\n> - for (i = 0; i < nr; i++)\n> - free(info->rev[i].name);\n>\n> FREE_AND_NULL(info->rev);\n"},{"id":"436446","messageId":"xmqq4kafcesa.fsf@gitster.g","threadId":"56531","inReplyTo":"pull.1092.git.git.1631972978.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Squash leaks in t0000","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T17:55:49Z","receivedAt":"2021-09-20T17:58:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrzej Hunt via GitGitGadget\" writes:\n\n> Carlo points out that t0000 currently doesn't pass with leak-checking\n> enabled in:\n> https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71\n>\n> Here's a series that I've sat on for a while, which adds some UNLEAK's to\n> \"fix\" this situation - see the individual patches for a justification of why\n> an UNLEAK seems appropriate.\n\nIt seems that discussion on 1/2 seemed to be heading in an\nimprovement but has petered out? \n\nI think the simplest fix in these two patches are worth taking, even\nif we plan to further improve either by refining the granularity of\nUNLEAK application or by introducing repo_clear_revisions() as Carlo\nmentions (which is a preferred way to do this if we can manage it),\non top.\n\nThanks.\n"},{"id":"436496","messageId":"YUj/gFRh6pwrZalY@carlos-mbp.lan","threadId":"56531","inReplyTo":"CAPig+cT-ajKsoj19ChPnkNByf-6P-vX=SG0NmgYt8CXyNH8y-w@mail.gmail.com","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-20T21:39:12Z","receivedAt":"2021-09-21T02:20:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Sep 20, 2021 at 02:06:01AM -0400, Eric Sunshine wrote:\n> On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón\n> wrote:\n> > Subject: [PATCH] revision: remove dup() of name in add_rev_cmdline()\n> >\n> > df835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n> > 2013-05-25) adds it, probably introducing a leak.\n> >\n> > All names we will ever get will either come from the commandline\n> > or be pointers to a static buffer in hex.c, so it is safe not to\n> > xstrdup and clean them up (just like the struct object *item).\n> \n> I haven't been following this thread closely, but the mention of the\n> static buffer in hex.c invalidates the premise of this patch, as far\n> as I can tell. The \"static buffer\" is actually a ring of four buffers\n> which oid_to_hex() uses, one after another, into which it formats an\n> OID as hex. This allows a caller to format up to -- and only up to --\n> four OIDs without worrying about allocating its own memory for the hex\n> result. Beyond four, the caller can't use oid_to_hex() without doing\n> some sort of memory management itself, whether that be duplicating the\n> result of oid_to_hex() or by allocating its own buffers and calling\n> oid_to_hex_r() instead.\n\nThanks; this then explains why as I was suspecting add_rev_cmdline_list()\nwas indeed buggy, and might had even triggered the workaround of doing the\nstrdup.\n\n> Therefore (if I'm reading this correctly), it is absolutely correct\n> for add_rev_cmdline() to be duplicating that string to ensure that the\n> hexified OID value remains valid, and incorrect for this patch to be\n> removing the call to xstrdup().\n\nIndeed, but the values that are being strdup were never used anyway, so\nI suspect the original code might had just put it as a logical default.\n\nWe might do better instead as shown in the following patch (again, second\nhunk not relevant for the current code); I suspect if we were to land this,\nthe last hunks probably should be done first in an independent patch, as\nwell.\n\nCarlo\n-------- >8 --------\nSubject: [PATCH] revision: remove xstrdup() of name in add_rev_cmdline()\n\na765499a08 (revision.c: treat A...B merge bases as if manually\nspecified, 2013-05-13) adds calls to this function in a loop,\nabusing oid_to_hex (at that time called sha1_to_hex).\n\ndf835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n2013-05-25) adds the strdup, introducing a leak.\n\nAll names we will ever get should come from the commandline or be\nconstant values, so it is safe not to xstrdup and clean them up.\n\nJust like the struct object *item, that is referenced in the same\nstruct, name isn't owned or managed so correct both issues by making\nsure all entries are indeed constant or valid global pointers (from\nthe real command line) and remove the leak.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n revision.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex ce62192dd8..829af28658 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1468,7 +1468,6 @@ static int limit_list(struct rev_info *revs)\n \n /*\n * Add an entry to refs->cmdline with the specified information.\n- * *name is copied.\n */\n static void add_rev_cmdline(struct rev_info *revs,\n \t\t\t struct object *item,\n@@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n \n \tALLOC_GROW(info->rev, nr + 1, info->alloc);\n \tinfo->rev[nr].item = item;\n-\tinfo->rev[nr].name = xstrdup(name);\n+\tinfo->rev[nr].name = name;\n \tinfo->rev[nr].whence = whence;\n \tinfo->rev[nr].flags = flags;\n \tinfo->nr++;\n@@ -1490,10 +1489,6 @@ static void add_rev_cmdline(struct rev_info *revs,\n static void clear_rev_cmdline(struct rev_info *revs)\n {\n \tstruct rev_cmdline_info *info = &revs->cmdline;\n-\tsize_t i, nr = info->nr;\n-\n-\tfor (i = 0; i < nr; i++)\n-\t\tfree(info->rev[i].name);\n \n \tFREE_AND_NULL(info->rev);\n \tinfo->nr = info->alloc = 0;\n@@ -1504,10 +1499,10 @@ static void add_rev_cmdline_list(struct rev_info *revs,\n \t\t\t\t int whence,\n \t\t\t\t unsigned flags)\n {\n+\tstatic const char *synthetic = \".synthetic\";\n \twhile (commit_list) {\n \t\tstruct object *object = &commit_list->item->object;\n-\t\tadd_rev_cmdline(revs, object, oid_to_hex(&object->oid),\n-\t\t\t\twhence, flags);\n+\t\tadd_rev_cmdline(revs, object, synthetic, whence, flags);\n \t\tcommit_list = commit_list->next;\n \t}\n }\n@@ -1753,7 +1748,7 @@ struct add_alternate_refs_data {\n static void add_one_alternate_ref(const struct object_id *oid,\n \t\t\t\t void *vdata)\n {\n-\tconst char *name = \".alternate\";\n+\tstatic const char *name = \".alternate\";\n \tstruct add_alternate_refs_data *data = vdata;\n \tstruct object *obj;\n \n@@ -1940,7 +1935,7 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \t\t\t struct object_context *a_oc,\n \t\t\t struct object_context *b_oc)\n {\n-\tconst char *a_name, *b_name;\n+\tstatic const char *a_name, *b_name;\n \tstruct object_id a_oid, b_oid;\n \tstruct object *a_obj, *b_obj;\n \tunsigned int a_flags, b_flags;\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436552","messageId":"YUlMzJ6WNPxefYlm@coredump.intra.peff.net","threadId":"56531","inReplyTo":"YUj/gFRh6pwrZalY@carlos-mbp.lan","subject":"Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:09:00Z","receivedAt":"2021-09-21T03:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 02:39:12PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> > Therefore (if I'm reading this correctly), it is absolutely correct\n> > for add_rev_cmdline() to be duplicating that string to ensure that the\n> > hexified OID value remains valid, and incorrect for this patch to be\n> > removing the call to xstrdup().\n> \n> Indeed, but the values that are being strdup were never used anyway, so\n> I suspect the original code might had just put it as a logical default.\n\nWe do look at them in a few cases, like \"fast-export\", but only if they\nare not marked UNINTERESTING. And add_rev_cmdline_list(), the variant\nthat writes the hex values, only ever gets called with the UNINTERESTING\nflag.\n\nSo I think you're right that these ones would never be seen. I did\nwonder if we'd ever show them with \"log --source\" or similar, but that\npulls the name from object_array_entry, I think.\n\n> -------- >8 --------\n> Subject: [PATCH] revision: remove xstrdup() of name in add_rev_cmdline()\n> \n> a765499a08 (revision.c: treat A...B merge bases as if manually\n> specified, 2013-05-13) adds calls to this function in a loop,\n> abusing oid_to_hex (at that time called sha1_to_hex).\n> \n> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,\n> 2013-05-25) adds the strdup, introducing a leak.\n> \n> All names we will ever get should come from the commandline or be\n> constant values, so it is safe not to xstrdup and clean them up.\n\nThis last paragraph is questionable, I think. We feed the argv from\nsetup_revisions() here, but that is not always coming from the actual\ncommand line. Most cases seem to finish with the traversal before what\nthey've passed in goes out of scope, but not all. The call in\nbisect_rev_setup() intentionally leaks the strvec (even though it\ndoesn't need to do so with the current code). The one in\ncmd__fast_rebase() does clear its strvec after setup_revisions() but\nbefore the actual traversal. I wouldn't be surprised if your patch\ntriggered memory problems there.\n\n> @@ -1504,10 +1499,10 @@ static void add_rev_cmdline_list(struct rev_info *revs,\n> \t\t\t\t int whence,\n> \t\t\t\t unsigned flags)\n> {\n> +\tstatic const char *synthetic = \".synthetic\";\n\nI don't think there's any point in making this static. It's not an\narray, but rather a pointer to a string literal. That string literal\nwill remain valid regardless (the standard does not guarantee we get the\n_same_ string literal every time, but that doesn't matter for our\npurposes. And in practice it will be the same one).\n\n> @@ -1753,7 +1748,7 @@ struct add_alternate_refs_data {\n> static void add_one_alternate_ref(const struct object_id *oid,\n> \t\t\t\t void *vdata)\n> {\n> -\tconst char *name = \".alternate\";\n> +\tstatic const char *name = \".alternate\";\n> \tstruct add_alternate_refs_data *data = vdata;\n> \tstruct object *obj;\n\nDitto here.\n\n> @@ -1940,7 +1935,7 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n> \t\t\t struct object_context *a_oc,\n> \t\t\t struct object_context *b_oc)\n> {\n> -\tconst char *a_name, *b_name;\n> +\tstatic const char *a_name, *b_name;\n> \tstruct object_id a_oid, b_oid;\n> \tstruct object *a_obj, *b_obj;\n> \tunsigned int a_flags, b_flags;\n\nAnd I don't see how this changes anything at all. Our pointers will live\non, but the memory they point to is not affected.\n\n-Peff\n"},{"id":"436687","messageId":"87r1dh8r62.fsf@evledraar.gmail.com","threadId":"56531","inReplyTo":"xmqq4kafcesa.fsf@gitster.g","subject":"Re: [PATCH 0/2] Squash leaks in t0000","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T23:01:55Z","receivedAt":"2021-09-21T23:06:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Junio C Hamano wrote:\n\n> \"Andrzej Hunt via GitGitGadget\" writes:\n>\n>> Carlo points out that t0000 currently doesn't pass with leak-checking\n>> enabled in:\n>> https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71\n>>\n>> Here's a series that I've sat on for a while, which adds some UNLEAK's to\n>> \"fix\" this situation - see the individual patches for a justification of why\n>> an UNLEAK seems appropriate.\n>\n> It seems that discussion on 1/2 seemed to be heading in an\n> improvement but has petered out? \n>\n> I think the simplest fix in these two patches are worth taking, even\n> if we plan to further improve either by refining the granularity of\n> UNLEAK application or by introducing repo_clear_revisions() as Carlo\n> mentions (which is a preferred way to do this if we can manage it),\n> on top.\n\nI think per Andrzej's own [1] it's best to not pick up this series.\n\nI've got a lot of memory leak fixes queued up locally, I'm just waiting\non the SANITIZE=leak CI mode to land on master so I can add new tests to\nthe whitelist as I fix the memory leaks, that includes \"real\" fixes for\nthe ones Andrzej's added \"UNLEAK()\"'s for here.\n\nHence my meniton of this sort of thing being counter-productive[2],\ni.e. I'd need to monkeypatch revert this on top just to make sure I was\nstill finding leaks that are hidden by these new UNLEAK() (which hide\nsome really common ones).\n\n1. https://lore.kernel.org/git/05754f9c-cd58-30f5-e2d3-58b9221d2770@ahunt.org/\n2. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/\n"}]}