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

> On Thu, Sep 12, 2024 at 12:22:16PM +0200, Patrick Steinhardt wrote:
>
>> > 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.
>
> Hmm. I think just:
>
>   ./git for-each-ref --format='%(if)%(then)%(if)%(then)%(if)%(then)'
>
> should trigger it, and running it in the debugger I can see that we exit
> the function with multiple entries.
>
> Valgrind claims the memory is still reachable, but I don't see how. The
> "state" variable is accessible only inside that function. The only thing
> we do after returning is die(). I wonder if it is a false negative
> because the stack is left undisturbed (especially because the compiler
> knows that die() does not return).

> At any rate, I think the same would apply to the earlier error returns:
> ...
> All that said, I am content to leave it for now. Even if it's a real
> leak, it's one that happens once per program right before exiting with
> an error. Most of the value in cleaning up trivial leaks like that are
> to reduce the noise from analyzers so that we can find the much more
> important leaks that scale with the input. If the analyzers aren't
> complaining and we think it's trivial, it may not be worth spending a
> lot of time on.