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@xxxxxx>
## 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@xxxxxx>
@@ 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@xxxxxx>
## 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@xxxxxx>
+ ## 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
