Re: [PATCH] format-patch: unleak "-v <num>"

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2023, 18:35 UTC
Message-ID: <xmqqpmbecoom.fsf@gitster.g>
In-Reply-To: <Y8WJnGHs5nM5GwBM@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:
>
>> The "subject_prefix" member of "struct revision" usually is always
>> set to a borrowed string (either a string literal like "PATCH" that
>> appear in the program text as a hardcoded default, or the value of
>> "format.subjectprefix") and is never freed when the containing
>> revision structure is released.  The "-v <num>" codepath however
>> violates this rule and stores a pointer to an allocated string to
>> this member, relinquishing the responsibility to free it when it is
>> done using the revision structure, leading to a small one-time leak.
>> 
>> Instead, keep track of the string it allocates to let the revision
>> structure borrow, and clean it up when it is done.
>
> FWIW, this looks obviously correct to me.
>
> The word "unleak" in the subject made me think about UNLEAK(), so this
> is a small tangent. This is exactly the kind of case that I designed
> UNLEAK() for, because the solution really is "while you are assigning to
> X, also keep a copy of the pointer in Y to be freed later".

Yup. I was originally planning to use UNLEAK(), but it felt ugly to UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes and owned pointer some other times, which is the exact reason why I started looking for a clean way to plug this leak. So I ended up with declaring that the member should only store a borrowed pointer.

Show 5 quoted lines

> And UNLEAK() is just "keep a copy of the pointer in Y to know that we
> _could_ free it later". And of course "do nothing if we are not
> leak-detecting". But since we seem to be moving away from UNLEAK(), and
> since it would not even save any lines here, I'm perfectly happy with
> this solution.

The first sentence needs to be rephrased, as it does not make much sense to have something usually be X and always be X at the same time (I'd just remove "always" from there).

Thanks.