git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 0/6] more small leak fixes
Date: Fri, 14 Aug 2020 12:25:25 -0400	[thread overview]
Message-ID: <20200814162525.GA595840@coredump.intra.peff.net> (raw)
In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net>

On Fri, Aug 14, 2020 at 12:13:28PM -0400, Jeff King wrote:

> Based on the discussion over in [1], I wondered how close we were to
> being able to run some test scripts with a leak-checker like LSan not
> complaining. The answer is...not close.
> 
> I picked t5526 more or less at random (it was the first failure when I
> did a parallel "make test") to see what it would take to get it passing.
> After much effort...I have t5526.1, the setup test, running clean. :)

For reference, here are the UNLEAK() calls I had to add to silence the
false positives. Some of these are kind of sketchy. For example,
declaring that wt_status_collect_changes_index() is allowed to leak is a
bit low-level for my tastes (in general it's probably only called once
per program, but it's getting quite far from the bottom of the stack).
But there's actually no convenient way to free the various bits of a
rev_info, so marking it with UNLEAK() is an expedient hack.

---
 builtin/clone.c             | 9 +++++++++
 builtin/init-db.c           | 1 +
 builtin/rev-list.c          | 1 +
 builtin/submodule--helper.c | 4 ++++
 parse-options.c             | 1 +
 wt-status.c                 | 3 +++
 6 files changed, 19 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index b087ee40c2..b2578c3c50 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -998,6 +998,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
+	UNLEAK(path);
 	if (path)
 		repo = absolute_pathdup(repo_name);
 	else if (strchr(repo_name, ':')) {
@@ -1047,6 +1048,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		work_tree = dir;
 		git_dir = mkpathdup("%s/.git", dir);
 	}
+	UNLEAK(git_dir);
 
 	atexit(remove_junk);
 	sigchain_push_common(remove_junk_on_signal);
@@ -1166,6 +1168,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	transport->family = family;
 
 	path = get_repo_path(remote->url[0], &is_bundle);
+	UNLEAK(path);
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local) {
 		if (option_depth)
@@ -1336,5 +1339,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&ref_prefixes);
+
+	UNLEAK(remote_head_points_at);
+	UNLEAK(refs);
+	UNLEAK(mapped_refs);
+	UNLEAK(repo);
+
 	return err;
 }
diff --git a/builtin/init-db.c b/builtin/init-db.c
index f70076d38e..04087ed7e6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -665,6 +665,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	UNLEAK(real_git_dir);
 	UNLEAK(git_dir);
 	UNLEAK(work_tree);
+	UNLEAK(template_dir);
 
 	flags |= INIT_DB_EXIST_OK;
 	return init_db(git_dir, real_git_dir, template_dir, hash_algo,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f520111eda..71c92f6747 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -506,6 +506,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		revs.do_not_die_on_missing_tree = 1;
 
 	argc = setup_revisions(argc, argv, &revs, &s_r_opt);
+	UNLEAK(revs);
 
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a59d8e4bda..850c8ff966 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -734,6 +734,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 		info.flags |= OPT_QUIET;
 
 	for_each_listed_submodule(&list, init_submodule_cb, &info);
+	UNLEAK(list);
 
 	return 0;
 }
@@ -1874,6 +1875,8 @@ static int update_submodules(struct submodule_update_clone *suc)
 				   update_clone_task_finished, suc, "submodule",
 				   "parallel/update");
 
+	UNLEAK(*suc);
+
 	/*
 	 * We saved the output and put it out all at once now.
 	 * That means:
@@ -2133,6 +2136,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
 		strbuf_release(&sb);
 	}
 
+	repo_clear(&subrepo);
 	return 0;
 }
 
diff --git a/parse-options.c b/parse-options.c
index c57618d537..b87cb3d70a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -675,6 +675,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			UNLEAK(newopt[i].help);
 			break;
 		}
 
diff --git a/wt-status.c b/wt-status.c
index d75399085d..7f605958f2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -616,6 +616,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;
 	copy_pathspec(&rev.prune_data, &s->pathspec);
 	run_diff_files(&rev, 0);
+	UNLEAK(rev);
 }
 
 static void wt_status_collect_changes_index(struct wt_status *s)
@@ -627,6 +628,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	memset(&opt, 0, sizeof(opt));
 	opt.def = s->is_initial ? empty_tree_oid_hex() : s->reference;
 	setup_revisions(0, NULL, &rev, &opt);
+	UNLEAK(rev);
 
 	rev.diffopt.flags.override_submodule_config = 1;
 	rev.diffopt.ita_invisible_in_index = 1;
@@ -652,6 +654,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;
 	copy_pathspec(&rev.prune_data, &s->pathspec);
 	run_diff_index(&rev, 1);
+	UNLEAK(rev);
 }
 
 static void wt_status_collect_changes_initial(struct wt_status *s)
-- 
2.28.0.596.g9c08d63829


  parent reply	other threads:[~2020-08-14 16:25 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-14 16:13 Jeff King
2020-08-14 16:14 ` [PATCH 1/6] submodule--helper: use strbuf_release() to free strbufs Jeff King
2020-08-14 16:14 ` [PATCH 2/6] checkout: fix leak of non-existent branch names Jeff King
2020-08-14 16:17 ` [PATCH 3/6] config: fix leaks from git_config_get_string_const() Jeff King
2020-08-14 16:19 ` [PATCH 4/6] config: drop git_config_get_string_const() Jeff King
2020-08-14 20:21   ` Junio C Hamano
2020-08-15  6:34     ` Jeff King
2020-08-17 17:36       ` Junio C Hamano
2020-08-14 16:19 ` [PATCH 5/6] config: fix leak in git_config_get_expiry_in_days() Jeff King
2020-08-14 16:20 ` [PATCH 6/6] submodule--helper: fix leak of core.worktree value Jeff King
2020-08-14 16:25 ` Jeff King [this message]
2020-08-14 16:27   ` [PATCH 0/6] more small leak fixes Jeff King
2020-08-17 21:32 ` [PATCH v2 0/7] " Jeff King
2020-08-17 21:33   ` [PATCH v2 1/7] clear_pattern_list(): clear embedded hashmaps Jeff King
2020-08-17 21:33   ` [PATCH v2 2/7] submodule--helper: use strbuf_release() to free strbufs Jeff King
2020-08-17 21:33   ` [PATCH v2 3/7] checkout: fix leak of non-existent branch names Jeff King
2020-08-17 21:33   ` [PATCH v2 4/7] config: fix leaks from git_config_get_string_const() Jeff King
2020-08-17 21:33   ` [PATCH v2 5/7] config: drop git_config_get_string_const() Jeff King
2020-08-17 21:33   ` [PATCH v2 6/7] config: fix leak in git_config_get_expiry_in_days() Jeff King
2020-08-17 21:33   ` [PATCH v2 7/7] submodule--helper: fix leak of core.worktree value Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200814162525.GA595840@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH 0/6] more small leak fixes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).