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

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2237 bytes --]



On 7/9/2025 8:42 PM, Jeff King wrote:
> On Thu, Jul 10, 2025 at 11:00:38AM +0800, Lidong Yan wrote:
> 
>> 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;

Ya, you're right. I just thought the reflog_expiry_cleanup would only be
called by this function. It did pass the tests... I'll see if I can add
a test case covering this since its caused a bit more trouble than I
thought it would.

>  }
>  
> 
> -Peff


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]