| Message ID | ca5370d572d5750e5fb21c84d4a4134669e7e3c1.1728624670.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.9) | expand |
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
On Fri, Oct 18, 2024 at 02:03:26PM +0200, Toon Claes wrote: > 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` ? An even better idea is to lift the buffers outside of the loop. Like this we don't have to reallocate them on every iteration and can easily release them once at the end of the function. Patrick
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 682d74505bf..5c0bfb735a9 100644 --- a/trailer.c +++ b/trailer.c @@ -249,17 +249,23 @@ static char *apply_command(struct conf_info *conf, const char *arg) static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok) { if (arg_tok->conf.command || arg_tok->conf.cmd) { - const char *arg; + char *value_to_free = NULL; + char *arg; + if (arg_tok->value && arg_tok->value[0]) { - arg = arg_tok->value; + arg = (char *)arg_tok->value; } else { if (in_tok && in_tok->value) arg = xstrdup(in_tok->value); else arg = xstrdup(""); + value_to_free = arg_tok->value; } + arg_tok->value = apply_command(&arg_tok->conf, arg); - free((char *)arg); + + free(value_to_free); + free(arg); } } @@ -1114,6 +1120,7 @@ void format_trailers(const struct process_trailer_options *opts, if (item->token) { struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; + strbuf_addstr(&tok, item->token); strbuf_addstr(&val, item->value); @@ -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; 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);
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> --- t/t7513-interpret-trailers.sh | 1 + trailer.c | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-)