threads / patch / 63641

patchFix memory leak in function handle_content_type

Subject: [PATCH] Fix memory leak in function handle_content_type

## tl;dr

8 messages between Jun 13, 2025 and Jun 16, 2025. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Alex via GitGitGadget· Jun 13, 2025, 16:52 UTC · lore
From: jinyaoguo <guo846@purdue.edu>

The function handle_content_type allocates memory for boundary using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) is true, the function returns without freeing boundary.

Signed-off-by: Alex Guo <alexguo1023@gmail.com>
---
    Fix memory leak in function handle_content_type
    
    The function handle_content_type allocates memory for boundary using
    xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=
    &mi->content[MAX_BOUNDARIES]) is true, the function returns without
    freeing boundary.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1997%2Fmugitya03%2Fmlk-2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1997/mugitya03/mlk-2-v1
Pull-Request: https://github.com/git/git/pull/1997
 mailinfo.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to mailinfo.c +3 −0
diff --git a/mailinfo.c b/mailinfo.c
index ee4597da6be..e0ea358311f 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
 			error("Too many boundaries to handle");
 			mi->input_error = -1;
 			mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
+			strbuf_release(boundary);
+			free(boundary);
+			boundary = NULL;
 			return;
 		}
 		*(mi->content_top) = boundary;

base-commit: 9edff09aec9b5aaa3d5528129bb279a4d34cf5b3

-- 
gitgitgadget
Kristoffer Haugsbakk· Jun 13, 2025, 16:59 UTC · re: Alex via GitGitGadget · lore

Re: [PATCH] Fix memory leak in function handle_content_type

On Fri, Jun 13, 2025, at 18:52, Alex via GitGitGadget wrote:
Show 5 quoted lines
> From: jinyaoguo <guo846@purdue.edu>
>
> [snip]
>
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>

Like what was said on another patch[1] the From and Signed-off-by names need to match. I didn’t see a resolution to that?

If forwarding you need to add your signoff after theirs.
[1]: https://lore.kernel.org/git/xmqq1psfxgyv.fsf@gitster.g/
-- 
Kristoffer Haugsbakk
Jinyao Guo· Jun 13, 2025, 18:36 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Hi Kristoffer,

Sorry 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. I’ve now added individual “Signed-off-by” lines for both accounts.

From b39b1a8176a344a2fa2c46a6d0ffc27a7bfd9edd Mon Sep 17 00:00:00 2001
From: jinyaoguo <guo846@purdue.edu>
Date: Thu, 12 Jun 2025 18:48:24 -0400
Subject: [PATCH] Fix memory leak in function handle_content_type

The function handle_content_type allocates memory for boundary using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) is true, the function returns without freeing boundary.

Signed-off-by: Alex Guo <alexguo1023@gmail.com>
Signed-off-by: jinyaoguo <guo846@purdue.edu>
---
 mailinfo.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to mailinfo.c +3 −2
diff --git a/mailinfo.c b/mailinfo.c
index ee4597da6b..e0ea358311 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
                        error("Too many boundaries to handle");
                        mi->input_error = -1;
                        mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
+                       strbuf_release(boundary);
+                       free(boundary);
+                       boundary = NULL;
                        return;
                }
                *(mi->content_top) = boundary;
--
2.34.1

________________________________________
From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Sent: Friday, June 13, 2025 12:59
To: Josh Soref <gitgitgadget@gmail.com>; git@vger.kernel.org <git@vger.kernel.org>
Cc: Alex <alexguo1023@gmail.com>; Jinyao Guo <guo846@purdue.edu>
Subject: Re: [PATCH] Fix memory leak in function handle_content_type

[You don't often get email from kristofferhaugsbakk@fastmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

---- External Email: Use caution with attachments, links, or sharing data ----


On Fri, Jun 13, 2025, at 18:52, Alex via GitGitGadget wrote:
> From: jinyaoguo <guo846@purdue.edu>
>
> [snip]
>
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>

Like what was said on another patch[1] the From and Signed-off-by names
need to match.  I didn’t see a resolution to that?

If forwarding you need to add your signoff after theirs.

[1]: https://lore.kernel.org/git/xmqq1psfxgyv.fsf@gitster.g/

--
Kristoffer Haugsbakk
Junio C Hamano· Jun 13, 2025, 19:06 UTC · re: Jinyao Guo · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Jinyao Guo <guo846@purdue.edu> writes:
> ... I believe
> the confusion came from using two different Git identities.  I’ve
> now added individual “Signed-off-by” lines for both accounts.
Please do not do this, if these two are the same single person.

Instead, pick the one you want to be known as to this community, and use that consistently while working on this project.

Thanks.
Jinyao Guo· Jun 13, 2025, 19:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Sure. I'll choose one account and use it consistently.
Here is the edited patch:
From 04b286cb2e736c3a53287b6ddf406e704f19fb2e Mon Sep 17 00:00:00 2001
From: jinyaoguo <guo846@purdue.edu>
Date: Thu, 12 Jun 2025 18:48:24 -0400
Subject: [PATCH] Fix memory leak in function handle_content_type

The function handle_content_type allocates memory for boundary using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) is true, the function returns without freeing boundary.

Signed-off-by: jinyaoguo <guo846@purdue.edu>
---
 mailinfo.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to mailinfo.c +3 −0
