Re: [PATCH] chdir-notify: UNLEAK registrated callback entries

From: Elijah Newren <newren@gmail.com>
Date: Nov 17, 2020, 04:49 UTC
Message-ID: <20201117044923.3703483-1-newren@gmail.com>
In-Reply-To: <58f36a04-afa7-3cf3-ce0a-ad53004dd774@web.de>

Hi,

On Sat, Nov 14, 2020 at 1:55 PM René Scharfe <l.s.r@web.de> wrote:

Show 13 quoted lines

>
> Am 14.11.20 um 22:40 schrieb René Scharfe:
> > chdir_notify_register() allows registering functions to notify when
> > chdir() is called.  There is no way to unsubscribe or shut this
> > mechanism down, so these entries are present until the program ends.
> >
> > Valgrind reports allocations for these registrations as "possibly lost",
> > probably because it doesn't see through list.h's offsetof tricks.
>
> Right, avoiding list.h like below lets Valgrind classify the memory as
> "still reachable", without UNLEAK.  Not sure if it's worth it, though.
>
> Note my somewhat concerning knee-jerk reaction to write some macros. ;)

I've got a local patch that changes these to actually be freed as yet another route we could go. It's mixed with a few (half-baked) revision-related memory cleanups that should be separated, and I'm unsure if there are other places that would need a finalize_globals() call too, but it's another approach to consider...

-- >8 --
Subject: [RFC PATCH] Make merge-ort run leak check free

Tweaks to revision walking, adding a chdir_deregister_all() (so that all watchers can be cleared as well as free'ing their associated memory), and maybe other stuff.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/fast-rebase.c |  2 ++
 cache.h               |  3 +++
 chdir-notify.c        | 15 +++++++++++++++
 chdir-notify.h        |  5 +++++
 git.c                 | 10 ++++++++--
 revision.c            | 11 +++++++++++
 revision.h            |  6 ++++++
 setup.c               |  5 +++++
 8 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c
index 19333ca42d..32cab36ad3 100644
--- a/builtin/fast-rebase.c
+++ b/builtin/fast-rebase.c
@@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)
 		last_commit = create_commit(result.tree, commit, last_commit);
 	}
 	fprintf(stderr, "\nDone.\n");
+	rev_info_free(&revs);
+	memset(&revs, 0, sizeof(revs));
 
 	merge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);
 
diff --git a/cache.h b/cache.h
index c0072d43b1..b9b543f75b 100644
--- a/cache.h
+++ b/cache.h
@@ -604,6 +604,9 @@ const char *setup_git_directory(void);
 char *prefix_path(const char *prefix, int len, const char *path);
 char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);
 
+/* Release resources related to global config. */
+void finalize_globals(void);
+
 /*
  * Concatenate "prefix" (if len is non-zero) and "path", with no
  * connecting characters (so "prefix" should end with a "/").
diff --git a/chdir-notify.c b/chdir-notify.c
index 5f7f2c2ac2..ea7e52cfea 100644
--- a/chdir-notify.c
+++ b/chdir-notify.c
@@ -91,3 +91,18 @@ char *reparent_relative_path(const char *old_cwd,
 
 	return ret;
 }
+
+void chdir_deregister_all(void)
+{
+	struct list_head *pos, *tmp;
+
+	if (list_empty(&chdir_notify_entries))
+		return;
+
+	list_for_each_safe(pos, tmp, &chdir_notify_entries) {
+		struct chdir_notify_entry *e =
+			list_entry(pos, struct chdir_notify_entry, list);
+		free(e);
+	}
+	memset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));
+}
diff --git a/chdir-notify.h b/chdir-notify.h
index 366e4c1ee9..0cd3e1f633 100644
--- a/chdir-notify.h
+++ b/chdir-notify.h
@@ -70,4 +70,9 @@ char *reparent_relative_path(const char *old_cwd,
 			     const char *new_cwd,
 			     const char *path);
 
+/*
+ * Deregister all subscribers and free any associated memory.
+ */
+void chdir_deregister_all(void);
+
 #endif /* CHDIR_NOTIFY_H */
diff --git a/git.c b/git.c
index af84f11e69..4b04988909 100644
--- a/git.c
+++ b/git.c
@@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			trace2_cmd_name("_run_shell_alias_");
 
 			ret = run_command(&child);
+			finalize_globals();
 			if (ret >= 0)   /* normal exit */
 				exit(ret);
 
@@ -697,8 +698,11 @@ static void handle_builtin(int argc, const char **argv)
 	}
 
 	builtin = get_builtin(cmd);
-	if (builtin)
-		exit(run_builtin(builtin, argc, argv));
+	if (builtin) {
+		int result = run_builtin(builtin, argc, argv);
+		finalize_globals();
+		exit(result);
+	}
 	strvec_clear(&args);
 }
 
@@ -742,6 +746,7 @@ static void execv_dashed_external(const char **argv)
 	 * generic string as our trace2 command verb to indicate that we
 	 * launched a dashed command.
 	 */
+	finalize_globals();
 	if (status >= 0)
 		exit(status);
 	else if (errno != ENOENT)
@@ -796,6 +801,7 @@ static int run_argv(int *argcp, const char ***argv)
 			 */
 			i = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |
 						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
+			finalize_globals();
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/revision.c b/revision.c
index c6e169e3eb..b4f276d854 100644
--- a/revision.c
+++ b/revision.c
@@ -1419,6 +1419,8 @@ static int limit_list(struct rev_info *revs)
 			slop = still_interesting(list, date, slop, &interesting_cache);
 			if (slop)
 				continue;
+			free_commit_list(list);
+			revs->commits = NULL;
 			break;
 		}
 		if (revs->min_age != -1 && (commit->date > revs->min_age) &&
@@ -3281,6 +3283,15 @@ static void set_children(struct rev_info *revs)
 	}
 }
 
+void rev_info_free(struct rev_info *revs)
+{
+	int i;
+
+	for (i = 0; i < revs->cmdline.nr; i++)
+		free((char*)revs->cmdline.rev[i].name);
+	free(revs->cmdline.rev);
+}
+
 void reset_revision_walk(void)
 {
 	clear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);
diff --git a/revision.h b/revision.h
index f6bf860d19..5dd98f2f6d 100644
--- a/revision.h
+++ b/revision.h
@@ -362,6 +362,12 @@ void repo_init_revisions(struct repository *r,
 int setup_revisions(int argc, const char **argv, struct rev_info *revs,
 		    struct setup_revision_opt *);
 
+/**
+ * Free information associated with a rev_info once no longer needed.
+ * Will not free revs itself.
+ */
+void rev_info_free(struct rev_info *revs);
+
 void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const struct option *options,
 			const char * const usagestr[]);
diff --git a/setup.c b/setup.c
index c04cd25a30..cd958f5f5c 100644
--- a/setup.c
+++ b/setup.c
@@ -1411,6 +1411,11 @@ void sanitize_stdfds(void)
 		close(fd);
 }
 
+void finalize_globals(void)
+{
+	chdir_deregister_all();
+}
+
 int daemonize(void)
 {
 #ifdef NO_POSIX_GOODIES

-- 
2.29.2.529.gc090dab8c5