{"thread":{"id":"63642","subject":"[PATCH] Allocate msg only after fatal checks to avoid leaks","startedAt":"2025-06-13T19:32:25Z","lastAt":"2025-06-15T00:45:49Z","messageCount":7,"participants":["Alex via GitGitGadget","Junio C Hamano","lidongyan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520236","messageId":"pull.1998.git.git.1749843142000.gitgitgadget@gmail.com","threadId":"63642","inReplyTo":null,"subject":"[PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"Alex via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-13T19:32:21Z","receivedAt":"2025-06-13T19:32:25Z","isPatch":true,"sender":{"key":"ajb44.geo@yahoo.com","avatar":null},"body":"From: jinyaoguo \n\nIn parse_reuse_arg, we previously called xmalloc and strbuf_init\nbefore resolving the ref and reading the object, leading to a\nleaked msg on die() paths. This change moves the allocation of\nstruct note_msg until after repo_get_oid and\nrepo_read_object_file succeed, ensuring no heap memory is held\nwhen a fatal error is triggered.\n\nSigned-off-by: jinyaoguo \n---\n Allocate msg only after fatal checks to avoid leaks\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1998%2Fmugitya03%2Fmlk-3-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1998/mugitya03/mlk-3-v1\nPull-Request: https://github.com/git/git/pull/1998\n\n builtin/notes.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a3f433ca4c0..6df8a7998fb 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -308,7 +308,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n-\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n+\tstruct note_msg *msg;\n \tchar *value;\n \tstruct object_id object;\n \tenum object_type type;\n@@ -316,17 +316,17 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstrbuf_init(&msg->buf, 0);\n \tif (repo_get_oid(the_repository, arg, &object))\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n \tif (!(value = repo_read_object_file(the_repository, &object, &type, &len)))\n \t\tdie(_(\"failed to read object '%s'.\"), arg);\n-\tif (type != OBJ_BLOB) {\n-\t\tstrbuf_release(&msg->buf);\n-\t\tfree(value);\n-\t\tfree(msg);\n-\t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n-\t}\n+ if (type != OBJ_BLOB) {\n+ free(value);\n+ die(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n+ }\n+\n+ msg = xmalloc(sizeof(*msg));\n+ strbuf_init(&msg->buf, 0);\n \n \tstrbuf_add(&msg->buf, value, len);\n \tfree(value);\n\nbase-commit: 9edff09aec9b5aaa3d5528129bb279a4d34cf5b3\n-- \ngitgitgadget\n"},{"id":"520240","messageId":"xmqqzfeb74yy.fsf@gitster.g","threadId":"63642","inReplyTo":"pull.1998.git.git.1749843142000.gitgitgadget@gmail.com","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-13T20:37:41Z","receivedAt":"2025-06-13T20:37:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex via GitGitGadget\" writes:\n\n> -\tif (type != OBJ_BLOB) {\n> -\t\tstrbuf_release(&msg->buf);\n> -\t\tfree(value);\n> -\t\tfree(msg);\n> -\t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n> -\t}\n> + if (type != OBJ_BLOB) {\n> + free(value);\n> + die(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n> + }\n> +\n> + msg = xmalloc(sizeof(*msg));\n> + strbuf_init(&msg->buf, 0);\n\nALl the new lines seem to be indented by four spaces. Check with\nDocumantation/CodingGuidelines.\n\nAlso, Documantation/SubmittingPatches::[[real-name]] asks folks to\nuse their real name as authorname. You prefer your purdue\naddress, that is fine, but let's do something like\n\n From: Jinyao Guo \n Signed-off-by: Jinyao Guo \n\n"},{"id":"520247","messageId":"3993AF96-E03D-46AB-B18E-8E6C1108EC45@smail.nju.edu.cn","threadId":"63642","inReplyTo":"pull.1998.git.git.1749843142000.gitgitgadget@gmail.com","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-14T08:26:09Z","receivedAt":"2025-06-14T08:26:57Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Alex via GitGitGadget writes：\n> \n> From: jinyaoguo \n> \n> In parse_reuse_arg, we previously called xmalloc and strbuf_init\n> before resolving the ref and reading the object, leading to a\n> leaked msg on die() paths. This change moves the allocation of\n\nA memory leak on the die() path shouldn't be considered a real leak,\nright? Since the OS will clean up all memory once the process\nterminates, explicitly freeing msg isn't necessary in this case.\n\nLidong"},{"id":"520249","messageId":"xmqqcyb672mc.fsf@gitster.g","threadId":"63642","inReplyTo":"3993AF96-E03D-46AB-B18E-8E6C1108EC45@smail.nju.edu.cn","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-14T15:40:43Z","receivedAt":"2025-06-14T15:40:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lidongyan <502024330056@smail.nju.edu.cn> writes:\n\n> Alex via GitGitGadget writes：\n>> \n>> From: jinyaoguo \n>> \n>> In parse_reuse_arg, we previously called xmalloc and strbuf_init\n>> before resolving the ref and reading the object, leading to a\n>> leaked msg on die() paths. This change moves the allocation of\n>\n> A memory leak on the die() path shouldn't be considered a real leak,\n> right? Since the OS will clean up all memory once the process\n> terminates, explicitly freeing msg isn't necessary in this case.\n\nIt may not matter in practice, but I think the leak checking\nmachinery like sanitizers would still complain, so I view efforts on\nplugging such leaks in the error code paths more about decluttering\nthe leak checker output to help us spot the real leaks.\n"},{"id":"520251","messageId":"089E9F89-5B21-4035-B500-8255622DA92A@smail.nju.edu.cn","threadId":"63642","inReplyTo":"xmqqcyb672mc.fsf@gitster.g","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-14T15:50:35Z","receivedAt":"2025-06-14T15:51:18Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes：\n> \n> lidongyan <502024330056@smail.nju.edu.cn> writes:\n> \n>> Alex via GitGitGadget writes：\n>>> \n>>> From: jinyaoguo \n>>> \n>>> In parse_reuse_arg, we previously called xmalloc and strbuf_init\n>>> before resolving the ref and reading the object, leading to a\n>>> leaked msg on die() paths. This change moves the allocation of\n>> \n>> A memory leak on the die() path shouldn't be considered a real leak,\n>> right? Since the OS will clean up all memory once the process\n>> terminates, explicitly freeing msg isn't necessary in this case.\n> \n> It may not matter in practice, but I think the leak checking\n> machinery like sanitizers would still complain, so I view efforts on\n> plugging such leaks in the error code paths more about decluttering\n> the leak checker output to help us spot the real leaks.\n\nMakes sense. However, inserting a free-like statement in die() would be\nmessier than using goto, since each die() has a unique message and we\nneed to free stuff at each die()."},{"id":"520253","messageId":"20250614230158.GA2568638@coredump.intra.peff.net","threadId":"63642","inReplyTo":"xmqqcyb672mc.fsf@gitster.g","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-14T23:01:58Z","receivedAt":"2025-06-14T23:02:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 14, 2025 at 08:40:43AM -0700, Junio C Hamano wrote:\n\n> > A memory leak on the die() path shouldn't be considered a real leak,\n> > right? Since the OS will clean up all memory once the process\n> > terminates, explicitly freeing msg isn't necessary in this case.\n> \n> It may not matter in practice, but I think the leak checking\n> machinery like sanitizers would still complain, so I view efforts on\n> plugging such leaks in the error code paths more about decluttering\n> the leak checker output to help us spot the real leaks.\n\nI disagree here. These are not really leaks, and a leak-checker that\ncomplains about them is bad.\n\nWhen we call die(), the pointer to the buffer is still on the stack, and\nthus the memory is still reachable and not leaked. Some tools like\nvalgrind may still report these as \"still reachable\", but because they\ncategorize them properly we can ignore them[1].\n\nThe one exception we've seen is that an optimizing compiler may reorder\ninstructions to obliterate the stack (because it knows die() is marked\nwith NORETURN), causing a false positive. We dealt with that via\nd3775de074 (Makefile: force -O0 when compiling with SANITIZE=leak,\n2022-10-18). I don't think we've seen any recurrence since then.\n\nAnd while it may be tempting to say \"well, it does not hurt to free them\non the die() path\", in my opinion that way madness lies. You may have\naccess to some local variables that can be freed, but there will be many\nother heap allocations that you don't even know about! Here's a toy\nexample from a similar discussion a few years ago:\n\n https://lore.kernel.org/git/YNypPeoZTRiOxPPQ@coredump.intra.peff.net/\n\nSo I'd really prefer not to go down this route. And I think the existing\ncode in this patch's pre-image that calls free() before die() only on\none path should be simplified, so that all die() paths consistently do\nnot worry about this.\n\nI.e., this:\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex cc1163242f..f3d5eda104 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -321,12 +321,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n \tif (!(value = odb_read_object(the_repository->objects, &object, &type, &len)))\n \t\tdie(_(\"failed to read object '%s'.\"), arg);\n-\tif (type != OBJ_BLOB) {\n-\t\tstrbuf_release(&msg->buf);\n-\t\tfree(value);\n-\t\tfree(msg);\n+\tif (type != OBJ_BLOB)\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n-\t}\n \n \tstrbuf_add(&msg->buf, value, len);\n \tfree(value);\n\n-Peff\n\n[1] \"still reachable\" leaks _can_ be useful when returning from main,\n because they may show memory held in global structures that we might\n have been able to free at a more timely spot. But there are a lot of\n these in Git, most of which are not interesting (e.g., is freeing\n the_repository really worth caring about?), and I don't think there\n is a good way to tell the difference.\n\n More pontificating from that earlier discussion:\n\n https://lore.kernel.org/git/YN3iIaovvG7XgLQP@coredump.intra.peff.net/\n"},{"id":"520260","messageId":"xmqqh60h6ddy.fsf@gitster.g","threadId":"63642","inReplyTo":"20250614230158.GA2568638@coredump.intra.peff.net","subject":"Re: [PATCH] Allocate msg only after fatal checks to avoid leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-15T00:45:45Z","receivedAt":"2025-06-15T00:45:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> And while it may be tempting to say \"well, it does not hurt to free them\n> on the die() path\", in my opinion that way madness lies. You may have\n> access to some local variables that can be freed, but there will be many\n> other heap allocations that you don't even know about! Here's a toy\n> example from a similar discussion a few years ago:\n>\n> https://lore.kernel.org/git/YNypPeoZTRiOxPPQ@coredump.intra.peff.net/\n\nYeah, I recall that discussion and the example. Yes, we should not\nhave to crawl up from a direct caller of die() and free everything\nthese stack frames hold.\n\n> I.e., this:\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index cc1163242f..f3d5eda104 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -321,12 +321,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n> \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n> \tif (!(value = odb_read_object(the_repository->objects, &object, &type, &len)))\n> \t\tdie(_(\"failed to read object '%s'.\"), arg);\n> -\tif (type != OBJ_BLOB) {\n> -\t\tstrbuf_release(&msg->buf);\n> -\t\tfree(value);\n> -\t\tfree(msg);\n> +\tif (type != OBJ_BLOB)\n> \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n> -\t}\n\nMuch nicer.\n"}]}