On Tue, Sep 10, 2024 at 08:57:15AM +0200, Patrick Steinhardt wrote: > 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. Ah, thanks for explaining. The patch makes much more sense now. :) In particular, this: > @@ -1169,6 +1170,8 @@ static void pop_stack_element(struct ref_formatting_stack **stack) > if (prev) > strbuf_addbuf(&prev->output, ¤t->output); > strbuf_release(¤t->output); > + if (current->at_end_data_free) > + current->at_end_data_free(current->at_end_data); > free(current); > *stack = prev; > } which frees on pop, replaces the manual: > @@ -1228,15 +1231,13 @@ static void if_then_else_handler(struct ref_formatting_stack **stack) > } > > *stack = cur; > - free(if_then_else); > } free that was happening in the success case. I think putting this on top of my series makes sense. -Peff
