Re: [PATCH v2] reflog: close leak of reflog expire entry

From: Jeff King <peff@peff.net>
Date: Jul 10, 2025, 03:42 UTC
Message-ID: <20250710034241.GA2057509@coredump.intra.peff.net>
In-Reply-To: <D34FE2DE-EE5B-43F3-A706-1AC133AA72F1@gmail.com>

On Thu, Jul 10, 2025 at 11:00:38AM +0800, Lidong Yan wrote:

Show 25 quoted lines

> In builtin/reflog.c, we have code like
> 
> ---
> 	for (i = 0; i < argc; i++) {
> 		char *ref;
> 		struct expire_reflog_policy_cb cb = { .opts = opts };
> 
> 		if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {
> 			status |= error(_("reflog could not be found: '%s'"), argv[i]);
> 			continue;
> 		}
> 		reflog_expire_options_set_refname(&cb.opts, ref);
> 		status |= refs_reflog_expire(get_main_ref_store(the_repository),
> 					     ref, flags,
> 					     reflog_expiry_prepare,
> 					     should_prune_fn,
> 					     reflog_expiry_cleanup,
> 					     &cb);
> 		free(ref);
> 	}
> +      reflog_clear_expire_config(&opts);
> ---
> 
> I think allowing reblog_expiry_cleanup() to free all opt->entries might
> cause reblog_expire_options_set_refname() to behave incorrectly.

Hmm, yeah. We are calling this in a loop, so we'd want the config to persist until the loop ends. I didn't test, but I'd guess that:

  git -c 'gc.refs/heads/*.reflogExpire=now' \
    reflog expire refs/heads/foo refs/heads/bar

would apply the config for "foo" but not for "bar". So I think reflog_expiry_cleanup() has to just clean up per-traversal data, not the config.

So the call at the end here looks reasonable, but the call in reflog_expiry_cleanup() is wrong. I guess it was trying to cover the call in reflog_expire_condition(). That probably just needs a manual:

diff --git a/builtin/gc.c b/builtin/gc.c
index 845876ff02..37f5437365 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -346,6 +346,7 @@ static int reflog_expire_condition(struct gc_config *cfg UNUSED)
 				 count_reflog_entries, &data);
 
 	reflog_expiry_cleanup(&data.policy);
+	reflog_clear_expire_config(&data.policy);
 	return data.count >= data.limit;
 }
 

-Peff