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

[Patrick Steinhardt <ps@pks.im>]

Hi,

this is the third revision of my 9th series of memory leak fixes.
Changes compared to v2:

  - Remove an unnecessary cast.

  - Fix a duplicate newline.

  - Polish a couple of commit messages.

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 output 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                            |  5 +++--
 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                                 | 22 +++++++++++++------
 transport-helper.c                        |  2 ++
 upload-pack.c                             |  1 +
 38 files changed, 115 insertions(+), 35 deletions(-)

Range-diff against v2:
 1:  89b66411354 !  1:  fbb9e68e2f2 builtin/ls-remote: plug leaking server options
    @@ Metadata
      ## Commit message ##
         builtin/ls-remote: plug leaking server options
     
    -    The server options populated via `OPT_STRING_LIST()` is never cleared,
    -    causing a memory leak. Plug it.
    +    The list of server options populated via `OPT_STRING_LIST()` is never
    +    cleared, causing a memory leak. Plug it.
     
         This leak is exposed by t5702, but plugging it alone does not make the
         whole test suite pass.
 2:  1c42e194b20 =  2:  17136f4bdb3 t/helper: fix leaks in "reach" test tool
 3:  cb4eee37b40 !  3:  74b21470194 grep: fix leak in `grep_splice_or()`
    @@ Commit message
         grep: fix leak in `grep_splice_or()`
     
         In `grep_splice_or()` we search for the next `TRUE` node in our tree of
    -    grep exrpessions and replace it with the given new expression. But we
    +    grep expressions and replace it with the given new expression. But we
         don't free the old node, which causes a memory leak. Plug it.
     
         This leak is exposed by t7810, but plugging it alone isn't sufficient to
 4:  6b2c8842ef5 =  4:  d716f93169a builtin/grep: fix leak with `--max-count=0`
 5:  7527b31a28f =  5:  aeb8a19d28d revision: fix leaking bloom filters
 6:  60af98cb2c7 !  6:  24d9d9b1358 diff-lib: fix leaking diffopts in `do_diff_cache()`
    @@ Commit message
         In `do_diff_cache()` we initialize a new `rev_info` and then overwrite
         its `diffopt` with a user-provided set of options. This can leak memory
         because `repo_init_revisions()` may end up allocating memory for the
    -    `diffopt` itself depending on the configuration. And as that field is
    -    overwritten we won't ever free that.
    +    `diffopt` itself depending on the configuration. And since that field is
    +    overwritten we won't ever free it.
     
         Plug the memory leak by releasing the diffopts before we overwrite them.
     
 7:  5d5f6867f91 !  7:  58ebef7e757 pretty: clear signature check
    @@ Metadata
      ## Commit message ##
         pretty: clear signature check
     
    -    The signature check in of the formatting context is never getting
    -    released. Fix this to plug the resulting memory leak.
    +    The signature check in the formatting context is never getting released.
    +    Fix this to plug the resulting memory leak.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 8:  0d503e40194 =  8:  c5813079d44 upload-pack: fix leaking URI protocols
 9:  9f967dfe5d5 =  9:  f66fa4e0e25 builtin/commit: fix leaking change data contents
