From: Toon Claes <toon@iotcl.com>
Date: Fri, 18 Oct 2024 12:03:26 GMT
Subject: Re: [PATCH 10/21] trailer: fix leaking trailer values
Message-ID: <878quld3e9.fsf@iotcl.com>
In-Reply-To: <ca5370d572d5750e5fb21c84d4a4134669e7e3c1.1728624670.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> Fix leaking trailer values when replacing the value with a command or
> when the token value is empty.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> diff --git a/trailer.c b/trailer.c
> index 682d74505bf..5c0bfb735a9 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1124,7 +1131,7 @@ void format_trailers(const struct process_trailer_options *opts,
>  			 * corresponding value).
>  			 */
>  			if (opts->trim_empty && !strlen(item->value))
> -				continue;
> +				goto next;

While this is technically correct, I found it rather hard to understand
what's happening. What do you think about instead turning the `if` below
in an `else if` ?

>  
>  			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>  				if (opts->separator && out->len != origlen)
> @@ -1145,9 +1152,10 @@ void format_trailers(const struct process_trailer_options *opts,
>  				if (!opts->separator)
>  					strbuf_addch(out, '\n');
>  			}
> +
> +next:
>  			strbuf_release(&tok);
>  			strbuf_release(&val);
> -
>  		} else if (!opts->only_trailers) {
>  			if (opts->separator && out->len != origlen) {
>  				strbuf_addbuf(out, opts->separator);
> -- 
> 2.47.0.dirty

--
Toon