Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms
- From
Jeff King <peff@peff.net>
- Date
- Sep 12, 2024, 11:18 UTC
- Message-ID
- <20240912111858.GA617985@coredump.intra.peff.net>
- In-Reply-To
- <ZuLA0SBqhBbBdcd1@pks.im>
On Thu, Sep 12, 2024 at 12:22:16PM +0200, Patrick Steinhardt wrote:
Show 19 quoted lines
> > 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:
diff --git a/ref-filter.c b/ref-filter.c index b06e18a569..a339f0ab0f 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3454,7 +3454,8 @@ int format_ref_array_item(struct ref_array_item *info, pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf); if (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) || atomv->handler(atomv, &state, error_buf)) { - pop_stack_element(&state.stack); + while (state.stack->prev) + pop_stack_element(&state.stack); return -1; } } @@ -3466,7 +3467,8 @@ int format_ref_array_item(struct ref_array_item *info, struct atom_value resetv = ATOM_VALUE_INIT; resetv.s = GIT_COLOR_RESET; if (append_atom(&resetv, &state, error_buf)) { - pop_stack_element(&state.stack); + while (state.stack->prev) + pop_stack_element(&state.stack); return -1; } } I wasn't sure why the non-error code path wouldn't need the same, but it looks like there's some popping that happens in the various callbacks? I'm not very familiar with this code, and it's hard to follow the flow through the function pointers. 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. -Peff