Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms
On Tue, Sep 10, 2024 at 08:57:15AM +0200, Patrick Steinhardt wrote:
Show 14 quoted lines
> 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:
Show 9 quoted lines
> @@ -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:
Show 6 quoted lines
> @@ -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