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@xxxxxx>
> ---
> 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
