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

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

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

find_cfg_ent() allocates a struct reflog_expire_entry_option via
FLEX_ALLOC_MEM and inserts it into a linked list in the
reflog_expire_options structure. The entries in this list are never
freed, resulting in a leak in cmd_reflog_expire and the gc reflog expire
maintenance task:

Direct leak of 39 byte(s) in 1 object(s) allocated from:
    #0 0x7ff975ee6883 in calloc (/lib64/libasan.so.8+0xe6883)
    #1 0x0000010edada in xcalloc ../wrapper.c:154
    #2 0x000000df0898 in find_cfg_ent ../reflog.c:28
    #3 0x000000df0898 in reflog_expire_config ../reflog.c:70
    #4 0x00000095c451 in configset_iter ../config.c:2116
    #5 0x0000006d29e7 in git_config ../config.h:724
    #6 0x0000006d29e7 in cmd_reflog_expire ../builtin/reflog.c:205
    #7 0x0000006d504c in cmd_reflog ../builtin/reflog.c:419
    #8 0x0000007e4054 in run_builtin ../git.c:480
    #9 0x0000007e4054 in handle_builtin ../git.c:746
    #10 0x0000007e8a35 in run_argv ../git.c:813
    #11 0x0000007e8a35 in cmd_main ../git.c:953
    #12 0x000000441e8f in main ../common-main.c:9
    #13 0x7ff9754115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4)
    #14 0x7ff9754116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7)
    #15 0x000000444184 in _start (/home/jekeller/libexec/git-core/git+0x444184)

Close this leak by adding a reflog_clear_expire_config() function which
iterates the linked list and frees its elements. Call it upon exit of
cmd_reflog_expire() and in reflog_expiry_cleanup().

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v2:
- Actually fix the leak properly. (Thanks Jeff for catching my brain fart!)
- Link to v1: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v1-1-34d5461cf8f5@gmail.com
---
 reflog.h         |  2 ++
 builtin/reflog.c |  3 +++
 reflog.c         | 15 +++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/reflog.h b/reflog.h
