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);
