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

[PATCH v3] 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 reflog_expire_condition().

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v3:
- Remove the incorrect call in reflog_expiry_cleanup()
- Add a call in reflog_expire_condition()
- Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@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/gc.c     |  1 +
 builtin/reflog.c |  3 +++
 reflog.c         | 14 ++++++++++++++
 4 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/gc.c b/builtin/gc.c
index 845876ff0286..37f543736599 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;
 }
 
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..e2a2f3ad3e30 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)
 {

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

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

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

[Jeff King]

On Mon, Jul 21, 2025 at 04:39:37PM -0700, Jacob Keller wrote:

> Changes in v3:
> - Remove the incorrect call in reflog_expiry_cleanup()
> - Add a call in reflog_expire_condition()
> - Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com

This looks correct to me except...

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 845876ff0286..37f543736599 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;
>  }
>  

This needs to pass &data.policy.opts, no?

I think we might also want this test on top (or I'd be happy to see it
squashed in). It shows off your fix when built with SANITIZE=leak, and
also catches the bug that v2 of your patch had.

-Peff

-- >8 --
Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config

We have long supported the ability to set reflog expiration config for
individual, going back to 3cb22b8efe (Per-ref reflog expiry
configuration, 2008-06-15). But we have never had any tests.

Let's add a very basic one that checks that we apply the config
correctly to a subset of refs (and not elsewhere). This also
triggers the leaky code fixed by the previous commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 42b501f163..362e90d7d6 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '
 	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
 '
 
+test_expect_success 'expire with pattern config' '
+	# Split refs/heads/ into two roots so we can apply config to each. Make
+	# two branches per root to verify that config is applied correctly
+	# multiple times.
+	git branch root1/branch1 &&
+	git branch root1/branch2 &&
+	git branch root2/branch1 &&
+	git branch root2/branch2 &&
+
+	test_config "gc.reflogexpire" "never" &&
+	test_config "gc.refs/heads/root2/*.reflogExpire" "now" &&
+	git reflog expire \
+		root1/branch1 root1/branch2 \
+		root2/branch1 root2/branch2 &&
+
+	cat >expect <<-\EOF &&
+	root1/branch1@{0}
+	root1/branch2@{0}
+	EOF
+	git log -g --branches="root*" --format=%gD >actual.raw &&
+	# The sole reflog entry of each branch points to the same commit, so
+	# the order in which they are shown is nondeterministic. We just care
+	# about the what was expired (and what was not), so sort to get a known
+	# order.
+	sort <actual.raw >actual.sorted &&
+	test_cmp expect actual.sorted
+'
+
 test_expect_success 'checkout should not delete log for packed ref' '
 	test $(git reflog main | wc -l) = 4 &&
 	git branch foo &&
-- 
2.50.1.589.g6e88b11be3

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config
>
> We have long supported the ability to set reflog expiration config for
> individual, going back to 3cb22b8efe (Per-ref reflog expiry
> configuration, 2008-06-15). But we have never had any tests.

Yikes.  I completely forgot adding that feature, but it seems I also
forgot to add tests when I added it.  My bad.

"individual" -> "individual refs" or something?  I was confused
after my initial read, which sounded as if we are talking about
allowing individual users to set the configuration variable ;-)

> Let's add a very basic one that checks that we apply the config
> correctly to a subset of refs (and not elsewhere). This also
> triggers the leaky code fixed by the previous commit.

Thanks.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 42b501f163..362e90d7d6 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '
>  	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
>  '
>  
> +test_expect_success 'expire with pattern config' '
> +	# Split refs/heads/ into two roots so we can apply config to each. Make
> +	# two branches per root to verify that config is applied correctly
> +	# multiple times.
> +	git branch root1/branch1 &&
> +	git branch root1/branch2 &&
> +	git branch root2/branch1 &&
> +	git branch root2/branch2 &&
> +
> +	test_config "gc.reflogexpire" "never" &&
> +	test_config "gc.refs/heads/root2/*.reflogExpire" "now" &&
> +	git reflog expire \
> +		root1/branch1 root1/branch2 \
> +		root2/branch1 root2/branch2 &&
> +
> +	cat >expect <<-\EOF &&
> +	root1/branch1@{0}
> +	root1/branch2@{0}
> +	EOF
> +	git log -g --branches="root*" --format=%gD >actual.raw &&
> +	# The sole reflog entry of each branch points to the same commit, so
> +	# the order in which they are shown is nondeterministic. We just care
> +	# about the what was expired (and what was not), so sort to get a known
> +	# order.
> +	sort <actual.raw >actual.sorted &&
> +	test_cmp expect actual.sorted
> +'
> +
>  test_expect_success 'checkout should not delete log for packed ref' '
>  	test $(git reflog main | wc -l) = 4 &&
>  	git branch foo &&

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

[Jeff King]

