On Tue, Sep 10, 2024 at 09:48:32AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > When parsing `%(if)` atoms we expect a few other atoms to exist to
> > complete it, like `%(then)` and `%(end)`. Whether or not we have seen
> > these other atoms is tracked in an allocated `if_then_else` structure,
> > which gets free'd by the `if_then_else_handler()` once we have parsed
> > the complete conditional expression.
> >
> > This results in a memory leak when the `%(if)` atom is not terminated
> > correctly and thus incomplete. We never end up executing its handler and
> > thus don't end up freeing the structure.
> >
> > Plug this memory leak by introducing a new `at_end_data_free` callback
> > function. If set, we'll execute it in `pop_stack_element()` and pass it
> > the `at_end_data` variable with the intent to free its state. Wire it up
> > for the `%(if)` atom accordingly.
>
> Sounds good. We diagnose unclosed "%(if)", report mismatch, and
> die() soon, so plugging this may more about "let's silence leak
> checker so that it can be more effective to help us find real leaks
> that matter", not "this is leaking proportionally to the size of the
> user data, and must be plugged".
>
> I see this code snippet (not touched by your patch):
>
> if (state.stack->prev) {
> pop_stack_element(&state.stack);
> return strbuf_addf_ret(error_buf, -1, _("format: %%(end) atom missing"));
> }
>
> and wonder how this handles the case where state.stack->prev->prev
> is also not NULL. Shouldn't it be looping while .prev is not NULL?
>
> e.g.
>
> diff --git c/ref-filter.c w/ref-filter.c
> index b06e18a569..d2040f5047 100644
> --- c/ref-filter.c
> +++ w/ref-filter.c
> @@ -3471,7 +3471,8 @@ int format_ref_array_item(struct ref_array_item *info,
> }
> }
> if (state.stack->prev) {
> - pop_stack_element(&state.stack);
> + while (state.stack->prev)
> + pop_stack_element(&state.stack);
> return strbuf_addf_ret(error_buf, -1, _("format: %%(end) atom missing"));
> }
> strbuf_addbuf(final_buf, &state.stack->output);
Hm. It certainly feels like we should do that. I couldn't construct a
test case that fails with the leak sanitizer though. If it's a leak I'm
sure I'll eventually hit it when I continue down the road headed towards
leak-free-ness.
Patrick
