{"thread":{"id":"59769","subject":"[PATCH 0/2] a few format-patch leak fixes","startedAt":"2023-05-19T00:02:44Z","lastAt":"2023-05-19T16:56:28Z","messageCount":6,"participants":["Jeff King","Junio C Hamano","Konstantin Khomoutov"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"477572","messageId":"20230519000239.GA1975039@coredump.intra.peff.net","threadId":"59769","inReplyTo":null,"subject":"[PATCH 0/2] a few format-patch leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:02:39Z","receivedAt":"2023-05-19T00:02:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I got bit earlier by the leak-checker complaining about an old leak on\njc/send-email-pre-process-fix, so I started to poke at the code. I see\nthat you since silenced it via 20bd08aefb (t9001: mark the script as no\nlonger leak checker clean, 2023-05-17).\n\nI think that's an OK solution for the release period. But since I dug\ninto the actual leak, I didn't want those fixes to go to waste. :)\n\nSo these can be applied at our leisure post-release, though I think the\nfirst one could also be taken independently, and is enough to fix t9001\n(obviously we could also take them both, but the first one is the less\nrisky of the two, since we're in an -rc period).\n\n [1/2]: format-patch: free rev.message_id when exiting\n [2/2]: format-patch: free elements of rev.ref_message_ids list\n\n builtin/log.c | 15 ++++++++-------\n t/t9001-send-email.sh | 2 +-\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\n-Peff\n"},{"id":"477573","messageId":"20230519000325.GA1975194@coredump.intra.peff.net","threadId":"59769","inReplyTo":"20230519000239.GA1975039@coredump.intra.peff.net","subject":"[PATCH 1/2] format-patch: free rev.message_id when exiting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:03:25Z","receivedAt":"2023-05-19T00:03:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may allocate a message-id string via gen_message_id(), but we never\nfree it, causing a small leak. This can be demonstrated by running t9001\nwith a leak-checking build. The offending test is the one touched by\n3ece9bf0f9 (send-email: clear the $message_id after validation,\n2023-05-17), but the leak is much older than that. The test was simply\nunlucky enough to trigger the leaking code path for the first time.\n\nWe can fix this by freeing the string at the end of the function. We can\nalso re-mark the test script as leak-free, effectively reverting\n20bd08aefb (t9001: mark the script as no longer leak checker clean,\n2023-05-17).\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 1 +\n t/t9001-send-email.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 7d19578963..ab74760386 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2415,6 +2415,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&rdiff_title);\n \tstrbuf_release(&sprefix);\n \tfree(to_free);\n+\tfree(rev.message_id);\n \tif (rev.ref_message_ids)\n \t\tstring_list_clear(rev.ref_message_ids, 0);\n \tfree(rev.ref_message_ids);\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 2051103226..8d49eff91a 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -4,7 +4,7 @@ test_description='git send-email'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n-# no longer TEST_PASSES_SANITIZE_LEAK=true - format-patch --thread leaks\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # May be altered later in the test\n-- \n2.41.0.rc0.359.g22664e20e7\n\n"},{"id":"477574","messageId":"20230519000543.GB1975194@coredump.intra.peff.net","threadId":"59769","inReplyTo":"20230519000239.GA1975039@coredump.intra.peff.net","subject":"[PATCH 2/2] format-patch: free elements of rev.ref_message_ids list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:05:43Z","receivedAt":"2023-05-19T00:06:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are showing multiple patches with format-patch, we have to\nrepeatedly overwrite the rev.message_id field. We take care to avoid\nleaking the old value by either freeing it, or adding it to\nref_message_ids, a string list of ids to reference in subsequent\nmessages.\n\nBut unfortunately we do leak the value via that string list. We try\nto clear the string list, courtesy of 89f45cf4eb (format-patch: don't\nleak \"extra_headers\" or \"ref_message_ids\", 2022-04-13). But since it was\ninitialized as \"nodup\", the string list doesn't realize it owns the\nstrings, and it leaks them.\n\nWe have two options here:\n\n 1. Continue to init with \"nodup\", but then tweak the value of\n ref_message_ids.strdup_strings just before clearing.\n\n 2. Init with \"dup\", but use \"append_nodup\" when transferring ownership\n of strings to the list. Clearing just works.\n\nI picked the second here, as I think it calls attention to the tricky\npart (transferring ownership via the nodup call).\n\nThere's one other related fix we have to do, though. We also insert the\nresult of clean_message_id() into the list. This _sometimes_ allocates\nand sometimes does not, depending on whether we have to remove cruft\nfrom the end of the string. Let's teach it to consistently return an\nallocated string, so that the caller knows it must be freed.\n\nThere's no new test here, as the leak can already be see in t4014.44 (as\nwell as others in that script). We can't mark all of t4014 as leak-free,\nthough, as there are other unrelated leaks that it triggers.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ab74760386..fee5834c0f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1401,7 +1401,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t}\n }\n \n-static const char *clean_message_id(const char *msg_id)\n+static char *clean_message_id(const char *msg_id)\n {\n \tchar ch;\n \tconst char *a, *z, *m;\n@@ -1419,7 +1419,7 @@ static const char *clean_message_id(const char *msg_id)\n \tif (!z)\n \t\tdie(_(\"insane in-reply-to: %s\"), msg_id);\n \tif (++z == m)\n-\t\treturn a;\n+\t\treturn xstrdup(a);\n \treturn xmemdupz(a, z - a);\n }\n \n@@ -2305,11 +2305,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \n \tif (in_reply_to || thread || cover_letter) {\n \t\trev.ref_message_ids = xmalloc(sizeof(*rev.ref_message_ids));\n-\t\tstring_list_init_nodup(rev.ref_message_ids);\n+\t\tstring_list_init_dup(rev.ref_message_ids);\n \t}\n \tif (in_reply_to) {\n-\t\tconst char *msgid = clean_message_id(in_reply_to);\n-\t\tstring_list_append(rev.ref_message_ids, msgid);\n+\t\tchar *msgid = clean_message_id(in_reply_to);\n+\t\tstring_list_append_nodup(rev.ref_message_ids, msgid);\n \t}\n \trev.numbered_files = just_numbers;\n \trev.patch_suffix = fmt_patch_suffix;\n@@ -2365,8 +2365,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t\t && (!cover_letter || rev.nr > 1))\n \t\t\t\t\tfree(rev.message_id);\n \t\t\t\telse\n-\t\t\t\t\tstring_list_append(rev.ref_message_ids,\n-\t\t\t\t\t\t\t rev.message_id);\n+\t\t\t\t\tstring_list_append_nodup(rev.ref_message_ids,\n+\t\t\t\t\t\t\t\t rev.message_id);\n \t\t\t}\n \t\t\tgen_message_id(&rev, oid_to_hex(&commit->object.oid));\n \t\t}\n-- \n2.41.0.rc0.359.g22664e20e7\n"},{"id":"477580","messageId":"xmqqttw9qf6p.fsf@gitster.g","threadId":"59769","inReplyTo":"20230519000543.GB1975194@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: free elements of rev.ref_message_ids list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-19T01:32:14Z","receivedAt":"2023-05-19T01:32:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we are showing multiple patches with format-patch, we have to\n> repeatedly overwrite the rev.message_id field. We take care to avoid\n> leaking the old value by either freeing it, or adding it to\n> ref_message_ids, a string list of ids to reference in subsequent\n> messages.\n>\n> But unfortunately we do leak the value via that string list. We try\n> to clear the string list, courtesy of 89f45cf4eb (format-patch: don't\n> leak \"extra_headers\" or \"ref_message_ids\", 2022-04-13). But since it was\n> initialized as \"nodup\", the string list doesn't realize it owns the\n> strings, and it leaks them.\n>\n> We have two options here:\n>\n> 1. Continue to init with \"nodup\", but then tweak the value of\n> ref_message_ids.strdup_strings just before clearing.\n>\n> 2. Init with \"dup\", but use \"append_nodup\" when transferring ownership\n> of strings to the list. Clearing just works.\n>\n> I picked the second here, as I think it calls attention to the tricky\n> part (transferring ownership via the nodup call).\n\nThis is pretty much what I had in mind wheh I wrote the log message\nfor the follow-up \"Yikes, for now let's mark the test no longer\nsanitizer clean\" patch. Very pleased to see a clean-up I punted\nmaterialize while I was looking the other way ;-)\n\n> There's one other related fix we have to do, though. We also insert the\n> result of clean_message_id() into the list. This _sometimes_ allocates\n> and sometimes does not, depending on whether we have to remove cruft\n> from the end of the string. Let's teach it to consistently return an\n> allocated string, so that the caller knows it must be freed.\n\nYes!\n\nThanks.\n"},{"id":"477599","messageId":"20230519145431.mnn2rpptvplorsf2@carbon","threadId":"59769","inReplyTo":"20230519000543.GB1975194@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: free elements of rev.ref_message_ids list","fromName":"Konstantin Khomoutov","fromEmail":"kostix@bswap.ru","sentAt":"2023-05-19T14:54:31Z","receivedAt":"2023-05-19T15:33:40Z","isPatch":true,"sender":{"key":"kostix@bswap.ru","avatar":null},"body":"On Thu, May 18, 2023 at 08:05:43PM -0400, Jeff King wrote:\n\n[...]\n\n> There's no new test here, as the leak can already be see in t4014.44 (as\n> well as others in that script). We can't mark all of t4014 as leak-free,\n> though, as there are other unrelated leaks that it triggers.\n\nProbably s/be see/be seen/.\n\n[...]\n"},{"id":"477600","messageId":"xmqqpm6wqmz6.fsf@gitster.g","threadId":"59769","inReplyTo":"20230519145431.mnn2rpptvplorsf2@carbon","subject":"Re: [PATCH 2/2] format-patch: free elements of rev.ref_message_ids list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-19T16:56:13Z","receivedAt":"2023-05-19T16:56:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Konstantin Khomoutov writes:\n\n> On Thu, May 18, 2023 at 08:05:43PM -0400, Jeff King wrote:\n>\n> [...]\n>\n>> There's no new test here, as the leak can already be see in t4014.44 (as\n>> well as others in that script). We can't mark all of t4014 as leak-free,\n>> though, as there are other unrelated leaks that it triggers.\n>\n> Probably s/be see/be seen/.\n\nIndeed. Will locally amend.\nThanks.\n"}]}