[PATCH v2 00/20] Memory leak fixes (pt.5)

[Patrick Steinhardt <ps@pks.im>]

Hi,

this is version 2 of this patch series that fixes another set of memory
leaks.

Changes compared to v1:

  - Remove spaces between cast and variable.

  - Clarify why we move the `ctx.entries_nr` block.

  - Adapt sideband colors to use `git_config_get_string_tmp()` such that
    we do not have to allocate the strings in the first place.

  - Fix more memory leaks for config values in the remote state.

  - Refactor `unbundle()` to not free extra args passed by the caller
    anymore. Instead, this is now always done by the caler.

Thanks!

Patrick

Patrick Steinhardt (20):
  mailinfo: fix leaking header data
  convert: fix leaks when resetting attributes
  pretty: fix memory leaks when parsing pretty formats
  pretty: fix leaking key/value separator buffer
  builtin/merge-tree: fix leaking `-X` strategy options
  builtin/upload-archive: fix leaking args passed to `write_archive()`
  builtin/archive: fix leaking `OPT_FILENAME()` value
  midx-write: fix leaking hashfile on error cases
  builtin/repack: fix leaks when computing packs to repack
  t/helper: fix leaking multi-pack-indices in "read-midx"
  transport: fix leaking OID arrays in git:// transport data
  builtin/send-pack: fix leaking refspecs
  sideband: fix leaks when configuring sideband colors
  builtin/fetch-pack: fix leaking refs
  remote: fix leaking config strings
  remote: fix leaks when matching refspecs
  remote: fix leaking peer ref when expanding refmap
  builtin/fetch: fix leaking transaction with `--atomic`
  transport: fix leaking arguments when fetching from bundle
  transport: fix leaking negotiation tips

 archive.c                           | 10 ++++
 builtin/archive.c                   |  7 ++-
 builtin/bundle.c                    |  2 +
 builtin/fetch-pack.c                | 20 ++++---
 builtin/fetch.c                     |  8 +--
 builtin/merge-tree.c                | 13 ++++-
 builtin/repack.c                    | 36 +++++++++---
 builtin/send-pack.c                 |  1 +
 builtin/upload-archive.c            |  8 ++-
 bundle.c                            |  4 +-
 convert.c                           |  3 +
 mailinfo.c                          | 17 +++++-
 midx-write.c                        | 24 ++++----
 pretty.c                            | 13 ++++-
 remote.c                            | 85 +++++++++++++++++++++++------
 sideband.c                          | 15 +++--
 t/helper/test-read-midx.c           |  8 ++-
 t/t4150-am.sh                       |  1 +
 t/t4205-log-pretty-formats.sh       |  2 +
 t/t4301-merge-tree-write-tree.sh    |  1 +
 t/t5000-tar-tree.sh                 |  1 +
 t/t5003-archive-zip.sh              |  1 +
 t/t5100-mailinfo.sh                 |  1 +
 t/t5319-multi-pack-index.sh         |  2 +
 t/t5400-send-pack.sh                |  1 +
 t/t5401-update-hooks.sh             |  2 +
 t/t5408-send-pack-stdin.sh          |  2 +
 t/t5409-colorize-remote-messages.sh |  1 +
 t/t5501-fetch-push-alternates.sh    |  1 +
 t/t5505-remote.sh                   |  1 +
 t/t5510-fetch.sh                    |  1 +
 t/t5519-push-alternates.sh          |  1 +
 t/t5536-fetch-conflicts.sh          |  1 +
 t/t5548-push-porcelain.sh           |  1 +
 t/t5553-set-upstream.sh             |  1 +
 t/t5574-fetch-output.sh             |  1 +
 t/t5703-upload-pack-ref-in-want.sh  |  1 +
 t/t5812-proto-disable-http.sh       |  2 +
 t/t6050-replace.sh                  |  1 +
 t/t7704-repack-cruft.sh             |  1 +
 transport.c                         |  8 +++
 41 files changed, 237 insertions(+), 73 deletions(-)

