> -----Original Message-----
> From: Jeff King <peff@xxxxxxxx>
> Sent: Wednesday, July 9, 2025 3:37 PM
> To: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>
> Cc: git@xxxxxxxxxxxxxxx; Junio C Hamano <gitster@xxxxxxxxx>; Jacob Keller
> <jacob.keller@xxxxxxxxx>
> Subject: Re: [PATCH] reflog: close leak of reflog expire entry
>
> On Wed, Jul 09, 2025 at 02:49:14PM -0700, Jacob Keller wrote:
>
> > find_cfg_ent() allocates a struct reflog_expire_entry_option via
> > FLEX_ALLOC_MEM and returns its pointer to reflog_expire_config(). The
> > function exits without freeing the memory:
> >
> > 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 freeing the entry pointer on exit of the
> > reflog_expire_config() function. This frees both the entry structure and
> > its embedded pattern array thanks to the use of FLEX_ALLOC_MEM.
>
> Hmm, this can't be right, can it? The end of reflog_expire_config()
> looks like this:
>
> ent = find_cfg_ent(opts, pattern, pattern_len);
> if (!ent)
> return -1;
> switch (slot) {
> case REFLOG_EXPIRE_TOTAL:
> ent->expire_total = expire;
> break;
> case REFLOG_EXPIRE_UNREACH:
> ent->expire_unreachable = expire;
> break;
> }
> return 0;
>
> So if we free(ent), then what was the point of the function? We'd set
> some fields in it and then throw it away?
>
> And indeed, find_cfg_ent() seems to add the newly allocated entry to the
> list opt->entries list. So by freeing here, we're leaving a dangling
> pointer in that list.
>
> Probably that list needs to be cleaned up when cmd_reflog_expire()
> finishes?
>
> -Peff
Oh. Yep, you're right. My brain didn't quite process what was going on. Yea this definitely won't work.
Thanks,
Jake
