[PATCH 00/23] Memory leak fixes (pt.7)

[PATCH 00/23] Memory leak fixes (pt.7)

[Patrick Steinhardt]

Hi,

as usual, whenever the previous part of memory leak fixes has landed,
here's the next one. This series brings us down to 80 leaking test
suites. The 8th part is almost ready, too, bringing us down to 48
leaking suites. So the end is near, and I hope that our test suites are
leak free after the 10th part has landed.

The patch series is built on top of ed155187b4 (Sync with Git 2.46.1,
2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix
leaking keep-pack list, 2024-09-05) merged into it.

Thanks!

Patrick

Patrick Steinhardt (23):
  builtin/help: fix dangling reference to `html_path`
  builtin/help: fix leaking `html_path` when reading config multiple
    times
  git: fix leaking argv when handling builtins
  submodule: fix leaking update strategy
  builtin/submodule--helper: clear child process when not running it
  builtin/submodule--helper: fix leaking error buffer
  t/helper: fix leaking subrepo in nested submodule config helper
  builtin/submodule--helper: fix leaking remote ref on errors
  dir: fix off by one errors for ignored and untracked entries
  builtin/pull: fix leaking "ff" option
  diff: fix leaking orderfile option
  parse-options: free previous value of `OPTION_FILENAME`
  diffcore-order: fix leaking buffer when parsing orderfiles
  builtin/repack: fix leaking configuration
  builtin/difftool: plug several trivial memory leaks
  trace2: destroy context stored in thread-local storage
  submodule: fix leaking submodule ODB paths
  grep: fix leaking grep pattern
  promisor-remote: fix leaking partial clone filter
  builtin/maintenance: fix leaking config string
  builtin/maintenance: fix leak in `get_schedule_cmd()`
  revision: fix leaking parents when simplifying commits
  diffcore-break: fix leaking filespecs when merging broken pairs

 builtin/difftool.c                            |   6 +
 builtin/gc.c                                  | 131 +++++++++++-------
 builtin/help.c                                |  16 ++-
 builtin/pull.c                                |  11 +-
 builtin/repack.c                              |  57 ++++++--
 builtin/submodule--helper.c                   |  26 +++-
 combine-diff.c                                |   3 +-
 diff.c                                        |   7 +-
 diff.h                                        |   2 +-
 diffcore-break.c                              |   4 +-
 diffcore-order.c                              |  19 +--
 dir.c                                         |   6 +-
 git.c                                         |  22 ++-
 grep.c                                        |   2 +-
 parse-options.c                               |  22 +--
 promisor-remote.c                             |   2 +
 revision.c                                    |   5 +
 submodule-config.c                            |   2 +-
 submodule.c                                   |   9 +-
 submodule.h                                   |   6 +-
 t/helper/test-submodule-nested-repo-config.c  |   2 +-
 t/t0012-help.sh                               |   1 +
 t/t1414-reflog-walk.sh                        |   1 +
 ...common-prefixes-and-directory-traversal.sh |   1 +
 t/t4008-diff-break-rewrite.sh                 |   2 +
 t/t4022-diff-rewrite.sh                       |   1 +
 t/t4023-diff-rename-typechange.sh             |   1 +
 t/t4031-diff-rewrite-binary.sh                |   1 +
 t/t4056-diff-order.sh                         |   1 +
 t/t4204-patch-id.sh                           |   1 +
 t/t5310-pack-bitmaps.sh                       |   1 +
 t/t5326-multi-pack-bitmaps.sh                 |   2 +
 t/t5329-pack-objects-cruft.sh                 |   2 +
 t/t6004-rev-list-path-optim.sh                |   1 +
 t/t6019-rev-list-ancestry-path.sh             |   1 +
 t/t6111-rev-list-treesame.sh                  |   1 +
 t/t7061-wtstatus-ignore.sh                    |   1 +
 t/t7406-submodule-update.sh                   |   1 +
 t/t7407-submodule-foreach.sh                  |   1 +
 t/t7408-submodule-reference.sh                |   2 +
 t/t7411-submodule-config.sh                   |   1 +
 t/t7420-submodule-set-url.sh                  |   1 +
 t/t7521-ignored-mode.sh                       |   1 +
 t/t7524-commit-summary.sh                     |   2 +
 t/t7601-merge-pull-config.sh                  |   1 +
 t/t7700-repack.sh                             |   1 +
 t/t7800-difftool.sh                           |   1 +
 t/t7814-grep-recurse-submodules.sh            |   1 +
 t/t7900-maintenance.sh                        |   1 +
 trace2/tr2_tls.c                              |  10 +-
 50 files changed, 279 insertions(+), 124 deletions(-)

-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 01/23] builtin/help: fix dangling reference to `html_path`

[Patrick Steinhardt]

In `get_html_page_path()` we may end up assigning the return value of
`system_path()` to the global `html_path` variable. But as we also
assign the returned value to `to_free`, we will deallocate its memory
upon returning from the function. Consequently, `html_path` will now
point to deallocated memory.

Fix this issue by instead assigning the value to a separate local
variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/help.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b986..282ea5721fa 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -513,23 +513,24 @@ static void show_info_page(const char *page)
 static void get_html_page_path(struct strbuf *page_path, const char *page)
 {
 	struct stat st;
+	const char *path = html_path;
 	char *to_free = NULL;
 
-	if (!html_path)
-		html_path = to_free = system_path(GIT_HTML_PATH);
+	if (!path)
+		path = to_free = system_path(GIT_HTML_PATH);
 
 	/*
 	 * Check that the page we're looking for exists.
 	 */
-	if (!strstr(html_path, "://")) {
-		if (stat(mkpath("%s/%s.html", html_path, page), &st)
+	if (!strstr(path, "://")) {
+		if (stat(mkpath("%s/%s.html", path, page), &st)
 		    || !S_ISREG(st.st_mode))
 			die("'%s/%s.html': documentation file not found.",
-				html_path, page);
+				path, page);
 	}
 
 	strbuf_init(page_path, 0);
-	strbuf_addf(page_path, "%s/%s.html", html_path, page);
+	strbuf_addf(page_path, "%s/%s.html", path, page);
 	free(to_free);
 }
 
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 01/23] builtin/help: fix dangling reference to `html_path`

[Justin Tobler]

On 24/09/16 01:45PM, Patrick Steinhardt wrote:
> In `get_html_page_path()` we may end up assigning the return value of
> `system_path()` to the global `html_path` variable. But as we also
> assign the returned value to `to_free`, we will deallocate its memory
> upon returning from the function. Consequently, `html_path` will now
> point to deallocated memory.
> 
> Fix this issue by instead assigning the value to a separate local
> variable.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/help.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b986..282ea5721fa 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -513,23 +513,24 @@ static void show_info_page(const char *page)
>  static void get_html_page_path(struct strbuf *page_path, const char *page)
>  {
>  	struct stat st;
> +	const char *path = html_path;
>  	char *to_free = NULL;
>  
> -	if (!html_path)
> -		html_path = to_free = system_path(GIT_HTML_PATH);
> +	if (!path)
> +		path = to_free = system_path(GIT_HTML_PATH);

Previously, `html_path` was being assigned the return value of
`system_path()` and consequently escaping this scope even though the
value was being freed. There is no reason to modify the global value to
begin with so we instead assign the value to a local variable. Makes
sense.

>  
>  	/*
>  	 * Check that the page we're looking for exists.
>  	 */
> -	if (!strstr(html_path, "://")) {
> -		if (stat(mkpath("%s/%s.html", html_path, page), &st)
> +	if (!strstr(path, "://")) {
> +		if (stat(mkpath("%s/%s.html", path, page), &st)
>  		    || !S_ISREG(st.st_mode))
>  			die("'%s/%s.html': documentation file not found.",
> -				html_path, page);
> +				path, page);
>  	}
>  
>  	strbuf_init(page_path, 0);
> -	strbuf_addf(page_path, "%s/%s.html", html_path, page);
> +	strbuf_addf(page_path, "%s/%s.html", path, page);
>  	free(to_free);
>  }
>  
> -- 
> 2.46.0.551.gc5ee8f2d1c.dirty
> 
> 

[PATCH 02/23] builtin/help: fix leaking `html_path` when reading config multiple times

[Patrick Steinhardt]

The `html_path` variable gets populated via `git_help_config()`, which
puts an allocated string into it if its value has been configured. We do
not clear the old value though, which causes a memory leak in case the
config exists multiple times.

Plug this leak. The leak is exposed by t0012, but plugging it alone is
not sufficient to make the test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/help.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 282ea5721fa..2c249cbca4a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -52,7 +52,7 @@ static enum help_action {
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
 } cmd_mode;
 
-static const char *html_path;
+static char *html_path;
 static int verbose = 1;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,
 	if (!strcmp(var, "help.htmlpath")) {
 		if (!value)
 			return config_error_nonbool(var);
+		free(html_path);
 		html_path = xstrdup(value);
 		return 0;
 	}
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 03/23] git: fix leaking argv when handling builtins

[Patrick Steinhardt]

In `handle_builtin()` we may end up creating an ad-hoc argv array in
case we see that the command line contains the "--help" parameter. In
this case we observe two memory leaks though:

  - We leak the `struct strvec` itself because we directly exit after
    calling `run_builtin()`, without bothering about any cleanups.

  - Even if we free'd that vector we'd end up leaking some of its
    strings because `run_builtin()` will modify the array.

Plug both of these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 git.c           | 22 +++++++++++++++++++---
 t/t0012-help.sh |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 9a618a2740f..3c7fabfda26 100644
--- a/git.c
+++ b/git.c
@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)
 static void handle_builtin(int argc, const char **argv)
 {
 	struct strvec args = STRVEC_INIT;
+	const char **argv_copy = NULL;
 	const char *cmd;
 	struct cmd_struct *builtin;
 
@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv)
 		}
 
 		argc++;
-		argv = args.v;
+
+		/*
+		 * `run_builtin()` will modify the argv array, so we need to
+		 * create a shallow copy such that we can free all of its
+		 * strings.
+		 */
+		CALLOC_ARRAY(argv_copy, argc + 1);
+		COPY_ARRAY(argv_copy, args.v, argc);
+
+		argv = argv_copy;
 	}
 
 	builtin = get_builtin(cmd);
-	if (builtin)
-		exit(run_builtin(builtin, argc, argv));
+	if (builtin) {
+		int ret = run_builtin(builtin, argc, argv);
+		strvec_clear(&args);
+		free(argv_copy);
+		exit(ret);
+	}
+
 	strvec_clear(&args);
+	free(argv_copy);
 }
 
 static void execv_dashed_external(const char **argv)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c21..9eae0d83563 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -2,6 +2,7 @@
 
 test_description='help'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 configure_help () {
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 04/23] submodule: fix leaking update strategy

[Patrick Steinhardt]

We're not freeing the submodule update strategy command. Provide a
helper function that does this for us and call it in
`update_data_release()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c | 1 +
 submodule-config.c          | 2 +-
 submodule.c                 | 5 +++++
 submodule.h                 | 6 ++++--
 t/t7406-submodule-update.sh | 1 +
 5 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bf8f9a40128..ed05dc51347 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2021,6 +2021,7 @@ struct update_data {
 static void update_data_release(struct update_data *ud)
 {
 	free(ud->displaypath);
+	submodule_update_strategy_release(&ud->update_strategy);
 	module_list_release(&ud->list);
 }
 
diff --git a/submodule-config.c b/submodule-config.c
index c8f2bb2bdd3..471637a725a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)
 	free((void *) entry->config->branch);
 	free((void *) entry->config->url);
 	free((void *) entry->config->ignore);
-	free((void *) entry->config->update_strategy.command);
+	submodule_update_strategy_release(&entry->config->update_strategy);
 	free(entry->config);
 }
 
diff --git a/submodule.c b/submodule.c
index 97d0d47b561..0e67984d770 100644
--- a/submodule.c
+++ b/submodule.c
@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,
 	return 0;
 }
 
+void submodule_update_strategy_release(struct submodule_update_strategy *strategy)
+{
+	free((char *) strategy->command);
+}
+
 const char *submodule_update_type_to_string(enum submodule_update_type type)
 {
 	switch (type) {
diff --git a/submodule.h b/submodule.h
index b50d29eba4f..4deb1b5f84e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,6 +41,10 @@ struct submodule_update_strategy {
 	.type = SM_UPDATE_UNSPECIFIED, \
 }
 
+int parse_submodule_update_strategy(const char *value,
+				    struct submodule_update_strategy *dst);
+void submodule_update_strategy_release(struct submodule_update_strategy *strategy);
+
 int is_gitmodules_unmerged(struct index_state *istate);
 int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,
 void die_path_inside_submodule(struct index_state *istate,
 			       const struct pathspec *ps);
 enum submodule_update_type parse_submodule_update_type(const char *value);
-int parse_submodule_update_strategy(const char *value,
-				    struct submodule_update_strategy *dst);
 const char *submodule_update_type_to_string(enum submodule_update_type type);
 void handle_ignore_submodules_arg(struct diff_options *, const char *);
 void show_submodule_diff_summary(struct diff_options *o, const char *path,
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 297c6c3b5cc..0f0c86f9cb2 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -12,6 +12,7 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 05/23] builtin/submodule--helper: clear child process when not running it

[Patrick Steinhardt]

In `runcommand_in_submodule_cb()` we may end up not executing the child
command when `argv` is empty. But we still populate the command with
environment variables and other things, which needs cleanup. This leads
to a memory leak because we do not call `finish_command()`.

Fix this by clearing the child process when we don't execute it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c  | 10 +++++++---
 t/t7407-submodule-foreach.sh |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ed05dc51347..fd1b6794083 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	if (!info->quiet)
 		printf(_("Entering '%s'\n"), displaypath);
 
-	if (info->argv[0] && run_command(&cp))
-		die(_("run_command returned non-zero status for %s\n."),
-			displaypath);
+	if (info->argv[0]) {
+		if (run_command(&cp))
+			die(_("run_command returned non-zero status for %s\n."),
+			    displaypath);
+	} else {
+		child_process_clear(&cp);
+	}
 
 	if (info->recursive) {
 		struct child_process cpr = CHILD_PROCESS_INIT;
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 8d7b234beb8..9f688932613 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -12,6 +12,7 @@ that are currently checked out.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 06/23] builtin/submodule--helper: fix leaking error buffer

[Patrick Steinhardt]

Fix leaking error buffer when `compute_alternate_path()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c    | 2 ++
 t/t7408-submodule-reference.sh | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fd1b6794083..ff1376f69fc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject(
 				; /* nothing */
 			}
 		}
+
+		strbuf_release(&err);
 		strbuf_release(&sb);
 	}
 
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index d6040e0a337..7e1afa9ce47 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -4,6 +4,8 @@
 #
 
 test_description='test clone --reference'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 base_dir=$(pwd)
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 07/23] t/helper: fix leaking subrepo in nested submodule config helper

[Patrick Steinhardt]

In the "submodule-nested-repo-config" helper we create a submodule
repository and print its configuration. We do not clear the repo,
causing a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-submodule-nested-repo-config.c | 2 +-
 t/t7411-submodule-config.sh                  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