diff --git a/mailinfo.c b/mailinfo.c
index ee4597da6b..e0ea358311 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
 			error("Too many boundaries to handle");
 			mi->input_error = -1;
 			mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
+			strbuf_release(boundary);
+			free(boundary);
+			boundary = NULL;
 			return;
 		}
 		*(mi->content_top) = boundary;

-- 
2.34.1 


Best,
Jinyao
________________________________________
From: Junio C Hamano <gitster@pobox.com>
Sent: Friday, June 13, 2025 15:06
To: Jinyao Guo <guo846@purdue.edu>
Cc: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>; Josh Soref <gitgitgadget@gmail.com>; git@vger.kernel.org <git@vger.kernel.org>; Alex <alexguo1023@gmail.com>
Subject: Re: [PATCH] Fix memory leak in function handle_content_type
 
---- External Email: Use caution with attachments, links, or sharing data ----


Jinyao Guo <guo846@purdue.edu> writes:

> ... I believe
> the confusion came from using two different Git identities.  I’ve
> now added individual “Signed-off-by” lines for both accounts.

Please do not do this, if these two are the same single person.

Instead, pick the one you want to be known as to this community, and
use that consistently while working on this project.

Thanks.
Junio C Hamano· Jun 14, 2025, 16:45 UTC · re: Jinyao Guo · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Jinyao Guo <guo846@purdue.edu> writes:
Show 10 quoted lines
> @@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
>  			error("Too many boundaries to handle");
>  			mi->input_error = -1;
>  			mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
> +			strbuf_release(boundary);
> +			free(boundary);
> +			boundary = NULL;
>  			return;
>  		}
>  		*(mi->content_top) = boundary;

"boundary" is a on-stack local variable. There is no need to assign NULL to it immediately before you return. In the post-context of this hunk, we free it but leave the variable pointing at a random place after that before returning.

Lidong Yan· Jun 15, 2025, 12:55 UTC · re: Jinyao Guo · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Jinyao Guo <guo846@purdue.edu> writes：
Show 36 quoted lines
> 
> Sure. I'll choose one account and use it consistently.
> 
> Here is the edited patch:
> 
> From 04b286cb2e736c3a53287b6ddf406e704f19fb2e Mon Sep 17 00:00:00 2001
> From: jinyaoguo <guo846@purdue.edu>
> Date: Thu, 12 Jun 2025 18:48:24 -0400
> Subject: [PATCH] Fix memory leak in function handle_content_type
> 
> The function handle_content_type allocates memory for boundary
> using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >=
> &mi->content[MAX_BOUNDARIES]) is true, the function returns
> without freeing boundary.
> 
> Signed-off-by: jinyaoguo <guo846@purdue.edu>
> ---
> mailinfo.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/mailinfo.c b/mailinfo.c
> index ee4597da6b..e0ea358311 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -266,6 +266,9 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
> error("Too many boundaries to handle");
> mi->input_error = -1;
> mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
> + strbuf_release(boundary);
> + free(boundary);
> + boundary = NULL;
> return;
> }
> *(mi->content_top) = boundary;
> -- 
> 2.34.1 
May be using goto here would be better. Like:
---
Show changes to mailinfo.c +2 −1
diff --git a/mailinfo.c b/mailinfo.c
index ee4597da6b..83358b7517 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -266,13 +266,14 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
                        error("Too many boundaries to handle");
                        mi->input_error = -1;
                        mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
-                       return;
+                       goto out;
                }
                *(mi->content_top) = boundary;
                boundary = NULL;
        }
        slurp_attr(line->buf, "charset=", &mi->charset);
 
+out:
        if (boundary) {
                strbuf_release(boundary);
                free(boundary);
—

Lidong
Junio C Hamano· Jun 16, 2025, 01:01 UTC · re: Lidong Yan · lore

Re: [PATCH] Fix memory leak in function handle_content_type

Lidong Yan <yldhome2d2@gmail.com> writes:
> Subject: Re: [PATCH] Fix memory leak in function handle_content_type
The subject should probably be something like
	Subject: [PATCH] mailinfo.c: plug memory leak in handle_content_type()
Show 24 quoted lines
> May be using goto here would be better. Like:
>
> ---
> diff --git a/mailinfo.c b/mailinfo.c
> index ee4597da6b..83358b7517 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -266,13 +266,14 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
>                         error("Too many boundaries to handle");
>                         mi->input_error = -1;
>                         mi->content_top = &mi->content[MAX_BOUNDARIES] - 1;
> -                       return;
> +                       goto out;
>                 }
>                 *(mi->content_top) = boundary;
>                 boundary = NULL;
>         }
>         slurp_attr(line->buf, "charset=", &mi->charset);
>  
> +out:
>         if (boundary) {
>                 strbuf_release(boundary);
>                 free(boundary);
> —
Yup, that one looks good enough.

If we wanted to clean up this code path even further, I think the first clean-up to do is to reconsider the use of "struct strbuf *" (instead of "const char *") in *(mi->content_top). strbuf is a fine and less error-prone mechanism to use while you have to manipulate character strings (like parsing from input line to formulate the boundary string), but once this function computed what was asked by the caller, the computed result (like the boundary string) almost always do not need to be editable. But such a code improvement is totally outside the topic of this patch.

← back to recent threads