On Tue, Jul 22, 2025 at 07:09:20AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config
> >
> > We have long supported the ability to set reflog expiration config for
> > individual, going back to 3cb22b8efe (Per-ref reflog expiry
> > configuration, 2008-06-15). But we have never had any tests.
> 
> Yikes.  I completely forgot adding that feature, but it seems I also
> forgot to add tests when I added it.  My bad.
> 
> "individual" -> "individual refs" or something?  I was confused
> after my initial read, which sounded as if we are talking about
> allowing individual users to set the configuration variable ;-)

Yep, exactly. I admit I didn't spend as much time on the commit message
for this one, since I figured it might just get squashed anyway. :)

-Peff

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

[Jacob Keller]

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



On 7/21/2025 9:54 PM, Jeff King wrote:
> On Mon, Jul 21, 2025 at 04:39:37PM -0700, Jacob Keller wrote:
> 
>> Changes in v3:
>> - Remove the incorrect call in reflog_expiry_cleanup()
>> - Add a call in reflog_expire_condition()
>> - Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com
> 
> This looks correct to me except...
> 
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index 845876ff0286..37f543736599 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;
>>  }
>>  
> 
> This needs to pass &data.policy.opts, no?
> 

You're right... I think I fixed that and forgot to actually commit it
before sending. Ugh.

> I think we might also want this test on top (or I'd be happy to see it
> squashed in). It shows off your fix when built with SANITIZE=leak, and
> also catches the bug that v2 of your patch had.
> 
> -Peff
> 

Sounds good. I'll send a v4 which squashes this in.

> -- >8 --
> Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config
> 
> We have long supported the ability to set reflog expiration config for
> individual, going back to 3cb22b8efe (Per-ref reflog expiry
> configuration, 2008-06-15). But we have never had any tests.
> 
> Let's add a very basic one that checks that we apply the config
> correctly to a subset of refs (and not elsewhere). This also
> triggers the leaky code fixed by the previous commit.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 42b501f163..362e90d7d6 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '
>  	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
>  '
>  
> +test_expect_success 'expire with pattern config' '
> +	# Split refs/heads/ into two roots so we can apply config to each. Make
> +	# two branches per root to verify that config is applied correctly
> +	# multiple times.
> +	git branch root1/branch1 &&
> +	git branch root1/branch2 &&
> +	git branch root2/branch1 &&
> +	git branch root2/branch2 &&
> +
> +	test_config "gc.reflogexpire" "never" &&
> +	test_config "gc.refs/heads/root2/*.reflogExpire" "now" &&
> +	git reflog expire \
> +		root1/branch1 root1/branch2 \
> +		root2/branch1 root2/branch2 &&
> +
> +	cat >expect <<-\EOF &&
> +	root1/branch1@{0}
> +	root1/branch2@{0}
> +	EOF
> +	git log -g --branches="root*" --format=%gD >actual.raw &&
> +	# The sole reflog entry of each branch points to the same commit, so
> +	# the order in which they are shown is nondeterministic. We just care
> +	# about the what was expired (and what was not), so sort to get a known
> +	# order.
> +	sort <actual.raw >actual.sorted &&
> +	test_cmp expect actual.sorted
> +'
> +
>  test_expect_success 'checkout should not delete log for packed ref' '
>  	test $(git reflog main | wc -l) = 4 &&
>  	git branch foo &&


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

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

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

>> This needs to pass &data.policy.opts, no?
>> 
>
> You're right... I think I fixed that and forgot to actually commit it
> before sending. Ugh.
>
>> I think we might also want this test on top (or I'd be happy to see it
>> squashed in). It shows off your fix when built with SANITIZE=leak, and
>> also catches the bug that v2 of your patch had.
>> 
>> -Peff
>> 
>
> Sounds good. I'll send a v4 which squashes this in.

OK, or you can tell me to squash what I queued on the
jk/unleak-reflog-expire-entry topic that ends at 7c091149 (fixup!
reflog: close leak of reflog expire entry, 2025-07-22) down into a
single patch (or two to keep Peff's test saparate).

Thanks.

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

[Jacob Keller]

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



On 7/22/2025 4:21 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>>> This needs to pass &data.policy.opts, no?
>>>
>>
>> You're right... I think I fixed that and forgot to actually commit it
>> before sending. Ugh.
>>
>>> I think we might also want this test on top (or I'd be happy to see it
>>> squashed in). It shows off your fix when built with SANITIZE=leak, and
>>> also catches the bug that v2 of your patch had.
>>>
>>> -Peff
>>>
>>
>> Sounds good. I'll send a v4 which squashes this in.
> 
> OK, or you can tell me to squash what I queued on the
> jk/unleak-reflog-expire-entry topic that ends at 7c091149 (fixup!
> reflog: close leak of reflog expire entry, 2025-07-22) down into a
> single patch (or two to keep Peff's test saparate).
> 
> Thanks.

I am about to send a v4 that squashes Peff's work in and adds a
Co-developed-by tag. I think that makes the most sense.

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

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

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> I am about to send a v4 that squashes Peff's work in and adds a
> Co-developed-by tag. I think that makes the most sense.

That's fine, except that we do not usually use the phrase
"Co-developed-by" around here X-<.