threads / patch / 63642

patchAllocate msg only after fatal checks to avoid leaks

Subject: [PATCH] Allocate msg only after fatal checks to avoid leaks

## tl;dr

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

replies: 6people: 4as markdown or json

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

In parse_reuse_arg, we previously called xmalloc and strbuf_init before resolving the ref and reading the object, leading to a leaked msg on die() paths. This change moves the allocation of struct note_msg until after repo_get_oid and repo_read_object_file succeed, ensuring no heap memory is held when a fatal error is triggered.

Signed-off-by: jinyaoguo <guo846@purdue.edu>
---
    Allocate msg only after fatal checks to avoid leaks
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1998%2Fmugitya03%2Fmlk-3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1998/mugitya03/mlk-3-v1
Pull-Request: https://github.com/git/git/pull/1998
 builtin/notes.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to builtin/notes.c +8 −8
diff --git a/builtin/notes.c b/builtin/notes.c
index a3f433ca4c0..6df8a7998fb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -308,7 +308,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
 static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 {
 	struct note_data *d = opt->value;
-	struct note_msg *msg = xmalloc(sizeof(*msg));
+	struct note_msg *msg;
 	char *value;
 	struct object_id object;
 	enum object_type type;
@@ -316,17 +316,17 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 
 	BUG_ON_OPT_NEG(unset);
 
-	strbuf_init(&msg->buf, 0);
 	if (repo_get_oid(the_repository, arg, &object))
 		die(_("failed to resolve '%s' as a valid ref."), arg);
 	if (!(value = repo_read_object_file(the_repository, &object, &type, &len)))
 		die(_("failed to read object '%s'."), arg);
-	if (type != OBJ_BLOB) {
-		strbuf_release(&msg->buf);
-		free(value);
-		free(msg);
-		die(_("cannot read note data from non-blob object '%s'."), arg);
-	}
+    if (type != OBJ_BLOB) {
+        free(value);
+        die(_("cannot read note data from non-blob object '%s'."), arg);
+    }
+
+    msg = xmalloc(sizeof(*msg));
+    strbuf_init(&msg->buf, 0);
 
 	strbuf_add(&msg->buf, value, len);
 	free(value);

base-commit: 9edff09aec9b5aaa3d5528129bb279a4d34cf5b3

-- 
gitgitgadget
Junio C Hamano· Jun 13, 2025, 20:37 UTC · re: Alex via GitGitGadget · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

"Alex via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 13 quoted lines
> -	if (type != OBJ_BLOB) {
> -		strbuf_release(&msg->buf);
> -		free(value);
> -		free(msg);
> -		die(_("cannot read note data from non-blob object '%s'."), arg);
> -	}
> +    if (type != OBJ_BLOB) {
> +        free(value);
> +        die(_("cannot read note data from non-blob object '%s'."), arg);
> +    }
> +
> +    msg = xmalloc(sizeof(*msg));
> +    strbuf_init(&msg->buf, 0);

ALl the new lines seem to be indented by four spaces. Check with Documantation/CodingGuidelines.

Also, Documantation/SubmittingPatches::[[real-name]] asks folks to use their real name as authorname. You prefer your purdue address, that is fine, but let's do something like

    From: Jinyao Guo <guo846@purdue.edu>
    Signed-off-by: Jinyao Guo <guo846@purdue.edu>
lidongyan· Jun 14, 2025, 08:26 UTC · re: Alex via GitGitGadget · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

Alex via GitGitGadget <gitgitgadget@gmail.com> writes：
Show 6 quoted lines
> 
> From: jinyaoguo <guo846@purdue.edu>
> 
> In parse_reuse_arg, we previously called xmalloc and strbuf_init
> before resolving the ref and reading the object, leading to a
> leaked msg on die() paths. This change moves the allocation of

A memory leak on the die() path shouldn't be considered a real leak, right? Since the OS will clean up all memory once the process terminates, explicitly freeing msg isn't necessary in this case.

Lidong
Junio C Hamano· Jun 14, 2025, 15:40 UTC · re: lidongyan · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

lidongyan <502024330056@smail.nju.edu.cn> writes:
Show 11 quoted lines
> Alex via GitGitGadget <gitgitgadget@gmail.com> writes：
>> 
>> From: jinyaoguo <guo846@purdue.edu>
>> 
>> In parse_reuse_arg, we previously called xmalloc and strbuf_init
>> before resolving the ref and reading the object, leading to a
>> leaked msg on die() paths. This change moves the allocation of
>
> A memory leak on the die() path shouldn't be considered a real leak,
> right? Since the OS will clean up all memory once the process
> terminates, explicitly freeing msg isn't necessary in this case.

It may not matter in practice, but I think the leak checking machinery like sanitizers would still complain, so I view efforts on plugging such leaks in the error code paths more about decluttering the leak checker output to help us spot the real leaks.

lidongyan· Jun 14, 2025, 15:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

Junio C Hamano <gitster@pobox.com> writes：
Show 19 quoted lines
> 
> lidongyan <502024330056@smail.nju.edu.cn> writes:
> 
>> Alex via GitGitGadget <gitgitgadget@gmail.com> writes：
>>> 
>>> From: jinyaoguo <guo846@purdue.edu>
>>> 
>>> In parse_reuse_arg, we previously called xmalloc and strbuf_init
>>> before resolving the ref and reading the object, leading to a
>>> leaked msg on die() paths. This change moves the allocation of
>> 
>> A memory leak on the die() path shouldn't be considered a real leak,
>> right? Since the OS will clean up all memory once the process
>> terminates, explicitly freeing msg isn't necessary in this case.
> 
> It may not matter in practice, but I think the leak checking
> machinery like sanitizers would still complain, so I view efforts on
> plugging such leaks in the error code paths more about decluttering
> the leak checker output to help us spot the real leaks.

Makes sense. However, inserting a free-like statement in die() would be messier than using goto, since each die() has a unique message and we need to free stuff at each die().

Jeff King· Jun 14, 2025, 23:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

On Sat, Jun 14, 2025 at 08:40:43AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> > A memory leak on the die() path shouldn't be considered a real leak,
> > right? Since the OS will clean up all memory once the process
> > terminates, explicitly freeing msg isn't necessary in this case.
> 
> It may not matter in practice, but I think the leak checking
> machinery like sanitizers would still complain, so I view efforts on
> plugging such leaks in the error code paths more about decluttering
> the leak checker output to help us spot the real leaks.

I disagree here. These are not really leaks, and a leak-checker that complains about them is bad.

When we call die(), the pointer to the buffer is still on the stack, and thus the memory is still reachable and not leaked. Some tools like valgrind may still report these as "still reachable", but because they categorize them properly we can ignore them[1].

The one exception we've seen is that an optimizing compiler may reorder instructions to obliterate the stack (because it knows die() is marked with NORETURN), causing a false positive. We dealt with that via d3775de074 (Makefile: force -O0 when compiling with SANITIZE=leak, 2022-10-18). I don't think we've seen any recurrence since then.

And while it may be tempting to say "well, it does not hurt to free them on the die() path", in my opinion that way madness lies. You may have access to some local variables that can be freed, but there will be many other heap allocations that you don't even know about! Here's a toy example from a similar discussion a few years ago:

  https://lore.kernel.org/git/YNypPeoZTRiOxPPQ@coredump.intra.peff.net/

So I'd really prefer not to go down this route. And I think the existing code in this patch's pre-image that calls free() before die() only on one path should be simplified, so that all die() paths consistently do not worry about this.

I.e., this:
Show changes to builtin/notes.c +1 −6
diff --git a/builtin/notes.c b/builtin/notes.c
index cc1163242f..f3d5eda104 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -321,12 +321,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 		die(_("failed to resolve '%s' as a valid ref."), arg);
 	if (!(value = odb_read_object(the_repository->objects, &object, &type, &len)))
 		die(_("failed to read object '%s'."), arg);
-	if (type != OBJ_BLOB) {
-		strbuf_release(&msg->buf);
-		free(value);
-		free(msg);
+	if (type != OBJ_BLOB)
 		die(_("cannot read note data from non-blob object '%s'."), arg);
-	}
 
 	strbuf_add(&msg->buf, value, len);
 	free(value);

-Peff

[1] "still reachable" leaks _can_ be useful when returning from main,
    because they may show memory held in global structures that we might
    have been able to free at a more timely spot. But there are a lot of
    these in Git, most of which are not interesting (e.g., is freeing
    the_repository really worth caring about?), and I don't think there
    is a good way to tell the difference.

    More pontificating from that earlier discussion:

      https://lore.kernel.org/git/YN3iIaovvG7XgLQP@coredump.intra.peff.net/
Junio C Hamano· Jun 15, 2025, 00:45 UTC · re: Jeff King · lore

Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> And while it may be tempting to say "well, it does not hurt to free them
> on the die() path", in my opinion that way madness lies. You may have
> access to some local variables that can be freed, but there will be many
> other heap allocations that you don't even know about! Here's a toy
> example from a similar discussion a few years ago:
>
>   https://lore.kernel.org/git/YNypPeoZTRiOxPPQ@coredump.intra.peff.net/

Yeah, I recall that discussion and the example. Yes, we should not have to crawl up from a direct caller of die() and free everything these stack frames hold.

Show 17 quoted lines
> I.e., this:
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index cc1163242f..f3d5eda104 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -321,12 +321,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
>  		die(_("failed to resolve '%s' as a valid ref."), arg);
>  	if (!(value = odb_read_object(the_repository->objects, &object, &type, &len)))
>  		die(_("failed to read object '%s'."), arg);
> -	if (type != OBJ_BLOB) {
> -		strbuf_release(&msg->buf);
> -		free(value);
> -		free(msg);
> +	if (type != OBJ_BLOB)
>  		die(_("cannot read note data from non-blob object '%s'."), arg);
> -	}
Much nicer.

← back to recent threads