10:  e3ffd59123f ! 10:  dac63bae39e trailer: fix leaking trailer values
    @@ trailer.c: static char *apply_command(struct conf_info *conf, const char *arg)
     +		char *arg;
     +
      		if (arg_tok->value && arg_tok->value[0]) {
    --			arg = arg_tok->value;
    -+			arg = (char *)arg_tok->value;
    + 			arg = arg_tok->value;
      		} else {
    - 			if (in_tok && in_tok->value)
    +@@ trailer.c: static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
      				arg = xstrdup(in_tok->value);
      			else
      				arg = xstrdup("");
11:  5b851453bce ! 11:  82269e5d5be trailer: fix leaking strbufs when formatting trailers
    @@ Metadata
      ## Commit message ##
         trailer: fix leaking strbufs when formatting trailers
     
    -    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.
    +    When formatting trailer lines we iterate through each of the trailers
    +    and munge their respective token/value pairs according to the trailer
    +    options. When formatting a trailer that has its `item->token` pointer
    +    set we perform the munging in two local buffers. In the case where we
    +    figure out that the value is empty and `trim_empty` is set we just skip
    +    over the trailer item. But the buffers are local to the loop and we
    +    don't release their contents, leading to a memory leak.
    +
    +    Plug this leak by lifting the 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>
     
    @@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      	size_t origlen = out->len;
      	struct list_head *pos;
      	struct trailer_item *item;
    - 
    -+
    +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
      	list_for_each(pos, trailers) {
      		item = list_entry(pos, struct trailer_item, list);
      		if (item->token) {
12:  60c3f6146f3 = 12:  a627e03702e builtin/commit: fix leaking cleanup config
13:  ea81cd8db86 = 13:  40e0c2a2cac transport-helper: fix leaking import/export marks
14:  b700ab9079f = 14:  ffa5d9eae7e builtin/tag: fix leaking key ID on failure to sign
15:  76bbcb3fe30 ! 15:  70dd0cb6933 combine-diff: fix leaking lost lines
    @@ Commit message
         The `cnt` variable tracks the number of lines in a patch diff. It can
         happen though that there are no newlines, in which case we'd still end
         up allocating our array of `sline`s. In fact, we always allocate it with
    -    `cnt + 2` entries. But when we loop through the array to clear it at the
    -    end of this function we only loop until `lno < cnt`, and thus we may not
    -    end up releasing whatever the two extra `sline`s contain.
    +    `cnt + 2` entries: one extra entry for the deletion hunk at the end, and
    +    another entry that we don't seem to ever populate at all but acts as a
    +    kind of sentinel value.
     
    -    Plug this memory leak.
    +    When we loop through the array to clear it at the end of this function
    +    we only loop until `lno < cnt`, and thus we may not end up releasing
    +    whatever the two extra `sline`s contain. While that shouldn't matter for
    +    the sentinel value, it does matter for the extra deletion hunk sline.
    +    Regardless of that, plug this memory leak by releasing both extra
    +    entries, which makes the logic a bit easier to reason about.
    +
    +    While at it, fix the formatting of a local comment, which incidentally
    +    also provides the necessary context for why we overallocate the `sline`
    +    array.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## combine-diff.c ##
    +@@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
    + 	result_file.ptr = result;
    + 	result_file.size = result_size;
    + 
    +-	/* Even p_lno[cnt+1] is valid -- that is for the end line number
    ++	/*
    ++	 * Even p_lno[cnt+1] is valid -- that is for the end line number
    + 	 * for deletion hunk at the end.
    + 	 */
    + 	CALLOC_ARRAY(sline[0].p_lno, st_mult(st_add(cnt, 2), num_parent));
     @@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
      	}
      	free(result);
16:  d6b96a4012d = 16:  b248f266a91 dir: release untracked cache data
17:  3aa6bac5079 = 17:  76e9a6d5792 sparse-index: correctly free EWAH contents
18:  132fe750906 = 18:  70f16486d77 t/helper: stop re-initialization of `the_repository`
19:  b8b702eeb28 = 19:  f056d31ca50 t/helper: fix leaking buffer in "dump-untracked-cache"
20:  ad309ac1b37 = 20:  714c9286e7a dir: fix leak when parsing "status.showUntrackedFiles"
21:  5e243f9ee53 ! 21:  0ff65c1213b builtin/merge: release outbut buffer after performing merge
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    builtin/merge: release outbut buffer after performing merge
    +    builtin/merge: release output buffer after performing merge
     
         The `obuf` member of `struct merge_options` is used to buffer output in
         some cases. In order to not discard its allocated memory we only release
22:  b75376e3725 = 22:  d9e122bb5db list-objects-filter-options: work around reported leak on error
-- 
2.47.0.229.g8f8d6eee53.dirty