{"thread":{"id":"63641","subject":"[PATCH] Fix memory leak in function handle_content_type","startedAt":"2025-06-13T16:53:00Z","lastAt":"2025-06-16T01:01:22Z","messageCount":8,"participants":["Alex via GitGitGadget","Kristoffer Haugsbakk","Jinyao Guo","Junio C Hamano","Lidong Yan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520230","messageId":"pull.1997.git.git.1749833577767.gitgitgadget@gmail.com","threadId":"63641","inReplyTo":null,"subject":"[PATCH] Fix memory leak in function handle_content_type","fromName":"Alex via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-13T16:52:57Z","receivedAt":"2025-06-13T16:53:00Z","isPatch":true,"sender":{"key":"ajb44.geo@yahoo.com","avatar":null},"body":"From: jinyaoguo <guo846@purdue.edu>\n\nThe function handle_content_type allocates memory for boundary\nusing xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=\n&mi->content[MAX_BOUNDARIES]) is true, the function returns\nwithout freeing boundary.\n\nSigned-off-by: Alex Guo <alexguo1023@gmail.com>\n---\n    Fix memory leak in function handle_content_type\n    \n    The function handle_content_type allocates memory for boundary using\n    xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=\n    &mi->content[MAX_BOUNDARIES]) is true, the function returns without\n    freeing boundary.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1997%2Fmugitya03%2Fmlk-2-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1997/mugitya03/mlk-2-v1\nPull-Request: https://github.com/git/git/pull/1997\n\n mailinfo.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ee4597da6be..e0ea358311f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n \t\t\terror(\"Too many boundaries to handle\");\n \t\t\tmi->input_error = -1;\n \t\t\tmi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n+\t\t\tstrbuf_release(boundary);\n+\t\t\tfree(boundary);\n+\t\t\tboundary = NULL;\n \t\t\treturn;\n \t\t}\n \t\t*(mi->content_top) = boundary;\n\nbase-commit: 9edff09aec9b5aaa3d5528129bb279a4d34cf5b3\n-- \ngitgitgadget\n"},{"id":"520231","messageId":"44066126-ece9-4c77-b38a-292b6f748955@app.fastmail.com","threadId":"63641","inReplyTo":"pull.1997.git.git.1749833577767.gitgitgadget@gmail.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-06-13T16:59:48Z","receivedAt":"2025-06-13T17:01:21Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Jun 13, 2025, at 18:52, Alex via GitGitGadget wrote:\n> From: jinyaoguo <guo846@purdue.edu>\n>\n> [snip]\n>\n> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n\nLike what was said on another patch[1] the From and Signed-off-by names\nneed to match.  I didn’t see a resolution to that?\n\nIf forwarding you need to add your signoff after theirs.\n\n[1]: https://lore.kernel.org/git/xmqq1psfxgyv.fsf@gitster.g/\n\n-- \nKristoffer Haugsbakk\n"},{"id":"520232","messageId":"SA1PR22MB3999874B7FD9FFF7D90C9F89E477A@SA1PR22MB3999.namprd22.prod.outlook.com","threadId":"63641","inReplyTo":"44066126-ece9-4c77-b38a-292b6f748955@app.fastmail.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Jinyao Guo","fromEmail":"guo846@purdue.edu","sentAt":"2025-06-13T18:36:42Z","receivedAt":"2025-06-13T18:36:44Z","isPatch":true,"sender":{"key":"guo846@purdue.edu","avatar":"https://avatars.githubusercontent.com/u/70044335?v=4"},"body":"Hi Kristoffer,\n\nSorry for that. I’m still getting familiar with the Git patch submission workflow and missed the sign-off requirement. I believe the confusion came from using two different Git identities.\nI’ve now added individual “Signed-off-by” lines for both accounts.\n\nFrom b39b1a8176a344a2fa2c46a6d0ffc27a7bfd9edd Mon Sep 17 00:00:00 2001\nFrom: jinyaoguo <guo846@purdue.edu>\nDate: Thu, 12 Jun 2025 18:48:24 -0400\nSubject: [PATCH] Fix memory leak in function handle_content_type\n\nThe function handle_content_type allocates memory for boundary\nusing xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=\n&mi->content[MAX_BOUNDARIES]) is true, the function returns\nwithout freeing boundary.\n\nSigned-off-by: Alex Guo <alexguo1023@gmail.com>\nSigned-off-by: jinyaoguo <guo846@purdue.edu>\n---\n mailinfo.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ee4597da6b..e0ea358311 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n                        error(\"Too many boundaries to handle\");\n                        mi->input_error = -1;\n                        mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n+                       strbuf_release(boundary);\n+                       free(boundary);\n+                       boundary = NULL;\n                        return;\n                }\n                *(mi->content_top) = boundary;\n--\n2.34.1\n\n________________________________________\nFrom: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>\nSent: Friday, June 13, 2025 12:59\nTo: Josh Soref <gitgitgadget@gmail.com>; git@vger.kernel.org <git@vger.kernel.org>\nCc: Alex <alexguo1023@gmail.com>; Jinyao Guo <guo846@purdue.edu>\nSubject: Re: [PATCH] Fix memory leak in function handle_content_type\n\n[You don't often get email from kristofferhaugsbakk@fastmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]\n\n---- External Email: Use caution with attachments, links, or sharing data ----\n\n\nOn Fri, Jun 13, 2025, at 18:52, Alex via GitGitGadget wrote:\n> From: jinyaoguo <guo846@purdue.edu>\n>\n> [snip]\n>\n> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n\nLike what was said on another patch[1] the From and Signed-off-by names\nneed to match.  I didn’t see a resolution to that?\n\nIf forwarding you need to add your signoff after theirs.\n\n[1]: https://lore.kernel.org/git/xmqq1psfxgyv.fsf@gitster.g/\n\n--\nKristoffer Haugsbakk\n"},{"id":"520234","messageId":"xmqq7c1f8nr2.fsf@gitster.g","threadId":"63641","inReplyTo":"SA1PR22MB3999874B7FD9FFF7D90C9F89E477A@SA1PR22MB3999.namprd22.prod.outlook.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-13T19:06:41Z","receivedAt":"2025-06-13T19:06:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jinyao Guo <guo846@purdue.edu> writes:\n\n> ... I believe\n> the confusion came from using two different Git identities.  I’ve\n> now added individual “Signed-off-by” lines for both accounts.\n\nPlease do not do this, if these two are the same single person.\n\nInstead, pick the one you want to be known as to this community, and\nuse that consistently while working on this project.\n\nThanks.\n"},{"id":"520235","messageId":"SA1PR22MB399911638F342E1AA20F014AE477A@SA1PR22MB3999.namprd22.prod.outlook.com","threadId":"63641","inReplyTo":"xmqq7c1f8nr2.fsf@gitster.g","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Jinyao Guo","fromEmail":"guo846@purdue.edu","sentAt":"2025-06-13T19:26:45Z","receivedAt":"2025-06-13T19:26:49Z","isPatch":true,"sender":{"key":"guo846@purdue.edu","avatar":"https://avatars.githubusercontent.com/u/70044335?v=4"},"body":"Sure. I'll choose one account and use it consistently.\n\nHere is the edited patch:\n\nFrom 04b286cb2e736c3a53287b6ddf406e704f19fb2e Mon Sep 17 00:00:00 2001\nFrom: jinyaoguo <guo846@purdue.edu>\nDate: Thu, 12 Jun 2025 18:48:24 -0400\nSubject: [PATCH] Fix memory leak in function handle_content_type\n\nThe function handle_content_type allocates memory for boundary\nusing xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=\n&mi->content[MAX_BOUNDARIES]) is true, the function returns\nwithout freeing boundary.\n\nSigned-off-by: jinyaoguo <guo846@purdue.edu>\n---\n mailinfo.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ee4597da6b..e0ea358311 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n \t\t\terror(\"Too many boundaries to handle\");\n \t\t\tmi->input_error = -1;\n \t\t\tmi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n+\t\t\tstrbuf_release(boundary);\n+\t\t\tfree(boundary);\n+\t\t\tboundary = NULL;\n \t\t\treturn;\n \t\t}\n \t\t*(mi->content_top) = boundary;\n-- \n2.34.1 \n\n\nBest,\nJinyao\n________________________________________\nFrom: Junio C Hamano <gitster@pobox.com>\nSent: Friday, June 13, 2025 15:06\nTo: Jinyao Guo <guo846@purdue.edu>\nCc: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>; Josh Soref <gitgitgadget@gmail.com>; git@vger.kernel.org <git@vger.kernel.org>; Alex <alexguo1023@gmail.com>\nSubject: Re: [PATCH] Fix memory leak in function handle_content_type\n \n---- External Email: Use caution with attachments, links, or sharing data ----\n\n\nJinyao Guo <guo846@purdue.edu> writes:\n\n> ... I believe\n> the confusion came from using two different Git identities.  I’ve\n> now added individual “Signed-off-by” lines for both accounts.\n\nPlease do not do this, if these two are the same single person.\n\nInstead, pick the one you want to be known as to this community, and\nuse that consistently while working on this project.\n\nThanks."},{"id":"520252","messageId":"xmqq1prm6zlx.fsf@gitster.g","threadId":"63641","inReplyTo":"SA1PR22MB399911638F342E1AA20F014AE477A@SA1PR22MB3999.namprd22.prod.outlook.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-14T16:45:46Z","receivedAt":"2025-06-14T16:45:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jinyao Guo <guo846@purdue.edu> writes:\n\n> @@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n>  \t\t\terror(\"Too many boundaries to handle\");\n>  \t\t\tmi->input_error = -1;\n>  \t\t\tmi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n> +\t\t\tstrbuf_release(boundary);\n> +\t\t\tfree(boundary);\n> +\t\t\tboundary = NULL;\n>  \t\t\treturn;\n>  \t\t}\n>  \t\t*(mi->content_top) = boundary;\n\n\"boundary\" is a on-stack local variable.  There is no need to assign\nNULL to it immediately before you return.  In the post-context of\nthis hunk, we free it but leave the variable pointing at a random\nplace after that before returning.\n\n\n\n"},{"id":"520265","messageId":"8C3C7F5F-9442-4C63-9280-D46A683C9F4D@gmail.com","threadId":"63641","inReplyTo":"SA1PR22MB399911638F342E1AA20F014AE477A@SA1PR22MB3999.namprd22.prod.outlook.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-15T12:55:08Z","receivedAt":"2025-06-15T12:55:24Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jinyao Guo <guo846@purdue.edu> writes：\n> \n> Sure. I'll choose one account and use it consistently.\n> \n> Here is the edited patch:\n> \n> From 04b286cb2e736c3a53287b6ddf406e704f19fb2e Mon Sep 17 00:00:00 2001\n> From: jinyaoguo <guo846@purdue.edu>\n> Date: Thu, 12 Jun 2025 18:48:24 -0400\n> Subject: [PATCH] Fix memory leak in function handle_content_type\n> \n> The function handle_content_type allocates memory for boundary\n> using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=\n> &mi->content[MAX_BOUNDARIES]) is true, the function returns\n> without freeing boundary.\n> \n> Signed-off-by: jinyaoguo <guo846@purdue.edu>\n> ---\n> mailinfo.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/mailinfo.c b/mailinfo.c\n> index ee4597da6b..e0ea358311 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n> error(\"Too many boundaries to handle\");\n> mi->input_error = -1;\n> mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n> + strbuf_release(boundary);\n> + free(boundary);\n> + boundary = NULL;\n> return;\n> }\n> *(mi->content_top) = boundary;\n> -- \n> 2.34.1 \n\nMay be using goto here would be better. Like:\n\n---\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ee4597da6b..83358b7517 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -266,13 +266,14 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n                        error(\"Too many boundaries to handle\");\n                        mi->input_error = -1;\n                        mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n-                       return;\n+                       goto out;\n                }\n                *(mi->content_top) = boundary;\n                boundary = NULL;\n        }\n        slurp_attr(line->buf, \"charset=\", &mi->charset);\n \n+out:\n        if (boundary) {\n                strbuf_release(boundary);\n                free(boundary);\n—\n\nLidong\n\n"},{"id":"520274","messageId":"xmqqldps4i00.fsf@gitster.g","threadId":"63641","inReplyTo":"8C3C7F5F-9442-4C63-9280-D46A683C9F4D@gmail.com","subject":"Re: [PATCH] Fix memory leak in function handle_content_type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T01:01:19Z","receivedAt":"2025-06-16T01:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan <yldhome2d2@gmail.com> writes:\n\n> Subject: Re: [PATCH] Fix memory leak in function handle_content_type\n\nThe subject should probably be something like\n\n\tSubject: [PATCH] mailinfo.c: plug memory leak in handle_content_type()\n\n> May be using goto here would be better. Like:\n>\n> ---\n> diff --git a/mailinfo.c b/mailinfo.c\n> index ee4597da6b..83358b7517 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -266,13 +266,14 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n>                         error(\"Too many boundaries to handle\");\n>                         mi->input_error = -1;\n>                         mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;\n> -                       return;\n> +                       goto out;\n>                 }\n>                 *(mi->content_top) = boundary;\n>                 boundary = NULL;\n>         }\n>         slurp_attr(line->buf, \"charset=\", &mi->charset);\n>  \n> +out:\n>         if (boundary) {\n>                 strbuf_release(boundary);\n>                 free(boundary);\n> —\n\nYup, that one looks good enough.\n\nIf we wanted to clean up this code path even further, I think the\nfirst clean-up to do is to reconsider the use of \"struct strbuf *\"\n(instead of \"const char *\") in *(mi->content_top).  strbuf is a fine\nand less error-prone mechanism to use while you have to manipulate\ncharacter strings (like parsing from input line to formulate the\nboundary string), but once this function computed what was asked by\nthe caller, the computed result (like the boundary string) almost\nalways do not need to be editable.  But such a code improvement is\ntotally outside the topic of this patch.\n\n"}]}