{"thread":{"id":"46675","subject":"[PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","startedAt":"2017-08-27T07:37:51Z","lastAt":"2017-10-11T06:00:44Z","messageCount":43,"participants":["Martin Ågren","Jeff King","Lars Schneider","Stefan Beller","Brandon Williams","Michael Haggerty","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"327256","messageId":"20170827073732.546-1-martin.agren@gmail.com","threadId":"46675","inReplyTo":null,"subject":"[PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-27T07:37:32Z","receivedAt":"2017-08-27T07:37:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\npacket_write_fmt_gently()\", 2016-10-16). As a result, for each call to\npacket_write_fmt_1, we allocate and leak a buffer.\n\nWe could keep the strbuf non-static and instead make sure we always\nrelease it before returning (but not before we die, so that we don't\ntouch errno). That would also prepare us for threaded use. But until\nthat needs to happen, let's just restore the static-ness so that we get\nback to a situation where we (eventually) do not continuosly keep\nallocating memory.\n\nSigned-off-by: Martin Ågren \n---\nI waffled between \"fixing the memory leak\" by releasing the buffer and\n\"fixing the static-ness\" as in this patch. I can see arguments both ways\nand don't have any strong opinion.\n\n pkt-line.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 7db911957..f364944b9 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -136,9 +136,10 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n static int packet_write_fmt_1(int fd, int gently,\n \t\t\t const char *fmt, va_list args)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n \tssize_t count;\n \n+\tstrbuf_reset(&buf);\n \tformat_packet(&buf, fmt, args);\n \tcount = write_in_full(fd, buf.buf, buf.len);\n \tif (count == buf.len)\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327259","messageId":"20170827154152.4n2ykna7e4l27fyn@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170827073732.546-1-martin.agren@gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-27T15:41:52Z","receivedAt":"2017-08-27T15:41:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 27, 2017 at 09:37:32AM +0200, Martin Ågren wrote:\n\n> The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\n> packet_write_fmt_gently()\", 2016-10-16). As a result, for each call to\n> packet_write_fmt_1, we allocate and leak a buffer.\n> \n> We could keep the strbuf non-static and instead make sure we always\n> release it before returning (but not before we die, so that we don't\n> touch errno). That would also prepare us for threaded use. But until\n> that needs to happen, let's just restore the static-ness so that we get\n> back to a situation where we (eventually) do not continuosly keep\n> allocating memory.\n\nOuch. So this means that git since v2.11 is basically leaking every\nnon-byte pack sent by upload-pack (so all of the ref advertisement and\nwant/have negotiation).\n\nI'm surprised nobody noticed the extra memory use, but I guess those\nthings aren't usually _too_ big.\n\n> Signed-off-by: Martin Ågren \n> ---\n> I waffled between \"fixing the memory leak\" by releasing the buffer and\n> \"fixing the static-ness\" as in this patch. I can see arguments both ways\n> and don't have any strong opinion.\n\nI think this is a good fix for now as it takes as back to the pre-bug\nstate. The only downside with the static buffer is that it's not\nreentrant. Since the function is just inherently writing out the result\nand then forgetting it, in a single thread there's no opportunity for a\nsub-function to try writing another packet. And I don't think we have\nany code paths that write packets from multiple threads. That may be\nsomething we do eventually, but we can deal with it then (and until\nthen, it's nice to avoid the malloc/free overhead).\n\n-Peff\n"},{"id":"327261","messageId":"9E4606AF-8814-42DE-8D3A-3A15C1B1723C@gmail.com","threadId":"46675","inReplyTo":"20170827073732.546-1-martin.agren@gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-27T18:21:54Z","receivedAt":"2017-08-27T18:22:00Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Aug 2017, at 09:37, Martin Ågren wrote:\n> \n> The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\n> packet_write_fmt_gently()\", 2016-10-16). As a result, for each call to\n> packet_write_fmt_1, we allocate and leak a buffer.\n\nOh :-( Thanks for detecting and fixing the leak.\n\nHow did you actually find it? Reading the code or did you use some\ntool?\n\n\n> We could keep the strbuf non-static and instead make sure we always\n> release it before returning (but not before we die, so that we don't\n> touch errno). That would also prepare us for threaded use. But until\n> that needs to happen, let's just restore the static-ness so that we get\n> back to a situation where we (eventually) do not continuosly keep\n> allocating memory.\n> \n> Signed-off-by: Martin Ågren \n> ---\n> I waffled between \"fixing the memory leak\" by releasing the buffer and\n> \"fixing the static-ness\" as in this patch. I can see arguments both ways\n> and don't have any strong opinion.\n> \n> pkt-line.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index 7db911957..f364944b9 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -136,9 +136,10 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n> static int packet_write_fmt_1(int fd, int gently,\n> \t\t\t const char *fmt, va_list args)\n> {\n> -\tstruct strbuf buf = STRBUF_INIT;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> \tssize_t count;\n> \n> +\tstrbuf_reset(&buf);\n> \tformat_packet(&buf, fmt, args);\n> \tcount = write_in_full(fd, buf.buf, buf.len);\n> \tif (count == buf.len)\n> -- \n> 2.14.1.151.g45c1275a3.dirty\n\nLooks good to me!\n\n- Lars"},{"id":"327262","messageId":"20170827182545.e7athkdqkjnxrpna@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170827154152.4n2ykna7e4l27fyn@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-27T18:25:45Z","receivedAt":"2017-08-27T18:25:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 27, 2017 at 11:41:52AM -0400, Jeff King wrote:\n\n> Ouch. So this means that git since v2.11 is basically leaking every\n> non-byte pack sent by upload-pack (so all of the ref advertisement and\n> want/have negotiation).\n\nDetermined experimentally that the answer is: yes.\n\nThe increase in ref advertisement is roughly linear with the size of\nyour packed-refs, so for most people this wasn't a big deal (and even if\nyou have insanely large packed-refs, it's \"only\" 2-3x the RAM;\npotentially a problem, but only if you're close to the tipping point\nalready).\n\n-Peff\n"},{"id":"327265","messageId":"CAN0heSraJFbbog7FKpAtmob9W6_5-AS1StZFVW6xUwMDWfMYgg@mail.gmail.com","threadId":"46675","inReplyTo":"9E4606AF-8814-42DE-8D3A-3A15C1B1723C@gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-27T19:09:15Z","receivedAt":"2017-08-27T19:09:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 27 August 2017 at 20:21, Lars Schneider wrote:\n>\n>> On 27 Aug 2017, at 09:37, Martin Ågren wrote:\n>>\n>> The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\n>> packet_write_fmt_gently()\", 2016-10-16). As a result, for each call to\n>> packet_write_fmt_1, we allocate and leak a buffer.\n>\n> Oh :-( Thanks for detecting and fixing the leak.\n>\n> How did you actually find it? Reading the code or did you use some\n> tool?\n\nValgrind found it for me. Most leaks it reports are \"ok\" since we're\nabout to exit, it just takes more or less effort to realize that...\n\nMartin\n"},{"id":"327266","messageId":"20170827191508.bkydifqihq3ke3rw@sigill.intra.peff.net","threadId":"46675","inReplyTo":"CAN0heSraJFbbog7FKpAtmob9W6_5-AS1StZFVW6xUwMDWfMYgg@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-27T19:15:09Z","receivedAt":"2017-08-27T19:15:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 27, 2017 at 09:09:15PM +0200, Martin Ågren wrote:\n\n> On 27 August 2017 at 20:21, Lars Schneider wrote:\n> >\n> >> On 27 Aug 2017, at 09:37, Martin Ågren wrote:\n> >>\n> >> The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\n> >> packet_write_fmt_gently()\", 2016-10-16). As a result, for each call to\n> >> packet_write_fmt_1, we allocate and leak a buffer.\n> >\n> > Oh :-( Thanks for detecting and fixing the leak.\n> >\n> > How did you actually find it? Reading the code or did you use some\n> > tool?\n> \n> Valgrind found it for me. Most leaks it reports are \"ok\" since we're\n> about to exit, it just takes more or less effort to realize that...\n\nThis is one more thing it would be nice to have as part of our\nperf-testing framework. It will run two versions of Git across a battery\nof tests and report on the runtime differences for each. It would be\ngreat if it did the same for peak heap. The tricky thing is that you\nsometimes need repositories that are exaggerated in size in one\ndimension to notice the differences as significant. So I don't know if\nwe would need new tests for this, or if existing \"this repo has a lot of\nrefs\" tests would have caught this.\n\nAnother approach, of course, is to get valgrind (or asan) to a\nzero-leaks-reported steady state, and then even small leak reports would\nbe worth investigating. I'm not sure how easy it is to get there, and\nwhether it's less work to actually plug free-on-exit leaks or somehow\nsuppress the reports. I think most free-on-exit false positives would\nshow up as \"leaked but still reachable\", whereas leaks like this that\ncan grow without bound mean would not be reachable (we throw away the\npointer to the memory at the end of the function).\n\n-Peff\n"},{"id":"327267","messageId":"179AC8FB-5991-4200-9AAC-2F8D0914D5F9@gmail.com","threadId":"46675","inReplyTo":"CAN0heSraJFbbog7FKpAtmob9W6_5-AS1StZFVW6xUwMDWfMYgg@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-27T20:04:55Z","receivedAt":"2017-08-27T20:05:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Aug 2017, at 21:09, Martin Ågren wrote:\n> \n> On 27 August 2017 at 20:21, Lars Schneider wrote:\n>> \n>>> On 27 Aug 2017, at 09:37, Martin Ågren wrote:\n>>> \n>>> The static-ness was silently dropped in commit 70428d1a5 (\"pkt-line: add\n>>> packet_write_fmt_gently()\", 2016-10-16). As a result, for each call to\n>>> packet_write_fmt_1, we allocate and leak a buffer.\n>> \n>> Oh :-( Thanks for detecting and fixing the leak.\n>> \n>> How did you actually find it? Reading the code or did you use some\n>> tool?\n> \n> Valgrind found it for me. Most leaks it reports are \"ok\" since we're\n> about to exit, it just takes more or less effort to realize that...\n\nI am sorry if I am bugging you here, but I would really like to \nunderstand how you found it.\n\nI did run all tests under valgrind using \"make valgrind\" and I found\nthe following files with potential issues:\n\ncat valgrind.out | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n7102\n 2 clone.c\n 33 common-main.c\n 6 connect.c\n 64 git.c\n 4 ls-remote.c\n 126 run-command.c\n 12 transport.c\n 7 worktree.c\n\n\nNo mention of \"pkt-line.c\". Did you run Git with valgrind on one of \nyour repositories to find it?\n\nThanks,\nLars"},{"id":"327268","messageId":"20170827232338.hm5t7t7c2xaa3zyl@sigill.intra.peff.net","threadId":"46675","inReplyTo":"179AC8FB-5991-4200-9AAC-2F8D0914D5F9@gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-27T23:23:39Z","receivedAt":"2017-08-27T23:23:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote:\n\n> I did run all tests under valgrind using \"make valgrind\" and I found\n> the following files with potential issues:\n> \n> cat valgrind.out | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n> 7102\n> 2 clone.c\n> 33 common-main.c\n> 6 connect.c\n> 64 git.c\n> 4 ls-remote.c\n> 126 run-command.c\n> 12 transport.c\n> 7 worktree.c\n\nI'm not sure where valgrind.out comes from. The individual\ntest-results/*.out files may have valgrind output, but I don't think\nthey usually contain leak output.\n\nDoing \"valgrind ./git-upload-pack . /dev/null\" mentions\nleaked memory but not the locations. Adding --leak-check=full shows that\nmost of it comes from format_packet().\n\nAnd applying Martin's patch drops the \"definitely lost\" category down to\n0 bytes (there's still 550k in \"still reachable\", but those are in the\n\"exit will free them for us\" category).\n\n> No mention of \"pkt-line.c\". Did you run Git with valgrind on one of \n> your repositories to find it?\n\nI'm curious, too. I don't think the valgrind setup in our test suite is\ngreat for finding leaks right now.\n\n-Peff\n"},{"id":"327269","messageId":"CAN0heSoUqcOqVspZkbPahWQdtVpSdtSZoCFWu0ZQJfN3F0mD2g@mail.gmail.com","threadId":"46675","inReplyTo":"20170827232338.hm5t7t7c2xaa3zyl@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-28T04:11:33Z","receivedAt":"2017-08-28T04:11:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 28 August 2017 at 01:23, Jeff King wrote:\n> On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote:\n>\n>> I did run all tests under valgrind using \"make valgrind\" and I found\n>> the following files with potential issues:\n>>\n>> cat valgrind.out | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n>> 7102\n>> 2 clone.c\n>> 33 common-main.c\n>> 6 connect.c\n>> 64 git.c\n>> 4 ls-remote.c\n>> 126 run-command.c\n>> 12 transport.c\n>> 7 worktree.c\n>\n> I'm not sure where valgrind.out comes from. The individual\n> test-results/*.out files may have valgrind output, but I don't think\n> they usually contain leak output.\n>\n> Doing \"valgrind ./git-upload-pack . /dev/null\" mentions\n> leaked memory but not the locations. Adding --leak-check=full shows that\n> most of it comes from format_packet().\n>\n> And applying Martin's patch drops the \"definitely lost\" category down to\n> 0 bytes (there's still 550k in \"still reachable\", but those are in the\n> \"exit will free them for us\" category).\n>\n>> No mention of \"pkt-line.c\". Did you run Git with valgrind on one of\n>> your repositories to find it?\n>\n> I'm curious, too. I don't think the valgrind setup in our test suite is\n> great for finding leaks right now.\n\nSorry for being brief. I've patched t/valgrind/valgrind.sh to say\n\"--leak-check=yes\". Then I run \"./t0000 --valgrind\", simply because\nrunning the complete suite gives more reports than I could possibly\nprocess.\n\nThen I check the first few leaks, verify that they're \"ok\" and add\nthem to a suppressions-list. Lather, rinse, repeat. A couple of very\ntargeted and well-motivated suppressions in git.git could perhaps be\nmotivated, but there are many many reported leaks. My suppressions-list\nis getting gross.\n\nI started with t0000 and t0001 because I figure, once I have those basic\nsuppressions in place (or leaks fixed), I can run some other more\ninteresting tests. Of course, the concept of \"this leak is ok\" is a bit\nsubjective. For example, we might do \"return !!create_object(...);\"\n(function name invented on the fly), which is a leak, and unreachable.\nBut if this is only done once in builtin/foo.c and the object created is\nsmall, then this could be deemed \"ok\", since in practice this leak will\nnever bring anyone over the cliff. If clean-ups in such code would not\njust be code churn, then I can of course adjust my definition of \"ok\"\naccordingly.\n\nThis is not an attempt to find and fix a huge number of leaks, it's more\nto have a good reason to go through call-stacks, convince myself I know\nwhat the code wants to do and how it does it.\n\nLooking at only \"unreachable\" leaks seems like it should be an\nimprovement for finding the interesting cases. I'll have less time for\nGit this week, but can try it out as time permits.\n\nThanks for your feedback, both of you.\n\nMartin\n"},{"id":"327283","messageId":"CAGZ79ka3+T+TgakKDcHSmoc83mOdSDgoN=S=P5PtaYygo0nGWA@mail.gmail.com","threadId":"46675","inReplyTo":"CAN0heSoUqcOqVspZkbPahWQdtVpSdtSZoCFWu0ZQJfN3F0mD2g@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-28T17:52:51Z","receivedAt":"2017-08-28T17:52:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">>> No mention of \"pkt-line.c\". Did you run Git with valgrind on one of\n>>> your repositories to find it?\n>>\n>> I'm curious, too. I don't think the valgrind setup in our test suite is\n>> great for finding leaks right now.\n>\n\nThis discussion comes up every once in a while,\nthe last time I was involved in this discussion I proposed\nto have an \"optional_free(void *)\", which only frees memory\nin e.g. the developer build/debug build.\n\nThat way we can have a strict \"no leaks in developer build\"\npolicy (as it is easy to detect!), but it doesn't slow down the\nwidely distributed version of Git.\n\nIIRC, then the discussion focused on \"what if we misuse\noptional_free and the real free\", such that the widely distributed\nversion just leaks large amounts of memory and the developers\ndo not notice, but quite the opposite(!) the developers would feel\nsafe to ignore any memory complaints, because the developer build\nis clean.\n\nStefan\n"},{"id":"327285","messageId":"20170828175855.vs2s7lpxxhwlfhdi@sigill.intra.peff.net","threadId":"46675","inReplyTo":"CAGZ79ka3+T+TgakKDcHSmoc83mOdSDgoN=S=P5PtaYygo0nGWA@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-28T17:58:56Z","receivedAt":"2017-08-28T17:59:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 28, 2017 at 10:52:51AM -0700, Stefan Beller wrote:\n\n> >> I'm curious, too. I don't think the valgrind setup in our test suite is\n> >> great for finding leaks right now.\n> \n> This discussion comes up every once in a while,\n> the last time I was involved in this discussion I proposed\n> to have an \"optional_free(void *)\", which only frees memory\n> in e.g. the developer build/debug build.\n> \n> That way we can have a strict \"no leaks in developer build\"\n> policy (as it is easy to detect!), but it doesn't slow down the\n> widely distributed version of Git.\n\nPersonally I am not that worried about slowing down program-exit with\nsome free() calls (though I would reserve judgement to see how slow it\nactually is).\n\nIt is that I do not want to deal with the complexity and maintenance\nheadache of dropping values which are program-lifetime caches. If we\nintroduce a free-all-object-structs function, now everybody needs to\nremember to call it (even if they didn't realize they were allocating\nobject structs in the first place, as it may have happened under the\nhood of a sub-function). And we have to wonder what might happen when\nsomebody calls that function _not_ at program exit.\n\nIf we can declare \"reachable things are not leaks\" (and certainly\nvalgrind is aware of that distinction), then all of that goes away. And\nyou're just left with local variables in main() (or our cmd_*\nequivalents) that appear as leaks. And then we can solve that either by\nfreeing them, or just calling them program-lifetime and telling the\ncompiler that it's so by declaring them \"static\".\n\n-Peff\n"},{"id":"327329","messageId":"B1E291F2-86FF-4982-A092-92FAED65385C@gmail.com","threadId":"46675","inReplyTo":"CAN0heSoUqcOqVspZkbPahWQdtVpSdtSZoCFWu0ZQJfN3F0mD2g@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-29T17:51:52Z","receivedAt":"2017-08-29T17:52:22Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 28 Aug 2017, at 06:11, Martin Ågren wrote:\n> \n> On 28 August 2017 at 01:23, Jeff King wrote:\n>> On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote:\n>> \n>>> I did run all tests under valgrind using \"make valgrind\" and I found\n>>> the following files with potential issues:\n>>> \n>>> cat valgrind.out | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n>>> 7102\n>>> 2 clone.c\n>>> 33 common-main.c\n>>> 6 connect.c\n>>> 64 git.c\n>>> 4 ls-remote.c\n>>> 126 run-command.c\n>>> 12 transport.c\n>>> 7 worktree.c\n>> \n>> I'm not sure where valgrind.out comes from. The individual\n>> test-results/*.out files may have valgrind output, but I don't think\n>> they usually contain leak output.\n>> \n>> Doing \"valgrind ./git-upload-pack . /dev/null\" mentions\n>> leaked memory but not the locations. Adding --leak-check=full shows that\n>> most of it comes from format_packet().\n>> \n>> And applying Martin's patch drops the \"definitely lost\" category down to\n>> 0 bytes (there's still 550k in \"still reachable\", but those are in the\n>> \"exit will free them for us\" category).\n>> \n>>> No mention of \"pkt-line.c\". Did you run Git with valgrind on one of\n>>> your repositories to find it?\n>> \n>> I'm curious, too. I don't think the valgrind setup in our test suite is\n>> great for finding leaks right now.\n> \n> Sorry for being brief. I've patched t/valgrind/valgrind.sh to say\n> \"--leak-check=yes\". Then I run \"./t0000 --valgrind\", simply because\n> running the complete suite gives more reports than I could possibly\n> process.\n\nI set $TOOL_OPTIONS in valgrind.sh: to \n'--leak-check=full --errors-for-leak-kinds=definite'\n\n... but I also had to adjust t/test-lib-functions.sh:test_create_repo\nas I ran into the error \"cannot run git init -- have you built things yet?\".\n\nWith these changes I was able to see the leak running valgrind with t5110.\n\n---\n\nWhat if we run a few selected tests with valgrind and count all files that\nvalgrind mentions (a single leak has multiple file mentions because of\nthe stack trace and other leak indicators). We record these counts and let \nTravisCI scream if one of the numbers increases.\n\nI wonder how stable/fragile such a metric would be as a simple refactoring \ncould easily change these numbers. Below I ran valgrind on t5510 before and\nafter Martin's patch. The diff below clearly shows the pkt-line leak.\n\nWould it make sense to pursue something like this in TravisCI to avoid \n\"pkt-line\" kind of leaks in the future? \n\n\n## Valgrind run with leak\n\n$ ./t5510-fetch.sh --valgrind | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n15529\n 39 abspath.c\n 30 add.c\n 34 branch.c\n 10 bundle.c\n 268 clone.c\n 13 commit.c\n 471 common-main.c\n 52 config.c\n 50 connect.c\n 4 connected.c\n 1 diff-lib.c\n 102 dir.c\n 120 environment.c\n 4 fetch-pack.c\n 47 fetch.c\n 14 files-backend.c\n 1 git-compat-util.h\n 871 git.c\n 96 init-db.c\n 26 iterator.c\n 1 list-objects.c\n 4 log-tree.c\n 2 object.c\n 1 pack-objects.c\n 6 parse-options.c\n 10 pathspec.c\n 83 pkt-line.c\n 6 precompose_utf8.c\n 2 push.c\n 67 refs.c\n 96 remote.c\n 90 repository.c\n 4 rev-list.c\n 10 revision.c\n 165 run-command.c\n 42 setup.c\n 288 strbuf.c\n 51 strbuf.h\n 2 tag.c\n 107 transport.c\n 10 tree-diff.c\n 85 upload-pack.c\n 1 usage.c\n 455 wrapper.c\n\n\n## Valgrind run with Matrin's patch to fix the leak\n$ ./t5510-fetch.sh --valgrind | perl -nE 'say /^==.+\\((.+)\\:.+\\)$/' | sort | uniq -c\n14931\n 39 abspath.c\n 30 add.c\n 34 branch.c\n 10 bundle.c\n 268 clone.c\n 13 commit.c\n 433 common-main.c\n 52 config.c\n 50 connect.c\n 6 connected.c\n 1 diff-lib.c\n 102 dir.c\n 120 environment.c\n 4 fetch-pack.c\n 53 fetch.c\n 14 files-backend.c\n 1 git-compat-util.h\n 879 git.c\n 96 init-db.c\n 1 iterator.c\n 1 list-objects.c\n 4 log-tree.c\n 2 object.c\n 1 pack-objects.c\n 6 parse-options.c\n 10 pathspec.c\n 6 precompose_utf8.c\n 2 push.c\n 26 refs.c\n 96 remote.c\n 90 repository.c\n 6 rev-list.c\n 14 revision.c\n 171 run-command.c\n 42 setup.c\n 246 strbuf.c\n 50 strbuf.h\n 2 tag.c\n 107 transport.c\n 10 tree-diff.c\n 2 upload-pack.c\n 1 usage.c\n 415 wrapper.c\n\n\n## Diff\n\n15529 | 14931 \n 39 abspath.c 39 abspath.c\n 30 add.c 30 add.c\n 34 branch.c 34 branch.c\n 10 bundle.c 10 bundle.c\n 268 clone.c 268 clone.c\n 13 commit.c 13 commit.c\n 471 common-main.c | 433 common-main.c\n 52 config.c 52 config.c\n 50 connect.c 50 connect.c\n 4 connected.c | 6 connected.c\n 1 diff-lib.c 1 diff-lib.c\n 102 dir.c 102 dir.c\n 120 environment.c 120 environment.c\n 4 fetch-pack.c 4 fetch-pack.c\n 47 fetch.c | 53 fetch.c\n 14 files-backend.c 14 files-backend.c\n 1 git-compat-util.h 1 git-compat-util.h\n 871 git.c | 879 git.c\n 96 init-db.c 96 init-db.c\n 26 iterator.c | 1 iterator.c\n 1 list-objects.c 1 list-objects.c\n 4 log-tree.c 4 log-tree.c\n 2 object.c 2 object.c\n 1 pack-objects.c 1 pack-objects.c\n 6 parse-options.c 6 parse-options.c\n 10 pathspec.c 10 pathspec.c\n 83 pkt-line.c <\n 6 precompose_utf8.c 6 precompose_utf8.c\n 2 push.c 2 push.c\n 67 refs.c | 26 refs.c\n 96 remote.c 96 remote.c\n 90 repository.c 90 repository.c\n 4 rev-list.c | 6 rev-list.c\n 10 revision.c | 14 revision.c\n 165 run-command.c | 171 run-command.c\n 42 setup.c 42 setup.c\n 288 strbuf.c | 246 strbuf.c\n 51 strbuf.h | 50 strbuf.h\n 2 tag.c 2 tag.c\n 107 transport.c 107 transport.c\n 10 tree-diff.c 10 tree-diff.c\n 85 upload-pack.c | 2 upload-pack.c\n 1 usage.c 1 usage.c\n 455 wrapper.c | 415 wrapp \n\n\nThanks,\nLars"},{"id":"327333","messageId":"20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net","threadId":"46675","inReplyTo":"B1E291F2-86FF-4982-A092-92FAED65385C@gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-29T18:53:41Z","receivedAt":"2017-08-29T18:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote:\n\n> I set $TOOL_OPTIONS in valgrind.sh: to \n> '--leak-check=full --errors-for-leak-kinds=definite'\n> \n> ... but I also had to adjust t/test-lib-functions.sh:test_create_repo\n> as I ran into the error \"cannot run git init -- have you built things yet?\".\n\nYeah, this is a general problem with run-time analyzers. If we're not\nmostly error free than the setup code often breaks before you even get\nto the interesting part of the test.\n\nIt looks like the config code has a minor-ish leak. Patch to follow.\n\n> What if we run a few selected tests with valgrind and count all files that\n> valgrind mentions (a single leak has multiple file mentions because of\n> the stack trace and other leak indicators). We record these counts and let \n> TravisCI scream if one of the numbers increases.\n> \n> I wonder how stable/fragile such a metric would be as a simple refactoring \n> could easily change these numbers. Below I ran valgrind on t5510 before and\n> after Martin's patch. The diff below clearly shows the pkt-line leak.\n> \n> Would it make sense to pursue something like this in TravisCI to avoid \n> \"pkt-line\" kind of leaks in the future?\n\nI don't think that would work, because simply adding new tests would\nbump the leak count, without the code actually growing worse.\n\nBut think about your strategy for a moment: what you're really trying to\ndo is say \"these existing leaks are OK because they are too many for us\nto count, but we want to make sure we don't add _new_ ones\". We already\nhave two techniques for distinguishing old ones from new ones:\n\n 1. Mark existing ones with valgrind suppressions so they do not warn\n at all.\n\n 2. Fix them, so that the \"existing\" count drops to zero.\n\nOption (2), of course, has the added side effect that it's actually\nfixing potential problems. :)\n\n-Peff\n"},{"id":"327334","messageId":"20170829185850.tfmjoa5u5sfuwpgi@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net","subject":"[PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-29T18:58:50Z","receivedAt":"2017-08-29T18:58:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:\n\n> It looks like the config code has a minor-ish leak. Patch to follow.\n\nHere it is.\n\n-- >8 --\nSubject: [PATCH] config: use a static lock_file struct\n\nWhen modifying git config, we xcalloc() a struct lock_file\nbut never free it. This is necessary because the tempfile\ncode (upon which the locking code is built) requires that\nthe resulting struct remain valid through the life of the\nprogram. However, it also confuses leak-checkers like\nvalgrind because only the inner \"struct tempfile\" is still\nreachable; no pointer to the outer lock_file is kept.\n\nOther code paths solve this by using a single static lock\nstruct. We can do the same here, because we know that we'll\nonly lock and modify one config file at a time (and\nassertions within the lockfile code will ensure that this\nremains the case).\n\nThat removes a real leak (when we fail to free the struct\nafter locking fails) as well as removes the valgrind false\npositive. It also means that doing N sequential\nconfig-writes will use a constant amount of memory, rather\nthan leaving stale lock_files for each.\n\nNote that since \"lock\" is no longer a pointer, it can't be\nNULL anymore. But that's OK. We used that feature only to\navoid calling rollback_lock_file() on an already-committed\nlock. Since the lockfile code keeps its own \"active\" flag,\nit's a noop to rollback an inactive lock, and we don't have\nto worry about this ourselves.\n\nSigned-off-by: Jeff King \n---\nIn the long run we may want to drop the \"tempfiles must remain forever\"\nrule. This is certainly not the first time it has caused confusion or\nleaks. And I don't think it's a fundamental issue, just the way the code\nis written. But in the interim, this fix is probably worth doing.\n\n config.c | 24 +++++++-----------------\n 1 file changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d0d8ce823a..1603f96e40 100644\n--- a/config.c\n+++ b/config.c\n@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n {\n \tint fd = -1, in_fd = -1;\n \tint ret;\n-\tstruct lock_file *lock = NULL;\n+\tstatic struct lock_file lock;\n \tchar *filename_buf = NULL;\n \tchar *contents = NULL;\n \tsize_t contents_sz;\n@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror_errno(\"could not lock config file %s\", config_filename);\n \t\tfree(store.key);\n@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tclose(in_fd);\n \t\tin_fd = -1;\n \n-\t\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n-\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(lock));\n+\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n+\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(&lock));\n \t\t\tret = CONFIG_NO_WRITE;\n \t\t\tgoto out_free;\n \t\t}\n@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tcontents = NULL;\n \t}\n \n-\tif (commit_lock_file(lock) < 0) {\n+\tif (commit_lock_file(&lock) < 0) {\n \t\terror_errno(\"could not write config file %s\", config_filename);\n \t\tret = CONFIG_NO_WRITE;\n-\t\tlock = NULL;\n \t\tgoto out_free;\n \t}\n \n-\t/*\n-\t * lock is committed, so don't try to roll it back below.\n-\t * NOTE: Since lockfile.c keeps a linked list of all created\n-\t * lock_file structures, it isn't safe to free(lock). It's\n-\t * better to just leave it hanging around.\n-\t */\n-\tlock = NULL;\n \tret = 0;\n \n \t/* Invalidate the config cache */\n \tgit_config_clear();\n \n out_free:\n-\tif (lock)\n-\t\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \tfree(filename_buf);\n \tif (contents)\n \t\tmunmap(contents, contents_sz);\n@@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(get_lock_file_path(lock));\n+\tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n \n }\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327335","messageId":"20170829190928.GD131745@google.com","threadId":"46675","inReplyTo":"20170829185850.tfmjoa5u5sfuwpgi@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T19:09:28Z","receivedAt":"2017-08-29T19:09:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:\n> \n> > It looks like the config code has a minor-ish leak. Patch to follow.\n> \n> Here it is.\n> \n> -- >8 --\n> Subject: [PATCH] config: use a static lock_file struct\n> \n> When modifying git config, we xcalloc() a struct lock_file\n> but never free it. This is necessary because the tempfile\n> code (upon which the locking code is built) requires that\n> the resulting struct remain valid through the life of the\n> program. However, it also confuses leak-checkers like\n> valgrind because only the inner \"struct tempfile\" is still\n> reachable; no pointer to the outer lock_file is kept.\n\nIs this just due to a limitation in the tempfile code? Would it be\npossible to improve the tempfile code such that we don't need to require\nthat a tempfile, once created, is required to exist for the remaining\nlife of the program?\n\n> \n> Other code paths solve this by using a single static lock\n> struct. We can do the same here, because we know that we'll\n> only lock and modify one config file at a time (and\n> assertions within the lockfile code will ensure that this\n> remains the case).\n> \n> That removes a real leak (when we fail to free the struct\n> after locking fails) as well as removes the valgrind false\n> positive. It also means that doing N sequential\n> config-writes will use a constant amount of memory, rather\n> than leaving stale lock_files for each.\n> \n> Note that since \"lock\" is no longer a pointer, it can't be\n> NULL anymore. But that's OK. We used that feature only to\n> avoid calling rollback_lock_file() on an already-committed\n> lock. Since the lockfile code keeps its own \"active\" flag,\n> it's a noop to rollback an inactive lock, and we don't have\n> to worry about this ourselves.\n> \n> Signed-off-by: Jeff King \n> ---\n> In the long run we may want to drop the \"tempfiles must remain forever\"\n> rule. This is certainly not the first time it has caused confusion or\n> leaks. And I don't think it's a fundamental issue, just the way the code\n> is written. But in the interim, this fix is probably worth doing.\n> \n> config.c | 24 +++++++-----------------\n> 1 file changed, 7 insertions(+), 17 deletions(-)\n> \n> diff --git a/config.c b/config.c\n> index d0d8ce823a..1603f96e40 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> {\n> \tint fd = -1, in_fd = -1;\n> \tint ret;\n> -\tstruct lock_file *lock = NULL;\n> +\tstatic struct lock_file lock;\n> \tchar *filename_buf = NULL;\n> \tchar *contents = NULL;\n> \tsize_t contents_sz;\n> @@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> \t * The lock serves a purpose in addition to locking: the new\n> \t * contents of .git/config will be written into it.\n> \t */\n> -\tlock = xcalloc(1, sizeof(struct lock_file));\n> -\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n> +\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n> \tif (fd < 0) {\n> \t\terror_errno(\"could not lock config file %s\", config_filename);\n> \t\tfree(store.key);\n> @@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> \t\tclose(in_fd);\n> \t\tin_fd = -1;\n> \n> -\t\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n> -\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(lock));\n> +\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n> +\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(&lock));\n> \t\t\tret = CONFIG_NO_WRITE;\n> \t\t\tgoto out_free;\n> \t\t}\n> @@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> \t\tcontents = NULL;\n> \t}\n> \n> -\tif (commit_lock_file(lock) < 0) {\n> +\tif (commit_lock_file(&lock) < 0) {\n> \t\terror_errno(\"could not write config file %s\", config_filename);\n> \t\tret = CONFIG_NO_WRITE;\n> -\t\tlock = NULL;\n> \t\tgoto out_free;\n> \t}\n> \n> -\t/*\n> -\t * lock is committed, so don't try to roll it back below.\n> -\t * NOTE: Since lockfile.c keeps a linked list of all created\n> -\t * lock_file structures, it isn't safe to free(lock). It's\n> -\t * better to just leave it hanging around.\n> -\t */\n> -\tlock = NULL;\n> \tret = 0;\n> \n> \t/* Invalidate the config cache */\n> \tgit_config_clear();\n> \n> out_free:\n> -\tif (lock)\n> -\t\trollback_lock_file(lock);\n> +\trollback_lock_file(&lock);\n> \tfree(filename_buf);\n> \tif (contents)\n> \t\tmunmap(contents, contents_sz);\n> @@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> \treturn ret;\n> \n> write_err_out:\n> -\tret = write_error(get_lock_file_path(lock));\n> +\tret = write_error(get_lock_file_path(&lock));\n> \tgoto out_free;\n> \n> }\n> -- \n> 2.14.1.721.gc5bc1565f1\n> \n\n-- \nBrandon Williams\n"},{"id":"327336","messageId":"20170829191051.GE131745@google.com","threadId":"46675","inReplyTo":"20170829190928.GD131745@google.com","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T19:10:51Z","receivedAt":"2017-08-29T19:10:59Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Brandon Williams wrote:\n> On 08/29, Jeff King wrote:\n> > On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:\n> > \n> > > It looks like the config code has a minor-ish leak. Patch to follow.\n> > \n> > Here it is.\n> > \n> > -- >8 --\n> > Subject: [PATCH] config: use a static lock_file struct\n> > \n> > When modifying git config, we xcalloc() a struct lock_file\n> > but never free it. This is necessary because the tempfile\n> > code (upon which the locking code is built) requires that\n> > the resulting struct remain valid through the life of the\n> > program. However, it also confuses leak-checkers like\n> > valgrind because only the inner \"struct tempfile\" is still\n> > reachable; no pointer to the outer lock_file is kept.\n> \n> Is this just due to a limitation in the tempfile code? Would it be\n> possible to improve the tempfile code such that we don't need to require\n> that a tempfile, once created, is required to exist for the remaining\n> life of the program?\n> \n> > \n> > Other code paths solve this by using a single static lock\n> > struct. We can do the same here, because we know that we'll\n> > only lock and modify one config file at a time (and\n> > assertions within the lockfile code will ensure that this\n> > remains the case).\n> > \n> > That removes a real leak (when we fail to free the struct\n> > after locking fails) as well as removes the valgrind false\n> > positive. It also means that doing N sequential\n> > config-writes will use a constant amount of memory, rather\n> > than leaving stale lock_files for each.\n> > \n> > Note that since \"lock\" is no longer a pointer, it can't be\n> > NULL anymore. But that's OK. We used that feature only to\n> > avoid calling rollback_lock_file() on an already-committed\n> > lock. Since the lockfile code keeps its own \"active\" flag,\n> > it's a noop to rollback an inactive lock, and we don't have\n> > to worry about this ourselves.\n> > \n> > Signed-off-by: Jeff King \n> > ---\n> > In the long run we may want to drop the \"tempfiles must remain forever\"\n> > rule. This is certainly not the first time it has caused confusion or\n> > leaks. And I don't think it's a fundamental issue, just the way the code\n> > is written. But in the interim, this fix is probably worth doing.\n\nI didn't read far enough apparently :) I took a look at this once and\nfound that the in order to do this we would just need to be careful in\nmodifying a list of tempfiles.\n\n> > \n> > config.c | 24 +++++++-----------------\n> > 1 file changed, 7 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/config.c b/config.c\n> > index d0d8ce823a..1603f96e40 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> > {\n> > \tint fd = -1, in_fd = -1;\n> > \tint ret;\n> > -\tstruct lock_file *lock = NULL;\n> > +\tstatic struct lock_file lock;\n> > \tchar *filename_buf = NULL;\n> > \tchar *contents = NULL;\n> > \tsize_t contents_sz;\n> > @@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> > \t * The lock serves a purpose in addition to locking: the new\n> > \t * contents of .git/config will be written into it.\n> > \t */\n> > -\tlock = xcalloc(1, sizeof(struct lock_file));\n> > -\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n> > +\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n> > \tif (fd < 0) {\n> > \t\terror_errno(\"could not lock config file %s\", config_filename);\n> > \t\tfree(store.key);\n> > @@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> > \t\tclose(in_fd);\n> > \t\tin_fd = -1;\n> > \n> > -\t\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n> > -\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(lock));\n> > +\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n> > +\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(&lock));\n> > \t\t\tret = CONFIG_NO_WRITE;\n> > \t\t\tgoto out_free;\n> > \t\t}\n> > @@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> > \t\tcontents = NULL;\n> > \t}\n> > \n> > -\tif (commit_lock_file(lock) < 0) {\n> > +\tif (commit_lock_file(&lock) < 0) {\n> > \t\terror_errno(\"could not write config file %s\", config_filename);\n> > \t\tret = CONFIG_NO_WRITE;\n> > -\t\tlock = NULL;\n> > \t\tgoto out_free;\n> > \t}\n> > \n> > -\t/*\n> > -\t * lock is committed, so don't try to roll it back below.\n> > -\t * NOTE: Since lockfile.c keeps a linked list of all created\n> > -\t * lock_file structures, it isn't safe to free(lock). It's\n> > -\t * better to just leave it hanging around.\n> > -\t */\n> > -\tlock = NULL;\n> > \tret = 0;\n> > \n> > \t/* Invalidate the config cache */\n> > \tgit_config_clear();\n> > \n> > out_free:\n> > -\tif (lock)\n> > -\t\trollback_lock_file(lock);\n> > +\trollback_lock_file(&lock);\n> > \tfree(filename_buf);\n> > \tif (contents)\n> > \t\tmunmap(contents, contents_sz);\n> > @@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n> > \treturn ret;\n> > \n> > write_err_out:\n> > -\tret = write_error(get_lock_file_path(lock));\n> > +\tret = write_error(get_lock_file_path(&lock));\n> > \tgoto out_free;\n> > \n> > }\n> > -- \n> > 2.14.1.721.gc5bc1565f1\n> > \n> \n> -- \n> Brandon Williams\n\n-- \nBrandon Williams\n"},{"id":"327337","messageId":"20170829191217.dt65wazf7qh5qs3k@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170829190928.GD131745@google.com","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-29T19:12:17Z","receivedAt":"2017-08-29T19:12:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:\n\n> > -- >8 --\n> > Subject: [PATCH] config: use a static lock_file struct\n> > \n> > When modifying git config, we xcalloc() a struct lock_file\n> > but never free it. This is necessary because the tempfile\n> > code (upon which the locking code is built) requires that\n> > the resulting struct remain valid through the life of the\n> > program. However, it also confuses leak-checkers like\n> > valgrind because only the inner \"struct tempfile\" is still\n> > reachable; no pointer to the outer lock_file is kept.\n> \n> Is this just due to a limitation in the tempfile code? Would it be\n> possible to improve the tempfile code such that we don't need to require\n> that a tempfile, once created, is required to exist for the remaining\n> life of the program?\n\nYes. Like I wrote below:\n\n> > ---\n> > In the long run we may want to drop the \"tempfiles must remain forever\"\n> > rule. This is certainly not the first time it has caused confusion or\n> > leaks. And I don't think it's a fundamental issue, just the way the code\n> > is written. But in the interim, this fix is probably worth doing.\n\nThe main issue is that the caller needs to make sure they're removed\nfrom the list (via commit or rollback) before being freed.\n\nAs far as I know anyway. This restriction dates back to the very early\ndays of the lockfile code and has been carried through the various\ntempfile-cleanup refactorings over the years (mostly because each was\nafraid to make functional changes).\n\n+cc Michael, who did most comprehensive cleanup of that code.\n\n-Peff\n"},{"id":"327338","messageId":"CAN0heSomjqiSStq3eqGgCe21b0Pr0gbAQLL-2EEB6Zfnjj+Mjg@mail.gmail.com","threadId":"46675","inReplyTo":"20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-29T19:22:08Z","receivedAt":"2017-08-29T19:22:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 29 August 2017 at 20:53, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote:\n>> What if we run a few selected tests with valgrind and count all files that\n>> valgrind mentions (a single leak has multiple file mentions because of\n>> the stack trace and other leak indicators). We record these counts and let\n>> TravisCI scream if one of the numbers increases.\n>>\n>> I wonder how stable/fragile such a metric would be as a simple refactoring\n>> could easily change these numbers. Below I ran valgrind on t5510 before and\n>> after Martin's patch. The diff below clearly shows the pkt-line leak.\n>>\n>> Would it make sense to pursue something like this in TravisCI to avoid\n>> \"pkt-line\" kind of leaks in the future?\n>\n> I don't think that would work, because simply adding new tests would\n> bump the leak count, without the code actually growing worse.\n>\n> But think about your strategy for a moment: what you're really trying to\n> do is say \"these existing leaks are OK because they are too many for us\n> to count, but we want to make sure we don't add _new_ ones\". We already\n> have two techniques for distinguishing old ones from new ones:\n>\n> 1. Mark existing ones with valgrind suppressions so they do not warn\n> at all.\n>\n> 2. Fix them, so that the \"existing\" count drops to zero.\n>\n> Option (2), of course, has the added side effect that it's actually\n> fixing potential problems. :)\n\nOne idea would be to report that \"t1234 used to complete without leaks,\nbut now there is a leak\". Of course, the leak could be triggered by some\n\"irrelevant\" setup step that was just added and not by the functionality\nthat is actually tested. But maybe then the reaction might be \"oh, let's\nfix this one leak so that this test is leak-free again\". If that then\nalso reduces the leakiness of some other tests, good. That might help\nagainst the \"there are too many leaks, where should I start?\"-effect.\n\nOr similarly \"This stack-trace was reported. I haven't seen it before.\"\nIt could be a refactoring, it could be an old leak that hasn't been\ntriggered in that way before, or it could be a new leak.\n\nBut to be honest, I think that only works -- in a psychological sense --\nonce the number of leaks is small enough, however much that is.\n\nOne take-away for me from this thread is that memory-leak-plugging seems\nto be appreciated, i.e., it's not just an academic problem. I think I'll\nlook into it some more, i.e., slowly pursue option 2. :-) That might help\ngive some insights on how to identify interesting leaks.\n\nMartin\n"},{"id":"327341","messageId":"20170829214809.xxpsp76eqhd4hupo@sigill.intra.peff.net","threadId":"46675","inReplyTo":"CAN0heSomjqiSStq3eqGgCe21b0Pr0gbAQLL-2EEB6Zfnjj+Mjg@mail.gmail.com","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-29T21:48:09Z","receivedAt":"2017-08-29T21:48:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 09:22:08PM +0200, Martin Ågren wrote:\n\n> One take-away for me from this thread is that memory-leak-plugging seems\n> to be appreciated, i.e., it's not just an academic problem. I think I'll\n> look into it some more, i.e., slowly pursue option 2. :-) That might help\n> give some insights on how to identify interesting leaks.\n\nYes, I think if it's tractable that's the best path forward. It's not\nclear to me how many of the things valgrind reports are false positives\n(and in what way). Certainly the lock_file patch I just posted seems\nlike something worth addressing, even if we _tend_ to only leak a small\nnumber of such structs per program invocation. It's not a problem now,\nbut it's a sign of a bad pattern.\n\n-Peff\n"},{"id":"327345","messageId":"01375356-5d39-99af-9e91-35083ed03f42@alum.mit.edu","threadId":"46675","inReplyTo":"20170829191217.dt65wazf7qh5qs3k@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T03:25:18Z","receivedAt":"2017-08-30T03:25:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/29/2017 09:12 PM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:\n> \n>>> -- >8 --\n>>> Subject: [PATCH] config: use a static lock_file struct\n>>>\n>>> When modifying git config, we xcalloc() a struct lock_file\n>>> but never free it. This is necessary because the tempfile\n>>> code (upon which the locking code is built) requires that\n>>> the resulting struct remain valid through the life of the\n>>> program. However, it also confuses leak-checkers like\n>>> valgrind because only the inner \"struct tempfile\" is still\n>>> reachable; no pointer to the outer lock_file is kept.\n>>\n>> Is this just due to a limitation in the tempfile code? Would it be\n>> possible to improve the tempfile code such that we don't need to require\n>> that a tempfile, once created, is required to exist for the remaining\n>> life of the program?\n> \n> Yes. Like I wrote below:\n> \n>>> ---\n>>> In the long run we may want to drop the \"tempfiles must remain forever\"\n>>> rule. This is certainly not the first time it has caused confusion or\n>>> leaks. And I don't think it's a fundamental issue, just the way the code\n>>> is written. But in the interim, this fix is probably worth doing.\n> \n> The main issue is that the caller needs to make sure they're removed\n> from the list (via commit or rollback) before being freed.\n> \n> As far as I know anyway. This restriction dates back to the very early\n> days of the lockfile code and has been carried through the various\n> tempfile-cleanup refactorings over the years (mostly because each was\n> afraid to make functional changes).\n> \n> +cc Michael, who did most comprehensive cleanup of that code.\n\nIt was surprisingly hard trying to get that code to do the right thing,\nnon-racily, in every error path. Since `remove_tempfiles()` can be\ncalled any time (even from a signal handler), the linked list of\n`tempfile` objects has to be kept valid at all times but can't use\nmutexes. I didn't have the energy to keep going and make the lock\nobjects freeable.\n\nI suppose the task could be made a bit easier by using `sigprocmask(2)`\nor `pthread_sigmask(3)` to make its running environment a little bit\nless hostile.\n\nMichael\n"},{"id":"327346","messageId":"20170830043147.culn63luzdsbpuuw@sigill.intra.peff.net","threadId":"46675","inReplyTo":"01375356-5d39-99af-9e91-35083ed03f42@alum.mit.edu","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T04:31:48Z","receivedAt":"2017-08-30T04:31:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote:\n\n> >>> In the long run we may want to drop the \"tempfiles must remain forever\"\n> >>> rule. This is certainly not the first time it has caused confusion or\n> >>> leaks. And I don't think it's a fundamental issue, just the way the code\n> >>> is written. But in the interim, this fix is probably worth doing.\n> > \n> > The main issue is that the caller needs to make sure they're removed\n> > from the list (via commit or rollback) before being freed.\n> > \n> > As far as I know anyway. This restriction dates back to the very early\n> > days of the lockfile code and has been carried through the various\n> > tempfile-cleanup refactorings over the years (mostly because each was\n> > afraid to make functional changes).\n> > \n> > +cc Michael, who did most comprehensive cleanup of that code.\n> \n> It was surprisingly hard trying to get that code to do the right thing,\n> non-racily, in every error path. Since `remove_tempfiles()` can be\n> called any time (even from a signal handler), the linked list of\n> `tempfile` objects has to be kept valid at all times but can't use\n> mutexes. I didn't have the energy to keep going and make the lock\n> objects freeable.\n> \n> I suppose the task could be made a bit easier by using `sigprocmask(2)`\n> or `pthread_sigmask(3)` to make its running environment a little bit\n> less hostile.\n\nI think there are really two levels of carefulness here:\n\n 1. Avoiding complicated things during a signal handler that may rely\n on having a sane state from the rest of the program (e.g.,\n half-formed entries, stdio locks, etc).\n\n 2. Being truly race-free in the face of a signal arriving while we're\n running arbitrary code that might have a tempfile struct in a funny\n state.\n\nI feel like right now we meet (1) and not (2). But I think if we keep to\nthat lower bar of (1), it might not be that bad. We're assuming now that\nthere's no race on the tempfile->active flag, for instance. We could\nprobably make a similar assumption about putting items onto or taking\nthem off of a linked list (it's not really atomic, but a single pointer\nassignment is probably \"atomic enough\" for our purposes).\n\nOr I dunno. There's a lot of \"volatile\" modifiers sprinkled around.\nMaybe those are enough to give us (2) as well (though in that case, I\nthink we'd still be as OK with the list manipulation as we are with the\nactive flag manipulation).\n\n-Peff\n"},{"id":"327347","messageId":"CAMy9T_Eq+XJyhXk99RA9AMUVx3L8qw+0=KfAOjYsPsTSVSLchQ@mail.gmail.com","threadId":"46675","inReplyTo":"20170830043147.culn63luzdsbpuuw@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T04:55:04Z","receivedAt":"2017-08-30T04:55:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Wed, Aug 30, 2017 at 6:31 AM, Jeff King wrote:\n> On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote:\n>> It was surprisingly hard trying to get that code to do the right thing,\n>> non-racily, in every error path. Since `remove_tempfiles()` can be\n>> called any time (even from a signal handler), the linked list of\n>> `tempfile` objects has to be kept valid at all times but can't use\n>> mutexes. I didn't have the energy to keep going and make the lock\n>> objects freeable.\n>>\n>> I suppose the task could be made a bit easier by using `sigprocmask(2)`\n>> or `pthread_sigmask(3)` to make its running environment a little bit\n>> less hostile.\n>\n> I think there are really two levels of carefulness here:\n>\n> 1. Avoiding complicated things during a signal handler that may rely\n> on having a sane state from the rest of the program (e.g.,\n> half-formed entries, stdio locks, etc).\n>\n> 2. Being truly race-free in the face of a signal arriving while we're\n> running arbitrary code that might have a tempfile struct in a funny\n> state.\n>\n> I feel like right now we meet (1) and not (2). But I think if we keep to\n> that lower bar of (1), it might not be that bad. We're assuming now that\n> there's no race on the tempfile->active flag, for instance. We could\n> probably make a similar assumption about putting items onto or taking\n> them off of a linked list (it's not really atomic, but a single pointer\n> assignment is probably \"atomic enough\" for our purposes).\n>\n> Or I dunno. There's a lot of \"volatile\" modifiers sprinkled around.\n> Maybe those are enough to give us (2) as well (though in that case, I\n> think we'd still be as OK with the list manipulation as we are with the\n> active flag manipulation).\n\nI did in fact strive for both (1) and (2), though I know that there\nare a couple of short races remaining in the code (plus who knows how\nmany bugs).\n\nActually, I think you're right that a single \"probably-atomic\" pointer\nassignment would be enough to remove an entry from the list safely. I\nwas thinking about what it would take to make the list thread-safe\n(which it currently is not, but probably doesn't need to be(?)).\nModifying the linked list is not that difficult if you assume that\nthere is only a single thread modifying the list at any given time.\n\nMichael\n"},{"id":"327348","messageId":"20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170830043147.culn63luzdsbpuuw@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T04:55:55Z","receivedAt":"2017-08-30T04:56:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 12:31:47AM -0400, Jeff King wrote:\n\n> > It was surprisingly hard trying to get that code to do the right thing,\n> > non-racily, in every error path. Since `remove_tempfiles()` can be\n> > called any time (even from a signal handler), the linked list of\n> > `tempfile` objects has to be kept valid at all times but can't use\n> > mutexes. I didn't have the energy to keep going and make the lock\n> > objects freeable.\n> > \n> > I suppose the task could be made a bit easier by using `sigprocmask(2)`\n> > or `pthread_sigmask(3)` to make its running environment a little bit\n> > less hostile.\n> \n> I think there are really two levels of carefulness here:\n> \n> 1. Avoiding complicated things during a signal handler that may rely\n> on having a sane state from the rest of the program (e.g.,\n> half-formed entries, stdio locks, etc).\n> \n> 2. Being truly race-free in the face of a signal arriving while we're\n> running arbitrary code that might have a tempfile struct in a funny\n> state.\n> \n> I feel like right now we meet (1) and not (2). But I think if we keep to\n> that lower bar of (1), it might not be that bad. We're assuming now that\n> there's no race on the tempfile->active flag, for instance. We could\n> probably make a similar assumption about putting items onto or taking\n> them off of a linked list (it's not really atomic, but a single pointer\n> assignment is probably \"atomic enough\" for our purposes).\n\nSomething like this, which AFAICT is about as safe as the existing code\nin its list manipulation. It retains the \"active\" flag as an additional\ncheck which I think isn't strictly necessary, but potentially catches\nsome logic errors.\n\nThe strbuf_reset() calls become strbuf_release(), since we're promising\nthe caller that they could now free the struct (or let it go out of\nscope) if they chose. Probably during a signal handler we should skip\nthat (we know the struct is off the list and non-active at that point,\nbut we could possibly hit libc's free() mutex).\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 6843710670..a7d964ebf8 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -69,7 +69,6 @@ static void remove_tempfiles(int skip_fclose)\n \t\t\t\ttempfile_list->fp = NULL;\n \t\t\tdelete_tempfile(tempfile_list);\n \t\t}\n-\t\ttempfile_list = tempfile_list->next;\n \t}\n }\n \n@@ -85,39 +84,54 @@ static void remove_tempfiles_on_signal(int signo)\n \traise(signo);\n }\n \n-/*\n- * Initialize *tempfile if necessary and add it to tempfile_list.\n- */\n-static void prepare_tempfile_object(struct tempfile *tempfile)\n+static void init_tempfile(struct tempfile *tempfile)\n+{\n+\ttempfile->fd = -1;\n+\ttempfile->fp = NULL;\n+\ttempfile->active = 0;\n+\ttempfile->owner = 0;\n+\ttempfile->next = NULL;\n+\tstrbuf_init(&tempfile->filename, 0);\n+}\n+\n+static void activate_tempfile(struct tempfile *tempfile)\n {\n-\tif (!tempfile_list) {\n-\t\t/* One-time initialization */\n+\tstatic volatile int initialized;\n+\n+\tif (!initialized) {\n \t\tsigchain_push_common(remove_tempfiles_on_signal);\n \t\tatexit(remove_tempfiles_on_exit);\n+\t\tinitialized = 1;\n \t}\n \n \tif (tempfile->active)\n-\t\tdie(\"BUG: prepare_tempfile_object called for active object\");\n-\tif (!tempfile->on_list) {\n-\t\t/* Initialize *tempfile and add it to tempfile_list: */\n-\t\ttempfile->fd = -1;\n-\t\ttempfile->fp = NULL;\n-\t\ttempfile->active = 0;\n-\t\ttempfile->owner = 0;\n-\t\tstrbuf_init(&tempfile->filename, 0);\n-\t\ttempfile->next = tempfile_list;\n-\t\ttempfile_list = tempfile;\n-\t\ttempfile->on_list = 1;\n-\t} else if (tempfile->filename.len) {\n-\t\t/* This shouldn't happen, but better safe than sorry. */\n-\t\tdie(\"BUG: prepare_tempfile_object called for improperly-reset object\");\n+\t\tdie(\"BUG: activate_tempfile called for active object\");\n+\n+\ttempfile->next = tempfile_list;\n+\ttempfile_list = tempfile;\n+\ttempfile->active = 1;\n+}\n+\n+static void deactivate_tempfile(struct tempfile *tempfile)\n+{\n+\tstruct tempfile *volatile *p;\n+\n+\tif (!tempfile->active)\n+\t\treturn;\n+\n+\ttempfile->active = 0;\n+\tfor (p = &tempfile_list; *p; p = &(*p)->next) {\n+\t\tif (*p == tempfile) {\n+\t\t\t*p = tempfile->next;\n+\t\t\tbreak;\n+\t\t}\n \t}\n }\n \n /* Make sure errno contains a meaningful value on error */\n int create_tempfile(struct tempfile *tempfile, const char *path)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tinit_tempfile(tempfile);\n \n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->fd = open(tempfile->filename.buf,\n@@ -127,11 +141,11 @@ int create_tempfile(struct tempfile *tempfile, const char *path)\n \t\ttempfile->fd = open(tempfile->filename.buf,\n \t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tstrbuf_release(&tempfile->filename);\n \t\treturn -1;\n \t}\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \tif (adjust_shared_perm(tempfile->filename.buf)) {\n \t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", tempfile->filename.buf);\n@@ -144,25 +158,25 @@ int create_tempfile(struct tempfile *tempfile, const char *path)\n \n void register_tempfile(struct tempfile *tempfile, const char *path)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tinit_tempfile(tempfile);\n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n }\n \n int mks_tempfile_sm(struct tempfile *tempfile,\n \t\t const char *template, int suffixlen, int mode)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tinit_tempfile(tempfile);\n \n \tstrbuf_add_absolute_path(&tempfile->filename, template);\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tstrbuf_release(&tempfile->filename);\n \t\treturn -1;\n \t}\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \treturn tempfile->fd;\n }\n \n@@ -171,7 +185,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,\n {\n \tconst char *tmpdir;\n \n-\tprepare_tempfile_object(tempfile);\n+\tinit_tempfile(tempfile);\n \n \ttmpdir = getenv(\"TMPDIR\");\n \tif (!tmpdir)\n@@ -180,11 +194,11 @@ int mks_tempfile_tsm(struct tempfile *tempfile,\n \tstrbuf_addf(&tempfile->filename, \"%s/%s\", tmpdir, template);\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tstrbuf_release(&tempfile->filename);\n \t\treturn -1;\n \t}\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \treturn tempfile->fd;\n }\n \n@@ -293,8 +307,8 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)\n \t\treturn -1;\n \t}\n \n-\ttempfile->active = 0;\n-\tstrbuf_reset(&tempfile->filename);\n+\tdeactivate_tempfile(tempfile);\n+\tstrbuf_release(&tempfile->filename);\n \treturn 0;\n }\n \n@@ -305,7 +319,7 @@ void delete_tempfile(struct tempfile *tempfile)\n \n \tif (!close_tempfile(tempfile)) {\n \t\tunlink_or_warn(tempfile->filename.buf);\n-\t\ttempfile->active = 0;\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tdeactivate_tempfile(tempfile);\n+\t\tstrbuf_release(&tempfile->filename);\n \t}\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex 2f0038decd..df96f82e84 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -85,7 +85,6 @@ struct tempfile {\n \tvolatile int fd;\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n-\tchar on_list;\n \tstruct strbuf filename;\n };\n \n"},{"id":"327349","messageId":"20170830053108.g2xsn43rwulnwn3p@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170829214809.xxpsp76eqhd4hupo@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T05:31:08Z","receivedAt":"2017-08-30T05:31:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 05:48:09PM -0400, Jeff King wrote:\n\n> On Tue, Aug 29, 2017 at 09:22:08PM +0200, Martin Ågren wrote:\n> \n> > One take-away for me from this thread is that memory-leak-plugging seems\n> > to be appreciated, i.e., it's not just an academic problem. I think I'll\n> > look into it some more, i.e., slowly pursue option 2. :-) That might help\n> > give some insights on how to identify interesting leaks.\n> \n> Yes, I think if it's tractable that's the best path forward. It's not\n> clear to me how many of the things valgrind reports are false positives\n> (and in what way). Certainly the lock_file patch I just posted seems\n> like something worth addressing, even if we _tend_ to only leak a small\n> number of such structs per program invocation. It's not a problem now,\n> but it's a sign of a bad pattern.\n\nI wanted to see what it would take to get t0000 to pass with \"definite\"\nleak-checking on, as in:\n\ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 669ebaf68b..5f01c06280 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -10,7 +10,7 @@ test-*)\n \t;;\n esac\n \n-TOOL_OPTIONS='--leak-check=no'\n+TOOL_OPTIONS='--leak-check=full --errors-for-leak-kinds=definite'\n \n test -z \"$GIT_VALGRIND_ENABLED\" &&\n exec \"$program\" \"$@\"\n\nThe results gave an interesting overview of the problems I'd expect to\nface for the rest of the tests. I started with the config lock_file fix\nI sent earlier, but we needed a few more.\n\nThis one in cmd_update_index() can become static in the same way:\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex d955cd56b3..3bff7de720 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -908,6 +908,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,\n \n int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n+\tstatic struct lock_file lock_file;\n \tint newfd, entries, has_errors = 0, nul_term_line = 0;\n \tenum uc_mode untracked_cache = UC_UNSPECIFIED;\n \tint read_from_stdin = 0;\n@@ -917,7 +918,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n-\tstruct lock_file *lock_file;\n \tstruct parse_opt_ctx_t ctx;\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n@@ -1016,11 +1016,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n \t/* we will diagnose later if it turns out that we need to update it */\n-\tnewfd = hold_locked_index(lock_file, 0);\n+\tnewfd = hold_locked_index(&lock_file, 0);\n \tif (newfd < 0)\n \t\tlock_error = errno;\n \n@@ -1155,11 +1152,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t\t}\n-\t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n \t}\n \n-\trollback_lock_file(lock_file);\n+\trollback_lock_file(&lock_file);\n \n \treturn has_errors ? 1 : 0;\n }\n\nBut there's another one in write_index_as_tree(), which is less\nobviously a candidate for turning into a single static variable. We\nmight plausibly have multiple indices. I hacked around it with:\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2440d1dc89..f2a5aa138f 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -603,7 +603,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n \tint entries, was_valid, newfd;\n-\tstruct lock_file *lock_file;\n+\tstatic struct lock_file *lock_file;\n \n \t/*\n \t * We can't free this memory, it becomes part of a linked list\n\nwhich means that if we call this function once per program we'll keep a\nsingle static pointer, and valgrind will consider it \"reachable\". But if\nwe call multiple, they really will leak as before. So it's definitely a\nhack, though it was enough for t0000. I think I'd favor loosening the\n\"you can never free locks\" requirement instead, which makes this problem\njust go away (we could even declare it as a stack variable then).\n\nNext up is the fact that setup_git_env() happily overwrites its own\npointers without freeing them if it's called twice. I expect this will\nbe a reasonably common problem for things like config or option parsing\nwhere we might see multiple values. In the simple cases, we just need to\nmake sure we do:\n\n free(foo);\n foo = xstrdup(...);\n\ninstead of just calling the latter.\n\nBut we can't always do that, because we sometimes point to string\nliterals like:\n\n static const char *foo = \"default_value\";\n ...\n foo = xstrfmt(\"computed_value: %s\", value_from_user);\n\nIf we assign to \"foo\" again, we need to free only when the previous\nvalue was from xstrfmt, not when it points to the string literal. But we\ncan't duplicate the string literal, because that default assignment is\noutside any function.\n\nWe could solve that by using a struct instead, with a few helpers. Or\npossibly we could just use strbufs, if we taught it to handle \"unowned\"\nstrings (i.e., where \"alloc\" is zero but \"len\" isn't necessarily, and we\ndon't just point to the slopbuf). And then you'd have something like:\n\n #define STRBUF_INIT_STR(x) { 0, strlen(x), (char *)(x) }\n ...\n static struct strbuf foo = STRBUF_INIT_STR(\"default_value\");\n ...\n strbuf_addf(&foo, \"computed_value...\");\n\nAnd then strbuf_grow() would have to learn to copy the existing bytes\nwhen it moves from the \"alloc==0\" case.\n\nAnyway, that actually ended up _not_ being how I solved this one. I was\npuzzled why we were calling setup_git_env() twice in the first place.\nAnd the answer is that setup_work_tree() frequently does a noop\nset_git_dir(). And we can just get rid of that:\n\ndiff --git a/setup.c b/setup.c\nindex 23950173fc..9104740e2d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -399,12 +399,9 @@ void setup_work_tree(void)\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n-\t/*\n-\t * NEEDSWORK: this call can essentially be set_git_dir(get_git_dir())\n-\t * which can cause some problems when trying to free the old value of\n-\t * gitdir.\n-\t */\n-\tset_git_dir(remove_leading_path(git_dir, work_tree));\n+\tgit_dir = remove_leading_path(git_dir, work_tree);\n+\tif (strcmp(get_git_dir(), git_dir))\n+\t\tset_git_dir(git_dir);\n \tinitialized = 1;\n }\n \n\nI think this is really papering over underlying problems, though in this\ncase they are fairly simple \"just call free()\" cases that don't need the\nstrbuf magic.\n\nNext up is update-index's add_one_path. This one is a real leak:\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex d562f2ec69..d955cd56b3 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -287,8 +287,10 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len\n \t}\n \toption = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n \toption |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n-\tif (add_cache_entry(ce, option))\n+\tif (add_cache_entry(ce, option)) {\n+\t\tfree(ce);\n \t\treturn error(\"%s: cannot add to the index - missing --add option?\", path);\n+\t}\n \treturn 0;\n }\n \n\nIt's not a big deal in practice since we exit soon, but since it's in a\nsub-function I think it's worth addressing (and the earlier error code\npath already does the same free, so I think this really is just a bug).\n\nAnd the final case I hit is in ls-files. Deep inside dir.c we allocate\nsome structures into the dir_struct. But there's no cleanup function, so\nof course we just let it go out of scope at the end of cmd_ls_files()\nand let the OS deal with it.\n\nObviously one solution is to add a cleanup function and call it at the\nend of cmd_ls_files(). But here's another trick:\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex e1339e6d17..5f2396392e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -504,7 +504,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n {\n \tint require_work_tree = 0, show_tag = 0, i;\n \tconst char *max_prefix;\n-\tstruct dir_struct dir;\n+\tstatic struct dir_struct dir;\n \tstruct exclude_list *el;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tstruct option builtin_ls_files_options[] = {\n\nBy marking it as static, it remains reachable even after the function\nexits, and valgrind doesn't complain about the leak. In a sense, this is\naccomplishing exactly what we want to say: there should be one of these\nper program run, and its lifetime is the same as that of the whole\nprogram.\n\nBut it does make me feel just a little dirty. If for some reason we\ncalled cmd_ls_files() multiple times in a single process, the current\nbehavior would be to have a small leak for each call. But the behavior\nwith the hunk above would be that the second run sees whatever cruft was\nleft in \"dir\" from the last run.\n\nOverall I suspect that running our cmd_*() functions multiple times in a\nsingle process is already going to cause chaos, because they often are\ntouching static globals, etc. So it's probably not that big a deal in\npractice to add one more variable to the list, and declare that calling\ncmd_ls_files() anywhere except as the main function of ls-files is\nforbidden.\n\n-Peff\n"},{"id":"327350","messageId":"20170830055539.xpgxeu3flmxs55av@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T05:55:39Z","receivedAt":"2017-08-30T05:55:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote:\n\n> > I feel like right now we meet (1) and not (2). But I think if we keep to\n> > that lower bar of (1), it might not be that bad. We're assuming now that\n> > there's no race on the tempfile->active flag, for instance. We could\n> > probably make a similar assumption about putting items onto or taking\n> > them off of a linked list (it's not really atomic, but a single pointer\n> > assignment is probably \"atomic enough\" for our purposes).\n> \n> Something like this, which AFAICT is about as safe as the existing code\n> in its list manipulation. It retains the \"active\" flag as an additional\n> check which I think isn't strictly necessary, but potentially catches\n> some logic errors.\n\nThe patch below demonstrates how this could be used to turn some\n\"xcalloc\" lock_files into stack variables that are allowed to go out of\nscope after we commit or rollback. This solves the three lock-related\nleaks reported by valgrind when running t0000.\n\n_But_ it also demonstrates an interesting downside of this approach.\nSome functions are lazy in their error paths. For instance, look at\nwrite_index_as_tree(). We take the lock early in the function, but may\nreturn before a commit or rollback if we hit an error. With the current\ncode this \"leaks\" the tempfile, which is wrong. But nobody notices\nbecause in practice the program exits soon after and we clean up the\ntempfile then.\n\nBut with a stack variable lock_file, that minor problem becomes a major\none: our stack variable got added to a global linked list and the\natexit() handler will try to read it. But now of course it will contain\ngarbage, since the variable went out of scope.\n\nSo it's probably safer to just let tempfile.c handle the whole lifetime,\nand have it put all live tempfiles on the heap, and free them when\nthey're deactivated. That means our creation signature becomes more\nlike:\n\n struct tempfile *create_tempfile(const char *path);\n\nand delete_tempfile() actually calls free() on it (though we'd probably\nwant to skip the free() from a signal handler for the usual reasons).\n\nI don't have a patch for that, but just food for thought while exploring\nthe implications of my earlier suggestion.\n\n---\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex d955cd56b3..bf7420b808 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -917,7 +917,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct parse_opt_ctx_t ctx;\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n@@ -1016,11 +1016,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n \t/* we will diagnose later if it turns out that we need to update it */\n-\tnewfd = hold_locked_index(lock_file, 0);\n+\tnewfd = hold_locked_index(&lock_file, 0);\n \tif (newfd < 0)\n \t\tlock_error = errno;\n \n@@ -1155,11 +1152,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t\t}\n-\t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n \t}\n \n-\trollback_lock_file(lock_file);\n+\trollback_lock_file(&lock_file);\n \n \treturn has_errors ? 1 : 0;\n }\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2440d1dc89..440f92aeca 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -603,19 +603,15 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n \tint entries, was_valid, newfd;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \n-\t/*\n-\t * We can't free this memory, it becomes part of a linked list\n-\t * parsed atexit()\n-\t */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n-\tnewfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);\n+\tnewfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n-\tif (entries < 0)\n+\tif (entries < 0) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn WRITE_TREE_UNREADABLE_INDEX;\n+\t}\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE)\n \t\tcache_tree_free(&index_state->cache_tree);\n \n@@ -624,10 +620,12 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \n \twas_valid = cache_tree_fully_valid(index_state->cache_tree);\n \tif (!was_valid) {\n-\t\tif (cache_tree_update(index_state, flags) < 0)\n+\t\tif (cache_tree_update(index_state, flags) < 0) {\n+\t\t\trollback_lock_file(&lock_file);\n \t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n+\t\t}\n \t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(index_state, lock_file, COMMIT_LOCK))\n+\t\t\tif (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))\n \t\t\t\tnewfd = -1;\n \t\t}\n \t\t/* Not being able to write is fine -- we are only interested\n@@ -641,15 +639,17 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \tif (prefix) {\n \t\tstruct cache_tree *subtree;\n \t\tsubtree = cache_tree_find(index_state->cache_tree, prefix);\n-\t\tif (!subtree)\n+\t\tif (!subtree) {\n+\t\t\trollback_lock_file(&lock_file);\n \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n+\t\t}\n \t\thashcpy(sha1, subtree->oid.hash);\n \t}\n \telse\n \t\thashcpy(sha1, index_state->cache_tree->oid.hash);\n \n \tif (0 <= newfd)\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \n \treturn 0;\n }\ndiff --git a/config.c b/config.c\nindex d0d8ce823a..7931182a54 100644\n--- a/config.c\n+++ b/config.c\n@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n {\n \tint fd = -1, in_fd = -1;\n \tint ret;\n-\tstruct lock_file *lock = NULL;\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar *filename_buf = NULL;\n \tchar *contents = NULL;\n \tsize_t contents_sz;\n@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror_errno(\"could not lock config file %s\", config_filename);\n \t\tfree(store.key);\n@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tclose(in_fd);\n \t\tin_fd = -1;\n \n-\t\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n-\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(lock));\n+\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n+\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(&lock));\n \t\t\tret = CONFIG_NO_WRITE;\n \t\t\tgoto out_free;\n \t\t}\n@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tcontents = NULL;\n \t}\n \n-\tif (commit_lock_file(lock) < 0) {\n+\tif (commit_lock_file(&lock) < 0) {\n \t\terror_errno(\"could not write config file %s\", config_filename);\n \t\tret = CONFIG_NO_WRITE;\n-\t\tlock = NULL;\n \t\tgoto out_free;\n \t}\n \n-\t/*\n-\t * lock is committed, so don't try to roll it back below.\n-\t * NOTE: Since lockfile.c keeps a linked list of all created\n-\t * lock_file structures, it isn't safe to free(lock). It's\n-\t * better to just leave it hanging around.\n-\t */\n-\tlock = NULL;\n \tret = 0;\n \n \t/* Invalidate the config cache */\n \tgit_config_clear();\n \n out_free:\n-\tif (lock)\n-\t\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \tfree(filename_buf);\n \tif (contents)\n \t\tmunmap(contents, contents_sz);\n@@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(get_lock_file_path(lock));\n+\tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n \n }\ndiff --git a/lockfile.h b/lockfile.h\nindex 572064939c..a7ba42bdf2 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -114,6 +114,8 @@ struct lock_file {\n \tstruct tempfile tempfile;\n };\n \n+#define LOCK_INIT { { NULL } }\n+\n /* String appended to a filename to derive the lockfile name: */\n #define LOCK_SUFFIX \".lock\"\n #define LOCK_SUFFIX_LEN 5\n"},{"id":"327357","messageId":"3e632fd3-7db9-4c38-c524-b56a06cfaa87@alum.mit.edu","threadId":"46675","inReplyTo":"20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T06:55:01Z","receivedAt":"2017-08-30T06:55:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/30/2017 06:55 AM, Jeff King wrote:\n> [...]\n> Something like this, which AFAICT is about as safe as the existing code\n> in its list manipulation. It retains the \"active\" flag as an additional\n> check which I think isn't strictly necessary, but potentially catches\n> some logic errors.\n> \n> The strbuf_reset() calls become strbuf_release(), since we're promising\n> the caller that they could now free the struct (or let it go out of\n> scope) if they chose. Probably during a signal handler we should skip\n> that (we know the struct is off the list and non-active at that point,\n> but we could possibly hit libc's free() mutex).\n\nThis looks OK to me aside from the one caveat below.\n\n> diff --git a/tempfile.c b/tempfile.c\n> index 6843710670..a7d964ebf8 100644\n> --- a/tempfile.c\n> +++ b/tempfile.c\n> [...]\n> +static void deactivate_tempfile(struct tempfile *tempfile)\n> +{\n> +\tstruct tempfile *volatile *p;\n> +\n> +\tif (!tempfile->active)\n> +\t\treturn;\n> +\n> +\ttempfile->active = 0;\n> +\tfor (p = &tempfile_list; *p; p = &(*p)->next) {\n> +\t\tif (*p == tempfile) {\n> +\t\t\t*p = tempfile->next;\n> +\t\t\tbreak;\n> +\t\t}\n> \t}\n> }\n\n`deactivate_tempfile()` is O(N) in the number of active tempfiles. This\ncould get noticeable for, say, updating 30k references, which involves\nobtaining 30k reference locks. I think that code adds the locks in\nalphabetical order and also removes them in alphabetical order, so the\noverall effort would go like O(N²). I'm guessing that this would be\nmeasurable but not fatal for realistic numbers of references, but it\nshould at least be benchmarked.\n\nThere are three obvious ways to make this O(1) again:\n\n* Make the list doubly-linked.\n* Make sure that all significant callers remove items in the *reverse*\norder that they were added, in which case the item to be removed would\nalways be found near the head of the list.\n* Change this class to keep track of a tail pointer and add new items at\nthe end of the list, and ensure that all significant callers remove\nlocks in the *same* order that they were added.\n\nThe second and third options might be easy or difficult (depend on how\nmuch current callers need to be changed) and certainly would be easy to\nbreak again as new callers are added in the future.\n\n> [...]\n\nMichael\n"},{"id":"327377","messageId":"8e09e4e1-984c-78d1-7b87-0bafe5346621@alum.mit.edu","threadId":"46675","inReplyTo":"20170830055539.xpgxeu3flmxs55av@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T07:07:53Z","receivedAt":"2017-08-30T07:08:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/30/2017 07:55 AM, Jeff King wrote:\n> On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote:\n> [...]\n> The patch below demonstrates how this could be used to turn some\n> \"xcalloc\" lock_files into stack variables that are allowed to go out of\n> scope after we commit or rollback. This solves the three lock-related\n> leaks reported by valgrind when running t0000.\n> \n> _But_ it also demonstrates an interesting downside of this approach.\n> Some functions are lazy in their error paths. For instance, look at\n> write_index_as_tree(). We take the lock early in the function, but may\n> return before a commit or rollback if we hit an error. With the current\n> code this \"leaks\" the tempfile, which is wrong. But nobody notices\n> because in practice the program exits soon after and we clean up the\n> tempfile then.\n> \n> But with a stack variable lock_file, that minor problem becomes a major\n> one: our stack variable got added to a global linked list and the\n> atexit() handler will try to read it. But now of course it will contain\n> garbage, since the variable went out of scope.\n> \n> So it's probably safer to just let tempfile.c handle the whole lifetime,\n> and have it put all live tempfiles on the heap, and free them when\n> they're deactivated. That means our creation signature becomes more\n> like:\n> \n> struct tempfile *create_tempfile(const char *path);\n> \n> and delete_tempfile() actually calls free() on it (though we'd probably\n> want to skip the free() from a signal handler for the usual reasons).\n\nI agree that the latter would be a nice, and relatively safe, design. It\nwould involve some fairly intrusive changes to client code, though.\n\nI think it would be possible to implement the new API while leaving the\nold one intact, to avoid having to rewrite all clients at once, and\npotentially to allow clients to avoid a malloc if they already have a\nconvenient place to embed a `struct tempfile` (except that now they'd be\nable to free it when done). For example, `create_tempfile(tempfile,\npath)` and its friends could accept NULL as the first argument, in which\ncase it would malloc a `struct tempfile` itself, and mark it as being\nowned by the tempfile module. Such objects would be freed when\ndeactivated. But if the caller passes in a non-NULL `tempfile` argument,\nthe old behavior would be retained.\n\nMichael\n"},{"id":"327462","messageId":"20170830195320.27w5mhnrcd2uosvz@sigill.intra.peff.net","threadId":"46675","inReplyTo":"3e632fd3-7db9-4c38-c524-b56a06cfaa87@alum.mit.edu","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T19:53:20Z","receivedAt":"2017-08-30T19:53:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:\n\n> > +\ttempfile->active = 0;\n> > +\tfor (p = &tempfile_list; *p; p = &(*p)->next) {\n> > +\t\tif (*p == tempfile) {\n> > +\t\t\t*p = tempfile->next;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > \t}\n> > }\n> \n> `deactivate_tempfile()` is O(N) in the number of active tempfiles. This\n> could get noticeable for, say, updating 30k references, which involves\n> obtaining 30k reference locks. I think that code adds the locks in\n> alphabetical order and also removes them in alphabetical order, so the\n> overall effort would go like O(N²). I'm guessing that this would be\n> measurable but not fatal for realistic numbers of references, but it\n> should at least be benchmarked.\n> \n> There are three obvious ways to make this O(1) again:\n> \n> * Make the list doubly-linked.\n\nYeah, I noticed this when writing it, and the doubly-linked list was my\nfirst thought. It's much more straight-forward than making guesses about\ntraversal order, and we already have a nice implementation in list.h.\n\nWhat made me hesitate for this demonstration was that it turns the\nremoval into _two_ pointer updates. That made me more nervous about the\nracy case where we get a single handler while already deleting\nsomething. Specifically when we have \"a <-> b <-> c\" and we've updated\n\"a->next\" to point to \"c\" but \"c->prev\" still points to \"b\".\n\nBut even with the singly-linked list we're not fully race-proof\n(somebody could set *p to NULL between the time we look at it and\ndereference it). What we really care about is not two versions of the\nfunction running simultaneously, but one version getting interrupted by\nanother and having the second one see a sane state (because we'll never\nreturn to the first signal handler; the second one will raise() and\ndie).\n\nAnd I think we're fine there even with a doubly-linked list. It's still\nthe single update of the \"next\" pointer that controls that second\ntraversal.\n\n-Peff\n"},{"id":"327463","messageId":"20170830195731.GB50018@google.com","threadId":"46675","inReplyTo":"20170830195320.27w5mhnrcd2uosvz@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-30T19:57:31Z","receivedAt":"2017-08-30T19:57:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/30, Jeff King wrote:\n> On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:\n> \n> > > +\ttempfile->active = 0;\n> > > +\tfor (p = &tempfile_list; *p; p = &(*p)->next) {\n> > > +\t\tif (*p == tempfile) {\n> > > +\t\t\t*p = tempfile->next;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > \t}\n> > > }\n> > \n> > `deactivate_tempfile()` is O(N) in the number of active tempfiles. This\n> > could get noticeable for, say, updating 30k references, which involves\n> > obtaining 30k reference locks. I think that code adds the locks in\n> > alphabetical order and also removes them in alphabetical order, so the\n> > overall effort would go like O(N²). I'm guessing that this would be\n> > measurable but not fatal for realistic numbers of references, but it\n> > should at least be benchmarked.\n> > \n> > There are three obvious ways to make this O(1) again:\n> > \n> > * Make the list doubly-linked.\n> \n> Yeah, I noticed this when writing it, and the doubly-linked list was my\n> first thought. It's much more straight-forward than making guesses about\n> traversal order, and we already have a nice implementation in list.h.\n\nI agree that a doubly-linked list is probably the best way to go in\norder to solve the performance issue.\n\n> \n> What made me hesitate for this demonstration was that it turns the\n> removal into _two_ pointer updates. That made me more nervous about the\n> racy case where we get a single handler while already deleting\n> something. Specifically when we have \"a <-> b <-> c\" and we've updated\n> \"a->next\" to point to \"c\" but \"c->prev\" still points to \"b\".\n> \n> But even with the singly-linked list we're not fully race-proof\n> (somebody could set *p to NULL between the time we look at it and\n> dereference it). What we really care about is not two versions of the\n> function running simultaneously, but one version getting interrupted by\n> another and having the second one see a sane state (because we'll never\n> return to the first signal handler; the second one will raise() and\n> die).\n> \n> And I think we're fine there even with a doubly-linked list. It's still\n> the single update of the \"next\" pointer that controls that second\n> traversal.\n> \n> -Peff\n\nI know it was mentioned earlier but if this is a critical section, and\nit would be bad if it was interrupted, then couldn't we turn off\ninterrupts before attempting to remove an item from the list?\n\n-- \nBrandon Williams\n"},{"id":"327464","messageId":"20170830201132.kmtp5mfu6qotn5s7@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170830195731.GB50018@google.com","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T20:11:32Z","receivedAt":"2017-08-30T20:11:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:\n\n> > And I think we're fine there even with a doubly-linked list. It's still\n> > the single update of the \"next\" pointer that controls that second\n> > traversal.\n> \n> I know it was mentioned earlier but if this is a critical section, and\n> it would be bad if it was interrupted, then couldn't we turn off\n> interrupts before attempting to remove an item from the list?\n\nIf we have to, that's an option. I mostly didn't want to get into the\nportability headaches of signal-handling if we can avoid it.\n\nRight now sigchain uses plain old signal(). We do use sigaction() in a\nfew places, but in a vanilla way. So the portability questions are:\n\n - can we rely on using more interesting bits of sigaction like\n sa_mask? Probably.\n\n - which flags should we be setting in sa_flags to get the same\n behavior we've been getting with signal()? Or alternatively, which\n behavior do we want?\n\nOn Linux and BSD systems, at least, signal() defers repeat instances of\nthe handled signal. So getting two quick SIGTERMs will not interrupt the\nhandler to deliver the second. But getting SIGTERM followed by SIGINT would.\n\nWe could extend that protection by having sigchain_push_common() set\nsa_mask to cover all of the related signals. On Linux and BSD the\ncurrent code using signal() also implies SA_RESTART. We could add that\nto our flags, though I suspect in practice it doesn't matter. Whenever\nwe establish a handler like this our intent is to never return from it.\n\nThat just protects us from calling the _same_ handler from itself. But\nthat's probably enough in practice, and means handlers don't have to\nworry about \"critical sections\". The other alternative is sigprocmask()\nto block signals entirely during a section. I'm not sure if there are\nportability questions there (it looks like we have a mingw wrapper\nthere, but it's a complete noop).\n\n-Peff\n"},{"id":"327469","messageId":"20170830210602.GC50018@google.com","threadId":"46675","inReplyTo":"20170830201132.kmtp5mfu6qotn5s7@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-30T21:06:02Z","receivedAt":"2017-08-30T21:06:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/30, Jeff King wrote:\n> On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:\n> \n> > > And I think we're fine there even with a doubly-linked list. It's still\n> > > the single update of the \"next\" pointer that controls that second\n> > > traversal.\n> > \n> > I know it was mentioned earlier but if this is a critical section, and\n> > it would be bad if it was interrupted, then couldn't we turn off\n> > interrupts before attempting to remove an item from the list?\n> \n> If we have to, that's an option. I mostly didn't want to get into the\n> portability headaches of signal-handling if we can avoid it.\n> \n> Right now sigchain uses plain old signal(). We do use sigaction() in a\n> few places, but in a vanilla way. So the portability questions are:\n> \n> - can we rely on using more interesting bits of sigaction like\n> sa_mask? Probably.\n> \n> - which flags should we be setting in sa_flags to get the same\n> behavior we've been getting with signal()? Or alternatively, which\n> behavior do we want?\n> \n> On Linux and BSD systems, at least, signal() defers repeat instances of\n> the handled signal. So getting two quick SIGTERMs will not interrupt the\n> handler to deliver the second. But getting SIGTERM followed by SIGINT would.\n> \n> We could extend that protection by having sigchain_push_common() set\n> sa_mask to cover all of the related signals. On Linux and BSD the\n> current code using signal() also implies SA_RESTART. We could add that\n> to our flags, though I suspect in practice it doesn't matter. Whenever\n> we establish a handler like this our intent is to never return from it.\n> \n> That just protects us from calling the _same_ handler from itself. But\n> that's probably enough in practice, and means handlers don't have to\n> worry about \"critical sections\". The other alternative is sigprocmask()\n> to block signals entirely during a section. I'm not sure if there are\n> portability questions there (it looks like we have a mingw wrapper\n> there, but it's a complete noop).\n\nYeah there's a lot about signals that I'm not very clear on. I do know\nthat Eric helped me out on the fork-exec series I worked on earlier in\nthe year and I believe it was to turn on/off signals during process\nlaunches in 45afb1ca9 (run-command: block signals between fork and\nexecve, 2017-04-19). Though that bit of code is strictly for unix so I\nwouldn't know how that would work on windows machines. Portability does\nseem to always be a challenging problem.\n\n-- \nBrandon Williams\n"},{"id":"327476","messageId":"20170831040907.a4qpydb3w2aghatm@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170830210602.GC50018@google.com","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-31T04:09:07Z","receivedAt":"2017-08-31T04:09:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 02:06:02PM -0700, Brandon Williams wrote:\n\n> > We could extend that protection by having sigchain_push_common() set\n> > sa_mask to cover all of the related signals. On Linux and BSD the\n> > current code using signal() also implies SA_RESTART. We could add that\n> > to our flags, though I suspect in practice it doesn't matter. Whenever\n> > we establish a handler like this our intent is to never return from it.\n> > \n> > That just protects us from calling the _same_ handler from itself. But\n> > that's probably enough in practice, and means handlers don't have to\n> > worry about \"critical sections\". The other alternative is sigprocmask()\n> > to block signals entirely during a section. I'm not sure if there are\n> > portability questions there (it looks like we have a mingw wrapper\n> > there, but it's a complete noop).\n> \n> Yeah there's a lot about signals that I'm not very clear on. I do know\n> that Eric helped me out on the fork-exec series I worked on earlier in\n> the year and I believe it was to turn on/off signals during process\n> launches in 45afb1ca9 (run-command: block signals between fork and\n> execve, 2017-04-19). Though that bit of code is strictly for unix so I\n> wouldn't know how that would work on windows machines. Portability does\n> seem to always be a challenging problem.\n\nBased on the sketch I wrote above, I figured it would be pretty easy to\nconvert sigchain to sigaction. But after taking a look at\ncompat/mingw.c, I don't think Windows would be on board. sigaction()\nthere is is a stub implementation that _only_ handles SIGALRM and\nnothing else.\n\nSo I think the best we could do is put big #ifdefs around it to use\nsigaction on other platforms, and fall back to signal() on Windows.\nThat's do-able, but my enthusiasm is waning as the complexity increases.\nGetting two SIGINTs in a row seems plausible, but we already handle that\nwell. Getting SIGINT and then a _different_ signal while we're in the\nhandler seems less likely in practice. The only combination I can think\nthat would be common is TERM+KILL, but of course we're not catching KILL\nin the first place.\n\nSo this seems like adding complexity for very little benefit.\n\n-Peff\n"},{"id":"327528","messageId":"20170902084451.h5pmbtolqcl2j3w4@sigill.intra.peff.net","threadId":"46675","inReplyTo":"8e09e4e1-984c-78d1-7b87-0bafe5346621@alum.mit.edu","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-02T08:44:51Z","receivedAt":"2017-09-02T08:45:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 09:07:53AM +0200, Michael Haggerty wrote:\n\n> > So it's probably safer to just let tempfile.c handle the whole lifetime,\n> > and have it put all live tempfiles on the heap, and free them when\n> > they're deactivated. That means our creation signature becomes more\n> > like:\n> > \n> > struct tempfile *create_tempfile(const char *path);\n> > \n> > and delete_tempfile() actually calls free() on it (though we'd probably\n> > want to skip the free() from a signal handler for the usual reasons).\n> \n> I agree that the latter would be a nice, and relatively safe, design. It\n> would involve some fairly intrusive changes to client code, though.\n>\n> I think it would be possible to implement the new API while leaving the\n> old one intact, to avoid having to rewrite all clients at once, and\n> potentially to allow clients to avoid a malloc if they already have a\n> convenient place to embed a `struct tempfile` (except that now they'd be\n> able to free it when done). For example, `create_tempfile(tempfile,\n> path)` and its friends could accept NULL as the first argument, in which\n> case it would malloc a `struct tempfile` itself, and mark it as being\n> owned by the tempfile module. Such objects would be freed when\n> deactivated. But if the caller passes in a non-NULL `tempfile` argument,\n> the old behavior would be retained.\n\nThere are actually very few callers of create_tempfile (I count two).\nEverybody just uses lock_file(). So I think we could get away with just\nmodifying the internals of the lock struct to hold a pointer, and then\nteaching commit_lock_file() et al to reset it to NULL after performing\nan operation that frees the tempfile. We could even modify\nrename_tempfile() and friends to take a pointer-to-pointer and NULL it\nitself. That would make it harder to get wrong.\n\nIn the long term I don't think there's a huge value to being able to\nplace a \"struct tempfile\" inside another struct, as opposed to holding a\npointer. It saves a malloc, but at the cost of opening up confusion\nabout lifetimes.\n\n-Peff\n"},{"id":"327530","messageId":"20170902135015.2hf4b3o4xivdd5x2@sigill.intra.peff.net","threadId":"46675","inReplyTo":"20170902084451.h5pmbtolqcl2j3w4@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-02T13:50:15Z","receivedAt":"2017-09-02T13:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2017 at 04:44:51AM -0400, Jeff King wrote:\n\n> There are actually very few callers of create_tempfile (I count two).\n> Everybody just uses lock_file(). So I think we could get away with just\n> modifying the internals of the lock struct to hold a pointer, and then\n> teaching commit_lock_file() et al to reset it to NULL after performing\n> an operation that frees the tempfile. We could even modify\n> rename_tempfile() and friends to take a pointer-to-pointer and NULL it\n> itself. That would make it harder to get wrong.\n\nI take it back. It's true that there are very few create_tempfile()\ncallers, but we'd want to have a similar change for mks_tempfile_*, and\nthere are a lot more of those.\n\nOne solution would be to add an extra level of indirection, like:\n\n struct tempfile {\n\tstruct the_part_that_we_put_on_our_global_list *data;\n };\n\nThat lets us keep normal value semantics for \"struct tempfile\", which is\nnice. And existing callers wouldn't have to be modified at all for the\ncreation/deletion bits. But I suspect it would also result in a lot of\nreplacements like s/temp->/temp.data->/ for any callers that look at the\nunderlying fields that have moved into the sub-struct.\n\n-Peff\n"},{"id":"327545","messageId":"xmqqy3ptbgyo.fsf@gitster.mtv.corp.google.com","threadId":"46675","inReplyTo":"20170828175855.vs2s7lpxxhwlfhdi@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T10:03:11Z","receivedAt":"2017-09-05T10:03:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 28, 2017 at 10:52:51AM -0700, Stefan Beller wrote:\n>\n>> >> I'm curious, too. I don't think the valgrind setup in our test suite is\n>> >> great for finding leaks right now.\n>> \n>> This discussion comes up every once in a while,\n>> the last time I was involved in this discussion I proposed\n>> to have an \"optional_free(void *)\", which only frees memory\n>> in e.g. the developer build/debug build.\n>> \n>> That way we can have a strict \"no leaks in developer build\"\n>> policy (as it is easy to detect!), but it doesn't slow down the\n>> widely distributed version of Git.\n>\n> Personally I am not that worried about slowing down program-exit with\n> some free() calls (though I would reserve judgement to see how slow it\n> actually is).\n>\n> It is that I do not want to deal with the complexity and maintenance\n> headache of dropping values which are program-lifetime caches. If we\n> introduce a free-all-object-structs function, now everybody needs to\n> remember to call it (even if they didn't realize they were allocating\n> object structs in the first place, as it may have happened under the\n> hood of a sub-function). And we have to wonder what might happen when\n> somebody calls that function _not_ at program exit.\n\nIn addition, the code earlier may have a variable point at a\ncompiled in literal string or an allocated string depending on the\nsituation and it would have been perfectly fine to rely on exit() to\nclean it up. \"We free all ourselves before exit()\" would mean these\ncodepaths now need to be updated to keep track of what needs to be\nand what must not be freed, or just duplicate everything to make the\nlife of that \"free everything\" part easier, which somehow feels like\na tail wagging a dog.\n\n> If we can declare \"reachable things are not leaks\" (and certainly\n> valgrind is aware of that distinction), then all of that goes away. And\n> you're just left with local variables in main() (or our cmd_*\n> equivalents) that appear as leaks. And then we can solve that either by\n> freeing them, or just calling them program-lifetime and telling the\n> compiler that it's so by declaring them \"static\".\n\nYup.\n"},{"id":"327546","messageId":"xmqqr2vlbgyk.fsf@gitster.mtv.corp.google.com","threadId":"46675","inReplyTo":"20170830053108.g2xsn43rwulnwn3p@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T10:03:15Z","receivedAt":"2017-09-05T10:03:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Overall I suspect that running our cmd_*() functions multiple times in a\n> single process is already going to cause chaos, because they often are\n> touching static globals, etc. So it's probably not that big a deal in\n> practice to add one more variable to the list, and declare that calling\n> cmd_ls_files() anywhere except as the main function of ls-files is\n> forbidden.\n\nIIRC there were a few existing violators of this rule around \"merge\"\nimplementation. It may be a reasonably low hanging fruit to find\nand fix them.\n\n#leftoverbits\n"},{"id":"327632","messageId":"xmqqmv6879zh.fsf@gitster.mtv.corp.google.com","threadId":"46675","inReplyTo":"20170829185850.tfmjoa5u5sfuwpgi@sigill.intra.peff.net","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T03:59:46Z","receivedAt":"2017-09-06T03:59:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In the long run we may want to drop the \"tempfiles must remain forever\"\n> rule. This is certainly not the first time it has caused confusion or\n> leaks. And I don't think it's a fundamental issue, just the way the code\n> is written. But in the interim, this fix is probably worth doing.\n\nI notice you also have a series to rework the \"must remain forever\"\non the list, but nevertheless let's take this one first.\n\nWill queue.\n\nThanks.\n\n"},{"id":"327649","messageId":"20170906124153.d2kmtes7wmne7xbi@sigill.intra.peff.net","threadId":"46675","inReplyTo":"xmqqmv6879zh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config: use a static lock_file struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T12:41:53Z","receivedAt":"2017-09-06T12:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2017 at 12:59:46PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In the long run we may want to drop the \"tempfiles must remain forever\"\n> > rule. This is certainly not the first time it has caused confusion or\n> > leaks. And I don't think it's a fundamental issue, just the way the code\n> > is written. But in the interim, this fix is probably worth doing.\n> \n> I notice you also have a series to rework the \"must remain forever\"\n> on the list, but nevertheless let's take this one first.\n> \n> Will queue.\n\nYes, this will conflict with that series (but the resolution should just\nbe to throw this one away). I'm fine with taking this in the meantime,\nthough it's certainly not urgent (my main goal was getting the pre-test\nbits of test-lib.sh to run with leak detection on; I don't think this\nwas actually affecting anybody in the real world).\n\n-Peff\n"},{"id":"330089","messageId":"20171010040604.26029-1-gitster@pobox.com","threadId":"46675","inReplyTo":"xmqqr2vlbgyk.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] Do not call cmd_*() as a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T04:06:02Z","receivedAt":"2017-10-10T04:06:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These two patches are for a recent #leftoverbits topic.\n\nhttps://public-inbox.org/git/xmqqr2vlbgyk.fsf@gitster.mtv.corp.google.com/\n\nThe cmd_foo() function is a moral equivalent of 'main' for a Git\nsubcommand 'git foo', and as such, it is allowed to do many things\nthat make it unsuitable to be called as a subroutine, including\n\n - call exit(3) to terminate the process;\n\n - allocate resource held and used throughout its lifetime, without\n releasing it upon return/exit;\n\n - rely on global variables being initialized at program startup,\n and update them as needed, making another clean invocation of the\n function impossible.\n\nCorrecting two callers by using helper API calls is not so hard.\n\nJunio C Hamano (2):\n describe: do not use cmd_*() as a subroutine\n merge-ours: do not use cmd_*() as a subroutine\n\n builtin/describe.c | 15 +++++++++++----\n builtin/merge-ours.c | 16 +++++++---------\n 2 files changed, 18 insertions(+), 13 deletions(-)\n\n-- \n2.15.0-rc0-203-g4c8d0e28b1\n\n"},{"id":"330090","messageId":"20171010040604.26029-2-gitster@pobox.com","threadId":"46675","inReplyTo":"20171010040604.26029-1-gitster@pobox.com","subject":"[PATCH 1/2] describe: do not use cmd_*() as a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T04:06:03Z","receivedAt":"2017-10-10T04:06:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The cmd_foo() function is a moral equivalent of 'main' for a Git\nsubcommand 'git foo', and as such, it is allowed to do many things\nthat make it unsuitable to be called as a subroutine, including\n\n - call exit(3) to terminate the process;\n\n - allocate resource held and used throughout its lifetime, without\n releasing it upon return/exit;\n\n - rely on global variables being initialized at program startup,\n and update them as needed, making another clean invocation of the\n function impossible.\n\nThe call to cmd_diff_index() \"git describe\" makes has been working\nby accident that the function did not call exit(3); it sets a bad\nprecedent for people to cut and paste.\n\nWe could invoke it via the run_command() interface, but the diff\nfamily of commands have helper functions in diff-lib.c that are\nmeant to be usable as subroutines, and using the latter does not\nmake the resulting code all that longer. Use it.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/describe.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 89ea1cdd60..50263759cb 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -7,12 +7,12 @@\n #include \"builtin.h\"\n #include \"exec_cmd.h\"\n #include \"parse-options.h\"\n+#include \"revision.h\"\n #include \"diff.h\"\n #include \"hashmap.h\"\n #include \"argv-array.h\"\n #include \"run-command.h\"\n \n-#define SEEN\t\t(1u << 0)\n #define MAX_TAGS\t(FLAG_BITS - 1)\n \n static const char * const describe_usage[] = {\n@@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t} else if (dirty) {\n \t\t\tstatic struct lock_file index_lock;\n-\t\t\tint fd;\n+\t\t\tstruct rev_info revs;\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\tint fd, result;\n \n \t\t\tread_cache_preload(NULL);\n \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n@@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tif (0 <= fd)\n \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n \n-\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n-\t\t\t\t\t diff_index_args, prefix))\n+\t\t\tinit_revisions(&revs, prefix);\n+\t\t\targv_array_pushv(&args, diff_index_args);\n+\t\t\tif (setup_revisions(args.argc, args.argv, &revs, NULL) != 1)\n+\t\t\t\tBUG(\"malformed internal diff-index command line\");\n+\t\t\tresult = run_diff_index(&revs, 0);\n+\n+\t\t\tif (!diff_result_code(&revs.diffopt, result))\n \t\t\t\tsuffix = NULL;\n \t\t\telse\n \t\t\t\tsuffix = dirty;\n-- \n2.15.0-rc0-203-g4c8d0e28b1\n\n"},{"id":"330091","messageId":"20171010040604.26029-3-gitster@pobox.com","threadId":"46675","inReplyTo":"20171010040604.26029-1-gitster@pobox.com","subject":"[PATCH 2/2] merge-ours: do not use cmd_*() as a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T04:06:04Z","receivedAt":"2017-10-10T04:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The call to cmd_diff_index() \"git merge-ours\" makes has been working\nby accident that the function did not call exit(3), and the caller\nexited almost immediately after making a call, but it sets a bad\nprecedent for people to cut and paste.\n\nFor finding out if the index exactly matches the HEAD (or a given\ntree-ish), there is index_differs_from() which is exactly written\nfor that purpose.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/merge-ours.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/merge-ours.c b/builtin/merge-ours.c\nindex 684411694f..beb0623d56 100644\n--- a/builtin/merge-ours.c\n+++ b/builtin/merge-ours.c\n@@ -9,26 +9,24 @@\n */\n #include \"git-compat-util.h\"\n #include \"builtin.h\"\n+#include \"diff.h\"\n \n static const char builtin_merge_ours_usage[] =\n \t\"git merge-ours ... -- HEAD ...\";\n \n-static const char *diff_index_args[] = {\n-\t\"diff-index\", \"--quiet\", \"--cached\", \"HEAD\", \"--\", NULL\n-};\n-#define NARGS (ARRAY_SIZE(diff_index_args) - 1)\n-\n int cmd_merge_ours(int argc, const char **argv, const char *prefix)\n {\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_merge_ours_usage);\n \n \t/*\n-\t * We need to exit with 2 if the index does not match our HEAD tree,\n-\t * because the current index is what we will be committing as the\n-\t * merge result.\n+\t * The contents of the current index becomes the tree we\n+\t * commit. The index must match HEAD, or this merge cannot go\n+\t * through.\n \t */\n-\tif (cmd_diff_index(NARGS, diff_index_args, prefix))\n+\tif (read_cache() < 0)\n+\t\tdie_errno(\"read_cache failed\");\n+\tif (index_differs_from(\"HEAD\", 0, 0))\n \t\texit(2);\n \texit(0);\n }\n-- \n2.15.0-rc0-203-g4c8d0e28b1\n\n"},{"id":"330116","messageId":"20171010134309.13803-1-szeder.dev@gmail.com","threadId":"46675","inReplyTo":"20171010040604.26029-2-gitster@pobox.com","subject":"Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-10-10T13:43:09Z","receivedAt":"2017-10-10T13:43:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> The cmd_foo() function is a moral equivalent of 'main' for a Git\n> subcommand 'git foo', and as such, it is allowed to do many things\n> that make it unsuitable to be called as a subroutine, including\n> \n> - call exit(3) to terminate the process;\n> \n> - allocate resource held and used throughout its lifetime, without\n> releasing it upon return/exit;\n> \n> - rely on global variables being initialized at program startup,\n> and update them as needed, making another clean invocation of the\n> function impossible.\n> \n> The call to cmd_diff_index() \"git describe\" makes has been working\n> by accident that the function did not call exit(3); it sets a bad\n> precedent for people to cut and paste.\n\nThe subject implies to me that this patch eliminates all cmd_*() calls\nin builtin/describe.c, but a call to cmd_name_rev() still remains.\n\nHowever, that call is special in the sense that cmd_describe() returns\nimmediately thereafter, so none of the above three points are an issue\nthere. Someone might argue that it still sets a bad precedent, but I\nwon't :) To avoid the direct cmd_name_rev() call we would have to use\nrun_command(), because there are no libified helper functions\navailable to do the job, adding the overhead of a fork()+exec(),\nthough only once or nonce, depending on cmdline options.\n\nMaybe you already considered all this WRT that cmd_name_rev() call, I\ndon't know. In any case, I think at least the subject line should\nspell out cmd_diff_index().\n\n\nGábor\n\n\n> We could invoke it via the run_command() interface, but the diff\n> family of commands have helper functions in diff-lib.c that are\n> meant to be usable as subroutines, and using the latter does not\n> make the resulting code all that longer. Use it.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/describe.c | 15 +++++++++++----\n> 1 file changed, 11 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 89ea1cdd60..50263759cb 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -7,12 +7,12 @@\n> #include \"builtin.h\"\n> #include \"exec_cmd.h\"\n> #include \"parse-options.h\"\n> +#include \"revision.h\"\n> #include \"diff.h\"\n> #include \"hashmap.h\"\n> #include \"argv-array.h\"\n> #include \"run-command.h\"\n> \n> -#define SEEN\t\t(1u << 0)\n> #define MAX_TAGS\t(FLAG_BITS - 1)\n> \n> static const char * const describe_usage[] = {\n> @@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\t}\n> \t\t} else if (dirty) {\n> \t\t\tstatic struct lock_file index_lock;\n> -\t\t\tint fd;\n> +\t\t\tstruct rev_info revs;\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\tint fd, result;\n> \n> \t\t\tread_cache_preload(NULL);\n> \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n> @@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\tif (0 <= fd)\n> \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n> \n> -\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n> -\t\t\t\t\t diff_index_args, prefix))\n> +\t\t\tinit_revisions(&revs, prefix);\n> +\t\t\targv_array_pushv(&args, diff_index_args);\n> +\t\t\tif (setup_revisions(args.argc, args.argv, &revs, NULL) != 1)\n> +\t\t\t\tBUG(\"malformed internal diff-index command line\");\n> +\t\t\tresult = run_diff_index(&revs, 0);\n> +\n> +\t\t\tif (!diff_result_code(&revs.diffopt, result))\n> \t\t\t\tsuffix = NULL;\n> \t\t\telse\n> \t\t\t\tsuffix = dirty;\n> -- \n> 2.15.0-rc0-203-g4c8d0e28b1\n"},{"id":"330152","messageId":"xmqqr2uamdfe.fsf@gitster.mtv.corp.google.com","threadId":"46675","inReplyTo":"20171010134309.13803-1-szeder.dev@gmail.com","subject":"Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-11T06:00:37Z","receivedAt":"2017-10-11T06:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Maybe you already considered all this WRT that cmd_name_rev() call, I\n> don't know. In any case, I think at least the subject line should\n> spell out cmd_diff_index().\n\nPerhaps. The intent was to eradicate cmd_*() used as a subroutine,\nso I'd rather opt to explain why name_rev() that does not return is\nOK in this case in the log message without changing anything else.\n\nThanks for looking it over.\n"}]}