{"thread":{"id":"57420","subject":"[PATCH 1/2] commit: fix \"author_ident\" leak","startedAt":"2022-02-16T08:21:16Z","lastAt":"2022-05-18T16:31:48Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"448559","messageId":"patch-1.2-25202a1d07f-20220216T081844Z-avarab@gmail.com","threadId":"57420","inReplyTo":"cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com","subject":"[PATCH 1/2] commit: fix \"author_ident\" leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T08:21:05Z","receivedAt":"2022-02-16T08:21:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a leak in cmd_commit(), since 4c28e4ada03 (commit: die before\nasking to edit the log message, 2010-12-20) we have been freeing the\n\"author_ident\" \"struct strbuf\", but not in the case where\nprepare_to_commit() returns non-zero.\n\nThis fixes a leak demonstrated by e.g. \"t3505-cherry-pick-empty.sh\",\nbut unfortunately we cannot mark it or other affected tests as passing\nnow with \"TEST_PASSES_SANITIZE_LEAK=true\" as we'll need to fix many\nother memory leaks before doing so.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6b99ac276d8..696b3527adf 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1689,6 +1689,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n+\tint ret = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1723,8 +1724,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t running hooks, writing the trees, and interacting with the user. */\n \tif (!prepare_to_commit(index_file, prefix,\n \t\t\t current_head, &s, &author_ident)) {\n+\t\tret = 1;\n \t\trollback_index_files();\n-\t\treturn 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Determine parents */\n@@ -1822,7 +1824,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\n \t}\n-\tstrbuf_release(&author_ident);\n \tfree_commit_extra_headers(extra);\n \n \tif (update_head_with_reflog(current_head, &oid, reflog_msg, &sb,\n@@ -1863,7 +1864,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tapply_autostash(git_path_merge_autostash(the_repository));\n \n+cleanup:\n+\tstrbuf_release(&author_ident);\n \tUNLEAK(err);\n \tUNLEAK(sb);\n-\treturn 0;\n+\treturn ret;\n }\n-- \n2.35.1.1028.g2d2d4be19de\n\n"},{"id":"448560","messageId":"cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com","threadId":"57420","inReplyTo":null,"subject":"[PATCH 0/2] commit: trivial leak fix, add 2 tests to linux-leaks CI","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T08:21:04Z","receivedAt":"2022-02-16T08:21:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This trivial series fixes a leak in \"git commit\", removes two UNLEAK()\nin favor of simply calling strbuf_release(), and marks two tests that\npreviously hit that UNLEAK() being run in the linux-leaks CI job.\n\nÆvar Arnfjörð Bjarmason (2):\n commit: fix \"author_ident\" leak\n commit: use strbuf_release() instead of UNLEAK()\n\n builtin/commit.c | 13 ++++++++-----\n t/t2203-add-intent.sh | 1 +\n t/t7011-skip-worktree-reading.sh | 1 +\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\n-- \n2.35.1.1028.g2d2d4be19de\n\n"},{"id":"448561","messageId":"patch-2.2-e3e78c9e40b-20220216T081844Z-avarab@gmail.com","threadId":"57420","inReplyTo":"cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com","subject":"[PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T08:21:06Z","receivedAt":"2022-02-16T08:21:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for\nreducing leak false positives, 2017-09-08) to release the memory using\nstrbuf_release() instead.\n\nThe tests being marked as passing with\n\"TEST_PASSES_SANITIZE_LEAK=true\" already passed before due to the\nUNLEAK(), but now they really don't leak memory, so let's mark them as\nsuch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit.c | 4 ++--\n t/t2203-add-intent.sh | 1 +\n t/t7011-skip-worktree-reading.sh | 1 +\n 3 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 696b3527adf..c38ae2b7656 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1866,7 +1866,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n cleanup:\n \tstrbuf_release(&author_ident);\n-\tUNLEAK(err);\n-\tUNLEAK(sb);\n+\tstrbuf_release(&err);\n+\tstrbuf_release(&sb);\n \treturn ret;\n }\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex db7ca559986..ebf58db2d18 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -2,6 +2,7 @@\n \n test_description='Intent to add'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'intent to add' '\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex 1761a2b1b99..4adac5acd57 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -5,6 +5,7 @@\n \n test_description='skip-worktree bit test'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >expect.full < writes:\n\n> Fix a leak in cmd_commit(), since 4c28e4ada03 (commit: die before\n> asking to edit the log message, 2010-12-20) we have been freeing the\n> \"author_ident\" \"struct strbuf\", but not in the case where\n\nIt took me an actual reading of the code to see that the above\nrefers to one and the same thing (i.e. \"author_ident variable of\ntype struct strbuf\"). I think it should be sufficient and clearer\nto just mention \"author_ident\" here.\n\n> prepare_to_commit() returns non-zero.\n\nGood eyes. \n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 6b99ac276d8..696b3527adf 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1689,6 +1689,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tstruct commit *current_head = NULL;\n> \tstruct commit_extra_header *extra = NULL;\n> \tstruct strbuf err = STRBUF_INIT;\n> +\tint ret = 0;\n> \n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n> @@ -1723,8 +1724,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t running hooks, writing the trees, and interacting with the user. */\n> \tif (!prepare_to_commit(index_file, prefix,\n> \t\t\t current_head, &s, &author_ident)) {\n> +\t\tret = 1;\n> \t\trollback_index_files();\n> -\t\treturn 1;\n> +\t\tgoto cleanup;\n> \t}\n> \n> \t/* Determine parents */\n> @@ -1822,7 +1824,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\trollback_index_files();\n> \t\tdie(_(\"failed to write commit object\"));\n> \t}\n> -\tstrbuf_release(&author_ident);\n> \tfree_commit_extra_headers(extra);\n\nHmph, if we hit one of the two die() after this point before we\nreach the \"cleanup\" label, author_ident will be left on the stack,\nwhich we may want to UNLEAK()?\n\nI am wondering if prepare_to_commit(), which is the one that is\nresponsible for allocating and using the information in the strbuf,\nshould be the one who is responsible for cleaning it when it failed\nto do its thing, but I do not think it is a good idea, because the\ncaller MUST release it in the success case anyway. So dealing with\nthe releasing here does make sense.\n\nBy jumping to the cleanup label, we not just release author_ident,\nbut we start unleak'ing err and sb as well, which shouldn't be a\nproblem, hopefully.\n\nWill queue.\n\nThanks.\n\n> \tif (update_head_with_reflog(current_head, &oid, reflog_msg, &sb,\n> @@ -1863,7 +1864,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \n> \tapply_autostash(git_path_merge_autostash(the_repository));\n> \n> +cleanup:\n> +\tstrbuf_release(&author_ident);\n> \tUNLEAK(err);\n> \tUNLEAK(sb);\n> -\treturn 0;\n> +\treturn ret;\n> }\n"},{"id":"448625","messageId":"xmqqa6equ1rq.fsf@gitster.g","threadId":"57420","inReplyTo":"patch-2.2-e3e78c9e40b-20220216T081844Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T18:03:53Z","receivedAt":"2022-02-16T18:03:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for\n> reducing leak false positives, 2017-09-08) to release the memory using\n> strbuf_release() instead.\n>\n> The tests being marked as passing with\n> \"TEST_PASSES_SANITIZE_LEAK=true\" already passed before due to the\n> UNLEAK(), but now they really don't leak memory, so let's mark them as\n> such.\n\nThat smells like a brave move.\n\nSpecifically, the cited commit turned an existing strbuf_release()\non &err into UNLEAK(). If that and the other strbuf (sb) were so\neasily releasable, why didn't we do so back then already?\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit.c | 4 ++--\n> t/t2203-add-intent.sh | 1 +\n> t/t7011-skip-worktree-reading.sh | 1 +\n> 3 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 696b3527adf..c38ae2b7656 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1866,7 +1866,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \n> cleanup:\n> \tstrbuf_release(&author_ident);\n> -\tUNLEAK(err);\n> -\tUNLEAK(sb);\n> +\tstrbuf_release(&err);\n> +\tstrbuf_release(&sb);\n> \treturn ret;\n> }\n"},{"id":"448626","messageId":"xmqqtucyslz9.fsf@gitster.g","threadId":"57420","inReplyTo":"xmqqa6equ1rq.fsf@gitster.g","subject":"Re: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T18:30:18Z","receivedAt":"2022-02-16T18:30:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for\n>> reducing leak false positives, 2017-09-08) to release the memory using\n>> strbuf_release() instead.\n>>\n>> The tests being marked as passing with\n>> \"TEST_PASSES_SANITIZE_LEAK=true\" already passed before due to the\n>> UNLEAK(), but now they really don't leak memory, so let's mark them as\n>> such.\n>\n> That smells like a brave move.\n>\n> Specifically, the cited commit turned an existing strbuf_release()\n> on &err into UNLEAK(). If that and the other strbuf (sb) were so\n> easily releasable, why didn't we do so back then already?\n\nI suspect that the answer to the above question is because these\nallocations are in the top-level cmd_commit() function, which is\nnever called recursively or repeatedly as a subroutine. The only\nsignificant thing that happens after we return from it is to exit.\n\nIn such a code path, marking a variable as UNLEAK() is a better\nthing to do than calling strbuf_release(). Both will work as a way\nto squelch sanitizers from reporting a leak that does not matter,\nbut calling strbuf_release() means we'd spend extra cycles to return\npieces of memory to the pool, even though we know that the pool\nitself will be cleaned immediately later at exit.\n\nWe already have UNLEAK to tell sanitizers not to distract us from\nspotting and plugging real leaks by reporting these apparent leaks\nthat do not matter. It is of somewhat dubious value to do a \"we\ncare too much about pleasing sanitizer and spend extra cycles at\nruntime while real users are doing real work\" change.\n"},{"id":"448764","messageId":"220218.861r00ib86.gmgdl@evledraar.gmail.com","threadId":"57420","inReplyTo":"xmqqtucyslz9.fsf@gitster.g","subject":"Whether to keep using UNLEAK() in built-ins (was: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK())","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T12:35:12Z","receivedAt":"2022-02-18T12:57:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Junio C Hamano wrote:\n\n[CC-ing some people using/interested in UNLEAK()]\n\n> Junio C Hamano writes:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for\n>>> reducing leak false positives, 2017-09-08) to release the memory using\n>>> strbuf_release() instead.\n>>>\n>>> The tests being marked as passing with\n>>> \"TEST_PASSES_SANITIZE_LEAK=true\" already passed before due to the\n>>> UNLEAK(), but now they really don't leak memory, so let's mark them as\n>>> such.\n>>\n>> That smells like a brave move.\n>>\n>> Specifically, the cited commit turned an existing strbuf_release()\n>> on &err into UNLEAK(). If that and the other strbuf (sb) were so\n>> easily releasable, why didn't we do so back then already?\n>\n> I suspect that the answer to the above question is because these\n> allocations are in the top-level cmd_commit() function, which is\n> never called recursively or repeatedly as a subroutine. The only\n> significant thing that happens after we return from it is to exit.\n>\n> In such a code path, marking a variable as UNLEAK() is a better\n> thing to do than calling strbuf_release(). Both will work as a way\n> to squelch sanitizers from reporting a leak that does not matter,\n> but calling strbuf_release() means we'd spend extra cycles to return\n> pieces of memory to the pool, even though we know that the pool\n> itself will be cleaned immediately later at exit.\n>\n> We already have UNLEAK to tell sanitizers not to distract us from\n> spotting and plugging real leaks by reporting these apparent leaks\n> that do not matter. It is of somewhat dubious value to do a \"we\n> care too much about pleasing sanitizer and spend extra cycles at\n> runtime while real users are doing real work\" change.\n\nWe've had several discussions about the utility of UNLEAK() as I've been\nsubmitting these patches, and I thought that if we weren't 100% on the\nsame page it was at least clear what I was going for here.\n\nPer https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ the\nreal goal I have in mind here is to use the built-ins as a stand-in for\ntesting whether the underlying APIs are leak-free.\n\nBecause of that having to reason about UNLEAK() doing the right thing or\nnot is just unneeded distraction. Yes for a \"struct strbuf\" it won't\nmatter, but most of what we UNLEAK() is more complex stuff like \"struct\nrev_info\". We won't really make headway making revision.c not leak\nmemory without using \"git log\" et al as the test subjects for whether\nthat API leaks.\n\nWe are also freeing most of this already even for built-ins, e.g. see\n(not all of these are applicable obviously, but per the numbers enough\nare to make the point):\n\n $ git grep '\\bstrbuf_release\\(' -- builtin | wc -l\n 456\n $ git grep '\\bUNLEAK\\(' -- builtin | wc -l\n 29\n\nSo one goal I've got with these patches is to eventually get rid of\nUNLEAK() entirely. We only use it in these few instances:\n\n $ git grep '\\bUNLEAK\\(' -- '*.c' | wc -l\n 31\n\nPer the above we'd want to convert any that deal with the complex\nstructures that are the big source of leaks to doing real releases for\ntesting the APIs. That'll leave only a handful of remaining legitimate\nuses, which I don't think are worth keeping some UNLEAK() API around\nfor, v.s. just freeing them.\n\nI think we've also somewhat been talking past each other in past\nexchanges (including me with Jeff King) about what you call \"real\nleaks\".\n\nWhen I'm referring to memory leaks I'm talking about them in the more\ninclusive sense explained in this valgrind documentation:\nhttps://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks;\nBasically \"a malloc() not followed by a free() is a leak\".\n\nBut you and Jeff King have (I think) commonly used that term to exclude\nwhat's called \"still reachable\" in that table. Those *are* memory leaks,\nbut as explained in that table just ones that arguably don't matter. Or\nat least ones most people tracing leaks don't want reported by default.\n\nUNLEAK() is just a mechanism for moving a memory leak from another\ncategory into \"still reachable\". That will make LSAN blind to it, and\nvalgrind in many cases due to it being a heap tracer. But it & other\nmemory leaking tools *will* report even on those if given the right\noptions.\n\nSo I've found it useful to get rid of UNLEAK() in some cases for those,\nbecause even if LSAN runs clean it's useful to run valgrind in its more\npedantic tracing modes to see even the \"still unreachable\", and if we\nshould care about it or not.\n\nSome of the leaks in those category *are* \"real leaks\". I.e. you can\nhave an ever-growing structure with global reach that's always \"still\nreachable\", but when using the API would eventually OOM you.\n\nSo while it's a useful heuristic for spotting \"will be cleaned up at\nexit anyway\" you can't rely on it, so I've been looking at them anyway.\n\nSo being able to just free() them so I can permanently ignore them as I\nfix more leaks would be useful to me, so I hope you'll agree on just\ntalking this as-is, thanks! :)\n"},{"id":"448776","messageId":"xmqqwnhs11he.fsf@gitster.g","threadId":"57420","inReplyTo":"220218.861r00ib86.gmgdl@evledraar.gmail.com","subject":"Re: Whether to keep using UNLEAK() in built-ins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-18T18:19:41Z","receivedAt":"2022-02-18T18:19:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> Specifically, the cited commit turned an existing strbuf_release()\n>>> on &err into UNLEAK(). If that and the other strbuf (sb) were so\n>>> easily releasable, why didn't we do so back then already?\n>>\n>> I suspect that the answer to the above question is because these\n>> allocations are in the top-level cmd_commit() function, which is\n>> never called recursively or repeatedly as a subroutine. The only\n>> significant thing that happens after we return from it is to exit.\n>>\n>> In such a code path, marking a variable as UNLEAK() is a better\n>> thing to do than calling strbuf_release(). Both will work as a way\n>> to squelch sanitizers from reporting a leak that does not matter,\n>> but calling strbuf_release() means we'd spend extra cycles to return\n>> pieces of memory to the pool, even though we know that the pool\n>> itself will be cleaned immediately later at exit.\n>>\n>> We already have UNLEAK to tell sanitizers not to distract us from\n>> spotting and plugging real leaks by reporting these apparent leaks\n>> that do not matter. It is of somewhat dubious value to do a \"we\n>> care too much about pleasing sanitizer and spend extra cycles at\n>> runtime while real users are doing real work\" change.\n\n> Per https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ the\n> real goal I have in mind here is to use the built-ins as a stand-in for\n> testing whether the underlying APIs are leak-free.\n>\n> Because of that having to reason about UNLEAK() doing the right thing or\n> not is just unneeded distraction. Yes for a \"struct strbuf\" it won't\n> matter, but most of what we UNLEAK() is more complex stuff like \"struct\n> rev_info\". We won't really make headway making revision.c not leak\n> memory without using \"git log\" et al as the test subjects for whether\n> that API leaks.\n\nI have to say that you have a wrong goal and wrong priority. The\nnumber of UNLEAK in cmd_foo() functions is not even a poor\napproximation of our progress.\n\nImagine that a subsystem that are repeatedly set-up and used during\na single program invocation was leaky. Let's say a program calls\ndiff_setup() to prepare the diff_options structure, feeds two things\nto compare, and calls diff_flush() to show the comparison result,\nbut let's assume this sequence leaks some resources.\n\nNow cmd_diff() may be such a program that does a setup, feeds two\ntrees, calls diff_flush() and exits. If we didn't do anything to\nit, diff_options may \"leak\". Marking it with UNLEAK may be a good\nmeasure, if we want to keep the leak checker from reporting a leak\nthat does not matter in practice so that we can concentrate on\nplugging real leaks that matter.\n\nBut consider cmd_log(), running something like \"git log -p\". It\niterates over commits, does the \nrepeatedly for each commit it encounters during the walk on the same\ndiff_options structure. Now, the leak in the code path does matter.\nIf diff_flush() is left leaky, it will show up in the leak checker's\noutput, and that is reporting real leaks that matter. The thing is,\nthe sequence whose leak matters is not in\ncmd_log(); it is much deeper in the revision walking machinery. We\ndo not want to paper over such leaks with UNLEAK.\n\nSee the difference? The number of UNLEAK OUTside built-ins does\nmatter. We do not want to have UNLEAK there to hide leaks in\npossible callees that may be called arbitrary number of times in\narbitrary order. Compared to that, UNLEAK in cmd_foo() to mark an\non-stack variable that was used only once without even a recursion\nis purely to squelch the leak checker from reporting leaks that does\nnot matter. For simple things like strbuf on stack of the top-level\ncmd_foo() functions, we could even leave strbuf_release() not called\nand leave the resource reclamation to _exit(2). That would cause\nthe leak checkers to report them and distract us, and UNLEAK is a\nbetter way to squelch the distraction without wasting extra cycles\nat runtime to actually free them.\n\nSo, don't look at \"built-ins as a stand-in\". It is a wrong\ndirection to go in to let the \"leak checker\" tail wag the dog.\n\nThanks.\n"},{"id":"448797","messageId":"220218.86fsoggdou.gmgdl@evledraar.gmail.com","threadId":"57420","inReplyTo":"xmqqwnhs11he.fsf@gitster.g","subject":"Re: Whether to keep using UNLEAK() in built-ins","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T19:31:23Z","receivedAt":"2022-02-18T19:47:07Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>>> Specifically, the cited commit turned an existing strbuf_release()\n>>>> on &err into UNLEAK(). If that and the other strbuf (sb) were so\n>>>> easily releasable, why didn't we do so back then already?\n>>>\n>>> I suspect that the answer to the above question is because these\n>>> allocations are in the top-level cmd_commit() function, which is\n>>> never called recursively or repeatedly as a subroutine. The only\n>>> significant thing that happens after we return from it is to exit.\n>>>\n>>> In such a code path, marking a variable as UNLEAK() is a better\n>>> thing to do than calling strbuf_release(). Both will work as a way\n>>> to squelch sanitizers from reporting a leak that does not matter,\n>>> but calling strbuf_release() means we'd spend extra cycles to return\n>>> pieces of memory to the pool, even though we know that the pool\n>>> itself will be cleaned immediately later at exit.\n>>>\n>>> We already have UNLEAK to tell sanitizers not to distract us from\n>>> spotting and plugging real leaks by reporting these apparent leaks\n>>> that do not matter. It is of somewhat dubious value to do a \"we\n>>> care too much about pleasing sanitizer and spend extra cycles at\n>>> runtime while real users are doing real work\" change.\n>\n>> Per https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ the\n>> real goal I have in mind here is to use the built-ins as a stand-in for\n>> testing whether the underlying APIs are leak-free.\n>>\n>> Because of that having to reason about UNLEAK() doing the right thing or\n>> not is just unneeded distraction. Yes for a \"struct strbuf\" it won't\n>> matter, but most of what we UNLEAK() is more complex stuff like \"struct\n>> rev_info\". We won't really make headway making revision.c not leak\n>> memory without using \"git log\" et al as the test subjects for whether\n>> that API leaks.\n>\n> I have to say that you have a wrong goal and wrong priority. The\n> number of UNLEAK in cmd_foo() functions is not even a poor\n> approximation of our progress.\n\nTo clarify I'm not saying it's an approximation of how close we are to\ngetting rid of memory leaks. I think I'll know better than anyone what\nthe current state of shoveling shit uphill that is.\n\nI was suggesting that these's a trivial number of these, and mostly we\nstrbuf_release() already, and that on modern libc's free() is pretty\nmuch free anyway, so worrying about using UNLEAK() v.s. just freeing\nbefore exit is some combination of a premature optimization and\nneedlessly retaining a special-case for the built-ins.\n\n> Imagine that a subsystem that are repeatedly set-up and used during\n> a single program invocation was leaky. Let's say a program calls\n> diff_setup() to prepare the diff_options structure, feeds two things\n> to compare, and calls diff_flush() to show the comparison result,\n> but let's assume this sequence leaks some resources.\n>\n> Now cmd_diff() may be such a program that does a setup, feeds two\n> trees, calls diff_flush() and exits. If we didn't do anything to\n> it, diff_options may \"leak\". Marking it with UNLEAK may be a good\n> measure, if we want to keep the leak checker from reporting a leak\n> that does not matter in practice so that we can concentrate on\n> plugging real leaks that matter.\n>\n> But consider cmd_log(), running something like \"git log -p\". It\n> iterates over commits, does the \n> repeatedly for each commit it encounters during the walk on the same\n> diff_options structure. Now, the leak in the code path does matter.\n> If diff_flush() is left leaky, it will show up in the leak checker's\n> output, and that is reporting real leaks that matter. The thing is,\n> the sequence whose leak matters is not in\n> cmd_log(); it is much deeper in the revision walking machinery. We\n> do not want to paper over such leaks with UNLEAK.\n>\n> See the difference? The number of UNLEAK OUTside built-ins does\n> matter. We do not want to have UNLEAK there to hide leaks in\n> possible callees that may be called arbitrary number of times in\n> arbitrary order. Compared to that, UNLEAK in cmd_foo() to mark an\n> on-stack variable that was used only once without even a recursion\n> is purely to squelch the leak checker from reporting leaks that does\n> not matter. For simple things like strbuf on stack of the top-level\n> cmd_foo() functions, we could even leave strbuf_release() not called\n> and leave the resource reclamation to _exit(2). That would cause\n> the leak checkers to report them and distract us, and UNLEAK is a\n> better way to squelch the distraction without wasting extra cycles\n> at runtime to actually free them.\n>\n> So, don't look at \"built-ins as a stand-in\". It is a wrong\n> direction to go in to let the \"leak checker\" tail wag the dog.\n\nYes, I see the difference and I fully accept the point you're making.\n\nI just don't find it to be a useful distinction in practice, because for\ne.g. \"git log\" we do have certain uses of the API which are only\nperformed by the top-level part of a \"git log --whatever\".\n\nSo if we take the view that e.g. the \"struct rev_info rev\" should be\nUNLEAK()'d because \"we're about to exit anyway\" then as soon as we *do*\nuse that part of the API we'll run into the previously unfixed and\nhidden leak.\n\nWhich is why as noted in the linked-to-above\nhttps://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ I think\nit's useful to fix leaks even in built-ins, because they're useful\ncanaries for those APIs.\n\nNow, in this case does it really matter? No, it doesn't currently. But I\nthink the direction we should be heading in is turning more of these\ncmd_whatever() into properly functioning libraries, as e.g. John Cai's\njust-sent series to do that for \"reflog delete\" does:\nhttps://lore.kernel.org/git/pull.1218.git.git.1645209647.gitgitgadget@gmail.com/\n\nOnce we do that the UNLEAK() here will absolutely need to be changed to\na strbuf_release() or equivalent, since if you make two commits with\nthat new library we'll have leaked memory.\n\nSo I think it makes sense to just fix leaks everywhere if it's easy\nwithout worrying about the distinction, particularly since I haven't\nseen that \"wasting cycles\" concern matter in practice.\n\nIf it did I'd think adding a \"GIT_DESTRUCT_LEVEL\" as I suggested in the\nlinked E-Mail would make more sense, since e.g. you could also use that\nif you knew you were walking 100 commits & exiting, which can be useful\nto further reduce free() churn, and is how a similar global in the Perl\nAPI (PERL_DESTRUCT_LEVEL) works.\n"},{"id":"455234","messageId":"xmqqzgjmcqlg.fsf@gitster.g","threadId":"57420","inReplyTo":"cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com","subject":"[PATCH] commit: fix \"author_ident\" leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T22:51:07Z","receivedAt":"2022-05-12T22:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Since 4c28e4ada03 (commit: die before asking to edit the log\nmessage, 2010-12-20), we have been \"leaking\" the \"author_ident\" when\nprepare_to_commit() fails. Instead of returning from right there,\nintroduce an exit status variable and jump to the clean-up label\nat the end.\n\nInstead of explicitly releasing the resource with strbuf_release(),\nmark the variable with UNLEAK() at the end, together with two other\nvariables that are already marked as such. If this were in a\nutility function that is called number of times, but these are\ndifferent, we should explicitly release resources that grow\nproportionally to the size of the problem being solved, but\ncmd_commit() is like main() and there is no point in spending extra\ncycles to release individual pieces of resource at the end, just\nbefore process exit will clean everything for us for free anyway.\n\nThis fixes a leak demonstrated by e.g. \"t3505-cherry-pick-empty.sh\",\nbut unfortunately we cannot mark it or other affected tests as passing\nnow with \"TEST_PASSES_SANITIZE_LEAK=true\" as we'll need to fix many\nother memory leaks before doing so.\n\nIncidentally there are two tests that always passes the leak checker\nwith or without this change. Mark them as such.\n\nThis is based on an earlier patch by Ævar, but takes a different\napproach that is more maintainable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/commit.c | 9 ++++++---\n t/t2203-add-intent.sh | 1 +\n t/t7011-skip-worktree-reading.sh | 1 +\n 3 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b9ed0374e3..4e8b3d3251 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1688,6 +1688,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n+\tint ret = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1722,8 +1723,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t running hooks, writing the trees, and interacting with the user. */\n \tif (!prepare_to_commit(index_file, prefix,\n \t\t\t current_head, &s, &author_ident)) {\n+\t\tret = 1;\n \t\trollback_index_files();\n-\t\treturn 1;\n+\t\tgoto cleanup;\n \t}\n \n \t/* Determine parents */\n@@ -1821,7 +1823,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\n \t}\n-\tstrbuf_release(&author_ident);\n \tfree_commit_extra_headers(extra);\n \n \tif (update_head_with_reflog(current_head, &oid, reflog_msg, &sb,\n@@ -1862,7 +1863,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tapply_autostash(git_path_merge_autostash(the_repository));\n \n+cleanup:\n+\tUNLEAK(author_ident);\n \tUNLEAK(err);\n \tUNLEAK(sb);\n-\treturn 0;\n+\treturn ret;\n }\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex db7ca55998..ebf58db2d1 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -2,6 +2,7 @@\n \n test_description='Intent to add'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'intent to add' '\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex 1761a2b1b9..4adac5acd5 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -5,6 +5,7 @@\n \n test_description='skip-worktree bit test'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >expect.full < Since 4c28e4ada03 (commit: die before asking to edit the log\n> message, 2010-12-20), we have been \"leaking\" the \"author_ident\" when\n> prepare_to_commit() fails. Instead of returning from right there,\n> introduce an exit status variable and jump to the clean-up label\n> at the end.\n>\n> Instead of explicitly releasing the resource with strbuf_release(),\n> mark the variable with UNLEAK() at the end, together with two other\n> variables that are already marked as such. If this were in a\n> utility function that is called number of times, but these are\n> different, we should explicitly release resources that grow\n> proportionally to the size of the problem being solved, but\n> cmd_commit() is like main() and there is no point in spending extra\n> cycles to release individual pieces of resource at the end, just\n> before process exit will clean everything for us for free anyway.\n>\n> This fixes a leak demonstrated by e.g. \"t3505-cherry-pick-empty.sh\",\n> but unfortunately we cannot mark it or other affected tests as passing\n> now with \"TEST_PASSES_SANITIZE_LEAK=true\" as we'll need to fix many\n> other memory leaks before doing so.\n>\n> Incidentally there are two tests that always passes the leak checker\n> with or without this change. Mark them as such.\n>\n> This is based on an earlier patch by Ævar, but takes a different\n> approach that is more maintainable.\n\nWe've talked about UNLEAK() v.s. strbuf_release() elsewhere, so let's\nleave that aside. I know your preferences in that area.\n\nBut even accounting for that, I don't see what the \"more maintainable\"\nhere refers to. The approach I suggested would s/UNLEAK/strbuf_release/\nin the 4th hunk, but otherwise be equivalent.\n\nSurely both are about as easy to maintain. To the extent that there's\nany difference at all I'd think the strbuf_release() would pull ahead,\nas it's guaranteed to do the right thing with all of our memory analysis\ntooling (some of which will have a noop UNLEAK()).\n\nJust a small question, I see this is in \"next\" already, and I'm fine\nwith this change either way.\n"},{"id":"455404","messageId":"xmqqtu9mx0p7.fsf@gitster.g","threadId":"57420","inReplyTo":"220517.86fsl86z1s.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] commit: fix \"author_ident\" leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-18T16:30:44Z","receivedAt":"2022-05-18T16:31:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But even accounting for that, I don't see what the \"more maintainable\"\n> here refers to. The approach I suggested would s/UNLEAK/strbuf_release/\n> in the 4th hunk, but otherwise be equivalent.\n\nJudicious use of UNLEAK() has documentation value to tell readers\nwhich use of pointer variables need to be explicitly released, and\nwhich pointer variables can just implicitly released by going out of\nscope at the end. There are also a few other small added benefits\n(they do not have to be changed when the helper needed to do the\nreal release changes, and not doing an explicit real release on\nresources when there no need to is conceptually cleaner). But they\nare icing on the cake.\n\nSorry for a late reply---I go offline every other Tuesday and\nyesterday was such a Tuesday.\n"}]}