{"thread":{"id":"63773","subject":"[PATCH] reflog: close leak of reflog expire entry","startedAt":"2025-07-09T21:49:55Z","lastAt":"2025-07-09T23:24:45Z","messageCount":3,"participants":["Jacob Keller","Jeff King","Keller, Jacob E"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"521714","messageId":"20250709-jk-fix-leak-reflog-expire-config-v1-1-34d5461cf8f5@gmail.com","threadId":"63773","inReplyTo":null,"subject":"[PATCH] reflog: close leak of reflog expire entry","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-09T21:49:14Z","receivedAt":"2025-07-09T21:49:55Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nfind_cfg_ent() allocates a struct reflog_expire_entry_option via\nFLEX_ALLOC_MEM and returns its pointer to reflog_expire_config(). The\nfunction exits without freeing the memory:\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 freeing the entry pointer on exit of the\nreflog_expire_config() function. This frees both the entry structure and\nits embedded pattern array thanks to the use of FLEX_ALLOC_MEM.\n\nSigned-off-by: Jacob Keller \n---\n reflog.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/reflog.c b/reflog.c\nindex 15d81ebea978..43647eaf89eb 100644\n--- a/reflog.c\n+++ b/reflog.c\n@@ -78,6 +78,9 @@ int reflog_expire_config(const char *var, const char *value,\n \t\tent->expire_unreachable = expire;\n \t\tbreak;\n \t}\n+\n+\tfree(ent);\n+\n \treturn 0;\n }\n \n\n---\nbase-commit: a30f80fde927d70950b3b4d1820813480968fb0d\nchange-id: 20250709-jk-fix-leak-reflog-expire-config-712ca6dc685a\n\nBest regards,\n-- \nJacob Keller \n\n"},{"id":"521718","messageId":"20250709223650.GA2046725@coredump.intra.peff.net","threadId":"63773","inReplyTo":"20250709-jk-fix-leak-reflog-expire-config-v1-1-34d5461cf8f5@gmail.com","subject":"Re: [PATCH] reflog: close leak of reflog expire entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-09T22:36:50Z","receivedAt":"2025-07-09T22:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 09, 2025 at 02:49:14PM -0700, Jacob Keller wrote:\n\n> find_cfg_ent() allocates a struct reflog_expire_entry_option via\n> FLEX_ALLOC_MEM and returns its pointer to reflog_expire_config(). The\n> function exits without freeing the memory:\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 freeing the entry pointer on exit of the\n> reflog_expire_config() function. This frees both the entry structure and\n> its embedded pattern array thanks to the use of FLEX_ALLOC_MEM.\n\nHmm, this can't be right, can it? The end of reflog_expire_config()\nlooks like this:\n\n ent = find_cfg_ent(opts, pattern, pattern_len);\n if (!ent)\n return -1;\n switch (slot) {\n case REFLOG_EXPIRE_TOTAL:\n ent->expire_total = expire;\n break;\n case REFLOG_EXPIRE_UNREACH:\n ent->expire_unreachable = expire;\n break;\n }\n return 0;\n\nSo if we free(ent), then what was the point of the function? We'd set\nsome fields in it and then throw it away?\n\nAnd indeed, find_cfg_ent() seems to add the newly allocated entry to the\nlist opt->entries list. So by freeing here, we're leaving a dangling\npointer in that list.\n\nProbably that list needs to be cleaned up when cmd_reflog_expire()\nfinishes?\n\n-Peff\n"},{"id":"521724","messageId":"CO1PR11MB50898FF5DA67F4F6A474CECFD649A@CO1PR11MB5089.namprd11.prod.outlook.com","threadId":"63773","inReplyTo":"20250709223650.GA2046725@coredump.intra.peff.net","subject":"RE: [PATCH] reflog: close leak of reflog expire entry","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-07-09T23:24:14Z","receivedAt":"2025-07-09T23:24:45Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\n> -----Original Message-----\n> From: Jeff King \n> Sent: Wednesday, July 9, 2025 3:37 PM\n> To: Keller, Jacob E \n> Cc: git@vger.kernel.org; Junio C Hamano ; Jacob Keller\n> \n> Subject: Re: [PATCH] reflog: close leak of reflog expire entry\n> \n> On Wed, Jul 09, 2025 at 02:49:14PM -0700, Jacob Keller wrote:\n> \n> > find_cfg_ent() allocates a struct reflog_expire_entry_option via\n> > FLEX_ALLOC_MEM and returns its pointer to reflog_expire_config(). The\n> > function exits without freeing the memory:\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\n> (/lib64/libc.so.6+0x36a7)\n> > #15 0x000000444184 in _start (/home/jekeller/libexec/git-\n> core/git+0x444184)\n> >\n> > Close this leak by freeing the entry pointer on exit of the\n> > reflog_expire_config() function. This frees both the entry structure and\n> > its embedded pattern array thanks to the use of FLEX_ALLOC_MEM.\n> \n> Hmm, this can't be right, can it? The end of reflog_expire_config()\n> looks like this:\n> \n> ent = find_cfg_ent(opts, pattern, pattern_len);\n> if (!ent)\n> return -1;\n> switch (slot) {\n> case REFLOG_EXPIRE_TOTAL:\n> ent->expire_total = expire;\n> break;\n> case REFLOG_EXPIRE_UNREACH:\n> ent->expire_unreachable = expire;\n> break;\n> }\n> return 0;\n> \n> So if we free(ent), then what was the point of the function? We'd set\n> some fields in it and then throw it away?\n> \n> And indeed, find_cfg_ent() seems to add the newly allocated entry to the\n> list opt->entries list. So by freeing here, we're leaving a dangling\n> pointer in that list.\n> \n> Probably that list needs to be cleaned up when cmd_reflog_expire()\n> finishes?\n> \n> -Peff\n\nOh. Yep, you're right. My brain didn't quite process what was going on. Yea this definitely won't work.\n\nThanks,\nJake\n"}]}