{"thread":{"id":"59094","subject":"[PATCH] format-patch: unleak \"-v \"","startedAt":"2023-01-15T08:03:46Z","lastAt":"2023-01-16T19:51:18Z","messageCount":4,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470380","messageId":"xmqqv8l8gr6s.fsf@gitster.g","threadId":"59094","inReplyTo":null,"subject":"[PATCH] format-patch: unleak \"-v \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T08:03:39Z","receivedAt":"2023-01-15T08:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"subject_prefix\" member of \"struct revision\" usually is always\nset to a borrowed string (either a string literal like \"PATCH\" that\nappear in the program text as a hardcoded default, or the value of\n\"format.subjectprefix\") and is never freed when the containing\nrevision structure is released. The \"-v \" codepath however\nviolates this rule and stores a pointer to an allocated string to\nthis member, relinquishing the responsibility to free it when it is\ndone using the revision structure, leading to a small one-time leak.\n\nInstead, keep track of the string it allocates to let the revision\nstructure borrow, and clean it up when it is done.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/log.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e1fe7bbe71..3dddaadc1e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1879,6 +1879,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct strbuf rdiff1 = STRBUF_INIT;\n \tstruct strbuf rdiff2 = STRBUF_INIT;\n \tstruct strbuf rdiff_title = STRBUF_INIT;\n+\tstruct strbuf sprefix = STRBUF_INIT;\n \tint creation_factor = -1;\n \n \tconst struct option builtin_format_patch_options[] = {\n@@ -2019,12 +2020,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tcover_from_description_mode = parse_cover_from_description(cover_from_description_arg);\n \n \tif (reroll_count) {\n-\t\tstruct strbuf sprefix = STRBUF_INIT;\n-\n \t\tstrbuf_addf(&sprefix, \"%s v%s\",\n \t\t\t rev.subject_prefix, reroll_count);\n \t\trev.reroll_count = reroll_count;\n-\t\trev.subject_prefix = strbuf_detach(&sprefix, NULL);\n+\t\trev.subject_prefix = sprefix.buf;\n \t}\n \n \tfor (i = 0; i < extra_hdr.nr; i++) {\n@@ -2388,6 +2387,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&rdiff1);\n \tstrbuf_release(&rdiff2);\n \tstrbuf_release(&rdiff_title);\n+\tstrbuf_release(&sprefix);\n \tfree(to_free);\n \tif (rev.ref_message_ids)\n \t\tstring_list_clear(rev.ref_message_ids, 0);\n-- \n2.39.0-198-ga38d39a4c5\n\n"},{"id":"470446","messageId":"Y8WJnGHs5nM5GwBM@coredump.intra.peff.net","threadId":"59094","inReplyTo":"xmqqv8l8gr6s.fsf@gitster.g","subject":"Re: [PATCH] format-patch: unleak \"-v \"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-16T17:30:04Z","receivedAt":"2023-01-16T17:50:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:\n\n> The \"subject_prefix\" member of \"struct revision\" usually is always\n> set to a borrowed string (either a string literal like \"PATCH\" that\n> appear in the program text as a hardcoded default, or the value of\n> \"format.subjectprefix\") and is never freed when the containing\n> revision structure is released. The \"-v \" codepath however\n> violates this rule and stores a pointer to an allocated string to\n> this member, relinquishing the responsibility to free it when it is\n> done using the revision structure, leading to a small one-time leak.\n> \n> Instead, keep track of the string it allocates to let the revision\n> structure borrow, and clean it up when it is done.\n\nFWIW, this looks obviously correct to me.\n\nThe word \"unleak\" in the subject made me think about UNLEAK(), so this\nis a small tangent. This is exactly the kind of case that I designed\nUNLEAK() for, because the solution really is \"while you are assigning to\nX, also keep a copy of the pointer in Y to be freed later\".\n\nAnd UNLEAK() is just \"keep a copy of the pointer in Y to know that we\n_could_ free it later\". And of course \"do nothing if we are not\nleak-detecting\". But since we seem to be moving away from UNLEAK(), and\nsince it would not even save any lines here, I'm perfectly happy with\nthis solution.\n\n-Peff\n"},{"id":"470451","messageId":"xmqqpmbecoom.fsf@gitster.g","threadId":"59094","inReplyTo":"Y8WJnGHs5nM5GwBM@coredump.intra.peff.net","subject":"Re: [PATCH] format-patch: unleak \"-v \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-16T18:35:53Z","receivedAt":"2023-01-16T18:42:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:\n>\n>> The \"subject_prefix\" member of \"struct revision\" usually is always\n>> set to a borrowed string (either a string literal like \"PATCH\" that\n>> appear in the program text as a hardcoded default, or the value of\n>> \"format.subjectprefix\") and is never freed when the containing\n>> revision structure is released. The \"-v \" codepath however\n>> violates this rule and stores a pointer to an allocated string to\n>> this member, relinquishing the responsibility to free it when it is\n>> done using the revision structure, leading to a small one-time leak.\n>> \n>> Instead, keep track of the string it allocates to let the revision\n>> structure borrow, and clean it up when it is done.\n>\n> FWIW, this looks obviously correct to me.\n>\n> The word \"unleak\" in the subject made me think about UNLEAK(), so this\n> is a small tangent. This is exactly the kind of case that I designed\n> UNLEAK() for, because the solution really is \"while you are assigning to\n> X, also keep a copy of the pointer in Y to be freed later\".\n\nYup. I was originally planning to use UNLEAK(), but it felt ugly to\nUNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes\nand owned pointer some other times, which is the exact reason why I\nstarted looking for a clean way to plug this leak. So I ended up\nwith declaring that the member should only store a borrowed pointer.\n\n> And UNLEAK() is just \"keep a copy of the pointer in Y to know that we\n> _could_ free it later\". And of course \"do nothing if we are not\n> leak-detecting\". But since we seem to be moving away from UNLEAK(), and\n> since it would not even save any lines here, I'm perfectly happy with\n> this solution.\n\nThe first sentence needs to be rephrased, as it does not make much\nsense to have something usually be X and always be X at the same\ntime (I'd just remove \"always\" from there).\n\nThanks.\n\n"},{"id":"470456","messageId":"Y8WqruNU5Z6ewJte@coredump.intra.peff.net","threadId":"59094","inReplyTo":"xmqqpmbecoom.fsf@gitster.g","subject":"Re: [PATCH] format-patch: unleak \"-v \"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-16T19:51:10Z","receivedAt":"2023-01-16T19:51:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 16, 2023 at 10:35:53AM -0800, Junio C Hamano wrote:\n\n> > The word \"unleak\" in the subject made me think about UNLEAK(), so this\n> > is a small tangent. This is exactly the kind of case that I designed\n> > UNLEAK() for, because the solution really is \"while you are assigning to\n> > X, also keep a copy of the pointer in Y to be freed later\".\n> \n> Yup. I was originally planning to use UNLEAK(), but it felt ugly to\n> UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes\n> and owned pointer some other times, which is the exact reason why I\n> started looking for a clean way to plug this leak. So I ended up\n> with declaring that the member should only store a borrowed pointer.\n\nThat's actually one of the nice things about UNLEAK(). It is OK to\nover-mark something that may or may not be allocated.\n\n> The first sentence needs to be rephrased, as it does not make much\n> sense to have something usually be X and always be X at the same\n> time (I'd just remove \"always\" from there).\n\nYep, agreed.\n\n-Peff\n"}]}