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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2024, 16:48 UTC
Message-ID: <xmqqseu7jvz3.fsf@gitster.g>
In-Reply-To: <4faf815b780218769520561ecf3abca384a2ee6c.1725951400.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

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.

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