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

[Jeff King <peff@peff.net>]

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:

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