Range-diff against v1:
 1:  69e30ea5179 =  1:  69e30ea5179 mailinfo: fix leaking header data
 2:  ed0f01bf92c =  2:  ed0f01bf92c convert: fix leaks when resetting attributes
 3:  82f3908f962 !  3:  8a1963685e7 pretty: fix memory leaks when parsing pretty formats
    @@ pretty.c: static int git_pretty_formats_config(const char *var, const char *valu
      		commit_formats_len++;
      	}
      
    -+	free((char *) commit_format->name);
    ++	free((char *)commit_format->name);
      	commit_format->name = xstrdup(name);
      	commit_format->format = CMIT_FMT_USERFORMAT;
      	if (git_config_string(&fmt, var, value))
      		return -1;
      
     -	if (skip_prefix(fmt, "format:", &commit_format->user_format)) {
    -+	free((char *) commit_format->user_format);
    ++	free((char *)commit_format->user_format);
     +	if (skip_prefix(fmt, "format:", &stripped)) {
      		commit_format->is_tformat = 0;
     -	} else if (skip_prefix(fmt, "tformat:", &commit_format->user_format)) {
 4:  696467780e6 =  4:  1c368a4489a pretty: fix leaking key/value separator buffer
 5:  53db2fc7206 =  5:  c62c5e9604e builtin/merge-tree: fix leaking `-X` strategy options
 6:  5b05a325218 =  6:  afdd7f90b13 builtin/upload-archive: fix leaking args passed to `write_archive()`
 7:  bad575df126 =  7:  38487f3f65b builtin/archive: fix leaking `OPT_FILENAME()` value
 8:  5f042ce5098 !  8:  693c93ddbf7 midx-write: fix leaking hashfile on error cases
    @@ Commit message
         hashfile and finalizing it anymore this is sufficient to fix the memory
         leak.
     
    +    While at it, also move around the block checking for `ctx.entries_nr`.
    +    This change is not required to fix the memory leak, but it feels natural
    +    to move together all massaging of parameters before we go with them and
    +    execute the actual logic.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## midx-write.c ##
 9:  5c820da9761 =  9:  6bca72e5c57 builtin/repack: fix leaks when computing packs to repack
10:  9caf5eeea93 = 10:  48b60279d18 t/helper: fix leaking multi-pack-indices in "read-midx"
11:  8e12c55536d = 11:  0cb440ef648 transport: fix leaking OID arrays in git:// transport data
12:  5d8e0a3d8b4 = 12:  f4300c9326b builtin/send-pack: fix leaking refspecs
13:  5d09959b642 ! 13:  28805c15a42 sideband: fix leaks when configuring sideband colors
    @@ Commit message
     
         We read a bunch of configs in `use_sideband_colors()` to configure the
         colors that Git should use. We never free the strings read from the
    -    config though, causing memory leaks. Fix those.
    +    config though, causing memory leaks.
    +
    +    Refactor the code to use `git_config_get_string_tmp()` instead, which
    +    does not allocate memory. As we throw the strings away after parsing
    +    them anyway there is no need to use allocated strings.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ sideband.c: static int use_sideband_colors(void)
      	const char *key = "color.remote";
      	struct strbuf sb = STRBUF_INIT;
     -	char *value;
    -+	char *value = NULL;
    ++	const char *value;
      	int i;
      
      	if (use_sideband_colors_cached >= 0)
    -@@ sideband.c: static int use_sideband_colors(void)
    - 	} else {
    + 		return use_sideband_colors_cached;
    + 
    +-	if (!git_config_get_string(key, &value)) {
    ++	if (!git_config_get_string_tmp(key, &value))
    + 		use_sideband_colors_cached = git_config_colorbool(key, value);
    +-	} else if (!git_config_get_string("color.ui", &value)) {
    ++	else if (!git_config_get_string_tmp("color.ui", &value))
    + 		use_sideband_colors_cached = git_config_colorbool("color.ui", value);
    +-	} else {
    ++	else
      		use_sideband_colors_cached = GIT_COLOR_AUTO;
    - 	}
    -+	FREE_AND_NULL(value);
    +-	}
      
      	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
      		strbuf_reset(&sb);
      		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
    - 		if (git_config_get_string(sb.buf, &value))
    - 			continue;
    --		if (color_parse(value, keywords[i].color))
    +-		if (git_config_get_string(sb.buf, &value))
     -			continue;
    +-		if (color_parse(value, keywords[i].color))
    ++		if (git_config_get_string_tmp(sb.buf, &value))
    + 			continue;
     +		color_parse(value, keywords[i].color);
    -+		FREE_AND_NULL(value);
      	}
     +
      	strbuf_release(&sb);