index 6ca069ce633..6dce9571531 100644
--- a/t/helper/test-submodule-nested-repo-config.c
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)
 	print_config_from_gitmodules(&subrepo, argv[2]);
 
 	submodule_free(the_repository);
-
+	repo_clear(&subrepo);
 	return 0;
 }
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 31271f8e0a6..af0de496e07 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -10,6 +10,7 @@ from the database and from the worktree works.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Patrick Steinhardt]

When `update_submodule()` fails we return with `die_message()`.
Curiously enough, this causes a memory leak because we use the
`run_process_parallel()` interfaces here, which swap out the die
routine.

Fix the leak by freeing the remote ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c  | 13 +++++++++----
 t/t7420-submodule-set-url.sh |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ff1376f69fc..a9bd93a7856 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)
 
 		if (!update_data->nofetch) {
 			if (fetch_in_submodule(update_data->sm_path, update_data->depth,
-					      0, NULL))
+					      0, NULL)) {
+				free(remote_ref);
 				return die_message(_("Unable to fetch in submodule path '%s'"),
 						   update_data->sm_path);
+			}
 		}
 
 		if (repo_resolve_gitlink_ref(the_repository, update_data->sm_path,
-					     remote_ref, &update_data->oid))
-			return die_message(_("Unable to find %s revision in submodule path '%s'"),
-					   remote_ref, update_data->sm_path);
+					     remote_ref, &update_data->oid)) {
+			ret = die_message(_("Unable to find %s revision in submodule path '%s'"),
+					  remote_ref, update_data->sm_path);
+			free(remote_ref);
+			return ret;
+		}
 
 		free(remote_ref);
 	}
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index bf7f15ee797..d7fe910bbe1 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -10,6 +10,7 @@ as expected.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Justin Tobler]

On 24/09/16 01:45PM, Patrick Steinhardt wrote:
> When `update_submodule()` fails we return with `die_message()`.
> Curiously enough, this causes a memory leak because we use the
> `run_process_parallel()` interfaces here, which swap out the die
> routine.

Naive question, is `update_submodule()` itself being run in parallel
here? Is that why the die routine gets swapped out so a child process
dying is handled differently? Also is it correct to say leaks are not
considered when we "die" normally? 

> Fix the leak by freeing the remote ref.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Patrick Steinhardt]

On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:
> On 24/09/16 01:45PM, Patrick Steinhardt wrote:
> > When `update_submodule()` fails we return with `die_message()`.
> > Curiously enough, this causes a memory leak because we use the
> > `run_process_parallel()` interfaces here, which swap out the die
> > routine.
> 
> Naive question, is `update_submodule()` itself being run in parallel
> here? Is that why the die routine gets swapped out so a child process
> dying is handled differently? Also is it correct to say leaks are not
> considered when we "die" normally? 

Hm. Revisiting this patch: my analysis was wrong. It's not the parallel
subsystem that swaps out `die()`, but it's the fact that we call
`die_message()`, which actually doesn't die. It really only prints the
message you would see when we call `die()`, nothing more.

I'll amend the commit message and send out the amended version once
there is more feedback to address.

Patrick

Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:
>> On 24/09/16 01:45PM, Patrick Steinhardt wrote:
>> > When `update_submodule()` fails we return with `die_message()`.
>> > Curiously enough, this causes a memory leak because we use the
>> > `run_process_parallel()` interfaces here, which swap out the die
>> > routine.
>> 
>> Naive question, is `update_submodule()` itself being run in parallel
>> here? Is that why the die routine gets swapped out so a child process
>> dying is handled differently? Also is it correct to say leaks are not
>> considered when we "die" normally? 
>
> Hm. Revisiting this patch: my analysis was wrong. It's not the parallel
> subsystem that swaps out `die()`, but it's the fact that we call
> `die_message()`, which actually doesn't die. It really only prints the
> message you would see when we call `die()`, nothing more.
>
> I'll amend the commit message and send out the amended version once
> there is more feedback to address.

So it has been a week and half since the series was posted and it
seems that this is the only thing you might want to touch up.

What's next?  Just have an updated patch [08/23] and nothing else
and be done with it?  A v2 round of 23-patch series hopefully will
see somebody other than Justin and I lend an extra set of eyes to
double check before we merge it to 'next'?

Thanks.

Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Patrick Steinhardt]

On Wed, Sep 25, 2024 at 01:26:17PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:
> >> On 24/09/16 01:45PM, Patrick Steinhardt wrote:
> >> > When `update_submodule()` fails we return with `die_message()`.
> >> > Curiously enough, this causes a memory leak because we use the
> >> > `run_process_parallel()` interfaces here, which swap out the die
> >> > routine.
> >> 
> >> Naive question, is `update_submodule()` itself being run in parallel
> >> here? Is that why the die routine gets swapped out so a child process
> >> dying is handled differently? Also is it correct to say leaks are not
> >> considered when we "die" normally? 
> >
> > Hm. Revisiting this patch: my analysis was wrong. It's not the parallel
> > subsystem that swaps out `die()`, but it's the fact that we call
> > `die_message()`, which actually doesn't die. It really only prints the
> > message you would see when we call `die()`, nothing more.
> >
> > I'll amend the commit message and send out the amended version once
> > there is more feedback to address.
> 
> So it has been a week and half since the series was posted and it
> seems that this is the only thing you might want to touch up.
> 
> What's next?  Just have an updated patch [08/23] and nothing else
> and be done with it?  A v2 round of 23-patch series hopefully will
> see somebody other than Justin and I lend an extra set of eyes to
> double check before we merge it to 'next'?

Makes sense, let's do it this way! I've sent a v2 a couple minutes ago.

Patrick

[PATCH 09/23] dir: fix off by one errors for ignored and untracked entries

[Patrick Steinhardt]

In `treat_directory()` we perform some logic to handle ignored and
untracked entries. When populating a directory with entries we first
save the current number of ignored/untracked entries and then populate
new entries at the end of our arrays that keep track of those entries.
When we figure out that all entries have been ignored/are untracked we
then remove this tail of entries from those vectors again. But there is
an off by one error in both paths that causes us to not free the first
ignored and untracked entries, respectively.

Fix these off-by-one errors to plug the resulting leak. While at it,
massage the code a bit to match our modern code style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 dir.c                                              | 6 ++----
 t/t3011-common-prefixes-and-directory-traversal.sh | 1 +
 t/t7061-wtstatus-ignore.sh                         | 1 +
 t/t7521-ignored-mode.sh                            | 1 +
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 5a23376bdae..787bcb7a1a4 100644
--- a/dir.c
+++ b/dir.c
@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 			 */
 			state = path_none;
 		} else {
-			int i;
-			for (i = old_ignored_nr + 1; i<dir->ignored_nr; ++i)
+			for (int i = old_ignored_nr; i < dir->ignored_nr; i++)
 				FREE_AND_NULL(dir->ignored[i]);
 			dir->ignored_nr = old_ignored_nr;
 		}
@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 	 */
 	if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
 	    !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
-		int i;
-		for (i = old_untracked_nr + 1; i<dir->nr; ++i)
+		for (int i = old_untracked_nr; i < dir->nr; i++)
 			FREE_AND_NULL(dir->entries[i]);
 		dir->nr = old_untracked_nr;
 	}
diff --git a/t/t3011-common-prefixes-and-directory-traversal.sh b/t/t3011-common-prefixes-and-directory-traversal.sh
index 3da5b2b6e79..69e44c387fa 100755
--- a/t/t3011-common-prefixes-and-directory-traversal.sh
+++ b/t/t3011-common-prefixes-and-directory-traversal.sh
@@ -2,6 +2,7 @@
 
 test_description='directory traversal handling, especially with common prefixes'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 2f9bea9793c..64145a05b1f 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -2,6 +2,7 @@
 
 test_description='git-status ignored files'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expected <<\EOF
diff --git a/t/t7521-ignored-mode.sh b/t/t7521-ignored-mode.sh
index a88b02b06ed..edce10f998e 100755
--- a/t/t7521-ignored-mode.sh
+++ b/t/t7521-ignored-mode.sh
@@ -2,6 +2,7 @@
 
 test_description='git status ignored modes'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup initial commit and ignore file' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 09/23] dir: fix off by one errors for ignored and untracked entries

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In `treat_directory()` we perform some logic to handle ignored and
> untracked entries. When populating a directory with entries we first
> save the current number of ignored/untracked entries and then populate
> new entries at the end of our arrays that keep track of those entries.
> When we figure out that all entries have been ignored/are untracked we
> then remove this tail of entries from those vectors again. But there is
> an off by one error in both paths that causes us to not free the first
> ignored and untracked entries, respectively.
>
> Fix these off-by-one errors to plug the resulting leak. While at it,
> massage the code a bit to match our modern code style.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  dir.c                                              | 6 ++----
>  t/t3011-common-prefixes-and-directory-traversal.sh | 1 +
>  t/t7061-wtstatus-ignore.sh                         | 1 +
>  t/t7521-ignored-mode.sh                            | 1 +
>  4 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 5a23376bdae..787bcb7a1a4 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>  			 */
>  			state = path_none;
>  		} else {
> -			int i;
> -			for (i = old_ignored_nr + 1; i<dir->ignored_nr; ++i)
> +			for (int i = old_ignored_nr; i < dir->ignored_nr; i++)
>  				FREE_AND_NULL(dir->ignored[i]);
>  			dir->ignored_nr = old_ignored_nr;

So the only effect this bug had was that the entry in the array at
the old_ignored_nr did not get freed, even though the code correctly
updated that the length of the dir->ignored[] after truncation.  I
speculated, before looking at the code change but only from what was
in the proposed log message, that the resulting array ended up to
have an extra entry, or to lose an entry, due to miscounting, but
that is not what is going on.  If the bug's nature was that we
screwed up the length of the updated array, somebody would certainly
have noticed a bug in the end-user observable behaviour that a path
that should be ignored is not getting ignored or the other way
around.  Interesting.

I would imagine this is something hard to spot with code inspection
alone, yet fairly easy for the leak checker to notice.  I am happy
to see that it does really help to have good code coverage in the
test suite.

Thanks.

[PATCH 10/23] builtin/pull: fix leaking "ff" option

[Patrick Steinhardt]

The `opt_ff` field gets populated either via `OPT_PASSTHRU` via
`config_get_ff()` or when `--rebase` is passed. So we sometimes end up
overriding the value in `opt_ff` with another value, but we do not free
the old value, causing a memory leak.

Adapt the type of the variable to be `char *` and consistently assign
allocated strings to it such that we can easily free it when it is being
overridden.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pull.c               | 11 +++++++----
 t/t7601-merge-pull-config.sh |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 4c54d8196fa..5d9d9e467e5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -84,7 +84,7 @@ static const char *opt_squash;
 static const char *opt_commit;
 static const char *opt_edit;
 static const char *cleanup_arg;
-static const char *opt_ff;
+static char *opt_ff;
 static const char *opt_verify_signatures;
 static const char *opt_verify;
 static int opt_autostash = -1;
@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		 * "--rebase" can override a config setting of
 		 * pull.ff=only.
 		 */
-		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
-			opt_ff = "--ff";
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) {
+			free(opt_ff);
+			opt_ff = xstrdup("--ff");
+		}
 	}
 
 	if (opt_rebase < 0)
@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
+			free(opt_ff);
+			opt_ff = xstrdup("--ff-only");
 			ret = run_merge();
 		} else {
 			ret = run_rebase(&newbase, &upstream);
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index a94387a75f2..7fd8c086af3 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing pull.* configuration parsing and other things.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 10/23] builtin/pull: fix leaking "ff" option

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The `opt_ff` field gets populated either via `OPT_PASSTHRU` via
> `config_get_ff()` or when `--rebase` is passed. So we sometimes end up
> overriding the value in `opt_ff` with another value, but we do not free
> the old value, causing a memory leak.

Good eyes.

I have to wonder if it will come back and bite us again that
OPT_PASSTHRU does not pass through but creates a new copy, while
OPT_STRING borrows from the argv[].  I guess that we could check in
parse_options_check() if the address of the same variable is passed
to both OPT_PASSTHRU (giving it a piece of memory we are responsible
for freeing) and OPT_STRING (giving it a piece of memory borrowed
from argv[], and would trigger a failure if given to free()).

In an extremely longer run, I wonder if it becomes necessary for us
to also make OPT_STRING to make a copy (which means strings that
come from the configuration and from parsing the command line
arguments are all allocated ones that we are responsible to free).

Thanks.

Re: [PATCH 10/23] builtin/pull: fix leaking "ff" option

[Patrick Steinhardt]

On Fri, Sep 20, 2024 at 10:00:04AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `opt_ff` field gets populated either via `OPT_PASSTHRU` via
> > `config_get_ff()` or when `--rebase` is passed. So we sometimes end up
> > overriding the value in `opt_ff` with another value, but we do not free
> > the old value, causing a memory leak.
> 
> Good eyes.
> 
> I have to wonder if it will come back and bite us again that
> OPT_PASSTHRU does not pass through but creates a new copy, while
> OPT_STRING borrows from the argv[].  I guess that we could check in
> parse_options_check() if the address of the same variable is passed
> to both OPT_PASSTHRU (giving it a piece of memory we are responsible
> for freeing) and OPT_STRING (giving it a piece of memory borrowed
> from argv[], and would trigger a failure if given to free()).
> 
> In an extremely longer run, I wonder if it becomes necessary for us
> to also make OPT_STRING to make a copy (which means strings that
> come from the configuration and from parsing the command line
> arguments are all allocated ones that we are responsible to free).

For now it seems like we are fine with `OPT_STRING` not copying the
value, and I haven't yet found a situation where we cannot work around
the alloc/don't alloc split. But it certainly does lead to an awkward
calling convention in many places that makes the resulting code way
harder to read than necessary if it did allocate.

I also doubt that avoiding an allocation for these gives us a noticeable
speedup. After all we're bounded by the number of arguments passed by
the user, so it's not like we would end up dupliciating thousands of
strings in the general case.

So yes, I think that it would simplify things quite a bit when all three
of them (OPT_PASSTHRU, OPT_STRING, config parsing) were duplicating the
values.

Patrick

[PATCH 11/23] diff: fix leaking orderfile option

[Patrick Steinhardt]

The `orderfile` diff option is being assigned via `OPT_FILENAME()`,
which assigns an allocated string to the variable. We never free it
though, causing a memory leak.

Change the type of the string to `char *` and free it to plug the leak.
This also requires us to use `xstrdup()` to assign the global config to
it in case it is set.

