Memory leak fixes (pt.9)

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.

 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.

@@ 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 slines. 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 slines 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 slines 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.

## 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 <[email protected]>

## 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