14:  1c94195488d = 14:  aac84c5a2b7 builtin/fetch-pack: fix leaking refs
15:  97346d6f944 ! 15:  532328b7814 remote: fix leaking config strings
    @@ Commit message
         We're leaking several config strings when assembling remotes, either
         because we do not free preceding values in case a config was set
         multiple times, or because we do not free them when releasing the remote
    -    state. Plug those leaks.
    +    state. This includes config strings for "branch" sections, "insteadOf",
    +    "pushInsteadOf", and "pushDefault".
    +
    +    Plug those leaks.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## remote.c ##
    +@@ remote.c: static struct branch *make_branch(struct remote_state *remote_state,
    + 	return ret;
    + }
    + 
    ++static void branch_release(struct branch *branch)
    ++{
    ++	free((char *)branch->name);
    ++	free((char *)branch->refname);
    ++	free(branch->remote_name);
    ++	free(branch->pushremote_name);
    ++	for (int i = 0; i < branch->merge_nr; i++)
    ++		refspec_item_clear(branch->merge[i]);
    ++	free(branch->merge);
    ++}
    ++
    + static struct rewrite *make_rewrite(struct rewrites *r,
    + 				    const char *base, size_t len)
    + {
    +@@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,
    + 	return ret;
    + }
    + 
    ++static void rewrites_release(struct rewrites *r)
    ++{
    ++	for (int i = 0; i < r->rewrite_nr; i++)
    ++		free((char *)r->rewrite[i]->base);
    ++	free(r->rewrite);
    ++	memset(r, 0, sizeof(*r));
    ++}
    ++
    + static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
    + {
    + 	ALLOC_GROW(rewrite->instead_of, rewrite->instead_of_nr + 1, rewrite->instead_of_alloc);
     @@ remote.c: static int handle_config(const char *key, const char *value,
      			return -1;
      		branch = make_branch(remote_state, name, namelen);
    @@ remote.c: static int handle_config(const char *key, const char *value,
      		return git_config_string(&remote->foreign_vcs, key, value);
      	}
      	return 0;
    -@@ remote.c: void remote_state_clear(struct remote_state *remote_state)
    +@@ remote.c: struct remote_state *remote_state_new(void)
    + 
    + void remote_state_clear(struct remote_state *remote_state)
    + {
    ++	struct hashmap_iter iter;
    ++	struct branch *b;
    + 	int i;
    + 
      	for (i = 0; i < remote_state->remotes_nr; i++)
      		remote_clear(remote_state->remotes[i]);
      	FREE_AND_NULL(remote_state->remotes);
    @@ remote.c: void remote_state_clear(struct remote_state *remote_state)
      	remote_state->remotes_alloc = 0;
      	remote_state->remotes_nr = 0;
      
    ++	rewrites_release(&remote_state->rewrites);
    ++	rewrites_release(&remote_state->rewrites_push);
    ++
    + 	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
    +-	hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
    ++	hashmap_for_each_entry(&remote_state->branches_hash, &iter, b, ent) {
    ++		branch_release(b);
    ++		free(b);
    ++	}
    ++	hashmap_clear(&remote_state->branches_hash);
    + }
    + 
    + /*
16:  e1d0be37636 = 16:  440b3d99372 remote: fix leaks when matching refspecs
17:  773fe580d75 = 17:  662ec4e6484 remote: fix leaking peer ref when expanding refmap
18:  9ede792550e = 18:  dbd8eaa2cb1 builtin/fetch: fix leaking transaction with `--atomic`
19:  b72f7d1ee39 ! 19:  4c5740afe43 transport: fix leaking arguments when fetching from bundle
    @@ Commit message
         transport: fix leaking arguments when fetching from bundle
     
         In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
    -    to `unbundle()`, but never free it. Fix this leak.
    +    to `unbundle()`, but never free it. And in theory we wouldn't have to
    +    because `unbundle()` already knows to free the vector for us. But it
    +    fails to do so when it exits early due to `verify_bundle()` failing.
    +
    +    The calling convention that the arguments are freed by the callee and
    +    not the caller feels somewhat weird. Refactor the code such that it is
    +    instead the responsibility of the caller to free the vector, adapting
    +    the only two callsites where we pass extra arguments. This also fixes
    +    the memory leak.
     
         This memory leak gets hit in t5510, but fixing it isn't sufficient to
         make the whole test suite pass.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## builtin/bundle.c ##
    +@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    + 			 &extra_index_pack_args, 0) ||
    + 		list_bundle_refs(&header, argc, argv);
    + 	bundle_header_release(&header);
    ++
    + cleanup:
    ++	strvec_clear(&extra_index_pack_args);
    + 	free(bundle_file);
    + 	return ret;
    + }
    +
    + ## bundle.c ##
    +@@ bundle.c: int unbundle(struct repository *r, struct bundle_header *header,
    + 	if (flags & VERIFY_BUNDLE_FSCK)
    + 		strvec_push(&ip.args, "--fsck-objects");
    + 
    +-	if (extra_index_pack_args) {
    ++	if (extra_index_pack_args)
    + 		strvec_pushv(&ip.args, extra_index_pack_args->v);
    +-		strvec_clear(extra_index_pack_args);
    +-	}
    + 
    + 	ip.in = bundle_fd;
    + 	ip.no_stdout = 1;
    +
      ## transport.c ##
     @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
      		       &extra_index_pack_args,
20:  e8f479deeb2 = 20:  c3d2b035761 transport: fix leaking negotiation tips
-- 
2.46.0.164.g477ce5ccd6.dirty