Re: [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 21, 2024 at 11:28:35AM +0200, Patrick Steinhardt wrote:
> We are populating, but never releasing two string buffers in
> `format_trailers()`, causing a memory leak. Plug this leak by lifting
> those buffers outside of the loop and releasing them on function return.
> This fixes the memory leaks, but also optimizes the loop as we don't
> have to reallocate the buffers on every single iteration.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t7513-interpret-trailers.sh |  1 +
>  trailer.c                     | 13 ++++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 0f7d8938d98..38d6ccaa001 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -5,6 +5,7 @@
>
>  test_description='git interpret-trailers'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  # When we want one trailing space at the end of each line, let's use sed
> diff --git a/trailer.c b/trailer.c
> index f1eca6d5d15..24e4e56fdf8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1111,16 +1111,19 @@ void format_trailers(const struct process_trailer_options *opts,
>  		     struct list_head *trailers,
>  		     struct strbuf *out)
>  {
> +	struct strbuf tok = STRBUF_INIT;
> +	struct strbuf val = STRBUF_INIT;
>  	size_t origlen = out->len;
>  	struct list_head *pos;
>  	struct trailer_item *item;
>
> +
>  	list_for_each(pos, trailers) {
>  		item = list_entry(pos, struct trailer_item, list);
>  		if (item->token) {
> -			struct strbuf tok = STRBUF_INIT;
> -			struct strbuf val = STRBUF_INIT;
> +			strbuf_reset(&tok);
>  			strbuf_addstr(&tok, item->token);
> +			strbuf_reset(&val);
>  			strbuf_addstr(&val, item->value);

I have a vague preference towards writing this as:

        strbuf_reset(&tok);
        strbuf_reset(&val);
        strbuf_addstr(&tok, item->token);
        strbuf_addstr(&val, item->value);

to make clear(er) to the reader that, OK, we are resetting both of these
buffers before using them at the beginning of the loop. To me it reads a
little clearer seeing both strbuf_reset() calls one right after the
other.

I don't feel all that strongly about it, but I also see that you have a
couple of small changes you're sitting on from Kristoffer's review, so I
figured I'd throw it out there anyway as we are expecting a new round to
address those.

Thanks,
Taylor