This leak is being hit in t7621, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 combine-diff.c | 3 +--
 diff.c         | 7 +++++--
 diff.h         | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 829a44e4167..f6b624dc288 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,
 {
 	struct combine_diff_path *paths = NULL;
 	int i, num_parent = parents->nr;
-
 	int output_format = opt->output_format;
-	const char *orderfile = opt->orderfile;
+	char *orderfile = opt->orderfile;
 
 	opt->output_format = DIFF_FORMAT_NO_OUTPUT;
 	/* tell diff_tree to emit paths in sorted (=tree) order */
diff --git a/diff.c b/diff.c
index 472479eb101..6555b8a32c1 100644
--- a/diff.c
+++ b/diff.c
@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value,
 	}
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
-	if (!strcmp(var, "diff.orderfile"))
+	if (!strcmp(var, "diff.orderfile")) {
+		FREE_AND_NULL(diff_order_file_cfg);
 		return git_config_pathname(&diff_order_file_cfg, var, value);
+	}
 
 	if (!strcmp(var, "diff.ignoresubmodules")) {
 		if (!value)
@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	if (diff_indent_heuristic)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
-	options->orderfile = diff_order_file_cfg;
+	options->orderfile = xstrdup_or_null(diff_order_file_cfg);
 
 	if (!options->flags.ignore_submodule_set)
 		options->flags.ignore_untracked_in_submodules = 1;
@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)
 		FREE_AND_NULL(options->objfind);
 	}
 
+	FREE_AND_NULL(options->orderfile);
 	for (size_t i = 0; i < options->anchors_nr; i++)
 		free(options->anchors[i]);
 	FREE_AND_NULL(options->anchors);
diff --git a/diff.h b/diff.h
index 9901c8ca8c8..b95d3c1e830 100644
--- a/diff.h
+++ b/diff.h
@@ -235,7 +235,7 @@ enum diff_submodule_format {
  * diffcore library with.
  */
 struct diff_options {
-	const char *orderfile;
+	char *orderfile;
 
 	/*
 	 * "--rotate-to=<file>" would start showing at <file> and when
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 12/23] parse-options: free previous value of `OPTION_FILENAME`

[Patrick Steinhardt]

The `OPTION_FILENAME` option always assigns either an allocated string
or `NULL` to the value. In case it is passed multiple times it does not
know to free the previous value though, which causes a memory leak.

Refactor the function to always free the previous value. None of the
sites where this option is used pass a string constant, so this change
is safe.

While at it, fix the argument of `fix_filename()` to be a string
constant. The only reason why it's not is because we use it as an
in-out-parameter, where the input is a constant and the output is not.
This is weird and unnecessary, as we can just return the result instead
of using the parameter for this.

This leak is being hit in t7621, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 30b9e68f8ac..33bfba0ed4a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,
 	return 0;
 }
 
-static void fix_filename(const char *prefix, char **file)
+static char *fix_filename(const char *prefix, const char *file)
 {
 	if (!file || !*file)
-		; /* leave as NULL */
+		return NULL;
 	else
-		*file = prefix_filename_except_for_dash(prefix, *file);
+		return prefix_filename_except_for_dash(prefix, file);
 }
 
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_FILENAME:
+	{
+		const char *value;
+
+		FREE_AND_NULL(*(char **)opt->value);
+
 		err = 0;
+
 		if (unset)
-			*(const char **)opt->value = NULL;
+			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			*(const char **)opt->value = (const char *)opt->defval;
+			value = (const char *) opt->defval;
 		else
-			err = get_arg(p, opt, flags, (const char **)opt->value);
+			err = get_arg(p, opt, flags, &value);
 
 		if (!err)
-			fix_filename(p->prefix, (char **)opt->value);
+			*(char **)opt->value = fix_filename(p->prefix, value);
 		return err;
-
+	}
 	case OPTION_CALLBACK:
 	{
 		const char *p_arg = NULL;
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 12/23] parse-options: free previous value of `OPTION_FILENAME`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The `OPTION_FILENAME` option always assigns either an allocated string
> or `NULL` to the value. In case it is passed multiple times it does not
> know to free the previous value though, which causes a memory leak.

Our earlier attitude was "the user gave --filename twice from the
command line, fully knowing that the last one wins rule will make
the earlier one have no effect; not worth worrying about".

But defining the memory ownership rules and make sure the
library-ish part of the codebase honors these rules is absolutely
the right thing to do.

We saw earlier that OPT_PASSTHRU correctly frees the original value
before assigning the new value, and this makes OPT_FILENAME do the
same thing.  Nice.

[PATCH 13/23] diffcore-order: fix leaking buffer when parsing orderfiles

[Patrick Steinhardt]

In `prepare_order()` we parse an orderfile and assign it to a global
array. In order to save on some allocations, we replace newlines with
NUL characters and then assign pointers into the allocated buffer to
that array. This can cause the buffer to be completely unreferenced
though in some cases, e.g. because the order file is empty or because we
had to use `xmemdupz()` to copy the lines instead of NUL-terminating
them.

Refactor the code to always `xmemdupz()` the strings. This is a bit
simpler, and it is rather unlikely that saving a handful of allocations
really matters. This allows us to release the string buffer and thus
plug the memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diffcore-order.c      | 19 +++++++------------
 t/t4056-diff-order.sh |  1 +
 t/t4204-patch-id.sh   |  1 +
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index e7d20ebd2d1..912513d3e67 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile)
 {
 	int cnt, pass;
 	struct strbuf sb = STRBUF_INIT;
-	void *map;
-	char *cp, *endp;
+	const char *cp, *endp;
 	ssize_t sz;
 
 	if (order)
@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile)
 	sz = strbuf_read_file(&sb, orderfile, 0);
 	if (sz < 0)
 		die_errno(_("failed to read orderfile '%s'"), orderfile);
-	map = strbuf_detach(&sb, NULL);
-	endp = (char *) map + sz;
+	endp = sb.buf + sz;
 
 	for (pass = 0; pass < 2; pass++) {
 		cnt = 0;
-		cp = map;
+		cp = sb.buf;
 		while (cp < endp) {
-			char *ep;
+			const char *ep;
 			for (ep = cp; ep < endp && *ep != '\n'; ep++)
 				;
 			/* cp to ep has one line */
@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)
 			else if (pass == 0)
 				cnt++;
 			else {
-				if (*ep == '\n') {
-					*ep = 0;
-					order[cnt] = cp;
-				} else {
-					order[cnt] = xmemdupz(cp, ep - cp);
-				}
+				order[cnt] = xmemdupz(cp, ep - cp);
 				cnt++;
 			}
 			if (ep < endp)
@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)
 			ALLOC_ARRAY(order, cnt);
 		}
 	}
+
+	strbuf_release(&sb);
 }
 
 static int match_order(const char *path)
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index aec1d9d1b42..32c5fcb9a27 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -5,6 +5,7 @@ test_description='diff order & rotate'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_files () {
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index dc8ddb10aff..c0a4a02dcfa 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,6 +5,7 @@ test_description='git patch-id'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 14/23] builtin/repack: fix leaking configuration

[Patrick Steinhardt]

When repacking, we assemble git-pack-objects(1) arguments both for the
"normal" pack and for the cruft pack. This configuration gets populated
with a bunch of `OPT_PASSTHRU` options that we end up passing to the
child process. These options are allocated, but never free'd.

Create a new `pack_objects_args_release()` function that releases the
memory for us and call it for both sets of options.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c              | 57 ++++++++++++++++++++++++++---------
 t/t5329-pack-objects-cruft.sh |  2 ++
 t/t7700-repack.sh             |  1 +
 3 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 3ee8cfa732f..c31d5653f1f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,
 		run_update_server_info = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "repack.cruftwindow"))
+	if (!strcmp(var, "repack.cruftwindow")) {
+		free(cruft_po_args->window);
 		return git_config_string(&cruft_po_args->window, var, value);
-	if (!strcmp(var, "repack.cruftwindowmemory"))
+	}
+	if (!strcmp(var, "repack.cruftwindowmemory")) {
+		free(cruft_po_args->window_memory);
 		return git_config_string(&cruft_po_args->window_memory, var, value);
-	if (!strcmp(var, "repack.cruftdepth"))
+	}
+	if (!strcmp(var, "repack.cruftdepth")) {
+		free(cruft_po_args->depth);
 		return git_config_string(&cruft_po_args->depth, var, value);
-	if (!strcmp(var, "repack.cruftthreads"))
+	}
+	if (!strcmp(var, "repack.cruftthreads")) {
+		free(cruft_po_args->threads);
 		return git_config_string(&cruft_po_args->threads, var, value);
+	}
 	return git_default_config(var, value, ctx, cb);
 }
 
+static void pack_objects_args_release(struct pack_objects_args *args)
+{
+	free(args->window);
+	free(args->window_memory);
+	free(args->depth);
+	free(args->threads);
+	list_objects_filter_release(&args->filter_options);
+}
+
 struct existing_packs {
 	struct string_list kept_packs;
 	struct string_list non_kept_packs;
@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	const char *unpack_unreachable = NULL;
 	int keep_unreachable = 0;
 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
-	struct pack_objects_args po_args = {NULL};
-	struct pack_objects_args cruft_po_args = {NULL};
+	struct pack_objects_args po_args = { 0 };
+	struct pack_objects_args cruft_po_args = { 0 };
 	int write_midx = 0;
 	const char *cruft_expiration = NULL;
 	const char *expire_to = NULL;
 	const char *filter_to = NULL;
+	const char *opt_window = NULL;
+	const char *opt_window_memory = NULL;
+	const char *opt_depth = NULL;
+	const char *opt_threads = NULL;
 
 	struct option builtin_repack_options[] = {
 		OPT_BIT('a', NULL, &pack_everything,
@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("with -A, do not loosen objects older than this")),
 		OPT_BOOL('k', "keep-unreachable", &keep_unreachable,
 				N_("with -a, repack unreachable objects")),
-		OPT_STRING(0, "window", &po_args.window, N_("n"),
+		OPT_STRING(0, "window", &opt_window, N_("n"),
 				N_("size of the window used for delta compression")),
-		OPT_STRING(0, "window-memory", &po_args.window_memory, N_("bytes"),
+		OPT_STRING(0, "window-memory", &opt_window_memory, N_("bytes"),
 				N_("same as the above, but limit memory size instead of entries count")),
-		OPT_STRING(0, "depth", &po_args.depth, N_("n"),
+		OPT_STRING(0, "depth", &opt_depth, N_("n"),
 				N_("limits the maximum delta depth")),
-		OPT_STRING(0, "threads", &po_args.threads, N_("n"),
+		OPT_STRING(0, "threads", &opt_threads, N_("n"),
 				N_("limits the maximum number of threads")),
 		OPT_MAGNITUDE(0, "max-pack-size", &po_args.max_pack_size,
 				N_("maximum size of each packfile")),
@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_repack_options,
 				git_repack_usage, 0);
 
+	po_args.window = xstrdup_or_null(opt_window);
+	po_args.window_memory = xstrdup_or_null(opt_window_memory);
+	po_args.depth = xstrdup_or_null(opt_depth);
+	po_args.threads = xstrdup_or_null(opt_threads);
+
 	if (delete_redundant && repository_format_precious_objects)
 		die(_("cannot delete packs in a precious-objects repo"));
 
@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		const char *pack_prefix = find_pack_prefix(packdir, packtmp);
 
 		if (!cruft_po_args.window)
-			cruft_po_args.window = po_args.window;
+			cruft_po_args.window = xstrdup_or_null(po_args.window);
 		if (!cruft_po_args.window_memory)
-			cruft_po_args.window_memory = po_args.window_memory;
+			cruft_po_args.window_memory = xstrdup_or_null(po_args.window_memory);
 		if (!cruft_po_args.depth)
-			cruft_po_args.depth = po_args.depth;
+			cruft_po_args.depth = xstrdup_or_null(po_args.depth);
 		if (!cruft_po_args.threads)
-			cruft_po_args.threads = po_args.threads;
+			cruft_po_args.threads = xstrdup_or_null(po_args.threads);
 		if (!cruft_po_args.max_pack_size)
 			cruft_po_args.max_pack_size = po_args.max_pack_size;
 
@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&names, 1);
 	existing_packs_release(&existing);
 	free_pack_geometry(&geometry);
-	list_objects_filter_release(&po_args.filter_options);
+	pack_objects_args_release(&po_args);
+	pack_objects_args_release(&cruft_po_args);
 
 	return ret;
 }
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index fc5fedbe9b0..445739d06c9 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='cruft pack related pack-objects tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 objdir=.git/objects
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index be1188e7365..c4c3d1a15d9 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "${TEST_DIRECTORY}/lib-bitmap.sh"
 . "${TEST_DIRECTORY}/lib-midx.sh"
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 14/23] builtin/repack: fix leaking configuration

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When repacking, we assemble git-pack-objects(1) arguments both for the
> "normal" pack and for the cruft pack. This configuration gets populated
> with a bunch of `OPT_PASSTHRU` options that we end up passing to the
> child process. These options are allocated, but never free'd.
>
> Create a new `pack_objects_args_release()` function that releases the
> memory for us and call it for both sets of options.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/repack.c              | 57 ++++++++++++++++++++++++++---------
>  t/t5329-pack-objects-cruft.sh |  2 ++
>  t/t7700-repack.sh             |  1 +
>  3 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 3ee8cfa732f..c31d5653f1f 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,
>  		run_update_server_info = git_config_bool(var, value);
>  		return 0;
>  	}
> -	if (!strcmp(var, "repack.cruftwindow"))
> +	if (!strcmp(var, "repack.cruftwindow")) {
> +		free(cruft_po_args->window);
>  		return git_config_string(&cruft_po_args->window, var, value);

Surely, as git_config_string() gives an allocated string, the
ownership rules should be that the receiving variable/struct member
is responsible for properly freeing it, which means that the current
value must be discarded before the new value is taken.  Looking
good.

I wonder if any of these targets (like the cruft_po_args->window we
see here) can be updated by parsing the command line arguments (with
OPT_STRING)?  It turns out that this worry is not unfounded but you
already covered it ...

>  		if (!cruft_po_args.window)
> -			cruft_po_args.window = po_args.window;
> +			cruft_po_args.window = xstrdup_or_null(po_args.window);

... here, so we are safe.

Looking good.

Thanks.

[PATCH 15/23] builtin/difftool: plug several trivial memory leaks

[Patrick Steinhardt]

There are several leaking data structures in git-difftool(1). Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/difftool.c  | 6 ++++++
 t/t7800-difftool.sh | 1 +
 2 files changed, 7 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index dcc68e190c2..1a68ab66993 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	if (fp)
 		fclose(fp);
 
+	hashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry);
+	hashmap_clear_and_free(&wt_modified, struct path_entry, entry);
+	hashmap_clear_and_free(&tmp_modified, struct path_entry, entry);
+	hashmap_clear_and_free(&submodules, struct pair_entry, entry);
+	hashmap_clear_and_free(&symlinks2, struct pair_entry, entry);
+	release_index(&wtindex);
 	free(lbase_dir);
 	free(rbase_dir);
 	strbuf_release(&info);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index cc917b257e3..f67b9345b8a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -11,6 +11,7 @@ Testing basic diff tool invocation
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 difftool_test_setup ()
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 16/23] trace2: destroy context stored in thread-local storage

[Patrick Steinhardt]

Each thread may have a specific context in the trace2 subsystem that we
set up via thread-local storage. We do not set up a destructor for this
data though, which means that the context data will leak.

