Re: [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 21, 2024, 20:58 UTC
- Message-ID
- <ZxbAhyilmPR7jF39@nand.local>
- In-Reply-To
- <5b851453bcea945f95c3f29138e510d8448e96e6.1729502824.git.ps@pks.im>
On Mon, Oct 21, 2024 at 11:28:35AM +0200, Patrick Steinhardt wrote:
Show 48 quoted lines
> 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