[PATCH v2 00/22] Memory leak fixes (pt.9)

[Patrick Steinhardt <ps@pks.im>]

Hi,

this is the second version of my 9th series of memory leak fixes.

Changes compared to v1:

  - Split up the trailer fixes into two separate patches so that we can
    explain them standalone.

  - Adapt the second trailer memory leak fix to instead pull up the
    strbufs out of the loop such that we can reuse them. This makes the
    code flow more naturally and optimizes the loop.

Thanks!

Patrick

Patrick Steinhardt (22):
  builtin/ls-remote: plug leaking server options
  t/helper: fix leaks in "reach" test tool
  grep: fix leak in `grep_splice_or()`
  builtin/grep: fix leak with `--max-count=0`
  revision: fix leaking bloom filters
  diff-lib: fix leaking diffopts in `do_diff_cache()`
  pretty: clear signature check
  upload-pack: fix leaking URI protocols
  builtin/commit: fix leaking change data contents
  trailer: fix leaking trailer values
  trailer: fix leaking strbufs when formatting trailers
  builtin/commit: fix leaking cleanup config
  transport-helper: fix leaking import/export marks
  builtin/tag: fix leaking key ID on failure to sign
  combine-diff: fix leaking lost lines
  dir: release untracked cache data
  sparse-index: correctly free EWAH contents
  t/helper: stop re-initialization of `the_repository`
  t/helper: fix leaking buffer in "dump-untracked-cache"
  dir: fix leak when parsing "status.showUntrackedFiles"
  builtin/merge: release outbut buffer after performing merge
  list-objects-filter-options: work around reported leak on error

 builtin/commit.c                          | 26 +++++++++++++++++------
 builtin/grep.c                            | 13 +++++++++---
 builtin/ls-remote.c                       |  1 +
 builtin/merge.c                           |  1 +
 builtin/tag.c                             |  2 +-
 combine-diff.c                            |  2 +-
 diff-lib.c                                |  1 +
 dir.c                                     | 12 +++++++++--
 grep.c                                    |  1 +
 list-objects-filter-options.c             | 17 ++++++---------
 pretty.c                                  |  1 +
 revision.c                                |  5 +++++
 sparse-index.c                            |  7 ++++--
 t/helper/test-dump-untracked-cache.c      |  2 ++
 t/helper/test-reach.c                     | 10 +++++++++
 t/helper/test-read-cache.c                |  2 --
 t/t4038-diff-combined.sh                  |  1 +
 t/t4202-log.sh                            |  1 +
 t/t4216-log-bloom.sh                      |  1 +
 t/t5702-protocol-v2.sh                    |  1 +
 t/t5801-remote-helpers.sh                 |  1 +
 t/t6112-rev-list-filters-objects.sh       |  1 +
 t/t6424-merge-unrelated-index-changes.sh  |  1 +
 t/t6600-test-reach.sh                     |  1 +
 t/t7004-tag.sh                            |  1 +
 t/t7031-verify-tag-signed-ssh.sh          |  1 +
 t/t7063-status-untracked-cache.sh         |  1 +
 t/t7500-commit-template-squash-signoff.sh |  1 +
 t/t7502-commit-porcelain.sh               |  1 +
 t/t7510-signed-commit.sh                  |  1 +
 t/t7513-interpret-trailers.sh             |  1 +
 t/t7519-status-fsmonitor.sh               |  1 +
 t/t7528-signed-commit-ssh.sh              |  1 +
 t/t7610-mergetool.sh                      |  1 +
 t/t7810-grep.sh                           |  1 +
 trailer.c                                 | 25 +++++++++++++++-------
 transport-helper.c                        |  2 ++
 upload-pack.c                             |  1 +
 38 files changed, 115 insertions(+), 35 deletions(-)

Range-diff against v1:
 1:  89b66411354 =  1:  89b66411354 builtin/ls-remote: plug leaking server options
 2:  1c42e194b20 =  2:  1c42e194b20 t/helper: fix leaks in "reach" test tool
 3:  cb4eee37b40 =  3:  cb4eee37b40 grep: fix leak in `grep_splice_or()`
 4:  6b2c8842ef5 =  4:  6b2c8842ef5 builtin/grep: fix leak with `--max-count=0`
 5:  7527b31a28f =  5:  7527b31a28f revision: fix leaking bloom filters
 6:  60af98cb2c7 =  6:  60af98cb2c7 diff-lib: fix leaking diffopts in `do_diff_cache()`
 7:  5d5f6867f91 =  7:  5d5f6867f91 pretty: clear signature check
 8:  0d503e40194 =  8:  0d503e40194 upload-pack: fix leaking URI protocols
 9:  9f967dfe5d5 =  9:  9f967dfe5d5 builtin/commit: fix leaking change data contents
 -:  ----------- > 10:  e3ffd59123f trailer: fix leaking trailer values
10:  ca5370d572d ! 11:  5b851453bce trailer: fix leaking trailer values
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    trailer: fix leaking trailer values
    +    trailer: fix leaking strbufs when formatting trailers
     
    -    Fix leaking trailer values when replacing the value with a command or
    -    when the token value is empty.
    +    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
      # When we want one trailing space at the end of each line, let's use sed
     
      ## trailer.c ##
    -@@ trailer.c: 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)
    +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    + 		     struct list_head *trailers,
    + 		     struct strbuf *out)
      {
    - 	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);
    - 	}
    - }
    ++	struct strbuf tok = STRBUF_INIT;
    ++	struct strbuf val = STRBUF_INIT;
    + 	size_t origlen = out->len;
    + 	struct list_head *pos;
    + 	struct trailer_item *item;
      
    -@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
    - 		if (item->token) {
    - 			struct strbuf tok = STRBUF_INIT;
    - 			struct strbuf val = STRBUF_INIT;
     +
    + 	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);
      
    -@@ trailer.c: 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)
    + 			/*
     @@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      				if (!opts->separator)
      					strbuf_addch(out, '\n');
      			}
    -+
    -+next:
    - 			strbuf_release(&tok);
    - 			strbuf_release(&val);
    +-			strbuf_release(&tok);
    +-			strbuf_release(&val);
     -
      		} else if (!opts->only_trailers) {
      			if (opts->separator && out->len != origlen) {
      				strbuf_addbuf(out, opts->separator);
    +@@ trailer.c: 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,
11:  8ca4344ed86 = 12:  60c3f6146f3 builtin/commit: fix leaking cleanup config
12:  16d969ed7b1 = 13:  ea81cd8db86 transport-helper: fix leaking import/export marks
13:  9e4bc5bf73f = 14:  b700ab9079f builtin/tag: fix leaking key ID on failure to sign
14:  8d305d9b1c8 = 15:  76bbcb3fe30 combine-diff: fix leaking lost lines
15:  f977a033cf4 = 16:  d6b96a4012d dir: release untracked cache data
16:  170cc61edaa = 17:  3aa6bac5079 sparse-index: correctly free EWAH contents
17:  8e10ee844c6 = 18:  132fe750906 t/helper: stop re-initialization of `the_repository`
18:  71fd1c76b8a = 19:  b8b702eeb28 t/helper: fix leaking buffer in "dump-untracked-cache"
19:  8ccc246432d = 20:  ad309ac1b37 dir: fix leak when parsing "status.showUntrackedFiles"
20:  bc2206aa47e = 21:  5e243f9ee53 builtin/merge: release outbut buffer after performing merge
21:  6a2baf0d3e5 = 22:  b75376e3725 list-objects-filter-options: work around reported leak on error
-- 
2.47.0.72.gef8ce8f3d4.dirty