Plug this leak by installing a destructor. This leak is exposed by
t7814, but plugging it alone does not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 trace2/tr2_tls.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 4f75392952b..7b023c1bfc6 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)
 	return us - tr2tls_us_start_process;
 }
 
+static void tr2tls_key_destructor(void *payload)
+{
+	struct tr2tls_thread_ctx *ctx = payload;
+	free((char *)ctx->thread_name);
+	free(ctx->array_us_start);
+	free(ctx);
+}
+
 void tr2tls_init(void)
 {
 	tr2tls_start_process_clock();
 
-	pthread_key_create(&tr2tls_key, NULL);
+	pthread_key_create(&tr2tls_key, tr2tls_key_destructor);
 	init_recursive_mutex(&tr2tls_mutex);
 
 	tr2tls_thread_main =
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 17/23] submodule: fix leaking submodule ODB paths

[Patrick Steinhardt]

In `add_submodule_odb_by_path()` we add a path into a global string
list. The list is initialized with `NODUP`, which means that we do not
pass ownership of strings to the list. But we use `xstrdup()` when we
insert a path, with the consequence that the string will never get
free'd.

Plug the leak by marking the list as `DUP`. There is only a single
callsite where we insert paths anyway, and as explained above that
callsite was mishandling the allocation.

This leak is exposed by t7814, but plugging it does not make the whole
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0e67984d770..a07debc227f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)
 		die(_("staging updated .gitmodules failed"));
 }
 
-static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;
+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_DUP;
 
 void add_submodule_odb_by_path(const char *path)
 {
-	string_list_insert(&added_submodule_odb_paths, xstrdup(path));
+	string_list_insert(&added_submodule_odb_paths, path);
 }
 
 int register_all_submodule_odb_as_alternates(void)
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 18/23] grep: fix leaking grep pattern

[Patrick Steinhardt]

When creating a pattern via `create_grep_pat()` we allocate the pattern
member of the structure regardless of the token type. But later, when we
try to free the structure, we free the pattern member conditionally on
the token type and thus leak memory.

Plug this leak. The leak is exposed by t7814, but plugging it alone does
not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index e5761426e4f..701e58de04e 100644
--- a/grep.c
+++ b/grep.c
@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)
 				free_pcre2_pattern(p);
 			else
 				regfree(&p->regexp);
-			free(p->pattern);
 			break;
 		default:
 			break;
 		}
+		free(p->pattern);
 		free(p);
 	}
 }
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 19/23] promisor-remote: fix leaking partial clone filter

[Patrick Steinhardt]

The partial clone filter of a promisor remote is never free'd, causing
memory leaks. Furthermore, in case multiple partial clone filters are
defined for the same remote, we'd overwrite previous values without
freeing them.

Fix these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 promisor-remote.c                  | 2 ++
 t/t7814-grep-recurse-submodules.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/promisor-remote.c b/promisor-remote.c
index 317e1b127fe..9345ae3db23 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,
 		if (!r)
 			return 0;
 
+		FREE_AND_NULL(r->partial_clone_filter);
 		return git_config_string(&r->partial_clone_filter, var, value);
 	}
 
@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config)
 {
 	while (config->promisors) {
 		struct promisor_remote *r = config->promisors;
+		free(r->partial_clone_filter);
 		config->promisors = config->promisors->next;
 		free(r);
 	}
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 167fe661504..55ed630e77e 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -7,6 +7,7 @@ submodules.
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 20/23] builtin/maintenance: fix leaking config string

[Patrick Steinhardt]

When parsing the maintenance strategy from config we allocate a config
string, but do not free it after parsing it. Plug this leak by instead
using `git_config_get_string_tmp()`, which does not allocate any memory.

This leak is exposed by t7900, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7dac9714054..3acfa367ad1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 
 static void initialize_maintenance_strategy(void)
 {
-	char *config_str;
+	const char *config_str;
 
-	if (git_config_get_string("maintenance.strategy", &config_str))
+	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
 		return;
 
 	if (!strcasecmp(config_str, "incremental")) {
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 20/23] builtin/maintenance: fix leaking config string

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When parsing the maintenance strategy from config we allocate a config
> string, but do not free it after parsing it. Plug this leak by instead
> using `git_config_get_string_tmp()`, which does not allocate any memory.
>
> This leak is exposed by t7900, but plugging it alone does not make the
> test suite pass.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/gc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 7dac9714054..3acfa367ad1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  
>  static void initialize_maintenance_strategy(void)
>  {
> -	char *config_str;
> +	const char *config_str;
>  
> -	if (git_config_get_string("maintenance.strategy", &config_str))
> +	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
>  		return;

I am not a huge fan of the name of this function, because many
callers use the result as a fairly long-lived string (the primary
reason why they call them is to avoid having to free() the result at
the end), but this new caller's use is very much in line with the
"tmp" in its name.  We peek into the configset to learn the value of
this string, and check its value against "incremental".

Looking good.

Thanks.

[PATCH 21/23] builtin/maintenance: fix leak in `get_schedule_cmd()`

[Patrick Steinhardt]

The `get_schedule_cmd()` function allows us to override the schedule
command with a specific test command such that we can verify the
underlying logic in a platform-independent way. Its memory management is
somewhat wild though, because it basically gives up and assigns an
allocated string to the string constant output pointer. While this part
is marked with `UNLEAK()` to mask this, we also leak the local string
lists.

Rework the function such that it has a separate out parameter. If set,
we will assign it the final allocated command. Plug the other memory
leaks and create a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 127 ++++++++++++++++++++++++++---------------
 t/t7900-maintenance.sh |   1 +
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3acfa367ad1..b68a0be62c1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)
  * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.
  *   In this case, the *cmd value is read as input.
  *
- *   * if the input value *cmd is the key of one of the comma-separated list
- *     item, then *is_available is set to true and *cmd is modified and becomes
+ *   * if the input value cmd is the key of one of the comma-separated list
+ *     item, then *is_available is set to true and *out is set to
  *     the mock command.
  *
  *   * if the input value *cmd isn’t the key of any of the comma-separated list
- *     item, then *is_available is set to false.
+ *     item, then *is_available is set to false and *out is set to the original
+ *     command.
  *
  * Ex.:
  *   GIT_TEST_MAINT_SCHEDULER not set
  *     +-------+-------------------------------------------------+
  *     | Input |                     Output                      |
- *     | *cmd  | return code |       *cmd        | *is_available |
+ *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
- *     | "foo" |    false    | "foo" (unchanged) |  (unchanged)  |
+ *     | "foo" |    false    | NULL              |  (unchanged)  |
  *     +-------+-------------+-------------------+---------------+
  *
  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
  *     +-------+-------------------------------------------------+
  *     | Input |                     Output                      |
- *     | *cmd  | return code |       *cmd        | *is_available |
+ *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
  *     | "foo" |    true     |  "./mock.foo.sh"  |     true      |
- *     | "qux" |    true     | "qux" (unchanged) |     false     |
+ *     | "qux" |    true     | "qux" (allocated) |     false     |
  *     +-------+-------------+-------------------+---------------+
  */
-static int get_schedule_cmd(const char **cmd, int *is_available)
+static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
 {
 	char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
 	struct string_list_item *item;
@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)
 		if (string_list_split_in_place(&pair, item->string, ":", 2) != 2)
 			continue;
 
-		if (!strcmp(*cmd, pair.items[0].string)) {
-			*cmd = pair.items[1].string;
+		if (!strcmp(cmd, pair.items[0].string)) {
+			if (out)
+				*out = xstrdup(pair.items[1].string);
 			if (is_available)
 				*is_available = 1;
-			string_list_clear(&list, 0);
-			UNLEAK(testing);
-			return 1;
+			string_list_clear(&pair, 0);
+			goto out;
 		}
+
+		string_list_clear(&pair, 0);
 	}
 
+	if (out)
+		*out = xstrdup(cmd);
+
+out:
 	string_list_clear(&list, 0);
 	free(testing);
 	return 1;
@@ -1850,9 +1857,8 @@ static int get_random_minute(void)
 
 static int is_launchctl_available(void)
 {
-	const char *cmd = "launchctl";
 	int is_available;
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("launchctl", &is_available, NULL))
 		return is_available;
 
 #ifdef __APPLE__
@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)
 
 static int launchctl_boot_plist(int enable, const char *filename)
 {
-	const char *cmd = "launchctl";
+	char *cmd;
 	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
 	char *uid = launchctl_get_uid();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("launchctl", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, enable ? "bootstrap" : "bootout", uid,
 		     filename, NULL);
@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)
 
 	result = finish_command(&child);
 
+	free(cmd);
 	free(uid);
 	return result;
 }
@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 	static unsigned long lock_file_timeout_ms = ULONG_MAX;
 	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
 	struct stat st;
-	const char *cmd = "launchctl";
+	char *cmd;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("launchctl", NULL, &cmd);
 	preamble = "<?xml version=\"1.0\"?>\n"
 		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
 		   "<plist version=\"1.0\">"
@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 
 	free(filename);
 	free(name);
+	free(cmd);
 	strbuf_release(&plist);
 	strbuf_release(&plist2);
 	return 0;
@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)
 
 static int is_schtasks_available(void)
 {
-	const char *cmd = "schtasks";
 	int is_available;
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("schtasks", &is_available, NULL))
 		return is_available;
 
 #ifdef GIT_WINDOWS_NATIVE
@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)
 
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
-	const char *cmd = "schtasks";
+	char *cmd;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
 	char *name = schtasks_task_name(frequency);
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("schtasks", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
 	free(name);
+	free(cmd);
 
 	return run_command(&child);
 }
@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)
 
 static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)
 {
-	const char *cmd = "schtasks";
+	char *cmd;
 	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *xml;
@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	struct strbuf tfilename = STRBUF_INIT;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("schtasks", NULL, &cmd);
 
 	strbuf_addf(&tfilename, "%s/schedule_%s_XXXXXX",
 		    get_git_common_dir(), frequency);
@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 
 	delete_tempfile(&tfile);
 	free(name);
+	free(cmd);
 	return result;
 }
 
@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)
 
 static int is_crontab_available(void)
 {
-	const char *cmd = "crontab";
+	char *cmd;
 	int is_available;
+	int ret;
 
-	if (get_schedule_cmd(&cmd, &is_available))
-		return is_available;
+	if (get_schedule_cmd("crontab", &is_available, &cmd)) {
+		ret = is_available;
+		goto out;
+	}
 
 #ifdef __APPLE__
 	/*
 	 * macOS has cron, but it requires special permissions and will
 	 * create a UI alert when attempting to run this command.
 	 */
-	return 0;
+	ret = 0;
 #else
-	return check_crontab_process(cmd);
+	ret = check_crontab_process(cmd);
 #endif
+
+out:
+	free(cmd);
+	return ret;
 }
 
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)
 
 static int crontab_update_schedule(int run_maintenance, int fd)
 {
-	const char *cmd = "crontab";
+	char *cmd;
 	int result = 0;
 	int in_old_region = 0;
 	struct child_process crontab_list = CHILD_PROCESS_INIT;
@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	struct tempfile *tmpedit = NULL;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("crontab", NULL, &cmd);
 	strvec_split(&crontab_list.args, cmd);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
 	crontab_list.out = dup(fd);
 	crontab_list.git_cmd = 0;
 
-	if (start_command(&crontab_list))
-		return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
+	if (start_command(&crontab_list)) {
+		result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
+		goto out;
+	}
 
 	/* Ignore exit code, as an empty crontab will return error. */
 	finish_command(&crontab_list);
@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 		result = error(_("'crontab' died"));
 	else
 		fclose(cron_list);
+
 out:
 	delete_tempfile(&tmpedit);
+	free(cmd);
 	return result;
 }
 
@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)
 
 static int is_systemd_timer_available(void)
 {
-	const char *cmd = "systemctl";
 	int is_available;
 
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("systemctl", &is_available, NULL))
 		return is_available;
 
 	return real_is_systemd_timer_available();
@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,
 				     enum schedule_priority schedule,
 				     int minute)
 {
-	const char *cmd = "systemctl";
+	char *cmd = NULL;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
+	int ret;
 
 	/*
 	 * Disabling the systemd unit while it is already disabled makes
@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,
 	 * On the other hand, enabling a systemd unit which is already enabled
 	 * produces no error.
 	 */
-	if (!enable)
+	if (!enable) {
 		child.no_stderr = 1;
-	else if (systemd_timer_write_timer_file(schedule, minute))
-		return -1;
+	} else if (systemd_timer_write_timer_file(schedule, minute)) {
+		ret = -1;
+		goto out;
+	}
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("systemctl", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
 		     "--now", NULL);
 	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
 
-	if (start_command(&child))
-		return error(_("failed to start systemctl"));
-	if (finish_command(&child))
+	if (start_command(&child)) {
+		ret = error(_("failed to start systemctl"));
+		goto out;
+	}
+
+	if (finish_command(&child)) {
 		/*
 		 * Disabling an already disabled systemd unit makes
 		 * systemctl fail.
@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,
 		 *
 		 * Enabling an enabled systemd unit doesn't fail.
 		 */
-		if (enable)
-			return error(_("failed to run systemctl"));
-	return 0;
+		if (enable) {
+			ret = error(_("failed to run systemctl"));
+			goto out;
+		}
+	}
+
+	ret = 0;
+
+out:
+	free(cmd);
+	return ret;
 }
 
 /*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index abae7a97546..c62c42848a2 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -2,6 +2,7 @@
 
 test_description='git maintenance builtin'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_COMMIT_GRAPH=0
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 22/23] revision: fix leaking parents when simplifying commits

[Patrick Steinhardt]

When simplifying commits, e.g. because they are treesame with their
parents, we unset the commit's parent pointers but never free them. Plug
the resulting memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c                        | 5 +++++
 t/t1414-reflog-walk.sh            | 1 +
 t/t5310-pack-bitmaps.sh           | 1 +
 t/t5326-multi-pack-bitmaps.sh     | 2 ++
 t/t6004-rev-list-path-optim.sh    | 1 +
 t/t6019-rev-list-ancestry-path.sh | 1 +
 t/t6111-rev-list-treesame.sh      | 1 +
 7 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 2d7ad2bddff..e79f39e5555 100644
--- a/revision.c
+++ b/revision.c
@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 					ts->treesame[nth_parent] = 1;
 				continue;
 			}
+
+			free_commit_list(parent->next);
 			parent->next = NULL;
+			while (commit->parents != parent)
+				pop_commit(&commit->parents);
 			commit->parents = parent;
 
 			/*
@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 					die("cannot simplify commit %s (invalid %s)",
 					    oid_to_hex(&commit->object.oid),
 					    oid_to_hex(&p->object.oid));
+				free_commit_list(p->parents);
 				p->parents = NULL;
 			}
 		/* fallthrough */
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index be6c3f472c1..49d28166da0 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up some reflog entries' '
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a6de7c57643..7044c7d7c6d 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -2,6 +2,7 @@
 
 test_description='exercise basic bitmap functionality'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
 
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 832b92619c0..6eaa692f33b 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='exercise basic multi-pack bitmap functionality'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "${TEST_DIRECTORY}/lib-bitmap.sh"
 