index 63bb56280f4e..74b3f3c4f0ac 100644
--- a/reflog.h
+++ b/reflog.h
@@ -34,6 +34,8 @@ struct reflog_expire_options {
 int reflog_expire_config(const char *var, const char *value,
 			 const struct config_context *ctx, void *cb);
 
+void reflog_clear_expire_config(struct reflog_expire_options *opts);
+
 /*
  * Adapt the options so that they apply to the given refname. This applies any
  * per-reference reflog expiry configuration that may exist to the options.
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3acaf3e32c27..d4da41aaea73 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -283,6 +283,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 					     &cb);
 		free(ref);
 	}
+
+	reflog_clear_expire_config(&opts);
+
 	return status;
 }
 
diff --git a/reflog.c b/reflog.c
index 15d81ebea978..3ce1780924dd 100644
--- a/reflog.c
+++ b/reflog.c
@@ -81,6 +81,20 @@ int reflog_expire_config(const char *var, const char *value,
 	return 0;
 }
 
+void reflog_clear_expire_config(struct reflog_expire_options *opts)
+{
+	struct reflog_expire_entry_option *ent = opts->entries, *tmp;
+
+	while (ent) {
+		tmp = ent;
+		ent = ent->next;
+		free(tmp);
+	}
+
+	opts->entries = NULL;
+	opts->entries_tail = NULL;
+}
+
 void reflog_expire_options_set_refname(struct reflog_expire_options *cb,
 				       const char *ref)
 {
@@ -490,6 +504,7 @@ void reflog_expiry_cleanup(void *cb_data)
 	for (elem = cb->mark_list; elem; elem = elem->next)
 		clear_commit_marks(elem->item, REACHABLE);
 	free_commit_list(cb->mark_list);
+	reflog_clear_expire_config(&cb->opts);
 }
 
 int count_reflog_ent(struct object_id *ooid UNUSED,

---
base-commit: a30f80fde927d70950b3b4d1820813480968fb0d
change-id: 20250709-jk-fix-leak-reflog-expire-config-712ca6dc685a

Best regards,
--  
Jacob Keller <jacob.keller@gmail.com>

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

[Lidong Yan]

Jacob Keller <jacob.e.keller@intel.com> wrote:
> 
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> find_cfg_ent() allocates a struct reflog_expire_entry_option via
> FLEX_ALLOC_MEM and inserts it into a linked list in the
> reflog_expire_options structure. The entries in this list are never
> freed, resulting in a leak in cmd_reflog_expire and the gc reflog expire
> maintenance task:
> 
> Direct leak of 39 byte(s) in 1 object(s) allocated from:
>    #0 0x7ff975ee6883 in calloc (/lib64/libasan.so.8+0xe6883)
>    #1 0x0000010edada in xcalloc ../wrapper.c:154
>    #2 0x000000df0898 in find_cfg_ent ../reflog.c:28
>    #3 0x000000df0898 in reflog_expire_config ../reflog.c:70
>    #4 0x00000095c451 in configset_iter ../config.c:2116
>    #5 0x0000006d29e7 in git_config ../config.h:724
>    #6 0x0000006d29e7 in cmd_reflog_expire ../builtin/reflog.c:205
>    #7 0x0000006d504c in cmd_reflog ../builtin/reflog.c:419
>    #8 0x0000007e4054 in run_builtin ../git.c:480
>    #9 0x0000007e4054 in handle_builtin ../git.c:746
>    #10 0x0000007e8a35 in run_argv ../git.c:813
>    #11 0x0000007e8a35 in cmd_main ../git.c:953
>    #12 0x000000441e8f in main ../common-main.c:9
>    #13 0x7ff9754115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4)
>    #14 0x7ff9754116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7)
>    #15 0x000000444184 in _start (/home/jekeller/libexec/git-core/git+0x444184)
> 
> Close this leak by adding a reflog_clear_expire_config() function which
> iterates the linked list and frees its elements. Call it upon exit of
> cmd_reflog_expire() and in reflog_expiry_cleanup().
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> Changes in v2:
> - Actually fix the leak properly. (Thanks Jeff for catching my brain fart!)
> - Link to v1: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v1-1-34d5461cf8f5@gmail.com
> ---
> reflog.h         |  2 ++
> builtin/reflog.c |  3 +++
> reflog.c         | 15 +++++++++++++++
> 3 files changed, 20 insertions(+)
> 
> diff --git a/reflog.h b/reflog.h
> index 63bb56280f4e..74b3f3c4f0ac 100644
> --- a/reflog.h
> +++ b/reflog.h
> @@ -34,6 +34,8 @@ struct reflog_expire_options {
> int reflog_expire_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
> 
> +void reflog_clear_expire_config(struct reflog_expire_options *opts);
> +
> /*
>  * Adapt the options so that they apply to the given refname. This applies any
>  * per-reference reflog expiry configuration that may exist to the options.
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 3acaf3e32c27..d4da41aaea73 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -283,6 +283,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
>     &cb);
> free(ref);
> }
> +
> + reflog_clear_expire_config(&opts);
> +
> return status;
> }
> 
> diff --git a/reflog.c b/reflog.c
> index 15d81ebea978..3ce1780924dd 100644
> --- a/reflog.c
> +++ b/reflog.c
> @@ -81,6 +81,20 @@ int reflog_expire_config(const char *var, const char *value,
> return 0;
> }
> 
> +void reflog_clear_expire_config(struct reflog_expire_options *opts)
> +{
> + struct reflog_expire_entry_option *ent = opts->entries, *tmp;
> +
> + while (ent) {
> + tmp = ent;
> + ent = ent->next;
> + free(tmp);
> + }
> +
> + opts->entries = NULL;
> + opts->entries_tail = NULL;
> +}
> +

This looks correct.

> void reflog_expire_options_set_refname(struct reflog_expire_options *cb,
>       const char *ref)
> {
> @@ -490,6 +504,7 @@ void reflog_expiry_cleanup(void *cb_data)
> for (elem = cb->mark_list; elem; elem = elem->next)
> clear_commit_marks(elem->item, REACHABLE);
> free_commit_list(cb->mark_list);
> + reflog_clear_expire_config(&cb->opts);
> }
> 
> int count_reflog_ent(struct object_id *ooid UNUSED,
> 

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.

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

[Jeff King]

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;
 }
 

-Peff

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

[Jacob Keller]

[-- 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 --]