format-patch: unleak "-v <num>"

Commit Message

Junio C Hamano Jan. 15, 2023, 8:03 a.m. UTC

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.

Signed-off-by: Junio C Hamano <[e-mail]>
---
 builtin/log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jeff King Jan. 16, 2023, 5:30 p.m. UTC | #1

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".

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.

-Peff

Junio C Hamano Jan. 16, 2023, 6:35 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> 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.

> 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.

Jeff King Jan. 16, 2023, 7:51 p.m. UTC | #3

On Mon, Jan 16, 2023 at 10:35:53AM -0800, Junio C Hamano wrote:

> > 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.

That's actually one of the nice things about UNLEAK(). It is OK to
over-mark something that may or may not be allocated.

> 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).

Yep, agreed.

-Peff

Patch

diff --git a/builtin/log.c b/builtin/log.c
index e1fe7bbe71..3dddaadc1e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1879,6 +1879,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf rdiff1 = STRBUF_INIT;
 	struct strbuf rdiff2 = STRBUF_INIT;
 	struct strbuf rdiff_title = STRBUF_INIT;
+	struct strbuf sprefix = STRBUF_INIT;
 	int creation_factor = -1;
 
 	const struct option builtin_format_patch_options[] = {
@@ -2019,12 +2020,10 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
 	if (reroll_count) {
-		struct strbuf sprefix = STRBUF_INIT;
-
 		strbuf_addf(&sprefix, "%s v%s",
 			    rev.subject_prefix, reroll_count);
 		rev.reroll_count = reroll_count;
-		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
+		rev.subject_prefix = sprefix.buf;
 	}
 
 	for (i = 0; i < extra_hdr.nr; i++) {
@@ -2388,6 +2387,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	strbuf_release(&rdiff1);
 	strbuf_release(&rdiff2);
 	strbuf_release(&rdiff_title);
+	strbuf_release(&sprefix);
 	free(to_free);
 	if (rev.ref_message_ids)
 		string_list_clear(rev.ref_message_ids, 0);