| Message ID | 5b851453bcea945f95c3f29138e510d8448e96e6.1729502824.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory leak fixes (pt.9) | expand |
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
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); /* @@ -1151,9 +1154,6 @@ void format_trailers(const struct process_trailer_options *opts, if (!opts->separator) strbuf_addch(out, '\n'); } - strbuf_release(&tok); - strbuf_release(&val); - } else if (!opts->only_trailers) { if (opts->separator && out->len != origlen) { strbuf_addbuf(out, opts->separator); @@ -1165,6 +1165,9 @@ void format_trailers(const struct process_trailer_options *opts, strbuf_addch(out, '\n'); } } + + strbuf_release(&tok); + strbuf_release(&val); } void format_trailers_from_commit(const struct process_trailer_options *opts,
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(-)