diff --git a/t/t6004-rev-list-path-optim.sh b/t/t6004-rev-list-path-optim.sh
index cd4f420e2a1..5416241edea 100755
--- a/t/t6004-rev-list-path-optim.sh
+++ b/t/t6004-rev-list-path-optim.sh
@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index 738da23628b..1aabab69568 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh
@@ -29,6 +29,7 @@ test_description='--ancestry-path'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_merge () {
diff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh
index 90ff1416400..f63bc8d3da6 100755
--- a/t/t6111-rev-list-treesame.sh
+++ b/t/t6111-rev-list-treesame.sh
@@ -16,6 +16,7 @@ test_description='TREESAME and limiting'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 note () {
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 22/23] revision: fix leaking parents when simplifying commits

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When simplifying commits, e.g. because they are treesame with their
> parents, we unset the commit's parent pointers but never free them. Plug
> the resulting memory leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  revision.c                        | 5 +++++
>  t/t1414-reflog-walk.sh            | 1 +
>  t/t5310-pack-bitmaps.sh           | 1 +
>  t/t5326-multi-pack-bitmaps.sh     | 2 ++
>  t/t6004-rev-list-path-optim.sh    | 1 +
>  t/t6019-rev-list-ancestry-path.sh | 1 +
>  t/t6111-rev-list-treesame.sh      | 1 +
>  7 files changed, 12 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 2d7ad2bddff..e79f39e5555 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  					ts->treesame[nth_parent] = 1;
>  				continue;
>  			}
> +
> +			free_commit_list(parent->next);
>  			parent->next = NULL;

We have been walking commit->parents linked list, "parent" is the
current parent we are looking at.  We are simplifying the history
and decided that later parents are not needed, hence we discard the
remaining commit_list entries starting from parents->next.  But we
didn't discard; we just updated parent->next pointer and made the
pointed data unreachable, leaking.  So we free_commit_list().

> +			while (commit->parents != parent)
> +				pop_commit(&commit->parents);
>  			commit->parents = parent;

And this is the other direction, discarding the parents before the
current one we are looking at.

Of course it makes me wonder if we can just free_commit_list() the
whole thing and then add the current parent commit (in "p", which
was taken with "p = parent->item" before) as the sole parent, with
something like

	free_commit_list(commit->parents);
        commit->parents = NULL;
        /* parent = */ commit_list_insert(p, &commit->parents);

to replace the 5-line (2 to discard later parents, 3 to discard
earlier ones) code, but I do not think it becomes particularly
easier to read, so let's drop that.

> @@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  					die("cannot simplify commit %s (invalid %s)",
>  					    oid_to_hex(&commit->object.oid),
>  					    oid_to_hex(&p->object.oid));
> +				free_commit_list(p->parents);
>  				p->parents = NULL;
>  			}
>  		/* fallthrough */

Looking good.

Thanks.

[PATCH 23/23] diffcore-break: fix leaking filespecs when merging broken pairs

[Patrick Steinhardt]

When merging file pairs after they have been broken up we queue a new
file pair and discard the broken-up ones. The newly-queued file pair
reuses one filespec of the broken up pairs each, where the respective
other filespec gets discarded. But we only end up freeing the filespec's
data, not the filespec itself, and thus leak memory.

Fix these leaks by using `free_filespec()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diffcore-break.c                  | 4 ++--
 t/t4008-diff-break-rewrite.sh     | 2 ++
 t/t4022-diff-rewrite.sh           | 1 +
 t/t4023-diff-rename-typechange.sh | 1 +
 t/t4031-diff-rewrite-binary.sh    | 1 +
 t/t7524-commit-summary.sh         | 2 ++
 6 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/diffcore-break.c b/diffcore-break.c
index 831b66b5c3e..02735f80c65 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,
 	 * in the resulting tree.
 	 */
 	d->one->rename_used++;
-	diff_free_filespec_data(d->two);
-	diff_free_filespec_data(c->one);
+	free_filespec(d->two);
+	free_filespec(c->one);
 	free(d);
 	free(c);
 }
diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh
index 562aaf3a2a2..b0ef0026e08 100755
--- a/t/t4008-diff-break-rewrite.sh
+++ b/t/t4008-diff-break-rewrite.sh
@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites.
 
 Further, with -B and -M together, these should turn into two renames.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh ;# test-lib chdir's into trash
 
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index 6fed993ea0b..77bc36d5d8f 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -2,6 +2,7 @@
 
 test_description='rewrite diff'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff-data.sh
 
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
index 787605ce3fd..e6f4fe441e1 100755
--- a/t/t4023-diff-rename-typechange.sh
+++ b/t/t4023-diff-rename-typechange.sh
@@ -2,6 +2,7 @@
 
 test_description='typechange rename detection'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
index c4394a27b56..1b8cd3e4c97 100755
--- a/t/t4031-diff-rewrite-binary.sh
+++ b/t/t4031-diff-rewrite-binary.sh
@@ -2,6 +2,7 @@
 
 test_description='rewrite diff on binary file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # We must be large enough to meet the MINIMUM_BREAK_SIZE
diff --git a/t/t7524-commit-summary.sh b/t/t7524-commit-summary.sh
index 47b2f1dc22a..a8fceb6a47c 100755
--- a/t/t7524-commit-summary.sh
+++ b/t/t7524-commit-summary.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git commit summary'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 00/23] Memory leak fixes (pt.7)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The patch series is built on top of ed155187b4 (Sync with Git 2.46.1,
> 2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix
> leaking keep-pack list, 2024-09-05) merged into it.

I haven't said this, but I really appreciate contributors (like you)
pacing their series submission perfectly with the rate of related
topics in flight solidifying.  If people hold off sending new and
dependent topics for too long, that would waste available review and
testing bandwidth, but if people send them while the other topics
that they depend on are not yet in a good shape, it would require
conflict resolutions that may get wasted when the depended topics
have to be updated.

I've been trying to be more explicit in the "What's cooking" report
what my evaluation of each topic's "doneness" is, to help contributors
who need to send new topics that may conflict with the topics that
are already cooking, and I hope it helped in your (and other
contributors') pacing their submissions to help the process run more
smoothly.

THanks.

Re: [PATCH 00/23] Memory leak fixes (pt.7)

[Patrick Steinhardt]

On Thu, Sep 19, 2024 at 11:54:38AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The patch series is built on top of ed155187b4 (Sync with Git 2.46.1,
> > 2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix
> > leaking keep-pack list, 2024-09-05) merged into it.
> 
> I haven't said this, but I really appreciate contributors (like you)
> pacing their series submission perfectly with the rate of related
> topics in flight solidifying.  If people hold off sending new and
> dependent topics for too long, that would waste available review and
> testing bandwidth, but if people send them while the other topics
> that they depend on are not yet in a good shape, it would require
> conflict resolutions that may get wasted when the depended topics
> have to be updated.
> 
> I've been trying to be more explicit in the "What's cooking" report
> what my evaluation of each topic's "doneness" is, to help contributors
> who need to send new topics that may conflict with the topics that
> are already cooking, and I hope it helped in your (and other
> contributors') pacing their submissions to help the process run more
> smoothly.

This series of patch series was something where I wasn't immediately
sure for how well-received it will be. In order to make progress and not
be busy with memory leak fixes for the next couple years I had to keep a
steady pace that wasn't too slow. But sending out a 20-patch series
every other week could feel overwhelming to some folks.

So I'm happy to hear that this is well-received overall. Thanks, and
also thanks for your reviews!

Patrick

[PATCH v2 00/23] Memory leak fixes (pt.7)

[Patrick Steinhardt]

Hi,

this is the second version of another round of memory leak fixes.
There's only a single change compared to v1, namely a revised commit
message.

Thanks!

Patrick

Patrick Steinhardt (23):
  builtin/help: fix dangling reference to `html_path`
  builtin/help: fix leaking `html_path` when reading config multiple
    times
  git: fix leaking argv when handling builtins
  submodule: fix leaking update strategy
  builtin/submodule--helper: clear child process when not running it
  builtin/submodule--helper: fix leaking error buffer
  t/helper: fix leaking subrepo in nested submodule config helper
  builtin/submodule--helper: fix leaking remote ref on errors
  dir: fix off by one errors for ignored and untracked entries
  builtin/pull: fix leaking "ff" option
  diff: fix leaking orderfile option
  parse-options: free previous value of `OPTION_FILENAME`
  diffcore-order: fix leaking buffer when parsing orderfiles
  builtin/repack: fix leaking configuration
  builtin/difftool: plug several trivial memory leaks
  trace2: destroy context stored in thread-local storage
  submodule: fix leaking submodule ODB paths
  grep: fix leaking grep pattern
  promisor-remote: fix leaking partial clone filter
  builtin/maintenance: fix leaking config string
  builtin/maintenance: fix leak in `get_schedule_cmd()`
  revision: fix leaking parents when simplifying commits
  diffcore-break: fix leaking filespecs when merging broken pairs

 builtin/difftool.c                            |   6 +
 builtin/gc.c                                  | 131 +++++++++++-------
 builtin/help.c                                |  16 ++-
 builtin/pull.c                                |  11 +-
 builtin/repack.c                              |  57 ++++++--
 builtin/submodule--helper.c                   |  26 +++-
 combine-diff.c                                |   3 +-
 diff.c                                        |   7 +-
 diff.h                                        |   2 +-
 diffcore-break.c                              |   4 +-
 diffcore-order.c                              |  19 +--
 dir.c                                         |   6 +-
 git.c                                         |  22 ++-
 grep.c                                        |   2 +-
 parse-options.c                               |  22 +--
 promisor-remote.c                             |   2 +
 revision.c                                    |   5 +
 submodule-config.c                            |   2 +-
 submodule.c                                   |   9 +-
 submodule.h                                   |   6 +-
 t/helper/test-submodule-nested-repo-config.c  |   2 +-
 t/t0012-help.sh                               |   1 +
 t/t1414-reflog-walk.sh                        |   1 +
 ...common-prefixes-and-directory-traversal.sh |   1 +
 t/t4008-diff-break-rewrite.sh                 |   2 +
 t/t4022-diff-rewrite.sh                       |   1 +
 t/t4023-diff-rename-typechange.sh             |   1 +
 t/t4031-diff-rewrite-binary.sh                |   1 +
 t/t4056-diff-order.sh                         |   1 +
 t/t4204-patch-id.sh                           |   1 +
 t/t5310-pack-bitmaps.sh                       |   1 +
 t/t5326-multi-pack-bitmaps.sh                 |   2 +
 t/t5329-pack-objects-cruft.sh                 |   2 +
 t/t6004-rev-list-path-optim.sh                |   1 +
 t/t6019-rev-list-ancestry-path.sh             |   1 +
 t/t6111-rev-list-treesame.sh                  |   1 +
 t/t7061-wtstatus-ignore.sh                    |   1 +
 t/t7406-submodule-update.sh                   |   1 +
 t/t7407-submodule-foreach.sh                  |   1 +
 t/t7408-submodule-reference.sh                |   2 +
 t/t7411-submodule-config.sh                   |   1 +
 t/t7420-submodule-set-url.sh                  |   1 +
 t/t7521-ignored-mode.sh                       |   1 +
 t/t7524-commit-summary.sh                     |   2 +
 t/t7601-merge-pull-config.sh                  |   1 +
 t/t7700-repack.sh                             |   1 +
 t/t7800-difftool.sh                           |   1 +
 t/t7814-grep-recurse-submodules.sh            |   1 +
 t/t7900-maintenance.sh                        |   1 +
 trace2/tr2_tls.c                              |  10 +-
 50 files changed, 279 insertions(+), 124 deletions(-)

Range-diff against v1:
 1:  e3bed973af =  1:  e3bed973af builtin/help: fix dangling reference to `html_path`
 2:  4a59fe15ae =  2:  4a59fe15ae builtin/help: fix leaking `html_path` when reading config multiple times
 3:  ea3dd851ad =  3:  ea3dd851ad git: fix leaking argv when handling builtins
 4:  7cdd2691b7 =  4:  7cdd2691b7 submodule: fix leaking update strategy
 5:  0d0964a2be =  5:  0d0964a2be builtin/submodule--helper: clear child process when not running it
 6:  52d12e034b =  6:  52d12e034b builtin/submodule--helper: fix leaking error buffer
 7:  96bd7f01d5 =  7:  96bd7f01d5 t/helper: fix leaking subrepo in nested submodule config helper
 8:  d088703d31 !  8:  d5e7a24aac builtin/submodule--helper: fix leaking remote ref on errors
    @@ Metadata
      ## Commit message ##
         builtin/submodule--helper: fix leaking remote ref on errors
     
    -    When `update_submodule()` fails we return with `die_message()`.
    -    Curiously enough, this causes a memory leak because we use the
    -    `run_process_parallel()` interfaces here, which swap out the die
    -    routine.
    +    When `update_submodule()` fails we return with `die_message()`, which
    +    only causes us to print the same message as `die()` would without
    +    actually causing the process to die. We don't free memory in that case
    +    and thus leak memory.
     
         Fix the leak by freeing the remote ref.
     
 9:  d5c9cccb82 =  9:  fca161d389 dir: fix off by one errors for ignored and untracked entries
10:  747c9a76a2 = 10:  2338b5e2a8 builtin/pull: fix leaking "ff" option
11:  85c0f9e5f5 = 11:  cb08db4d37 diff: fix leaking orderfile option
12:  330b6c52a0 = 12:  650b89bcca parse-options: free previous value of `OPTION_FILENAME`
13:  c975dfe462 = 13:  cd79422087 diffcore-order: fix leaking buffer when parsing orderfiles
14:  a5f3931eee = 14:  e015d1704b builtin/repack: fix leaking configuration
15:  c79a5118e4 = 15:  7bb07ec2f0 builtin/difftool: plug several trivial memory leaks
16:  0fb3dc55e5 = 16:  30928eb8f9 trace2: destroy context stored in thread-local storage
17:  f1cb8122d1 = 17:  35f5de5467 submodule: fix leaking submodule ODB paths
18:  411df7248d = 18:  3d1cece660 grep: fix leaking grep pattern
19:  690de28bef = 19:  da7768bad5 promisor-remote: fix leaking partial clone filter
20:  ed4091255c = 20:  0a72fc83f7 builtin/maintenance: fix leaking config string
21:  46956bd8fb = 21:  95200b8a76 builtin/maintenance: fix leak in `get_schedule_cmd()`
22:  2a23df9a68 = 22:  8e7ea54863 revision: fix leaking parents when simplifying commits
23:  57a3a9e9f8 = 23:  8cbc41425f diffcore-break: fix leaking filespecs when merging broken pairs
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 01/23] builtin/help: fix dangling reference to `html_path`

[Patrick Steinhardt]

In `get_html_page_path()` we may end up assigning the return value of
`system_path()` to the global `html_path` variable. But as we also
assign the returned value to `to_free`, we will deallocate its memory
upon returning from the function. Consequently, `html_path` will now
point to deallocated memory.

Fix this issue by instead assigning the value to a separate local
variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/help.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..282ea5721f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -513,23 +513,24 @@ static void show_info_page(const char *page)
 static void get_html_page_path(struct strbuf *page_path, const char *page)
 {
 	struct stat st;
+	const char *path = html_path;
 	char *to_free = NULL;
 
-	if (!html_path)
-		html_path = to_free = system_path(GIT_HTML_PATH);
+	if (!path)
+		path = to_free = system_path(GIT_HTML_PATH);
 
 	/*
 	 * Check that the page we're looking for exists.
 	 */
-	if (!strstr(html_path, "://")) {
-		if (stat(mkpath("%s/%s.html", html_path, page), &st)
+	if (!strstr(path, "://")) {
+		if (stat(mkpath("%s/%s.html", path, page), &st)
 		    || !S_ISREG(st.st_mode))
 			die("'%s/%s.html': documentation file not found.",
-				html_path, page);
+				path, page);
 	}
 
 	strbuf_init(page_path, 0);
-	strbuf_addf(page_path, "%s/%s.html", html_path, page);
+	strbuf_addf(page_path, "%s/%s.html", path, page);
 	free(to_free);
 }
 
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 02/23] builtin/help: fix leaking `html_path` when reading config multiple times

[Patrick Steinhardt]

The `html_path` variable gets populated via `git_help_config()`, which
puts an allocated string into it if its value has been configured. We do
not clear the old value though, which causes a memory leak in case the
config exists multiple times.

Plug this leak. The leak is exposed by t0012, but plugging it alone is
not sufficient to make the test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/help.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 282ea5721f..2c249cbca4 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -52,7 +52,7 @@ static enum help_action {
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
 } cmd_mode;
 
-static const char *html_path;
+static char *html_path;
 static int verbose = 1;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,
 	if (!strcmp(var, "help.htmlpath")) {
 		if (!value)
 			return config_error_nonbool(var);
+		free(html_path);
 		html_path = xstrdup(value);
 		return 0;
 	}
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 03/23] git: fix leaking argv when handling builtins

[Patrick Steinhardt]

In `handle_builtin()` we may end up creating an ad-hoc argv array in
case we see that the command line contains the "--help" parameter. In
this case we observe two memory leaks though:

  - We leak the `struct strvec` itself because we directly exit after
    calling `run_builtin()`, without bothering about any cleanups.

  - Even if we free'd that vector we'd end up leaking some of its
    strings because `run_builtin()` will modify the array.

Plug both of these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 git.c           | 22 +++++++++++++++++++---
 t/t0012-help.sh |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 9a618a2740..3c7fabfda2 100644
--- a/git.c
+++ b/git.c
@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)
 static void handle_builtin(int argc, const char **argv)
 {
 	struct strvec args = STRVEC_INIT;
+	const char **argv_copy = NULL;
 	const char *cmd;
 	struct cmd_struct *builtin;
 
@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv)
 		}
 
 		argc++;
-		argv = args.v;
+
+		/*
+		 * `run_builtin()` will modify the argv array, so we need to
+		 * create a shallow copy such that we can free all of its
+		 * strings.
+		 */
+		CALLOC_ARRAY(argv_copy, argc + 1);
+		COPY_ARRAY(argv_copy, args.v, argc);
+
+		argv = argv_copy;
 	}
 
 	builtin = get_builtin(cmd);
-	if (builtin)
-		exit(run_builtin(builtin, argc, argv));
+	if (builtin) {
+		int ret = run_builtin(builtin, argc, argv);
+		strvec_clear(&args);
+		free(argv_copy);
+		exit(ret);
+	}
+
 	strvec_clear(&args);
+	free(argv_copy);
 }
 
 static void execv_dashed_external(const char **argv)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..9eae0d8356 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -2,6 +2,7 @@
 
 test_description='help'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 configure_help () {
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 04/23] submodule: fix leaking update strategy

[Patrick Steinhardt]

We're not freeing the submodule update strategy command. Provide a
helper function that does this for us and call it in
`update_data_release()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c | 1 +
 submodule-config.c          | 2 +-
 submodule.c                 | 5 +++++
 submodule.h                 | 6 ++++--
 t/t7406-submodule-update.sh | 1 +
 5 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bf8f9a4012..ed05dc5134 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2021,6 +2021,7 @@ struct update_data {
 static void update_data_release(struct update_data *ud)
 {
 	free(ud->displaypath);
+	submodule_update_strategy_release(&ud->update_strategy);
 	module_list_release(&ud->list);
 }
 
diff --git a/submodule-config.c b/submodule-config.c
index c8f2bb2bdd..471637a725 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)
 	free((void *) entry->config->branch);
 	free((void *) entry->config->url);
 	free((void *) entry->config->ignore);
