From: Jeff King <peff@peff.net>
Date: Tue, 10 Sep 2024 07:12:22 GMT
Subject: Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms
Message-ID: <20240910071222.GA1466029@coredump.intra.peff.net>
In-Reply-To: <4faf815b780218769520561ecf3abca384a2ee6c.1725951400.git.ps@pks.im>

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, &current->output);
>  	strbuf_release(&current->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