{"thread":{"id":"63814","subject":"[PATCH v3] reflog: close leak of reflog expire entry","startedAt":"2025-07-21T23:39:51Z","lastAt":"2025-07-22T23:35:40Z","messageCount":8,"participants":["Jacob Keller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"522378","messageId":"20250721-jk-fix-leak-reflog-expire-config-v3-1-c488b0586e80@gmail.com","threadId":"63814","inReplyTo":null,"subject":"[PATCH v3] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-21T23:39:37Z","receivedAt":"2025-07-21T23:39:51Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nfind_cfg_ent() allocates a struct reflog_expire_entry_option via\nFLEX_ALLOC_MEM and inserts it into a linked list in the\nreflog_expire_options structure. The entries in this list are never\nfreed, resulting in a leak in cmd_reflog_expire and the gc reflog expire\nmaintenance task:\n\nDirect leak of 39 byte(s) in 1 object(s) allocated from:\n    #0 0x7ff975ee6883 in calloc (/lib64/libasan.so.8+0xe6883)\n    #1 0x0000010edada in xcalloc ../wrapper.c:154\n    #2 0x000000df0898 in find_cfg_ent ../reflog.c:28\n    #3 0x000000df0898 in reflog_expire_config ../reflog.c:70\n    #4 0x00000095c451 in configset_iter ../config.c:2116\n    #5 0x0000006d29e7 in git_config ../config.h:724\n    #6 0x0000006d29e7 in cmd_reflog_expire ../builtin/reflog.c:205\n    #7 0x0000006d504c in cmd_reflog ../builtin/reflog.c:419\n    #8 0x0000007e4054 in run_builtin ../git.c:480\n    #9 0x0000007e4054 in handle_builtin ../git.c:746\n    #10 0x0000007e8a35 in run_argv ../git.c:813\n    #11 0x0000007e8a35 in cmd_main ../git.c:953\n    #12 0x000000441e8f in main ../common-main.c:9\n    #13 0x7ff9754115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4)\n    #14 0x7ff9754116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7)\n    #15 0x000000444184 in _start (/home/jekeller/libexec/git-core/git+0x444184)\n\nClose this leak by adding a reflog_clear_expire_config() function which\niterates the linked list and frees its elements. Call it upon exit of\ncmd_reflog_expire() and reflog_expire_condition().\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nChanges in v3:\n- Remove the incorrect call in reflog_expiry_cleanup()\n- Add a call in reflog_expire_condition()\n- Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com\n\nChanges in v2:\n- Actually fix the leak properly. (Thanks Jeff for catching my brain fart!)\n- Link to v1: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v1-1-34d5461cf8f5@gmail.com\n---\n reflog.h         |  2 ++\n builtin/gc.c     |  1 +\n builtin/reflog.c |  3 +++\n reflog.c         | 14 ++++++++++++++\n 4 files changed, 20 insertions(+)\n\ndiff --git a/reflog.h b/reflog.h\nindex 63bb56280f4e..74b3f3c4f0ac 100644\n--- a/reflog.h\n+++ b/reflog.h\n@@ -34,6 +34,8 @@ struct reflog_expire_options {\n int reflog_expire_config(const char *var, const char *value,\n \t\t\t const struct config_context *ctx, void *cb);\n \n+void reflog_clear_expire_config(struct reflog_expire_options *opts);\n+\n /*\n  * Adapt the options so that they apply to the given refname. This applies any\n  * per-reference reflog expiry configuration that may exist to the options.\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 845876ff0286..37f543736599 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -346,6 +346,7 @@ static int reflog_expire_condition(struct gc_config *cfg UNUSED)\n \t\t\t\t count_reflog_entries, &data);\n \n \treflog_expiry_cleanup(&data.policy);\n+\treflog_clear_expire_config(&data.policy);\n \treturn data.count >= data.limit;\n }\n \ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3acaf3e32c27..d4da41aaea73 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -283,6 +283,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t     &cb);\n \t\tfree(ref);\n \t}\n+\n+\treflog_clear_expire_config(&opts);\n+\n \treturn status;\n }\n \ndiff --git a/reflog.c b/reflog.c\nindex 15d81ebea978..e2a2f3ad3e30 100644\n--- a/reflog.c\n+++ b/reflog.c\n@@ -81,6 +81,20 @@ int reflog_expire_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+void reflog_clear_expire_config(struct reflog_expire_options *opts)\n+{\n+\tstruct reflog_expire_entry_option *ent = opts->entries, *tmp;\n+\n+\twhile (ent) {\n+\t\ttmp = ent;\n+\t\tent = ent->next;\n+\t\tfree(tmp);\n+\t}\n+\n+\topts->entries = NULL;\n+\topts->entries_tail = NULL;\n+}\n+\n void reflog_expire_options_set_refname(struct reflog_expire_options *cb,\n \t\t\t\t       const char *ref)\n {\n\n---\nbase-commit: a30f80fde927d70950b3b4d1820813480968fb0d\nchange-id: 20250709-jk-fix-leak-reflog-expire-config-712ca6dc685a\n\nBest regards,\n--  \nJacob Keller <jacob.keller@gmail.com>\n\n"},{"id":"522390","messageId":"20250722045456.GA824456@coredump.intra.peff.net","threadId":"63814","inReplyTo":"20250721-jk-fix-leak-reflog-expire-config-v3-1-c488b0586e80@gmail.com","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T04:54:56Z","receivedAt":"2025-07-22T04:54:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 21, 2025 at 04:39:37PM -0700, Jacob Keller wrote:\n\n> Changes in v3:\n> - Remove the incorrect call in reflog_expiry_cleanup()\n> - Add a call in reflog_expire_condition()\n> - Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com\n\nThis looks correct to me except...\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 845876ff0286..37f543736599 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -346,6 +346,7 @@ static int reflog_expire_condition(struct gc_config *cfg UNUSED)\n>  \t\t\t\t count_reflog_entries, &data);\n>  \n>  \treflog_expiry_cleanup(&data.policy);\n> +\treflog_clear_expire_config(&data.policy);\n>  \treturn data.count >= data.limit;\n>  }\n>  \n\nThis needs to pass &data.policy.opts, no?\n\nI think we might also want this test on top (or I'd be happy to see it\nsquashed in). It shows off your fix when built with SANITIZE=leak, and\nalso catches the bug that v2 of your patch had.\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config\n\nWe have long supported the ability to set reflog expiration config for\nindividual, going back to 3cb22b8efe (Per-ref reflog expiry\nconfiguration, 2008-06-15). But we have never had any tests.\n\nLet's add a very basic one that checks that we apply the config\ncorrectly to a subset of refs (and not elsewhere). This also\ntriggers the leaky code fixed by the previous commit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 42b501f163..362e90d7d6 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '\n \ttest_grep \"error: reflog could not be found: ${SQ}does-not-exist${SQ}\" stderr\n '\n \n+test_expect_success 'expire with pattern config' '\n+\t# Split refs/heads/ into two roots so we can apply config to each. Make\n+\t# two branches per root to verify that config is applied correctly\n+\t# multiple times.\n+\tgit branch root1/branch1 &&\n+\tgit branch root1/branch2 &&\n+\tgit branch root2/branch1 &&\n+\tgit branch root2/branch2 &&\n+\n+\ttest_config \"gc.reflogexpire\" \"never\" &&\n+\ttest_config \"gc.refs/heads/root2/*.reflogExpire\" \"now\" &&\n+\tgit reflog expire \\\n+\t\troot1/branch1 root1/branch2 \\\n+\t\troot2/branch1 root2/branch2 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\troot1/branch1@{0}\n+\troot1/branch2@{0}\n+\tEOF\n+\tgit log -g --branches=\"root*\" --format=%gD >actual.raw &&\n+\t# The sole reflog entry of each branch points to the same commit, so\n+\t# the order in which they are shown is nondeterministic. We just care\n+\t# about the what was expired (and what was not), so sort to get a known\n+\t# order.\n+\tsort <actual.raw >actual.sorted &&\n+\ttest_cmp expect actual.sorted\n+'\n+\n test_expect_success 'checkout should not delete log for packed ref' '\n \ttest $(git reflog main | wc -l) = 4 &&\n \tgit branch foo &&\n-- \n2.50.1.589.g6e88b11be3\n\n"},{"id":"522441","messageId":"xmqqms8wuxkf.fsf@gitster.g","threadId":"63814","inReplyTo":"20250722045456.GA824456@coredump.intra.peff.net","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T14:09:20Z","receivedAt":"2025-07-22T14:09:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config\n>\n> We have long supported the ability to set reflog expiration config for\n> individual, going back to 3cb22b8efe (Per-ref reflog expiry\n> configuration, 2008-06-15). But we have never had any tests.\n\nYikes.  I completely forgot adding that feature, but it seems I also\nforgot to add tests when I added it.  My bad.\n\n\"individual\" -> \"individual refs\" or something?  I was confused\nafter my initial read, which sounded as if we are talking about\nallowing individual users to set the configuration variable ;-)\n\n> Let's add a very basic one that checks that we apply the config\n> correctly to a subset of refs (and not elsewhere). This also\n> triggers the leaky code fixed by the previous commit.\n\nThanks.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++\n>  1 file changed, 28 insertions(+)\n>\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index 42b501f163..362e90d7d6 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '\n>  \ttest_grep \"error: reflog could not be found: ${SQ}does-not-exist${SQ}\" stderr\n>  '\n>  \n> +test_expect_success 'expire with pattern config' '\n> +\t# Split refs/heads/ into two roots so we can apply config to each. Make\n> +\t# two branches per root to verify that config is applied correctly\n> +\t# multiple times.\n> +\tgit branch root1/branch1 &&\n> +\tgit branch root1/branch2 &&\n> +\tgit branch root2/branch1 &&\n> +\tgit branch root2/branch2 &&\n> +\n> +\ttest_config \"gc.reflogexpire\" \"never\" &&\n> +\ttest_config \"gc.refs/heads/root2/*.reflogExpire\" \"now\" &&\n> +\tgit reflog expire \\\n> +\t\troot1/branch1 root1/branch2 \\\n> +\t\troot2/branch1 root2/branch2 &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\troot1/branch1@{0}\n> +\troot1/branch2@{0}\n> +\tEOF\n> +\tgit log -g --branches=\"root*\" --format=%gD >actual.raw &&\n> +\t# The sole reflog entry of each branch points to the same commit, so\n> +\t# the order in which they are shown is nondeterministic. We just care\n> +\t# about the what was expired (and what was not), so sort to get a known\n> +\t# order.\n> +\tsort <actual.raw >actual.sorted &&\n> +\ttest_cmp expect actual.sorted\n> +'\n> +\n>  test_expect_success 'checkout should not delete log for packed ref' '\n>  \ttest $(git reflog main | wc -l) = 4 &&\n>  \tgit branch foo &&\n"},{"id":"522507","messageId":"fd14c857-63a8-41e7-8361-bc816d4a47c4@intel.com","threadId":"63814","inReplyTo":"20250722045456.GA824456@coredump.intra.peff.net","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-22T23:10:24Z","receivedAt":"2025-07-22T23:10:28Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 7/21/2025 9:54 PM, Jeff King wrote:\n> On Mon, Jul 21, 2025 at 04:39:37PM -0700, Jacob Keller wrote:\n> \n>> Changes in v3:\n>> - Remove the incorrect call in reflog_expiry_cleanup()\n>> - Add a call in reflog_expire_condition()\n>> - Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com\n> \n> This looks correct to me except...\n> \n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index 845876ff0286..37f543736599 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -346,6 +346,7 @@ static int reflog_expire_condition(struct gc_config *cfg UNUSED)\n>>  \t\t\t\t count_reflog_entries, &data);\n>>  \n>>  \treflog_expiry_cleanup(&data.policy);\n>> +\treflog_clear_expire_config(&data.policy);\n>>  \treturn data.count >= data.limit;\n>>  }\n>>  \n> \n> This needs to pass &data.policy.opts, no?\n> \n\nYou're right... I think I fixed that and forgot to actually commit it\nbefore sending. Ugh.\n\n> I think we might also want this test on top (or I'd be happy to see it\n> squashed in). It shows off your fix when built with SANITIZE=leak, and\n> also catches the bug that v2 of your patch had.\n> \n> -Peff\n> \n\nSounds good. I'll send a v4 which squashes this in.\n\n> -- >8 --\n> Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config\n> \n> We have long supported the ability to set reflog expiration config for\n> individual, going back to 3cb22b8efe (Per-ref reflog expiry\n> configuration, 2008-06-15). But we have never had any tests.\n> \n> Let's add a very basic one that checks that we apply the config\n> correctly to a subset of refs (and not elsewhere). This also\n> triggers the leaky code fixed by the previous commit.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++\n>  1 file changed, 28 insertions(+)\n> \n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index 42b501f163..362e90d7d6 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '\n>  \ttest_grep \"error: reflog could not be found: ${SQ}does-not-exist${SQ}\" stderr\n>  '\n>  \n> +test_expect_success 'expire with pattern config' '\n> +\t# Split refs/heads/ into two roots so we can apply config to each. Make\n> +\t# two branches per root to verify that config is applied correctly\n> +\t# multiple times.\n> +\tgit branch root1/branch1 &&\n> +\tgit branch root1/branch2 &&\n> +\tgit branch root2/branch1 &&\n> +\tgit branch root2/branch2 &&\n> +\n> +\ttest_config \"gc.reflogexpire\" \"never\" &&\n> +\ttest_config \"gc.refs/heads/root2/*.reflogExpire\" \"now\" &&\n> +\tgit reflog expire \\\n> +\t\troot1/branch1 root1/branch2 \\\n> +\t\troot2/branch1 root2/branch2 &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\troot1/branch1@{0}\n> +\troot1/branch2@{0}\n> +\tEOF\n> +\tgit log -g --branches=\"root*\" --format=%gD >actual.raw &&\n> +\t# The sole reflog entry of each branch points to the same commit, so\n> +\t# the order in which they are shown is nondeterministic. We just care\n> +\t# about the what was expired (and what was not), so sort to get a known\n> +\t# order.\n> +\tsort <actual.raw >actual.sorted &&\n> +\ttest_cmp expect actual.sorted\n> +'\n> +\n>  test_expect_success 'checkout should not delete log for packed ref' '\n>  \ttest $(git reflog main | wc -l) = 4 &&\n>  \tgit branch foo &&\n\n"},{"id":"522508","messageId":"20250722231019.GA1598@coredump.intra.peff.net","threadId":"63814","inReplyTo":"xmqqms8wuxkf.fsf@gitster.g","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T23:10:19Z","receivedAt":"2025-07-22T23:10:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 07:09:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config\n> >\n> > We have long supported the ability to set reflog expiration config for\n> > individual, going back to 3cb22b8efe (Per-ref reflog expiry\n> > configuration, 2008-06-15). But we have never had any tests.\n> \n> Yikes.  I completely forgot adding that feature, but it seems I also\n> forgot to add tests when I added it.  My bad.\n> \n> \"individual\" -> \"individual refs\" or something?  I was confused\n> after my initial read, which sounded as if we are talking about\n> allowing individual users to set the configuration variable ;-)\n\nYep, exactly. I admit I didn't spend as much time on the commit message\nfor this one, since I figured it might just get squashed anyway. :)\n\n-Peff\n"},{"id":"522509","messageId":"xmqq5xfjrew1.fsf@gitster.g","threadId":"63814","inReplyTo":"fd14c857-63a8-41e7-8361-bc816d4a47c4@intel.com","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T23:21:02Z","receivedAt":"2025-07-22T23:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n>> This needs to pass &data.policy.opts, no?\n>> \n>\n> You're right... I think I fixed that and forgot to actually commit it\n> before sending. Ugh.\n>\n>> I think we might also want this test on top (or I'd be happy to see it\n>> squashed in). It shows off your fix when built with SANITIZE=leak, and\n>> also catches the bug that v2 of your patch had.\n>> \n>> -Peff\n>> \n>\n> Sounds good. I'll send a v4 which squashes this in.\n\nOK, or you can tell me to squash what I queued on the\njk/unleak-reflog-expire-entry topic that ends at 7c091149 (fixup!\nreflog: close leak of reflog expire entry, 2025-07-22) down into a\nsingle patch (or two to keep Peff's test saparate).\n\nThanks.\n"},{"id":"522510","messageId":"72f13c54-d5c7-4366-bba2-b641d9e2b0c7@intel.com","threadId":"63814","inReplyTo":"xmqq5xfjrew1.fsf@gitster.g","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-22T23:22:35Z","receivedAt":"2025-07-22T23:22:55Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 7/22/2025 4:21 PM, Junio C Hamano wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>>> This needs to pass &data.policy.opts, no?\n>>>\n>>\n>> You're right... I think I fixed that and forgot to actually commit it\n>> before sending. Ugh.\n>>\n>>> I think we might also want this test on top (or I'd be happy to see it\n>>> squashed in). It shows off your fix when built with SANITIZE=leak, and\n>>> also catches the bug that v2 of your patch had.\n>>>\n>>> -Peff\n>>>\n>>\n>> Sounds good. I'll send a v4 which squashes this in.\n> \n> OK, or you can tell me to squash what I queued on the\n> jk/unleak-reflog-expire-entry topic that ends at 7c091149 (fixup!\n> reflog: close leak of reflog expire entry, 2025-07-22) down into a\n> single patch (or two to keep Peff's test saparate).\n> \n> Thanks.\n\nI am about to send a v4 that squashes Peff's work in and adds a\nCo-developed-by tag. I think that makes the most sense.\n"},{"id":"522512","messageId":"xmqq1pq7re7q.fsf@gitster.g","threadId":"63814","inReplyTo":"72f13c54-d5c7-4366-bba2-b641d9e2b0c7@intel.com","subject":"Re: [PATCH v3] reflog: close leak of reflog expire entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T23:35:37Z","receivedAt":"2025-07-22T23:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> I am about to send a v4 that squashes Peff's work in and adds a\n> Co-developed-by tag. I think that makes the most sense.\n\nThat's fine, except that we do not usually use the phrase\n\"Co-developed-by\" around here X-<.\n\n"}]}