-	free((void *) entry->config->update_strategy.command);
+	submodule_update_strategy_release(&entry->config->update_strategy);
 	free(entry->config);
 }
 
diff --git a/submodule.c b/submodule.c
index 97d0d47b56..0e67984d77 100644
--- a/submodule.c
+++ b/submodule.c
@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,
 	return 0;
 }
 
+void submodule_update_strategy_release(struct submodule_update_strategy *strategy)
+{
+	free((char *) strategy->command);
+}
+
 const char *submodule_update_type_to_string(enum submodule_update_type type)
 {
 	switch (type) {
diff --git a/submodule.h b/submodule.h
index b50d29eba4..4deb1b5f84 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,6 +41,10 @@ struct submodule_update_strategy {
 	.type = SM_UPDATE_UNSPECIFIED, \
 }
 
+int parse_submodule_update_strategy(const char *value,
+				    struct submodule_update_strategy *dst);
+void submodule_update_strategy_release(struct submodule_update_strategy *strategy);
+
 int is_gitmodules_unmerged(struct index_state *istate);
 int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,
 void die_path_inside_submodule(struct index_state *istate,
 			       const struct pathspec *ps);
 enum submodule_update_type parse_submodule_update_type(const char *value);
-int parse_submodule_update_strategy(const char *value,
-				    struct submodule_update_strategy *dst);
 const char *submodule_update_type_to_string(enum submodule_update_type type);
 void handle_ignore_submodules_arg(struct diff_options *, const char *);
 void show_submodule_diff_summary(struct diff_options *o, const char *path,
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 297c6c3b5c..0f0c86f9cb 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -12,6 +12,7 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 05/23] builtin/submodule--helper: clear child process when not running it

[Patrick Steinhardt]

In `runcommand_in_submodule_cb()` we may end up not executing the child
command when `argv` is empty. But we still populate the command with
environment variables and other things, which needs cleanup. This leads
to a memory leak because we do not call `finish_command()`.

Fix this by clearing the child process when we don't execute it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c  | 10 +++++++---
 t/t7407-submodule-foreach.sh |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ed05dc5134..fd1b679408 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	if (!info->quiet)
 		printf(_("Entering '%s'\n"), displaypath);
 
-	if (info->argv[0] && run_command(&cp))
-		die(_("run_command returned non-zero status for %s\n."),
-			displaypath);
+	if (info->argv[0]) {
+		if (run_command(&cp))
+			die(_("run_command returned non-zero status for %s\n."),
+			    displaypath);
+	} else {
+		child_process_clear(&cp);
+	}
 
 	if (info->recursive) {
 		struct child_process cpr = CHILD_PROCESS_INIT;
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 8d7b234beb..9f68893261 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -12,6 +12,7 @@ that are currently checked out.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 06/23] builtin/submodule--helper: fix leaking error buffer

[Patrick Steinhardt]

Fix leaking error buffer when `compute_alternate_path()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c    | 2 ++
 t/t7408-submodule-reference.sh | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fd1b679408..ff1376f69f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject(
 				; /* nothing */
 			}
 		}
+
+		strbuf_release(&err);
 		strbuf_release(&sb);
 	}
 
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index d6040e0a33..7e1afa9ce4 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -4,6 +4,8 @@
 #
 
 test_description='test clone --reference'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 base_dir=$(pwd)
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 07/23] t/helper: fix leaking subrepo in nested submodule config helper

[Patrick Steinhardt]

In the "submodule-nested-repo-config" helper we create a submodule
repository and print its configuration. We do not clear the repo,
causing a memory leak. Plug it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/helper/test-submodule-nested-repo-config.c | 2 +-
 t/t7411-submodule-config.sh                  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
index 6ca069ce63..6dce957153 100644
--- a/t/helper/test-submodule-nested-repo-config.c
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)
 	print_config_from_gitmodules(&subrepo, argv[2]);
 
 	submodule_free(the_repository);
-
+	repo_clear(&subrepo);
 	return 0;
 }
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 31271f8e0a..af0de496e0 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -10,6 +10,7 @@ from the database and from the worktree works.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 08/23] builtin/submodule--helper: fix leaking remote ref on errors

[Patrick Steinhardt]

When `update_submodule()` fails we return with `die_message()`, which
only causes us to print the same message as `die()` would without
actually causing the process to die. We don't free memory in that case
and thus leak memory.

Fix the leak by freeing the remote ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c  | 13 +++++++++----
 t/t7420-submodule-set-url.sh |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ff1376f69f..a9bd93a785 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)
 
 		if (!update_data->nofetch) {
 			if (fetch_in_submodule(update_data->sm_path, update_data->depth,
-					      0, NULL))
+					      0, NULL)) {
+				free(remote_ref);
 				return die_message(_("Unable to fetch in submodule path '%s'"),
 						   update_data->sm_path);
+			}
 		}
 
 		if (repo_resolve_gitlink_ref(the_repository, update_data->sm_path,
-					     remote_ref, &update_data->oid))
-			return die_message(_("Unable to find %s revision in submodule path '%s'"),
-					   remote_ref, update_data->sm_path);
+					     remote_ref, &update_data->oid)) {
+			ret = die_message(_("Unable to find %s revision in submodule path '%s'"),
+					  remote_ref, update_data->sm_path);
+			free(remote_ref);
+			return ret;
+		}
 
 		free(remote_ref);
 	}
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index bf7f15ee79..d7fe910bbe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -10,6 +10,7 @@ as expected.
 '
 
 TEST_NO_CREATE_REPO=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 09/23] dir: fix off by one errors for ignored and untracked entries

[Patrick Steinhardt]

In `treat_directory()` we perform some logic to handle ignored and
untracked entries. When populating a directory with entries we first
save the current number of ignored/untracked entries and then populate
new entries at the end of our arrays that keep track of those entries.
When we figure out that all entries have been ignored/are untracked we
then remove this tail of entries from those vectors again. But there is
an off by one error in both paths that causes us to not free the first
ignored and untracked entries, respectively.

Fix these off-by-one errors to plug the resulting leak. While at it,
massage the code a bit to match our modern code style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 dir.c                                              | 6 ++----
 t/t3011-common-prefixes-and-directory-traversal.sh | 1 +
 t/t7061-wtstatus-ignore.sh                         | 1 +
 t/t7521-ignored-mode.sh                            | 1 +
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 5a23376bda..787bcb7a1a 100644
--- a/dir.c
+++ b/dir.c
@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 			 */
 			state = path_none;
 		} else {
-			int i;
-			for (i = old_ignored_nr + 1; i<dir->ignored_nr; ++i)
+			for (int i = old_ignored_nr; i < dir->ignored_nr; i++)
 				FREE_AND_NULL(dir->ignored[i]);
 			dir->ignored_nr = old_ignored_nr;
 		}
@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 	 */
 	if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
 	    !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
-		int i;
-		for (i = old_untracked_nr + 1; i<dir->nr; ++i)
+		for (int i = old_untracked_nr; i < dir->nr; i++)
 			FREE_AND_NULL(dir->entries[i]);
 		dir->nr = old_untracked_nr;
 	}
diff --git a/t/t3011-common-prefixes-and-directory-traversal.sh b/t/t3011-common-prefixes-and-directory-traversal.sh
index 3da5b2b6e7..69e44c387f 100755
--- a/t/t3011-common-prefixes-and-directory-traversal.sh
+++ b/t/t3011-common-prefixes-and-directory-traversal.sh
@@ -2,6 +2,7 @@
 
 test_description='directory traversal handling, especially with common prefixes'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 2f9bea9793..64145a05b1 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -2,6 +2,7 @@
 
 test_description='git-status ignored files'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expected <<\EOF
diff --git a/t/t7521-ignored-mode.sh b/t/t7521-ignored-mode.sh
index a88b02b06e..edce10f998 100755
--- a/t/t7521-ignored-mode.sh
+++ b/t/t7521-ignored-mode.sh
@@ -2,6 +2,7 @@
 
 test_description='git status ignored modes'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup initial commit and ignore file' '
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 10/23] builtin/pull: fix leaking "ff" option

[Patrick Steinhardt]

The `opt_ff` field gets populated either via `OPT_PASSTHRU` via
`config_get_ff()` or when `--rebase` is passed. So we sometimes end up
overriding the value in `opt_ff` with another value, but we do not free
the old value, causing a memory leak.

Adapt the type of the variable to be `char *` and consistently assign
allocated strings to it such that we can easily free it when it is being
overridden.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pull.c               | 11 +++++++----
 t/t7601-merge-pull-config.sh |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 4c54d8196f..5d9d9e467e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -84,7 +84,7 @@ static const char *opt_squash;
 static const char *opt_commit;
 static const char *opt_edit;
 static const char *cleanup_arg;
-static const char *opt_ff;
+static char *opt_ff;
 static const char *opt_verify_signatures;
 static const char *opt_verify;
 static int opt_autostash = -1;
@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		 * "--rebase" can override a config setting of
 		 * pull.ff=only.
 		 */
-		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
-			opt_ff = "--ff";
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) {
+			free(opt_ff);
+			opt_ff = xstrdup("--ff");
+		}
 	}
 
 	if (opt_rebase < 0)
@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
+			free(opt_ff);
+			opt_ff = xstrdup("--ff-only");
 			ret = run_merge();
 		} else {
 			ret = run_rebase(&newbase, &upstream);
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index a94387a75f..7fd8c086af 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing pull.* configuration parsing and other things.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 11/23] diff: fix leaking orderfile option

[Patrick Steinhardt]

The `orderfile` diff option is being assigned via `OPT_FILENAME()`,
which assigns an allocated string to the variable. We never free it
though, causing a memory leak.

Change the type of the string to `char *` and free it to plug the leak.
This also requires us to use `xstrdup()` to assign the global config to
it in case it is set.

This leak is being hit in t7621, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 combine-diff.c | 3 +--
 diff.c         | 7 +++++--
 diff.h         | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 829a44e416..f6b624dc28 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,
 {
 	struct combine_diff_path *paths = NULL;
 	int i, num_parent = parents->nr;
-
 	int output_format = opt->output_format;
-	const char *orderfile = opt->orderfile;
+	char *orderfile = opt->orderfile;
 
 	opt->output_format = DIFF_FORMAT_NO_OUTPUT;
 	/* tell diff_tree to emit paths in sorted (=tree) order */
diff --git a/diff.c b/diff.c
index 472479eb10..6555b8a32c 100644
--- a/diff.c
+++ b/diff.c
@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value,
 	}
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
-	if (!strcmp(var, "diff.orderfile"))
+	if (!strcmp(var, "diff.orderfile")) {
+		FREE_AND_NULL(diff_order_file_cfg);
 		return git_config_pathname(&diff_order_file_cfg, var, value);
+	}
 
 	if (!strcmp(var, "diff.ignoresubmodules")) {
 		if (!value)
@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	if (diff_indent_heuristic)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
-	options->orderfile = diff_order_file_cfg;
+	options->orderfile = xstrdup_or_null(diff_order_file_cfg);
 
 	if (!options->flags.ignore_submodule_set)
 		options->flags.ignore_untracked_in_submodules = 1;
@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)
 		FREE_AND_NULL(options->objfind);
 	}
 
+	FREE_AND_NULL(options->orderfile);
 	for (size_t i = 0; i < options->anchors_nr; i++)
 		free(options->anchors[i]);
 	FREE_AND_NULL(options->anchors);
diff --git a/diff.h b/diff.h
index 9901c8ca8c..b95d3c1e83 100644
--- a/diff.h
+++ b/diff.h
@@ -235,7 +235,7 @@ enum diff_submodule_format {
  * diffcore library with.
  */
 struct diff_options {
-	const char *orderfile;
+	char *orderfile;
 
 	/*
 	 * "--rotate-to=<file>" would start showing at <file> and when
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 12/23] parse-options: free previous value of `OPTION_FILENAME`

[Patrick Steinhardt]

The `OPTION_FILENAME` option always assigns either an allocated string
or `NULL` to the value. In case it is passed multiple times it does not
know to free the previous value though, which causes a memory leak.

Refactor the function to always free the previous value. None of the
sites where this option is used pass a string constant, so this change
is safe.

While at it, fix the argument of `fix_filename()` to be a string
constant. The only reason why it's not is because we use it as an
in-out-parameter, where the input is a constant and the output is not.
This is weird and unnecessary, as we can just return the result instead
of using the parameter for this.

This leak is being hit in t7621, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 30b9e68f8a..33bfba0ed4 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,
 	return 0;
 }
 
-static void fix_filename(const char *prefix, char **file)
+static char *fix_filename(const char *prefix, const char *file)
 {
 	if (!file || !*file)
-		; /* leave as NULL */
+		return NULL;
 	else
-		*file = prefix_filename_except_for_dash(prefix, *file);
+		return prefix_filename_except_for_dash(prefix, file);
 }
 
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_FILENAME:
+	{
+		const char *value;
+
+		FREE_AND_NULL(*(char **)opt->value);
+
 		err = 0;
+
 		if (unset)
-			*(const char **)opt->value = NULL;
+			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			*(const char **)opt->value = (const char *)opt->defval;
+			value = (const char *) opt->defval;
 		else
-			err = get_arg(p, opt, flags, (const char **)opt->value);
+			err = get_arg(p, opt, flags, &value);
 
 		if (!err)
-			fix_filename(p->prefix, (char **)opt->value);
+			*(char **)opt->value = fix_filename(p->prefix, value);
 		return err;
-
+	}
 	case OPTION_CALLBACK:
 	{
 		const char *p_arg = NULL;
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 13/23] diffcore-order: fix leaking buffer when parsing orderfiles

[Patrick Steinhardt]

In `prepare_order()` we parse an orderfile and assign it to a global
array. In order to save on some allocations, we replace newlines with
NUL characters and then assign pointers into the allocated buffer to
that array. This can cause the buffer to be completely unreferenced
though in some cases, e.g. because the order file is empty or because we
had to use `xmemdupz()` to copy the lines instead of NUL-terminating
them.

Refactor the code to always `xmemdupz()` the strings. This is a bit
simpler, and it is rather unlikely that saving a handful of allocations
really matters. This allows us to release the string buffer and thus
plug the memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diffcore-order.c      | 19 +++++++------------
 t/t4056-diff-order.sh |  1 +
 t/t4204-patch-id.sh   |  1 +
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index e7d20ebd2d..912513d3e6 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile)
 {
 	int cnt, pass;
 	struct strbuf sb = STRBUF_INIT;
-	void *map;
-	char *cp, *endp;
+	const char *cp, *endp;
 	ssize_t sz;
 
 	if (order)
@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile)
 	sz = strbuf_read_file(&sb, orderfile, 0);
 	if (sz < 0)
 		die_errno(_("failed to read orderfile '%s'"), orderfile);
-	map = strbuf_detach(&sb, NULL);
-	endp = (char *) map + sz;
+	endp = sb.buf + sz;
 
 	for (pass = 0; pass < 2; pass++) {
 		cnt = 0;
-		cp = map;
+		cp = sb.buf;
 		while (cp < endp) {
-			char *ep;
+			const char *ep;
 			for (ep = cp; ep < endp && *ep != '\n'; ep++)
 				;
 			/* cp to ep has one line */
@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)
 			else if (pass == 0)
 				cnt++;
 			else {
-				if (*ep == '\n') {
-					*ep = 0;
-					order[cnt] = cp;
-				} else {
-					order[cnt] = xmemdupz(cp, ep - cp);
-				}
+				order[cnt] = xmemdupz(cp, ep - cp);
 				cnt++;
 			}
 			if (ep < endp)
@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)
 			ALLOC_ARRAY(order, cnt);
 		}
 	}
+
+	strbuf_release(&sb);
 }
 
 static int match_order(const char *path)
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index aec1d9d1b4..32c5fcb9a2 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -5,6 +5,7 @@ test_description='diff order & rotate'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_files () {
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index dc8ddb10af..c0a4a02dcf 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,6 +5,7 @@ test_description='git patch-id'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 14/23] builtin/repack: fix leaking configuration

[Patrick Steinhardt]

When repacking, we assemble git-pack-objects(1) arguments both for the
"normal" pack and for the cruft pack. This configuration gets populated
with a bunch of `OPT_PASSTHRU` options that we end up passing to the
child process. These options are allocated, but never free'd.

Create a new `pack_objects_args_release()` function that releases the
memory for us and call it for both sets of options.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/repack.c              | 57 ++++++++++++++++++++++++++---------
 t/t5329-pack-objects-cruft.sh |  2 ++
 t/t7700-repack.sh             |  1 +
 3 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 3ee8cfa732..c31d5653f1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,
 		run_update_server_info = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "repack.cruftwindow"))
+	if (!strcmp(var, "repack.cruftwindow")) {
+		free(cruft_po_args->window);
 		return git_config_string(&cruft_po_args->window, var, value);
-	if (!strcmp(var, "repack.cruftwindowmemory"))
+	}
+	if (!strcmp(var, "repack.cruftwindowmemory")) {
+		free(cruft_po_args->window_memory);
 		return git_config_string(&cruft_po_args->window_memory, var, value);
-	if (!strcmp(var, "repack.cruftdepth"))
+	}
+	if (!strcmp(var, "repack.cruftdepth")) {
+		free(cruft_po_args->depth);
 		return git_config_string(&cruft_po_args->depth, var, value);
-	if (!strcmp(var, "repack.cruftthreads"))
+	}
+	if (!strcmp(var, "repack.cruftthreads")) {
+		free(cruft_po_args->threads);
 		return git_config_string(&cruft_po_args->threads, var, value);
+	}
 	return git_default_config(var, value, ctx, cb);
 }
 
+static void pack_objects_args_release(struct pack_objects_args *args)
+{
+	free(args->window);
+	free(args->window_memory);
+	free(args->depth);
+	free(args->threads);
+	list_objects_filter_release(&args->filter_options);
+}
+
 struct existing_packs {
 	struct string_list kept_packs;
 	struct string_list non_kept_packs;
@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	const char *unpack_unreachable = NULL;
 	int keep_unreachable = 0;
 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
-	struct pack_objects_args po_args = {NULL};
-	struct pack_objects_args cruft_po_args = {NULL};
+	struct pack_objects_args po_args = { 0 };
+	struct pack_objects_args cruft_po_args = { 0 };
 	int write_midx = 0;
 	const char *cruft_expiration = NULL;
 	const char *expire_to = NULL;
 	const char *filter_to = NULL;
+	const char *opt_window = NULL;
+	const char *opt_window_memory = NULL;
+	const char *opt_depth = NULL;
+	const char *opt_threads = NULL;
 
 	struct option builtin_repack_options[] = {
 		OPT_BIT('a', NULL, &pack_everything,
@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("with -A, do not loosen objects older than this")),
 		OPT_BOOL('k', "keep-unreachable", &keep_unreachable,
 				N_("with -a, repack unreachable objects")),
-		OPT_STRING(0, "window", &po_args.window, N_("n"),
+		OPT_STRING(0, "window", &opt_window, N_("n"),
 				N_("size of the window used for delta compression")),
-		OPT_STRING(0, "window-memory", &po_args.window_memory, N_("bytes"),
+		OPT_STRING(0, "window-memory", &opt_window_memory, N_("bytes"),
 				N_("same as the above, but limit memory size instead of entries count")),
-		OPT_STRING(0, "depth", &po_args.depth, N_("n"),
+		OPT_STRING(0, "depth", &opt_depth, N_("n"),
 				N_("limits the maximum delta depth")),
-		OPT_STRING(0, "threads", &po_args.threads, N_("n"),
+		OPT_STRING(0, "threads", &opt_threads, N_("n"),
 				N_("limits the maximum number of threads")),
 		OPT_MAGNITUDE(0, "max-pack-size", &po_args.max_pack_size,
 				N_("maximum size of each packfile")),
@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_repack_options,
 				git_repack_usage, 0);
 
+	po_args.window = xstrdup_or_null(opt_window);
+	po_args.window_memory = xstrdup_or_null(opt_window_memory);
+	po_args.depth = xstrdup_or_null(opt_depth);
+	po_args.threads = xstrdup_or_null(opt_threads);
+
 	if (delete_redundant && repository_format_precious_objects)
 		die(_("cannot delete packs in a precious-objects repo"));
 
@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		const char *pack_prefix = find_pack_prefix(packdir, packtmp);
 
 		if (!cruft_po_args.window)
-			cruft_po_args.window = po_args.window;
+			cruft_po_args.window = xstrdup_or_null(po_args.window);
 		if (!cruft_po_args.window_memory)
-			cruft_po_args.window_memory = po_args.window_memory;
+			cruft_po_args.window_memory = xstrdup_or_null(po_args.window_memory);
 		if (!cruft_po_args.depth)
-			cruft_po_args.depth = po_args.depth;
+			cruft_po_args.depth = xstrdup_or_null(po_args.depth);
 		if (!cruft_po_args.threads)
-			cruft_po_args.threads = po_args.threads;
+			cruft_po_args.threads = xstrdup_or_null(po_args.threads);
 		if (!cruft_po_args.max_pack_size)
 			cruft_po_args.max_pack_size = po_args.max_pack_size;
 
@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&names, 1);
 	existing_packs_release(&existing);
 	free_pack_geometry(&geometry);
-	list_objects_filter_release(&po_args.filter_options);
+	pack_objects_args_release(&po_args);
+	pack_objects_args_release(&cruft_po_args);
 
 	return ret;
 }
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index fc5fedbe9b..445739d06c 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='cruft pack related pack-objects tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 objdir=.git/objects
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index be1188e736..c4c3d1a15d 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -2,6 +2,7 @@
 
 test_description='git repack works correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "${TEST_DIRECTORY}/lib-bitmap.sh"
 . "${TEST_DIRECTORY}/lib-midx.sh"
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 15/23] builtin/difftool: plug several trivial memory leaks

[Patrick Steinhardt]

There are several leaking data structures in git-difftool(1). Plug them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/difftool.c  | 6 ++++++
 t/t7800-difftool.sh | 1 +
 2 files changed, 7 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index dcc68e190c..1a68ab6699 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	if (fp)
 		fclose(fp);
 
+	hashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry);
+	hashmap_clear_and_free(&wt_modified, struct path_entry, entry);
+	hashmap_clear_and_free(&tmp_modified, struct path_entry, entry);
+	hashmap_clear_and_free(&submodules, struct pair_entry, entry);
+	hashmap_clear_and_free(&symlinks2, struct pair_entry, entry);
+	release_index(&wtindex);
 	free(lbase_dir);
 	free(rbase_dir);
 	strbuf_release(&info);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index cc917b257e..f67b9345b8 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -11,6 +11,7 @@ Testing basic diff tool invocation
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 difftool_test_setup ()
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 16/23] trace2: destroy context stored in thread-local storage

[Patrick Steinhardt]

Each thread may have a specific context in the trace2 subsystem that we
set up via thread-local storage. We do not set up a destructor for this
data though, which means that the context data will leak.

Plug this leak by installing a destructor. This leak is exposed by
t7814, but plugging it alone does not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 trace2/tr2_tls.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 4f75392952..7b023c1bfc 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)
 	return us - tr2tls_us_start_process;
 }
 
+static void tr2tls_key_destructor(void *payload)
+{
+	struct tr2tls_thread_ctx *ctx = payload;
+	free((char *)ctx->thread_name);
+	free(ctx->array_us_start);
+	free(ctx);
+}
+
 void tr2tls_init(void)
 {
 	tr2tls_start_process_clock();
 
-	pthread_key_create(&tr2tls_key, NULL);
+	pthread_key_create(&tr2tls_key, tr2tls_key_destructor);
 	init_recursive_mutex(&tr2tls_mutex);
 
 	tr2tls_thread_main =
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 17/23] submodule: fix leaking submodule ODB paths

[Patrick Steinhardt]

In `add_submodule_odb_by_path()` we add a path into a global string
list. The list is initialized with `NODUP`, which means that we do not
pass ownership of strings to the list. But we use `xstrdup()` when we
insert a path, with the consequence that the string will never get
free'd.

Plug the leak by marking the list as `DUP`. There is only a single
callsite where we insert paths anyway, and as explained above that
callsite was mishandling the allocation.

This leak is exposed by t7814, but plugging it does not make the whole
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0e67984d77..a07debc227 100644
--- a/submodule.c
+++ b/submodule.c
@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)
 		die(_("staging updated .gitmodules failed"));
 }
 
-static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;
+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_DUP;
 
 void add_submodule_odb_by_path(const char *path)
 {
-	string_list_insert(&added_submodule_odb_paths, xstrdup(path));
+	string_list_insert(&added_submodule_odb_paths, path);
 }
 
 int register_all_submodule_odb_as_alternates(void)
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 18/23] grep: fix leaking grep pattern

[Patrick Steinhardt]

When creating a pattern via `create_grep_pat()` we allocate the pattern
member of the structure regardless of the token type. But later, when we
try to free the structure, we free the pattern member conditionally on
the token type and thus leak memory.

Plug this leak. The leak is exposed by t7814, but plugging it alone does
not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index e5761426e4..701e58de04 100644
--- a/grep.c
+++ b/grep.c
@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)
 				free_pcre2_pattern(p);
 			else
 				regfree(&p->regexp);
-			free(p->pattern);
 			break;
 		default:
 			break;
 		}
+		free(p->pattern);
 		free(p);
 	}
 }
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 19/23] promisor-remote: fix leaking partial clone filter

[Patrick Steinhardt]

The partial clone filter of a promisor remote is never free'd, causing
memory leaks. Furthermore, in case multiple partial clone filters are
defined for the same remote, we'd overwrite previous values without
freeing them.

Fix these leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 promisor-remote.c                  | 2 ++
 t/t7814-grep-recurse-submodules.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/promisor-remote.c b/promisor-remote.c
index 317e1b127f..9345ae3db2 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,
 		if (!r)
 			return 0;
 
+		FREE_AND_NULL(r->partial_clone_filter);
 		return git_config_string(&r->partial_clone_filter, var, value);
 	}
 
@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config)
 {
 	while (config->promisors) {
 		struct promisor_remote *r = config->promisors;
+		free(r->partial_clone_filter);
 		config->promisors = config->promisors->next;
 		free(r);
 	}
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 167fe66150..55ed630e77 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -7,6 +7,7 @@ submodules.
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 20/23] builtin/maintenance: fix leaking config string

[Patrick Steinhardt]

When parsing the maintenance strategy from config we allocate a config
string, but do not free it after parsing it. Plug this leak by instead
using `git_config_get_string_tmp()`, which does not allocate any memory.

This leak is exposed by t7900, but plugging it alone does not make the
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7dac971405..3acfa367ad 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 
 static void initialize_maintenance_strategy(void)
 {
-	char *config_str;
+	const char *config_str;
 
-	if (git_config_get_string("maintenance.strategy", &config_str))
+	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
 		return;
 
 	if (!strcasecmp(config_str, "incremental")) {
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 21/23] builtin/maintenance: fix leak in `get_schedule_cmd()`

[Patrick Steinhardt]

The `get_schedule_cmd()` function allows us to override the schedule
command with a specific test command such that we can verify the
underlying logic in a platform-independent way. Its memory management is
somewhat wild though, because it basically gives up and assigns an
allocated string to the string constant output pointer. While this part
is marked with `UNLEAK()` to mask this, we also leak the local string
lists.

Rework the function such that it has a separate out parameter. If set,
we will assign it the final allocated command. Plug the other memory
leaks and create a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 127 ++++++++++++++++++++++++++---------------
 t/t7900-maintenance.sh |   1 +
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3acfa367ad..b68a0be62c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)
  * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.
  *   In this case, the *cmd value is read as input.
  *
- *   * if the input value *cmd is the key of one of the comma-separated list
- *     item, then *is_available is set to true and *cmd is modified and becomes
+ *   * if the input value cmd is the key of one of the comma-separated list
+ *     item, then *is_available is set to true and *out is set to
  *     the mock command.
  *
  *   * if the input value *cmd isn’t the key of any of the comma-separated list
- *     item, then *is_available is set to false.
+ *     item, then *is_available is set to false and *out is set to the original
+ *     command.
  *
  * Ex.:
  *   GIT_TEST_MAINT_SCHEDULER not set
  *     +-------+-------------------------------------------------+
  *     | Input |                     Output                      |
- *     | *cmd  | return code |       *cmd        | *is_available |
+ *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
- *     | "foo" |    false    | "foo" (unchanged) |  (unchanged)  |
+ *     | "foo" |    false    | NULL              |  (unchanged)  |
  *     +-------+-------------+-------------------+---------------+
  *
  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
  *     +-------+-------------------------------------------------+
  *     | Input |                     Output                      |
- *     | *cmd  | return code |       *cmd        | *is_available |
+ *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
  *     | "foo" |    true     |  "./mock.foo.sh"  |     true      |
- *     | "qux" |    true     | "qux" (unchanged) |     false     |
+ *     | "qux" |    true     | "qux" (allocated) |     false     |
  *     +-------+-------------+-------------------+---------------+
  */
-static int get_schedule_cmd(const char **cmd, int *is_available)
+static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
 {
 	char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
 	struct string_list_item *item;
@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)
 		if (string_list_split_in_place(&pair, item->string, ":", 2) != 2)
 			continue;
 
-		if (!strcmp(*cmd, pair.items[0].string)) {
-			*cmd = pair.items[1].string;
+		if (!strcmp(cmd, pair.items[0].string)) {
+			if (out)
+				*out = xstrdup(pair.items[1].string);
 			if (is_available)
 				*is_available = 1;
-			string_list_clear(&list, 0);
-			UNLEAK(testing);
-			return 1;
+			string_list_clear(&pair, 0);
+			goto out;
 		}
+
+		string_list_clear(&pair, 0);
 	}
 
+	if (out)
+		*out = xstrdup(cmd);
+
+out:
 	string_list_clear(&list, 0);
 	free(testing);
 	return 1;
@@ -1850,9 +1857,8 @@ static int get_random_minute(void)
 
 static int is_launchctl_available(void)
 {
-	const char *cmd = "launchctl";
 	int is_available;
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("launchctl", &is_available, NULL))
 		return is_available;
 
 #ifdef __APPLE__
@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)
 
 static int launchctl_boot_plist(int enable, const char *filename)
 {
-	const char *cmd = "launchctl";
+	char *cmd;
 	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
 	char *uid = launchctl_get_uid();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("launchctl", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, enable ? "bootstrap" : "bootout", uid,
 		     filename, NULL);
@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)
 
 	result = finish_command(&child);
 
+	free(cmd);
 	free(uid);
 	return result;
 }
@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 	static unsigned long lock_file_timeout_ms = ULONG_MAX;
 	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
 	struct stat st;
-	const char *cmd = "launchctl";
+	char *cmd;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("launchctl", NULL, &cmd);
 	preamble = "<?xml version=\"1.0\"?>\n"
 		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
 		   "<plist version=\"1.0\">"
@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 
 	free(filename);
 	free(name);
+	free(cmd);
 	strbuf_release(&plist);
 	strbuf_release(&plist2);
 	return 0;
@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)
 
 static int is_schtasks_available(void)
 {
-	const char *cmd = "schtasks";
 	int is_available;
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("schtasks", &is_available, NULL))
 		return is_available;
 
 #ifdef GIT_WINDOWS_NATIVE
@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)
 
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
-	const char *cmd = "schtasks";
+	char *cmd;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
 	char *name = schtasks_task_name(frequency);
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("schtasks", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
 	free(name);
+	free(cmd);
 
 	return run_command(&child);
 }
@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)
 
 static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)
 {
-	const char *cmd = "schtasks";
+	char *cmd;
 	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *xml;
@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	struct strbuf tfilename = STRBUF_INIT;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("schtasks", NULL, &cmd);
 
 	strbuf_addf(&tfilename, "%s/schedule_%s_XXXXXX",
 		    get_git_common_dir(), frequency);
@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 
 	delete_tempfile(&tfile);
 	free(name);
+	free(cmd);
 	return result;
 }
 
@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)
 
 static int is_crontab_available(void)
 {
-	const char *cmd = "crontab";
+	char *cmd;
 	int is_available;
+	int ret;
 
-	if (get_schedule_cmd(&cmd, &is_available))
-		return is_available;
+	if (get_schedule_cmd("crontab", &is_available, &cmd)) {
+		ret = is_available;
+		goto out;
+	}
 
 #ifdef __APPLE__
 	/*
 	 * macOS has cron, but it requires special permissions and will
 	 * create a UI alert when attempting to run this command.
 	 */
-	return 0;
+	ret = 0;
 #else
-	return check_crontab_process(cmd);
+	ret = check_crontab_process(cmd);
 #endif
+
+out:
+	free(cmd);
+	return ret;
 }
 
 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)
 
 static int crontab_update_schedule(int run_maintenance, int fd)
 {
-	const char *cmd = "crontab";
+	char *cmd;
 	int result = 0;
 	int in_old_region = 0;
 	struct child_process crontab_list = CHILD_PROCESS_INIT;
@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	struct tempfile *tmpedit = NULL;
 	int minute = get_random_minute();
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("crontab", NULL, &cmd);
 	strvec_split(&crontab_list.args, cmd);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
 	crontab_list.out = dup(fd);
 	crontab_list.git_cmd = 0;
 
-	if (start_command(&crontab_list))
-		return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
+	if (start_command(&crontab_list)) {
+		result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
+		goto out;
+	}
 
 	/* Ignore exit code, as an empty crontab will return error. */
 	finish_command(&crontab_list);
@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 		result = error(_("'crontab' died"));
 	else
 		fclose(cron_list);
+
 out:
 	delete_tempfile(&tmpedit);
+	free(cmd);
 	return result;
 }
 
@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)
 
 static int is_systemd_timer_available(void)
 {
-	const char *cmd = "systemctl";
 	int is_available;
 
-	if (get_schedule_cmd(&cmd, &is_available))
+	if (get_schedule_cmd("systemctl", &is_available, NULL))
 		return is_available;
 
 	return real_is_systemd_timer_available();
@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,
 				     enum schedule_priority schedule,
 				     int minute)
 {
-	const char *cmd = "systemctl";
+	char *cmd = NULL;
 	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
+	int ret;
 
 	/*
 	 * Disabling the systemd unit while it is already disabled makes
@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,
 	 * On the other hand, enabling a systemd unit which is already enabled
 	 * produces no error.
 	 */
-	if (!enable)
+	if (!enable) {
 		child.no_stderr = 1;
-	else if (systemd_timer_write_timer_file(schedule, minute))
-		return -1;
+	} else if (systemd_timer_write_timer_file(schedule, minute)) {
+		ret = -1;
+		goto out;
+	}
 
-	get_schedule_cmd(&cmd, NULL);
+	get_schedule_cmd("systemctl", NULL, &cmd);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
 		     "--now", NULL);
 	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
 
-	if (start_command(&child))
-		return error(_("failed to start systemctl"));
-	if (finish_command(&child))
+	if (start_command(&child)) {
+		ret = error(_("failed to start systemctl"));
+		goto out;
+	}
+
+	if (finish_command(&child)) {
 		/*
 		 * Disabling an already disabled systemd unit makes
 		 * systemctl fail.
@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,
 		 *
 		 * Enabling an enabled systemd unit doesn't fail.
 		 */
-		if (enable)
-			return error(_("failed to run systemctl"));
-	return 0;
+		if (enable) {
+			ret = error(_("failed to run systemctl"));
+			goto out;
+		}
+	}
+
+	ret = 0;
+
+out:
+	free(cmd);
+	return ret;
 }
 
 /*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index abae7a9754..c62c42848a 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -2,6 +2,7 @@
 
 test_description='git maintenance builtin'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_COMMIT_GRAPH=0
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 22/23] revision: fix leaking parents when simplifying commits

[Patrick Steinhardt]

When simplifying commits, e.g. because they are treesame with their
parents, we unset the commit's parent pointers but never free them. Plug
the resulting memory leaks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c                        | 5 +++++
 t/t1414-reflog-walk.sh            | 1 +
 t/t5310-pack-bitmaps.sh           | 1 +
 t/t5326-multi-pack-bitmaps.sh     | 2 ++
 t/t6004-rev-list-path-optim.sh    | 1 +
 t/t6019-rev-list-ancestry-path.sh | 1 +
 t/t6111-rev-list-treesame.sh      | 1 +
 7 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 2d7ad2bddf..e79f39e555 100644
--- a/revision.c
+++ b/revision.c
@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 					ts->treesame[nth_parent] = 1;
 				continue;
 			}
+
+			free_commit_list(parent->next);
 			parent->next = NULL;
+			while (commit->parents != parent)
+				pop_commit(&commit->parents);
 			commit->parents = parent;
 
 			/*
@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 					die("cannot simplify commit %s (invalid %s)",
 					    oid_to_hex(&commit->object.oid),
 					    oid_to_hex(&p->object.oid));
+				free_commit_list(p->parents);
 				p->parents = NULL;
 			}
 		/* fallthrough */
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index be6c3f472c..49d28166da 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up some reflog entries' '
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a6de7c5764..7044c7d7c6 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -2,6 +2,7 @@
 
 test_description='exercise basic bitmap functionality'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
 
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 832b92619c..6eaa692f33 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='exercise basic multi-pack bitmap functionality'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "${TEST_DIRECTORY}/lib-bitmap.sh"
 
diff --git a/t/t6004-rev-list-path-optim.sh b/t/t6004-rev-list-path-optim.sh
index cd4f420e2a..5416241ede 100755
--- a/t/t6004-rev-list-path-optim.sh
+++ b/t/t6004-rev-list-path-optim.sh
@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index 738da23628..1aabab6956 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh
@@ -29,6 +29,7 @@ test_description='--ancestry-path'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_merge () {
diff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh
index 90ff141640..f63bc8d3da 100755
--- a/t/t6111-rev-list-treesame.sh
+++ b/t/t6111-rev-list-treesame.sh
@@ -16,6 +16,7 @@ test_description='TREESAME and limiting'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 note () {
-- 
2.46.2.852.g229c0bf0e5.dirty

[PATCH v2 23/23] diffcore-break: fix leaking filespecs when merging broken pairs

[Patrick Steinhardt]

When merging file pairs after they have been broken up we queue a new
file pair and discard the broken-up ones. The newly-queued file pair
reuses one filespec of the broken up pairs each, where the respective
other filespec gets discarded. But we only end up freeing the filespec's
data, not the filespec itself, and thus leak memory.

Fix these leaks by using `free_filespec()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diffcore-break.c                  | 4 ++--
 t/t4008-diff-break-rewrite.sh     | 2 ++
 t/t4022-diff-rewrite.sh           | 1 +
 t/t4023-diff-rename-typechange.sh | 1 +
 t/t4031-diff-rewrite-binary.sh    | 1 +
 t/t7524-commit-summary.sh         | 2 ++
 6 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/diffcore-break.c b/diffcore-break.c
index 831b66b5c3..02735f80c6 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,
 	 * in the resulting tree.
 	 */
 	d->one->rename_used++;
-	diff_free_filespec_data(d->two);
-	diff_free_filespec_data(c->one);
+	free_filespec(d->two);
+	free_filespec(c->one);
 	free(d);
 	free(c);
 }
diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh
index 562aaf3a2a..b0ef0026e0 100755
--- a/t/t4008-diff-break-rewrite.sh
+++ b/t/t4008-diff-break-rewrite.sh
@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites.
 
 Further, with -B and -M together, these should turn into two renames.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh ;# test-lib chdir's into trash
 
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index 6fed993ea0..77bc36d5d8 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -2,6 +2,7 @@
 
 test_description='rewrite diff'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff-data.sh
 
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
index 787605ce3f..e6f4fe441e 100755
--- a/t/t4023-diff-rename-typechange.sh
+++ b/t/t4023-diff-rename-typechange.sh
@@ -2,6 +2,7 @@
 
 test_description='typechange rename detection'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
index c4394a27b5..1b8cd3e4c9 100755
--- a/t/t4031-diff-rewrite-binary.sh
+++ b/t/t4031-diff-rewrite-binary.sh
@@ -2,6 +2,7 @@
 
 test_description='rewrite diff on binary file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # We must be large enough to meet the MINIMUM_BREAK_SIZE
diff --git a/t/t7524-commit-summary.sh b/t/t7524-commit-summary.sh
index 47b2f1dc22..a8fceb6a47 100755
--- a/t/t7524-commit-summary.sh
+++ b/t/t7524-commit-summary.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git commit summary'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.46.2.852.g229c0bf0e5.dirty