Re: [PATCH v2] builtin/log.c: prepend "RFC" on --rfc

[Taylor Blau <me@ttaylorr.com>]

On Mon, Aug 28, 2023 at 09:15:30AM -0700, Junio C Hamano wrote:
> In the current code before this patch, the rev.subject_prefix member
> holds one of:
>
>  * hardcoded "PATCH" in BSS (i.e. fmt_patch_subject_prefix)
>  * hardcoded "RFC PATCH" in BSS when "--rfc" is given
>  * value given to command line arg "--subject-prefix=<prefix>"
>  * value given to format.subjectprefix
>
> and the last one should be freed.  We are removing the second one
> and replacing it with whatever we will do when adding this feature
> so we should be able to make it freeable.  And I do not think it is
> hard to make the third one freeable.
>
> I wonder how far we can go to plug this leak by simply
>
>  - making subject_prefix_callback() xstrdup() its arg and free the
>    current value, unless it is the same pointer as
>    fmt_patch_subject_prefix, before assigning a new value, and
>
>  - making "format.subjectprefix" take the value in a temporary
>    variable from git_config_string(), call
>    subject_prefix_callback(), and free that temporary.

I am not super familiar with this code, so could easily be missing
something here, but I think that you can do this in a more direct way
like so:

--- 8< ---
diff --git a/builtin/log.c b/builtin/log.c
index 854216ee9c..f1c6c08f75 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -67,6 +67,7 @@ static const char *fmt_patch_subject_prefix = "PATCH";
 static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
 static const char *fmt_pretty;
 static int format_no_prefix;
+static char *subject_prefix;

 static const char * const builtin_log_usage[] = {
 	N_("git log [<options>] [<revision-range>] [[--] <path>...]"),
@@ -362,6 +363,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 static int cmd_log_deinit(int ret, struct rev_info *rev)
 {
 	release_revisions(rev);
+	free(subject_prefix);
 	return ret;
 }

@@ -1463,32 +1465,27 @@ static int keep_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }

-static int subject_prefix = 0;
-
 static int subject_prefix_callback(const struct option *opt, const char *arg,
 			    int unset)
 {
+	struct rev_info *revs = opt->value;
 	BUG_ON_OPT_NEG(unset);
-	subject_prefix = 1;
-	((struct rev_info *)opt->value)->subject_prefix = arg;
+
+	revs->subject_prefix = arg;
+
 	return 0;
 }

 static int rfc_callback(const struct option *opt, const char *arg, int unset)
 {
-	/*
-	 * The subject_prefix in rev_info is not heap-allocated except in this
-	 * specific case, so there is no obvious place to free it. Since this
-	 * value is retained for the lifetime of the process, we just
-	 * statically allocate storage for it here.
-	 */
-	static char *prefix;
+	struct rev_info *revs = opt->value;
+
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);

-	free(prefix);
-	prefix = xstrfmt("RFC %s", ((struct rev_info *)opt->value)->subject_prefix);
-	return subject_prefix_callback(opt, prefix, unset);
+	free(subject_prefix);
+	subject_prefix = xstrfmt("RFC %s", revs->subject_prefix);
+	return subject_prefix_callback(opt, subject_prefix, unset);
 }

 static int numbered_cmdline_opt = 0;
--- >8 ---

since we already have cmd_log_deinit(), which currently just calls
`release_revisions()` and then propagates a return code.

We can't foist freeing the subject_prefix into release_revisions, since
(as noted above), sometimes the value will point into the program's BSS.

But the only time we care about free()-ing subject_prefix is when we
xstrdup() a new value into it, and the only place we do that is in
rfc_callback().

Thanks,
Taylor