Re: [PATCH] Allocate msg only after fatal checks to avoid leaks
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 15, 2025, 00:45 UTC
- Message-ID
- <xmqqh60h6ddy.fsf@gitster.g>
- In-Reply-To
- <20250614230158.GA2568638@coredump.intra.peff.net>
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.