Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 10, 2024 at 09:48:32AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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