{"thread":{"id":"63774","subject":"[PATCH v2] reflog: close leak of reflog expire entry","startedAt":"2025-07-09T23:42:01Z","lastAt":"2025-07-10T15:54:29Z","messageCount":4,"participants":["Jacob Keller","Lidong Yan","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"521725","messageId":"20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com","threadId":"63774","inReplyTo":null,"subject":"[PATCH v2] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-09T23:41:17Z","receivedAt":"2025-07-09T23:42:01Z","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 in reflog_expiry_cleanup().\n\nSigned-off-by: Jacob Keller <jacob.keller@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/reflog.c |  3 +++\n reflog.c         | 15 +++++++++++++++\n 3 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/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..3ce1780924dd 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@@ -490,6 +504,7 @@ void reflog_expiry_cleanup(void *cb_data)\n \tfor (elem = cb->mark_list; elem; elem = elem->next)\n \t\tclear_commit_marks(elem->item, REACHABLE);\n \tfree_commit_list(cb->mark_list);\n+\treflog_clear_expire_config(&cb->opts);\n }\n \n int count_reflog_ent(struct object_id *ooid UNUSED,\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":"521728","messageId":"D34FE2DE-EE5B-43F3-A706-1AC133AA72F1@gmail.com","threadId":"63774","inReplyTo":"20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com","subject":"Re: [PATCH v2] reflog: close leak of reflog expire entry","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-07-10T03:00:38Z","receivedAt":"2025-07-10T03:00:52Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> wrote:\n> \n> From: Jacob Keller <jacob.keller@gmail.com>\n> \n> find_cfg_ent() allocates a struct reflog_expire_entry_option via\n> FLEX_ALLOC_MEM and inserts it into a linked list in the\n> reflog_expire_options structure. The entries in this list are never\n> freed, resulting in a leak in cmd_reflog_expire and the gc reflog expire\n> maintenance task:\n> \n> Direct 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> \n> Close this leak by adding a reflog_clear_expire_config() function which\n> iterates the linked list and frees its elements. Call it upon exit of\n> cmd_reflog_expire() and in reflog_expiry_cleanup().\n> \n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> Changes 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/reflog.c |  3 +++\n> reflog.c         | 15 +++++++++++++++\n> 3 files changed, 20 insertions(+)\n> \n> diff --git a/reflog.h b/reflog.h\n> index 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> 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.\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 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>     &cb);\n> free(ref);\n> }\n> +\n> + reflog_clear_expire_config(&opts);\n> +\n> return status;\n> }\n> \n> diff --git a/reflog.c b/reflog.c\n> index 15d81ebea978..3ce1780924dd 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> return 0;\n> }\n> \n> +void reflog_clear_expire_config(struct reflog_expire_options *opts)\n> +{\n> + struct reflog_expire_entry_option *ent = opts->entries, *tmp;\n> +\n> + while (ent) {\n> + tmp = ent;\n> + ent = ent->next;\n> + free(tmp);\n> + }\n> +\n> + opts->entries = NULL;\n> + opts->entries_tail = NULL;\n> +}\n> +\n\nThis looks correct.\n\n> void reflog_expire_options_set_refname(struct reflog_expire_options *cb,\n>       const char *ref)\n> {\n> @@ -490,6 +504,7 @@ void reflog_expiry_cleanup(void *cb_data)\n> for (elem = cb->mark_list; elem; elem = elem->next)\n> clear_commit_marks(elem->item, REACHABLE);\n> free_commit_list(cb->mark_list);\n> + reflog_clear_expire_config(&cb->opts);\n> }\n> \n> int count_reflog_ent(struct object_id *ooid UNUSED,\n> \n\nIn builtin/reflog.c, we have code like\n\n---\n\tfor (i = 0; i < argc; i++) {\n\t\tchar *ref;\n\t\tstruct expire_reflog_policy_cb cb = { .opts = opts };\n\n\t\tif (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {\n\t\t\tstatus |= error(_(\"reflog could not be found: '%s'\"), argv[i]);\n\t\t\tcontinue;\n\t\t}\n\t\treflog_expire_options_set_refname(&cb.opts, ref);\n\t\tstatus |= refs_reflog_expire(get_main_ref_store(the_repository),\n\t\t\t\t\t     ref, flags,\n\t\t\t\t\t     reflog_expiry_prepare,\n\t\t\t\t\t     should_prune_fn,\n\t\t\t\t\t     reflog_expiry_cleanup,\n\t\t\t\t\t     &cb);\n\t\tfree(ref);\n\t}\n+      reflog_clear_expire_config(&opts);\n---\n\nI think allowing reblog_expiry_cleanup() to free all opt->entries might\ncause reblog_expire_options_set_refname() to behave incorrectly."},{"id":"521729","messageId":"20250710034241.GA2057509@coredump.intra.peff.net","threadId":"63774","inReplyTo":"D34FE2DE-EE5B-43F3-A706-1AC133AA72F1@gmail.com","subject":"Re: [PATCH v2] reflog: close leak of reflog expire entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-10T03:42:41Z","receivedAt":"2025-07-10T03:42:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 10, 2025 at 11:00:38AM +0800, Lidong Yan wrote:\n\n> In builtin/reflog.c, we have code like\n> \n> ---\n> \tfor (i = 0; i < argc; i++) {\n> \t\tchar *ref;\n> \t\tstruct expire_reflog_policy_cb cb = { .opts = opts };\n> \n> \t\tif (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {\n> \t\t\tstatus |= error(_(\"reflog could not be found: '%s'\"), argv[i]);\n> \t\t\tcontinue;\n> \t\t}\n> \t\treflog_expire_options_set_refname(&cb.opts, ref);\n> \t\tstatus |= refs_reflog_expire(get_main_ref_store(the_repository),\n> \t\t\t\t\t     ref, flags,\n> \t\t\t\t\t     reflog_expiry_prepare,\n> \t\t\t\t\t     should_prune_fn,\n> \t\t\t\t\t     reflog_expiry_cleanup,\n> \t\t\t\t\t     &cb);\n> \t\tfree(ref);\n> \t}\n> +      reflog_clear_expire_config(&opts);\n> ---\n> \n> I think allowing reblog_expiry_cleanup() to free all opt->entries might\n> cause reblog_expire_options_set_refname() to behave incorrectly.\n\nHmm, yeah. We are calling this in a loop, so we'd want the config to\npersist until the loop ends. I didn't test, but I'd guess that:\n\n  git -c 'gc.refs/heads/*.reflogExpire=now' \\\n    reflog expire refs/heads/foo refs/heads/bar\n\nwould apply the config for \"foo\" but not for \"bar\". So I think\nreflog_expiry_cleanup() has to just clean up per-traversal data, not the\nconfig.\n\nSo the call at the end here looks reasonable, but the call in\nreflog_expiry_cleanup() is wrong. I guess it was trying to cover the\ncall in reflog_expire_condition(). That probably just needs a manual:\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 845876ff02..37f5437365 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-Peff\n"},{"id":"521761","messageId":"6fa10a33-7434-434a-9ef3-02fbaf21e1e1@intel.com","threadId":"63774","inReplyTo":"20250710034241.GA2057509@coredump.intra.peff.net","subject":"Re: [PATCH v2] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-10T15:54:01Z","receivedAt":"2025-07-10T15:54:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 7/9/2025 8:42 PM, Jeff King wrote:\n> On Thu, Jul 10, 2025 at 11:00:38AM +0800, Lidong Yan wrote:\n> \n>> In builtin/reflog.c, we have code like\n>>\n>> ---\n>> \tfor (i = 0; i < argc; i++) {\n>> \t\tchar *ref;\n>> \t\tstruct expire_reflog_policy_cb cb = { .opts = opts };\n>>\n>> \t\tif (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {\n>> \t\t\tstatus |= error(_(\"reflog could not be found: '%s'\"), argv[i]);\n>> \t\t\tcontinue;\n>> \t\t}\n>> \t\treflog_expire_options_set_refname(&cb.opts, ref);\n>> \t\tstatus |= refs_reflog_expire(get_main_ref_store(the_repository),\n>> \t\t\t\t\t     ref, flags,\n>> \t\t\t\t\t     reflog_expiry_prepare,\n>> \t\t\t\t\t     should_prune_fn,\n>> \t\t\t\t\t     reflog_expiry_cleanup,\n>> \t\t\t\t\t     &cb);\n>> \t\tfree(ref);\n>> \t}\n>> +      reflog_clear_expire_config(&opts);\n>> ---\n>>\n>> I think allowing reblog_expiry_cleanup() to free all opt->entries might\n>> cause reblog_expire_options_set_refname() to behave incorrectly.\n> \n> Hmm, yeah. We are calling this in a loop, so we'd want the config to\n> persist until the loop ends. I didn't test, but I'd guess that:\n> \n>   git -c 'gc.refs/heads/*.reflogExpire=now' \\\n>     reflog expire refs/heads/foo refs/heads/bar\n> \n> would apply the config for \"foo\" but not for \"bar\". So I think\n> reflog_expiry_cleanup() has to just clean up per-traversal data, not the\n> config.\n> \n> So the call at the end here looks reasonable, but the call in\n> reflog_expiry_cleanup() is wrong. I guess it was trying to cover the\n> call in reflog_expire_condition(). That probably just needs a manual:\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 845876ff02..37f5437365 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\nYa, you're right. I just thought the reflog_expiry_cleanup would only be\ncalled by this function. It did pass the tests... I'll see if I can add\na test case covering this since its caused a bit more trouble than I\nthought it would.\n\n>  }\n>  \n> \n> -Peff\n\n"}]}