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
