[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2

[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2

[andrzej]

From: Andrzej Hunt <andrzej@ahunt.org>

This series plugs more of the leaks that were found while running
t0002-t0099 with LSAN.

See also the first series (already merged) at [1]. I'm currently
expecting at least another 2 series before t0002-t0099 run leak free.
I'm not being particularly systematic about the order of patches -
although I am trying to send out "real" (if mostly small) leaks first,
before sending out the more boring patches that add free()/UNLEAK() to
cmd_* and direct helpers thereof.

ATB,

Andrzej

[1] https://lore.kernel.org/git/pull.929.git.1617994052.gitgitgadget@gmail.com/

Andrzej Hunt (12):
  fmt-merge-msg: free newly allocated temporary strings when done
  environment: move strbuf into block to plug leak
  builtin/submodule--helper: release unused strbuf to avoid leak
  for-each-repo: remove unnecessary argv copy to plug leak
  diffcore-rename: move old_dir/new_dir definition to plug leak
  ref-filter: also free head for ATOM_HEAD to avoid leak
  read-cache: call diff_setup_done to avoid leak
  convert: release strbuf to avoid leak
  builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv
  builtin/merge: free found_ref when done
  builtin/rebase: fix options.strategy memory lifecycle
  reset: clear_unpack_trees_porcelain to plug leak

 builtin/for-each-repo.c     | 14 ++++----------
 builtin/merge.c             |  3 ++-
 builtin/mv.c                |  5 +++++
 builtin/rebase.c            |  5 ++---
 builtin/submodule--helper.c |  6 ++++--
 convert.c                   |  2 ++
 diffcore-rename.c           | 10 +++++++---
 environment.c               |  7 +++----
 fmt-merge-msg.c             |  6 ++++--
 read-cache.c                |  1 +
 ref-filter.c                |  8 ++++++--
 reset.c                     |  4 ++--
 12 files changed, 42 insertions(+), 29 deletions(-)

-- 
2.26.2

[PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

origin starts off pointing to somewhere within line, which is owned by
the caller. Later we might allocate a new string using xmemdupz() or
xstrfmt(). To avoid leaking these new strings, we introduce a to_free
pointer - which allows us to safely free the newly allocated string when
we're done (we cannot just free origin directly as it might still be
pointing to line).

LSAN output from t0090:

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa71f49 in do_xmalloc wrapper.c:41:8
    #2 0xa720b0 in do_xmallocz wrapper.c:75:8
    #3 0xa720b0 in xmallocz wrapper.c:83:9
    #4 0xa720b0 in xmemdupz wrapper.c:99:16
    #5 0x8092ba in handle_line fmt-merge-msg.c:187:23
    #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7
    #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2
    #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3
    #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6b3fad in main common-main.c:52:11
    #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 fmt-merge-msg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 0f66818e0f..b969dc6ebb 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -105,90 +105,92 @@ static void add_merge_parent(struct merge_parents *table,
 static int handle_line(char *line, struct merge_parents *merge_parents)
 {
 	int i, len = strlen(line);
 	struct origin_data *origin_data;
 	char *src;
 	const char *origin, *tag_name;
+	char *to_free = NULL;
 	struct src_data *src_data;
 	struct string_list_item *item;
 	int pulling_head = 0;
 	struct object_id oid;
 	const unsigned hexsz = the_hash_algo->hexsz;
 
 	if (len < hexsz + 3 || line[hexsz] != '\t')
 		return 1;
 
 	if (starts_with(line + hexsz + 1, "not-for-merge"))
 		return 0;
 
 	if (line[hexsz + 1] != '\t')
 		return 2;
 
 	i = get_oid_hex(line, &oid);
 	if (i)
 		return 3;
 
 	if (!find_merge_parent(merge_parents, &oid, NULL))
 		return 0; /* subsumed by other parents */
 
 	CALLOC_ARRAY(origin_data, 1);
 	oidcpy(&origin_data->oid, &oid);
 
 	if (line[len - 1] == '\n')
 		line[len - 1] = 0;
 	line += hexsz + 2;
 
 	/*
 	 * At this point, line points at the beginning of comment e.g.
 	 * "branch 'frotz' of git://that/repository.git".
 	 * Find the repository name and point it with src.
 	 */
 	src = strstr(line, " of ");
 	if (src) {
 		*src = 0;
 		src += 4;
 		pulling_head = 0;
 	} else {
 		src = line;
 		pulling_head = 1;
 	}
 
 	item = unsorted_string_list_lookup(&srcs, src);
 	if (!item) {
 		item = string_list_append(&srcs, src);
 		item->util = xcalloc(1, sizeof(struct src_data));
 		init_src_data(item->util);
 	}
 	src_data = item->util;
 
 	if (pulling_head) {
 		origin = src;
 		src_data->head_status |= 1;
 	} else if (skip_prefix(line, "branch ", &origin)) {
 		origin_data->is_local_branch = 1;
 		string_list_append(&src_data->branch, origin);
 		src_data->head_status |= 2;
 	} else if (skip_prefix(line, "tag ", &tag_name)) {
 		origin = line;
 		string_list_append(&src_data->tag, tag_name);
 		src_data->head_status |= 2;
 	} else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
 		string_list_append(&src_data->r_branch, origin);
 		src_data->head_status |= 2;
 	} else {
 		origin = src;
 		string_list_append(&src_data->generic, line);
 		src_data->head_status |= 2;
 	}
 
 	if (!strcmp(".", src) || !strcmp(src, origin)) {
 		int len = strlen(origin);
 		if (origin[0] == '\'' && origin[len - 1] == '\'')
-			origin = xmemdupz(origin + 1, len - 2);
+			origin = to_free = xmemdupz(origin + 1, len - 2);
 	} else
-		origin = xstrfmt("%s of %s", origin, src);
+		origin = to_free = xstrfmt("%s of %s", origin, src);
 	if (strcmp(".", src))
 		origin_data->is_local_branch = 0;
 	string_list_append(&origins, origin)->util = origin_data;
+	free(to_free);
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> origin starts off pointing to somewhere within line, which is owned by
> the caller. Later we might allocate a new string using xmemdupz() or
> xstrfmt(). To avoid leaking these new strings, we introduce a to_free
> pointer - which allows us to safely free the newly allocated string when
> we're done (we cannot just free origin directly as it might still be
> pointing to line).
>
> LSAN output from t0090:
>
> Direct leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>     #1 0xa71f49 in do_xmalloc wrapper.c:41:8
>     #2 0xa720b0 in do_xmallocz wrapper.c:75:8
>     #3 0xa720b0 in xmallocz wrapper.c:83:9
>     #4 0xa720b0 in xmemdupz wrapper.c:99:16
>     #5 0x8092ba in handle_line fmt-merge-msg.c:187:23
>     #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7
>     #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2
>     #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3
>     #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16
>     #10 0x4ce83e in run_builtin git.c:475:11
>     #11 0x4ccafe in handle_builtin git.c:729:3
>     #12 0x4cb01c in run_argv git.c:818:4
>     #13 0x4cb01c in cmd_main git.c:949:19
>     #14 0x6b3fad in main common-main.c:52:11
>     #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  fmt-merge-msg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index 0f66818e0f..b969dc6ebb 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -105,90 +105,92 @@ static void add_merge_parent(struct merge_parents *table,
>  static int handle_line(char *line, struct merge_parents *merge_parents)
>  {
>         int i, len = strlen(line);
>         struct origin_data *origin_data;
>         char *src;
>         const char *origin, *tag_name;
> +       char *to_free = NULL;
>         struct src_data *src_data;
>         struct string_list_item *item;
>         int pulling_head = 0;
>         struct object_id oid;
>         const unsigned hexsz = the_hash_algo->hexsz;
>
>         if (len < hexsz + 3 || line[hexsz] != '\t')
>                 return 1;
>
>         if (starts_with(line + hexsz + 1, "not-for-merge"))
>                 return 0;
>
>         if (line[hexsz + 1] != '\t')
>                 return 2;
>
>         i = get_oid_hex(line, &oid);
>         if (i)
>                 return 3;
>
>         if (!find_merge_parent(merge_parents, &oid, NULL))
>                 return 0; /* subsumed by other parents */
>
>         CALLOC_ARRAY(origin_data, 1);
>         oidcpy(&origin_data->oid, &oid);
>
>         if (line[len - 1] == '\n')
>                 line[len - 1] = 0;
>         line += hexsz + 2;
>
>         /*
>          * At this point, line points at the beginning of comment e.g.
>          * "branch 'frotz' of git://that/repository.git".
>          * Find the repository name and point it with src.
>          */
>         src = strstr(line, " of ");
>         if (src) {
>                 *src = 0;
>                 src += 4;
>                 pulling_head = 0;
>         } else {
>                 src = line;
>                 pulling_head = 1;
>         }
>
>         item = unsorted_string_list_lookup(&srcs, src);
>         if (!item) {
>                 item = string_list_append(&srcs, src);
>                 item->util = xcalloc(1, sizeof(struct src_data));
>                 init_src_data(item->util);
>         }
>         src_data = item->util;
>
>         if (pulling_head) {
>                 origin = src;
>                 src_data->head_status |= 1;
>         } else if (skip_prefix(line, "branch ", &origin)) {
>                 origin_data->is_local_branch = 1;
>                 string_list_append(&src_data->branch, origin);
>                 src_data->head_status |= 2;
>         } else if (skip_prefix(line, "tag ", &tag_name)) {
>                 origin = line;
>                 string_list_append(&src_data->tag, tag_name);
>                 src_data->head_status |= 2;
>         } else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
>                 string_list_append(&src_data->r_branch, origin);
>                 src_data->head_status |= 2;
>         } else {
>                 origin = src;
>                 string_list_append(&src_data->generic, line);
>                 src_data->head_status |= 2;
>         }
>
>         if (!strcmp(".", src) || !strcmp(src, origin)) {
>                 int len = strlen(origin);
>                 if (origin[0] == '\'' && origin[len - 1] == '\'')
> -                       origin = xmemdupz(origin + 1, len - 2);
> +                       origin = to_free = xmemdupz(origin + 1, len - 2);
>         } else
> -               origin = xstrfmt("%s of %s", origin, src);
> +               origin = to_free = xstrfmt("%s of %s", origin, src);
>         if (strcmp(".", src))
>                 origin_data->is_local_branch = 0;
>         string_list_append(&origins, origin)->util = origin_data;
> +       free(to_free);
>         return 0;
>  }
>
> --
> 2.26.2

Makes sense.  The extended diff context makes this patch easier to
read and verify too; thanks.

[PATCH 02/12] environment: move strbuf into block to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

realpath is only populated if we execute the git_work_tree_initialized
block. However that block also causes us to return early, meaning we
never actually release the strbuf in the case where we populated it.
Therefore we move all strbuf related code into the block to guarantee
that we can't leak it.

LSAN output from t0095:

Direct leak of 129 byte(s) in 1 object(s) allocated from:
    #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x78f585 in xrealloc wrapper.c:126:8
    #2 0x713ff4 in strbuf_grow strbuf.c:98:2
    #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3
    #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7
    #5 0x5ae4a4 in set_git_work_tree environment.c:259:3
    #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2
    #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12
    #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2
    #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3
    #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11
    #11 0x4caded in main common-main.c:52:11
    #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).

It looks like this leak has existed since realpath was first added to
set_git_work_tree() in:
  3d7747e318 (real_path: remove unsafe API, 2020-03-10)

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 environment.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/environment.c b/environment.c
index 2f27008424..d6b22ede7e 100644
--- a/environment.c
+++ b/environment.c
@@ -249,25 +249,24 @@ static int git_work_tree_initialized;
 /*
  * Note.  This works only before you used a work tree.  This was added
  * primarily to support git-clone to work in a new repository it just
  * created, and is not meant to flip between different work trees.
  */
 void set_git_work_tree(const char *new_work_tree)
 {
-	struct strbuf realpath = STRBUF_INIT;
-
 	if (git_work_tree_initialized) {
+		struct strbuf realpath = STRBUF_INIT;
+
 		strbuf_realpath(&realpath, new_work_tree, 1);
 		new_work_tree = realpath.buf;
 		if (strcmp(new_work_tree, the_repository->worktree))
 			die("internal error: work tree has already been set\n"
 			    "Current worktree: %s\nNew worktree: %s",
 			    the_repository->worktree, new_work_tree);
+		strbuf_release(&realpath);
 		return;
 	}
 	git_work_tree_initialized = 1;
 	repo_set_worktree(the_repository, new_work_tree);
-
-	strbuf_release(&realpath);
 }
 
 const char *get_git_work_tree(void)
-- 
2.26.2

Re: [PATCH 02/12] environment: move strbuf into block to plug leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> realpath is only populated if we execute the git_work_tree_initialized
> block. However that block also causes us to return early, meaning we
> never actually release the strbuf in the case where we populated it.
> Therefore we move all strbuf related code into the block to guarantee
> that we can't leak it.
>
> LSAN output from t0095:
>
> Direct leak of 129 byte(s) in 1 object(s) allocated from:
>     #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0x78f585 in xrealloc wrapper.c:126:8
>     #2 0x713ff4 in strbuf_grow strbuf.c:98:2
>     #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3
>     #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7
>     #5 0x5ae4a4 in set_git_work_tree environment.c:259:3
>     #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2
>     #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12
>     #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2
>     #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3
>     #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11
>     #11 0x4caded in main common-main.c:52:11
>     #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).
>
> It looks like this leak has existed since realpath was first added to
> set_git_work_tree() in:
>   3d7747e318 (real_path: remove unsafe API, 2020-03-10)

Looking at that commit, it appears to have introduced other problems.
For example, the documentation for read_gitfile_gently() claims it
returns a value from a shared buffer, but that commit got rid of the
shared buffer so the documentation is no longer accurate.  The thing
that is returned is either the path that was passed in, or some newly
allocated path that differs, in which case the caller would be
responsible to free() it, but it looks like the callers aren't doing
so.  There may be others; as I didn't read the whole old patch, but it
looks like even this example could get messy.

I don't think you need to address the whole mess, fixing one of the
issues from it is fine and...

>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  environment.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 2f27008424..d6b22ede7e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -249,25 +249,24 @@ static int git_work_tree_initialized;
>  /*
>   * Note.  This works only before you used a work tree.  This was added
>   * primarily to support git-clone to work in a new repository it just
>   * created, and is not meant to flip between different work trees.
>   */
>  void set_git_work_tree(const char *new_work_tree)
>  {
> -       struct strbuf realpath = STRBUF_INIT;
> -
>         if (git_work_tree_initialized) {
> +               struct strbuf realpath = STRBUF_INIT;
> +
>                 strbuf_realpath(&realpath, new_work_tree, 1);
>                 new_work_tree = realpath.buf;
>                 if (strcmp(new_work_tree, the_repository->worktree))
>                         die("internal error: work tree has already been set\n"
>                             "Current worktree: %s\nNew worktree: %s",
>                             the_repository->worktree, new_work_tree);
> +               strbuf_release(&realpath);
>                 return;
>         }
>         git_work_tree_initialized = 1;
>         repo_set_worktree(the_repository, new_work_tree);
> -
> -       strbuf_release(&realpath);
>  }
>
>  const char *get_git_work_tree(void)
> --
> 2.26.2

This patch looks simple and correct.

Re: [PATCH 02/12] environment: move strbuf into block to plug leak

[René Scharfe]

Am 21.06.21 um 22:49 schrieb Elijah Newren:
> On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>> It looks like this leak has existed since realpath was first added to
>> set_git_work_tree() in:
>>   3d7747e318 (real_path: remove unsafe API, 2020-03-10)
>
> Looking at that commit, it appears to have introduced other problems.
> For example, the documentation for read_gitfile_gently() claims it
> returns a value from a shared buffer, but that commit got rid of the
> shared buffer so the documentation is no longer accurate.  The thing
> that is returned is either the path that was passed in, or some newly
> allocated path that differs, in which case the caller would be
> responsible to free() it, but it looks like the callers aren't doing
> so.

That comment is still correct.  The returned pointer references a shared
static buffer declared in read_gitfile_gently().  The control flow is a
bit hard to follow; path points to the static buffer if and only if
error_code is zero.  Using a dedicated variable for the result would
make that clearer, I think:

diff --git a/setup.c b/setup.c
index ead2f80cd8..75b0a4bea6 100644
--- a/setup.c
+++ b/setup.c
@@ -720,86 +720,87 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 /*
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found. The return value comes from
  * a shared buffer.
  *
  * On failure, if return_error_code is not NULL, return_error_code
  * will be set to an error code and NULL will be returned. If
  * return_error_code is NULL the function will die instead (for most
  * cases).
  */
 const char *read_gitfile_gently(const char *path, int *return_error_code)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
 	char *buf = NULL;
 	char *dir = NULL;
 	const char *slash;
 	struct stat st;
 	int fd;
 	ssize_t len;
 	static struct strbuf realpath = STRBUF_INIT;
+	const char *result = NULL;

 	if (stat(path, &st)) {
 		/* NEEDSWORK: discern between ENOENT vs other errors */
 		error_code = READ_GITFILE_ERR_STAT_FAILED;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
 		error_code = READ_GITFILE_ERR_NOT_A_FILE;
 		goto cleanup_return;
 	}
 	if (st.st_size > max_file_size) {
 		error_code = READ_GITFILE_ERR_TOO_LARGE;
 		goto cleanup_return;
 	}
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		error_code = READ_GITFILE_ERR_OPEN_FAILED;
 		goto cleanup_return;
 	}
 	buf = xmallocz(st.st_size);
 	len = read_in_full(fd, buf, st.st_size);
 	close(fd);
 	if (len != st.st_size) {
 		error_code = READ_GITFILE_ERR_READ_FAILED;
 		goto cleanup_return;
 	}
 	if (!starts_with(buf, "gitdir: ")) {
 		error_code = READ_GITFILE_ERR_INVALID_FORMAT;
 		goto cleanup_return;
 	}
 	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
 		len--;
 	if (len < 9) {
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
 	buf[len] = '\0';
 	dir = buf + 8;

 	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
 		size_t pathlen = slash+1 - path;
 		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
 			      (int)(len - 8), buf + 8);
 		free(buf);
 		buf = dir;
 	}
 	if (!is_git_directory(dir)) {
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}

 	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	result = realpath.buf;

 cleanup_return:
 	if (return_error_code)
 		*return_error_code = error_code;
 	else if (error_code)
 		read_gitfile_error_die(error_code, path, dir);

 	free(buf);
-	return error_code ? NULL : path;
+	return result;
 }

 static const char *setup_explicit_git_dir(const char *gitdirenv,

[PATCH 03/12] builtin/submodule--helper: release unused strbuf to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

relative_url() populates sb. In the normal return path, its buffer is
detached using strbuf_detach(). However the early return path does
nothing with sb, which means that sb's memory is leaked - therefore
we add a release to avoid this leak.

The reset is also only necessary for the normal return path, hence we
move it down to after the early-return to avoid unnecessary work.

LSAN output from t0060:

Direct leak of 121 byte(s) in 1 object(s) allocated from:
    #0 0x7f31246f28b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
    #1 0x98d7d6 in xrealloc wrapper.c:126
    #2 0x909a60 in strbuf_grow strbuf.c:98
    #3 0x90bf00 in strbuf_vaddf strbuf.c:401
    #4 0x90c321 in strbuf_addf strbuf.c:335
    #5 0x5cb78d in relative_url builtin/submodule--helper.c:182
    #6 0x5cbe46 in resolve_relative_url_test builtin/submodule--helper.c:248
    #7 0x410dcd in run_builtin git.c:475
    #8 0x410dcd in handle_builtin git.c:729
    #9 0x414087 in run_argv git.c:818
    #10 0x414087 in cmd_main git.c:949
    #11 0x40e9ec in main common-main.c:52
    #12 0x7f3123c41349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 121 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/submodule--helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ae6174ab05..4015d114b3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -188,11 +188,13 @@ static char *relative_url(const char *remote_url,
 		out = xstrdup(sb.buf + 2);
 	else
 		out = xstrdup(sb.buf);
-	strbuf_reset(&sb);
 
-	if (!up_path || !is_relative)
+	if (!up_path || !is_relative) {
+		strbuf_release(&sb);
 		return out;
+	}
 
+	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s%s", up_path, out);
 	free(out);
 	return strbuf_detach(&sb, NULL);
-- 
2.26.2

[PATCH 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

cmd_for_each_repo() copies argv into args (a strvec), which is later
passed into run_command_on_repo(), which in turn copies that strvec onto
the end of child.args. The initial copy is unnecessary (we never modify
args). We therefore choose to just pass argv directly into
run_command_on_repo(), which lets us avoid the copy and fixes the leak.

LSAN output from t0068:

Direct leak of 192 byte(s) in 1 object(s) allocated from:
    #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
    #1 0x98d7e6 in xrealloc wrapper.c:126
    #2 0x916914 in strvec_push_nodup strvec.c:19
    #3 0x916a6e in strvec_push strvec.c:26
    #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
    #5 0x410dcd in run_builtin git.c:475
    #6 0x410dcd in handle_builtin git.c:729
    #7 0x414087 in run_argv git.c:818
    #8 0x414087 in cmd_main git.c:949
    #9 0x40e9ec in main common-main.c:52
    #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 22 byte(s) in 2 object(s) allocated from:
    #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)
    #1 0x98d698 in xstrdup wrapper.c:29
    #2 0x916a63 in strvec_push strvec.c:26
    #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
    #4 0x410dcd in run_builtin git.c:475
    #5 0x410dcd in handle_builtin git.c:729
    #6 0x414087 in run_argv git.c:818
    #7 0x414087 in cmd_main git.c:949
    #8 0x40e9ec in main common-main.c:52
    #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)

See also discussion about the original implementation below - this code
appears to have evolved from a callback explaining the double-strvec-copy
pattern, but there's no strong reason to keep that now:
  https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/for-each-repo.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 52be64a437..fd86e5a861 100644
--- a/builtin/for-each-repo.c
+++ b/builtin/for-each-repo.c
@@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {
 	NULL
 };
 
-static int run_command_on_repo(const char *path,
-			       void *cbdata)
+static int run_command_on_repo(const char *path, int argc, const char ** argv)
 {
 	int i;
 	struct child_process child = CHILD_PROCESS_INIT;
-	struct strvec *args = (struct strvec *)cbdata;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "-C", path, NULL);
 
-	for (i = 0; i < args->nr; i++)
-		strvec_push(&child.args, args->v[i]);
+	for (i = 0; i < argc; i++)
+		strvec_push(&child.args, argv[i]);
 
 	return run_command(&child);
 }
@@ -29,37 +27,33 @@ static int run_command_on_repo(const char *path,
 int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
 {
 	static const char *config_key = NULL;
 	int i, result = 0;
 	const struct string_list *values;
-	struct strvec args = STRVEC_INIT;
 
 	const struct option options[] = {
 		OPT_STRING(0, "config", &config_key, N_("config"),
 			   N_("config key storing a list of repository paths")),
 		OPT_END()
 	};
 
 	argc = parse_options(argc, argv, prefix, options, for_each_repo_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
 	if (!config_key)
 		die(_("missing --config=<config>"));
 
-	for (i = 0; i < argc; i++)
-		strvec_push(&args, argv[i]);
-
 	values = repo_config_get_value_multi(the_repository,
 					     config_key);
 
 	/*
 	 * Do nothing on an empty list, which is equivalent to the case
 	 * where the config variable does not exist at all.
 	 */
 	if (!values)
 		return 0;
 
 	for (i = 0; !result && i < values->nr; i++)
-		result = run_command_on_repo(values->items[i].string, &args);
+		result = run_command_on_repo(values->items[i].string, argc, argv);
 
 	return result;
 }
-- 
2.26.2

Re: [PATCH 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak

[Elijah Newren]

Hi,

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> cmd_for_each_repo() copies argv into args (a strvec), which is later
> passed into run_command_on_repo(), which in turn copies that strvec onto
> the end of child.args. The initial copy is unnecessary (we never modify
> args). We therefore choose to just pass argv directly into
> run_command_on_repo(), which lets us avoid the copy and fixes the leak.
>
> LSAN output from t0068:
>
> Direct leak of 192 byte(s) in 1 object(s) allocated from:
>     #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
>     #1 0x98d7e6 in xrealloc wrapper.c:126
>     #2 0x916914 in strvec_push_nodup strvec.c:19
>     #3 0x916a6e in strvec_push strvec.c:26
>     #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
>     #5 0x410dcd in run_builtin git.c:475
>     #6 0x410dcd in handle_builtin git.c:729
>     #7 0x414087 in run_argv git.c:818
>     #8 0x414087 in cmd_main git.c:949
>     #9 0x40e9ec in main common-main.c:52
>     #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Indirect leak of 22 byte(s) in 2 object(s) allocated from:
>     #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)
>     #1 0x98d698 in xstrdup wrapper.c:29
>     #2 0x916a63 in strvec_push strvec.c:26
>     #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
>     #4 0x410dcd in run_builtin git.c:475
>     #5 0x410dcd in handle_builtin git.c:729
>     #6 0x414087 in run_argv git.c:818
>     #7 0x414087 in cmd_main git.c:949
>     #8 0x40e9ec in main common-main.c:52
>     #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> See also discussion about the original implementation below - this code
> appears to have evolved from a callback explaining the double-strvec-copy
> pattern, but there's no strong reason to keep that now:
>   https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/

The link you give shows that Stolee was looking forward to reviewing
your patch to fix this issue, so cc'ing him so he can do so.

In general, it helps to cc folks who were involved in earlier
conversations about an area being updated, or who know the code area
well.

> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  builtin/for-each-repo.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> index 52be64a437..fd86e5a861 100644
> --- a/builtin/for-each-repo.c
> +++ b/builtin/for-each-repo.c
> @@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {
>         NULL
>  };
>
> -static int run_command_on_repo(const char *path,
> -                              void *cbdata)
> +static int run_command_on_repo(const char *path, int argc, const char ** argv)
>  {
>         int i;
>         struct child_process child = CHILD_PROCESS_INIT;
> -       struct strvec *args = (struct strvec *)cbdata;
>
>         child.git_cmd = 1;
>         strvec_pushl(&child.args, "-C", path, NULL);
>
> -       for (i = 0; i < args->nr; i++)
> -               strvec_push(&child.args, args->v[i]);
> +       for (i = 0; i < argc; i++)
> +               strvec_push(&child.args, argv[i]);
>
>         return run_command(&child);
>  }
> @@ -29,37 +27,33 @@ static int run_command_on_repo(const char *path,
>  int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>  {
>         static const char *config_key = NULL;
>         int i, result = 0;
>         const struct string_list *values;
> -       struct strvec args = STRVEC_INIT;
>
>         const struct option options[] = {
>                 OPT_STRING(0, "config", &config_key, N_("config"),
>                            N_("config key storing a list of repository paths")),
>                 OPT_END()
>         };
>
>         argc = parse_options(argc, argv, prefix, options, for_each_repo_usage,
>                              PARSE_OPT_STOP_AT_NON_OPTION);
>
>         if (!config_key)
>                 die(_("missing --config=<config>"));
>
> -       for (i = 0; i < argc; i++)
> -               strvec_push(&args, argv[i]);
> -
>         values = repo_config_get_value_multi(the_repository,
>                                              config_key);
>
>         /*
>          * Do nothing on an empty list, which is equivalent to the case
>          * where the config variable does not exist at all.
>          */
>         if (!values)
>                 return 0;
>
>         for (i = 0; !result && i < values->nr; i++)
> -               result = run_command_on_repo(values->items[i].string, &args);
> +               result = run_command_on_repo(values->items[i].string, argc, argv);
>
>         return result;
>  }
> --
> 2.26.2

Patch makes sense to me

[PATCH 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

old_dir/new_dir are free()'d at the end of update_dir_rename_counts,
however if we return early we'll never free those strings. Therefore
we should move all new allocations after the possible early return,
avoiding a leak.

This seems like a fairly recent leak, that started happening since the
early-return was added in:
  1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)

LSAN output from t0022:

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71e48 in xstrdup wrapper.c:29:14
    #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12
    #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
    #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
    #5 0x7b4cfc in diffcore_std diff.c:6705:4
    #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
    #7 0x856574 in log_tree_diff log-tree.c:955:3
    #8 0x856574 in log_tree_commit log-tree.c:986:10
    #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
    #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6b3f3d in main common-main.c:52:11
    #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71e48 in xstrdup wrapper.c:29:14
    #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12
    #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
    #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
    #5 0x7b4cfc in diffcore_std diff.c:6705:4
    #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
    #7 0x856574 in log_tree_diff log-tree.c:955:3
    #8 0x856574 in log_tree_commit log-tree.c:986:10
    #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
    #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6b3f3d in main common-main.c:52:11
    #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 diffcore-rename.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 3375e24659..f7c728fe47 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
 				     const char *oldname,
 				     const char *newname)
 {
-	char *old_dir = xstrdup(oldname);
-	char *new_dir = xstrdup(newname);
-	char new_dir_first_char = new_dir[0];
+	char *old_dir;
+	char *new_dir;
+	const char new_dir_first_char = newname[0];
 	int first_time_in_loop = 1;
 
 	if (!info->setup)
@@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
 		 */
 		return;
 
+
+	old_dir = xstrdup(oldname);
+	new_dir = xstrdup(newname);
+
 	while (1) {
 		int drd_flag = NOT_RELEVANT;
 
-- 
2.26.2

Re: [PATCH 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> old_dir/new_dir are free()'d at the end of update_dir_rename_counts,
> however if we return early we'll never free those strings. Therefore
> we should move all new allocations after the possible early return,
> avoiding a leak.
>
> This seems like a fairly recent leak, that started happening since the
> early-return was added in:
>   1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)

The entire function was added relatively recently, just a few commits
before that one at
  0c4fd732f0 ("Move computation of dir_rename_count from merge-ort to
diffcore-rename", 2021-02-27)

> LSAN output from t0022:
>
> Direct leak of 7 byte(s) in 1 object(s) allocated from:
>     #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0xa71e48 in xstrdup wrapper.c:29:14
>     #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12
>     #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
>     #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
>     #5 0x7b4cfc in diffcore_std diff.c:6705:4
>     #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
>     #7 0x856574 in log_tree_diff log-tree.c:955:3
>     #8 0x856574 in log_tree_commit log-tree.c:986:10
>     #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
>     #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
>     #11 0x4ce83e in run_builtin git.c:475:11
>     #12 0x4ccafe in handle_builtin git.c:729:3
>     #13 0x4cb01c in run_argv git.c:818:4
>     #14 0x4cb01c in cmd_main git.c:949:19
>     #15 0x6b3f3d in main common-main.c:52:11
>     #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Direct leak of 7 byte(s) in 1 object(s) allocated from:
>     #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0xa71e48 in xstrdup wrapper.c:29:14
>     #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12
>     #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
>     #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
>     #5 0x7b4cfc in diffcore_std diff.c:6705:4
>     #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
>     #7 0x856574 in log_tree_diff log-tree.c:955:3
>     #8 0x856574 in log_tree_commit log-tree.c:986:10
>     #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
>     #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
>     #11 0x4ce83e in run_builtin git.c:475:11
>     #12 0x4ccafe in handle_builtin git.c:729:3
>     #13 0x4cb01c in run_argv git.c:818:4
>     #14 0x4cb01c in cmd_main git.c:949:19
>     #15 0x6b3f3d in main common-main.c:52:11
>     #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).

I ran this code under valgrind with specific tests, but apparently not
under enough different cases.  Thanks for catching this.

> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  diffcore-rename.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 3375e24659..f7c728fe47 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
>                                      const char *oldname,
>                                      const char *newname)
>  {
> -       char *old_dir = xstrdup(oldname);
> -       char *new_dir = xstrdup(newname);
> -       char new_dir_first_char = new_dir[0];
> +       char *old_dir;
> +       char *new_dir;
> +       const char new_dir_first_char = newname[0];
>         int first_time_in_loop = 1;
>
>         if (!info->setup)
> @@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
>                  */
>                 return;
>
> +
> +       old_dir = xstrdup(oldname);
> +       new_dir = xstrdup(newname);
> +
>         while (1) {
>                 int drd_flag = NOT_RELEVANT;
>
> --
> 2.26.2

The patch is correct.

[PATCH 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

u.head is populated using resolve_refdup(), which returns a newly
allocated string - hence we also need to free() it.

Found while running t0041 with LSAN:

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa8be98 in xstrdup wrapper.c:29:14
    #2 0x9481db in head_atom_parser ref-filter.c:549:17
    #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30
    #4 0x9400e3 in verify_ref_format ref-filter.c:974:8
    #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6
    #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3
    #7 0x4ce83e in run_builtin git.c:475:11
    #8 0x4ccafe in handle_builtin git.c:729:3
    #9 0x4cb01c in run_argv git.c:818:4
    #10 0x4cb01c in cmd_main git.c:949:19
    #11 0x6bdc2d in main common-main.c:52:11
    #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 ref-filter.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 4db0e40ff4..f8bfd25ae4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2225,8 +2225,12 @@ void ref_array_clear(struct ref_array *array)
 	FREE_AND_NULL(array->items);
 	array->nr = array->alloc = 0;
 
-	for (i = 0; i < used_atom_cnt; i++)
-		free((char *)used_atom[i].name);
+	for (i = 0; i < used_atom_cnt; i++) {
+		struct used_atom *atom = &used_atom[i];
+		if (atom->atom_type == ATOM_HEAD)
+			free(atom->u.head);
+		free((char *)atom->name);
+	}
 	FREE_AND_NULL(used_atom);
 	used_atom_cnt = 0;
 
-- 
2.26.2

Re: [PATCH 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> u.head is populated using resolve_refdup(), which returns a newly
> allocated string - hence we also need to free() it.
>
> Found while running t0041 with LSAN:
>
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0xa8be98 in xstrdup wrapper.c:29:14
>     #2 0x9481db in head_atom_parser ref-filter.c:549:17
>     #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30
>     #4 0x9400e3 in verify_ref_format ref-filter.c:974:8
>     #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6
>     #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3
>     #7 0x4ce83e in run_builtin git.c:475:11
>     #8 0x4ccafe in handle_builtin git.c:729:3
>     #9 0x4cb01c in run_argv git.c:818:4
>     #10 0x4cb01c in cmd_main git.c:949:19
>     #11 0x6bdc2d in main common-main.c:52:11
>     #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  ref-filter.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 4db0e40ff4..f8bfd25ae4 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2225,8 +2225,12 @@ void ref_array_clear(struct ref_array *array)
>         FREE_AND_NULL(array->items);
>         array->nr = array->alloc = 0;
>
> -       for (i = 0; i < used_atom_cnt; i++)
> -               free((char *)used_atom[i].name);
> +       for (i = 0; i < used_atom_cnt; i++) {
> +               struct used_atom *atom = &used_atom[i];
> +               if (atom->atom_type == ATOM_HEAD)
> +                       free(atom->u.head);
> +               free((char *)atom->name);
> +       }
>         FREE_AND_NULL(used_atom);
>         used_atom_cnt = 0;
>
> --
> 2.26.2

Makes sense.  I think builtin/branch.c and builtin/show-branch.c may
have similar problems with resolve_refdup() calls from a few greps.
You don't need to include those in this series, but if you want to
also tackle those, it would be nice.

[PATCH 07/12] read-cache: call diff_setup_done to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

repo_diff_setup() calls through to diff.c's static prep_parse_options(),
which in  turn allocates a new array into diff_opts.parseopts.
diff_setup_done() is responsible for freeing that array, and has the
benefit of verifying diff_opts too - hence we add a call to
diff_setup_done() to avoid leaking parseopts.

Output from the leak as found while running t0090 with LSAN:

Direct leak of 7120 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
    #2 0x7a7bae in prep_parse_options diff.c:5636:2
    #3 0x7a7bae in repo_diff_setup diff.c:4611:2
    #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
    #5 0x872233 in unclean merge-ort-wrappers.c:12:14
    #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
    #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
    #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
    #9 0x4ce83e in run_builtin git.c:475:11
    #10 0x4ccafe in handle_builtin git.c:729:3
    #11 0x4cb01c in run_argv git.c:818:4
    #12 0x4cb01c in cmd_main git.c:949:19
    #13 0x6bdc2d in main common-main.c:52:11
    #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/read-cache.c b/read-cache.c
index 77961a3885..212d604dd3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)
 int repo_index_has_changes(struct repository *repo,
 			   struct tree *tree,
 			   struct strbuf *sb)
 {
 	struct index_state *istate = repo->index;
 	struct object_id cmp;
 	int i;
 
 	if (tree)
 		cmp = tree->object.oid;
 	if (tree || !get_oid_tree("HEAD", &cmp)) {
 		struct diff_options opt;
 
 		repo_diff_setup(repo, &opt);
 		opt.flags.exit_with_status = 1;
 		if (!sb)
 			opt.flags.quick = 1;
+		diff_setup_done(&opt);
 		do_diff_cache(&cmp, &opt);
 		diffcore_std(&opt);
 		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
 			if (i)
 				strbuf_addch(sb, ' ');
 			strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
 		}
 		diff_flush(&opt);
 		return opt.flags.has_changes != 0;
 	} else {
 		/* TODO: audit for interaction with sparse-index. */
 		ensure_full_index(istate);
 		for (i = 0; sb && i < istate->cache_nr; i++) {
 			if (i)
 				strbuf_addch(sb, ' ');
 			strbuf_addstr(sb, istate->cache[i]->name);
 		}
 		return !!istate->cache_nr;
 	}
 }
-- 
2.26.2

Re: [PATCH 07/12] read-cache: call diff_setup_done to avoid leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> repo_diff_setup() calls through to diff.c's static prep_parse_options(),
> which in  turn allocates a new array into diff_opts.parseopts.
> diff_setup_done() is responsible for freeing that array, and has the
> benefit of verifying diff_opts too - hence we add a call to
> diff_setup_done() to avoid leaking parseopts.

Should the documentation near the top of diff.h also point out that
part of the purpose of diff_setup_done() is to free some memory?

> Output from the leak as found while running t0090 with LSAN:
>
> Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>     #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>     #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
>     #2 0x7a7bae in prep_parse_options diff.c:5636:2
>     #3 0x7a7bae in repo_diff_setup diff.c:4611:2
>     #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
>     #5 0x872233 in unclean merge-ort-wrappers.c:12:14
>     #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
>     #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
>     #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
>     #9 0x4ce83e in run_builtin git.c:475:11
>     #10 0x4ccafe in handle_builtin git.c:729:3
>     #11 0x4cb01c in run_argv git.c:818:4
>     #12 0x4cb01c in cmd_main git.c:949:19
>     #13 0x6bdc2d in main common-main.c:52:11
>     #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  read-cache.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 77961a3885..212d604dd3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)
>  int repo_index_has_changes(struct repository *repo,
>                            struct tree *tree,
>                            struct strbuf *sb)
>  {
>         struct index_state *istate = repo->index;
>         struct object_id cmp;
>         int i;
>
>         if (tree)
>                 cmp = tree->object.oid;
>         if (tree || !get_oid_tree("HEAD", &cmp)) {
>                 struct diff_options opt;
>
>                 repo_diff_setup(repo, &opt);
>                 opt.flags.exit_with_status = 1;
>                 if (!sb)
>                         opt.flags.quick = 1;
> +               diff_setup_done(&opt);
>                 do_diff_cache(&cmp, &opt);
>                 diffcore_std(&opt);
>                 for (i = 0; sb && i < diff_queued_diff.nr; i++) {
>                         if (i)
>                                 strbuf_addch(sb, ' ');
>                         strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
>                 }
>                 diff_flush(&opt);
>                 return opt.flags.has_changes != 0;
>         } else {
>                 /* TODO: audit for interaction with sparse-index. */
>                 ensure_full_index(istate);
>                 for (i = 0; sb && i < istate->cache_nr; i++) {
>                         if (i)
>                                 strbuf_addch(sb, ' ');
>                         strbuf_addstr(sb, istate->cache[i]->name);
>                 }
>                 return !!istate->cache_nr;
>         }
>  }
> --
> 2.26.2

Patch makes sense; a quick `git grep -e repo_diff_setup -e
diff_setup_done` doesn't flag any other areas of the code as having
the same bug.

[PATCH 08/12] convert: release strbuf to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

apply_multi_file_filter and async_query_available_blobs both query
subprocess output using subprocess_read_status, which writes data into
the identically named filter_status strbuf. We add a strbuf_release to
avoid leaking their contents.

Leak output seen when running t0021 with LSAN:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c2b5 in xrealloc wrapper.c:126:8
    #2 0x9ff99d in strbuf_grow strbuf.c:98:2
    #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2
    #4 0xa101d6 in subprocess_read_status sub-process.c:45:5
    #5 0x77793c in apply_multi_file_filter convert.c:886:8
    #6 0x77793c in apply_filter convert.c:1042:10
    #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7
    #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2
    #9 0x8b48cd in index_fd object-file.c:2248:9
    #10 0x597411 in hash_fd builtin/hash-object.c:43:9
    #11 0x596be1 in hash_object builtin/hash-object.c:59:2
    #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3
    #13 0x4ce83e in run_builtin git.c:475:11
    #14 0x4ccafe in handle_builtin git.c:729:3
    #15 0x4cb01c in run_argv git.c:818:4
    #16 0x4cb01c in cmd_main git.c:949:19
    #17 0x6bdc2d in main common-main.c:52:11
    #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

Direct leak of 120 byte(s) in 5 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c295 in xrealloc wrapper.c:126:8
    #2 0x9ff97d in strbuf_grow strbuf.c:98:2
    #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2
    #4 0xa101b6 in subprocess_read_status sub-process.c:45:5
    #5 0x775c73 in async_query_available_blobs convert.c:960:8
    #6 0x80029d in finish_delayed_checkout entry.c:183:9
    #7 0xa65d1e in check_updates unpack-trees.c:493:10
    #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8
    #9 0x525971 in checkout builtin/clone.c:815:6
    #10 0x525971 in cmd_clone builtin/clone.c:1409:8
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6bdc2d in main common-main.c:52:11
    #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 convert.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/convert.c b/convert.c
index fd9c84b025..0d6fb3410a 100644
--- a/convert.c
+++ b/convert.c
@@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	else
 		strbuf_swap(dst, &nbuf);
 	strbuf_release(&nbuf);
+	strbuf_release(&filter_status);
 	return !err;
 }
 
@@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
 
 	if (err)
 		handle_filter_error(&filter_status, entry, 0);
+	strbuf_release(&filter_status);
 	return !err;
 }
 
-- 
2.26.2

Re: [PATCH 08/12] convert: release strbuf to avoid leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> apply_multi_file_filter and async_query_available_blobs both query
> subprocess output using subprocess_read_status, which writes data into
> the identically named filter_status strbuf. We add a strbuf_release to
> avoid leaking their contents.
>
> Leak output seen when running t0021 with LSAN:
>
> Direct leak of 24 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa8c2b5 in xrealloc wrapper.c:126:8
>     #2 0x9ff99d in strbuf_grow strbuf.c:98:2
>     #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2
>     #4 0xa101d6 in subprocess_read_status sub-process.c:45:5
>     #5 0x77793c in apply_multi_file_filter convert.c:886:8
>     #6 0x77793c in apply_filter convert.c:1042:10
>     #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7
>     #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2
>     #9 0x8b48cd in index_fd object-file.c:2248:9
>     #10 0x597411 in hash_fd builtin/hash-object.c:43:9
>     #11 0x596be1 in hash_object builtin/hash-object.c:59:2
>     #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3
>     #13 0x4ce83e in run_builtin git.c:475:11
>     #14 0x4ccafe in handle_builtin git.c:729:3
>     #15 0x4cb01c in run_argv git.c:818:4
>     #16 0x4cb01c in cmd_main git.c:949:19
>     #17 0x6bdc2d in main common-main.c:52:11
>     #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).
>
> Direct leak of 120 byte(s) in 5 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa8c295 in xrealloc wrapper.c:126:8
>     #2 0x9ff97d in strbuf_grow strbuf.c:98:2
>     #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2
>     #4 0xa101b6 in subprocess_read_status sub-process.c:45:5
>     #5 0x775c73 in async_query_available_blobs convert.c:960:8
>     #6 0x80029d in finish_delayed_checkout entry.c:183:9
>     #7 0xa65d1e in check_updates unpack-trees.c:493:10
>     #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8
>     #9 0x525971 in checkout builtin/clone.c:815:6
>     #10 0x525971 in cmd_clone builtin/clone.c:1409:8
>     #11 0x4ce83e in run_builtin git.c:475:11
>     #12 0x4ccafe in handle_builtin git.c:729:3
>     #13 0x4cb01c in run_argv git.c:818:4
>     #14 0x4cb01c in cmd_main git.c:949:19
>     #15 0x6bdc2d in main common-main.c:52:11
>     #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  convert.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/convert.c b/convert.c
> index fd9c84b025..0d6fb3410a 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>         else
>                 strbuf_swap(dst, &nbuf);
>         strbuf_release(&nbuf);
> +       strbuf_release(&filter_status);
>         return !err;
>  }
>
> @@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
>
>         if (err)
>                 handle_filter_error(&filter_status, entry, 0);
> +       strbuf_release(&filter_status);
>         return !err;
>  }
>
> --
> 2.26.2

Makes sense; a `git grep -e STRBUF_INIT -e strbuf_release -e
strbuf_init convert.c` does a fairly good job of highlighting that
these appear to be the only two strbuf leaks in this file.

[PATCH 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

These leaks all happen at the end of cmd_mv, hence don't matter in any
way. But we still fix the easy ones and squash the rest to get us closer
to being able to run tests without leaks.

LSAN output from t0050:

Direct leak of 384 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa0a7e1 in add_entry string-list.c:44:2
    #3 0xa0a7e1 in string_list_insert string-list.c:58:14
    #4 0x5dac03 in cmd_mv builtin/mv.c:248:4
    #5 0x4ce83e in run_builtin git.c:475:11
    #6 0x4ccafe in handle_builtin git.c:729:3
    #7 0x4cb01c in run_argv git.c:818:4
    #8 0x4cb01c in cmd_main git.c:949:19
    #9 0x6bd9ad in main common-main.c:52:11
    #10 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bd09 in do_xmalloc wrapper.c:41:8
    #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
    #3 0x5da575 in cmd_mv builtin/mv.c:158:14
    #4 0x4ce83e in run_builtin git.c:475:11
    #5 0x4ccafe in handle_builtin git.c:729:3
    #6 0x4cb01c in run_argv git.c:818:4
    #7 0x4cb01c in cmd_main git.c:949:19
    #8 0x6bd9ad in main common-main.c:52:11
    #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bd09 in do_xmalloc wrapper.c:41:8
    #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
    #3 0x5da4e4 in cmd_mv builtin/mv.c:148:11
    #4 0x4ce83e in run_builtin git.c:475:11
    #5 0x4ccafe in handle_builtin git.c:729:3
    #6 0x4cb01c in run_argv git.c:818:4
    #7 0x4cb01c in cmd_main git.c:949:19
    #8 0x6bd9ad in main common-main.c:52:11
    #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xa8c119 in xcalloc wrapper.c:140:8
    #2 0x5da585 in cmd_mv builtin/mv.c:159:22
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6bd9ad in main common-main.c:52:11
    #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xa8c119 in xcalloc wrapper.c:140:8
    #2 0x5da4f8 in cmd_mv builtin/mv.c:149:10
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6bd9ad in main common-main.c:52:11
    #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 65 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa00226 in strbuf_grow strbuf.c:98:2
    #3 0xa00226 in strbuf_vaddf strbuf.c:394:3
    #4 0xa065c7 in xstrvfmt strbuf.c:981:2
    #5 0xa065c7 in xstrfmt strbuf.c:991:8
    #6 0x9e7ce7 in prefix_path_gently setup.c:115:15
    #7 0x9e7fa6 in prefix_path setup.c:128:12
    #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
    #9 0x5da575 in cmd_mv builtin/mv.c:158:14
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6bd9ad in main common-main.c:52:11
    #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 65 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa00226 in strbuf_grow strbuf.c:98:2
    #3 0xa00226 in strbuf_vaddf strbuf.c:394:3
    #4 0xa065c7 in xstrvfmt strbuf.c:981:2
    #5 0xa065c7 in xstrfmt strbuf.c:991:8
    #6 0x9e7ce7 in prefix_path_gently setup.c:115:15
    #7 0x9e7fa6 in prefix_path setup.c:128:12
    #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
    #9 0x5da4e4 in cmd_mv builtin/mv.c:148:11
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6bd9ad in main common-main.c:52:11
    #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 558 byte(s) leaked in 7 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/mv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 3fccdcb645..c2f96c8e89 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -303,5 +303,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("Unable to write new index file"));
 
+	string_list_clear(&src_for_dst, 0);
+	UNLEAK(source);
+	UNLEAK(dest_path);
+	free(submodule_gitfile);
+	free(modes);
 	return 0;
 }
-- 
2.26.2

[PATCH 10/12] builtin/merge: free found_ref when done

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

merge_name() calls dwim_ref(), which allocates a new string into
found_ref. Therefore add a free() to avoid leaking found_ref.

LSAN output from t0021:

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa8beb8 in xstrdup wrapper.c:29:14
    #2 0x954054 in expand_ref refs.c:671:12
    #3 0x953cb6 in repo_dwim_ref refs.c:644:22
    #4 0x5d3759 in dwim_ref refs.h:162:9
    #5 0x5d3759 in merge_name builtin/merge.c:517:6
    #6 0x5d3759 in collect_parents builtin/merge.c:1214:5
    #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6bdbfd in main common-main.c:52:11
    #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/merge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..7ad85c044a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	struct strbuf bname = STRBUF_INIT;
 	struct merge_remote_desc *desc;
 	const char *ptr;
-	char *found_ref;
+	char *found_ref = NULL;
 	int len, early;
 
 	strbuf_branchname(&bname, remote, 0);
@@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	strbuf_addf(msg, "%s\t\tcommit '%s'\n",
 		oid_to_hex(&remote_head->object.oid), remote);
 cleanup:
+	free(found_ref);
 	strbuf_release(&buf);
 	strbuf_release(&bname);
 }
-- 
2.26.2

Re: [PATCH 10/12] builtin/merge: free found_ref when done

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> merge_name() calls dwim_ref(), which allocates a new string into
> found_ref. Therefore add a free() to avoid leaking found_ref.
>
> LSAN output from t0021:
>
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0xa8beb8 in xstrdup wrapper.c:29:14
>     #2 0x954054 in expand_ref refs.c:671:12
>     #3 0x953cb6 in repo_dwim_ref refs.c:644:22
>     #4 0x5d3759 in dwim_ref refs.h:162:9
>     #5 0x5d3759 in merge_name builtin/merge.c:517:6
>     #6 0x5d3759 in collect_parents builtin/merge.c:1214:5
>     #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16
>     #8 0x4ce83e in run_builtin git.c:475:11
>     #9 0x4ccafe in handle_builtin git.c:729:3
>     #10 0x4cb01c in run_argv git.c:818:4
>     #11 0x4cb01c in cmd_main git.c:949:19
>     #12 0x6bdbfd in main common-main.c:52:11
>     #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  builtin/merge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8a843b1f5..7ad85c044a 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
>         struct strbuf bname = STRBUF_INIT;
>         struct merge_remote_desc *desc;
>         const char *ptr;
> -       char *found_ref;
> +       char *found_ref = NULL;
>         int len, early;
>
>         strbuf_branchname(&bname, remote, 0);
> @@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
>         strbuf_addf(msg, "%s\t\tcommit '%s'\n",
>                 oid_to_hex(&remote_head->object.oid), remote);
>  cleanup:
> +       free(found_ref);
>         strbuf_release(&buf);
>         strbuf_release(&bname);
>  }
> --
> 2.26.2

Makes sense, and a quick grep through the code doesn't suggest any
other obvious leaks from using dwim_ref().

[PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

This change:
- xstrdup()'s all string being used for replace_opts.strategy, to
  guarantee that replace_opts owns these strings. This is needed because
  sequencer_remove_state() will free replace_opts.strategy, and it's
  usually called as part of the usage of replace_opts.
- Removes xstrdup()'s being used to populate options.strategy in
  cmd_rebase(), which avoids leaking options.strategy, even in the
  case where strategy is never moved/copied into replace_opts.

These changes are needed because:
- We would always create a new string for options.strategy if we either
  get a strategy via options (OPT_STRING(...strategy...), or via
  GIT_TEST_MERGE_ALGORITHM.
- But only sometimes is this string copied into replace_opts - in which
  case it did get free()'d in sequencer_remove_state().
- The rest of the time, the newly allocated string would remain unused,
  causing a leak. But we can't just add a free because that can result
  in a double-free in those cases where replace_opts was populated.

An alternative approach would be to set options.strategy to NULL when
moving the pointer to replace_opts.strategy, combined with always
free()'ing options.strategy, but that seems like a more
complicated and wasteful approach.

This was first seen when running t0021 with LSAN, but t2012 helped catch
the fact that we can't just free(options.strategy) at the end of
cmd_rebase (as that can cause a double-free). LSAN output from t0021:

LSAN output from t0021:

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71eb8 in xstrdup wrapper.c:29:14
    #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6b3fad in main common-main.c:52:11
    #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/rebase.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d..9d81db0f3a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	replay.ignore_date = opts->ignore_date;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
 	if (opts->strategy)
-		replay.strategy = opts->strategy;
+		replay.strategy = xstrdup_or_null(opts->strategy);
 	else if (!replay.strategy && replay.default_strategy) {
 		replay.strategy = replay.default_strategy;
 		replay.default_strategy = NULL;
@@ -1723,7 +1723,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if (options.strategy) {
-		options.strategy = xstrdup(options.strategy);
 		switch (options.type) {
 		case REBASE_APPLY:
 			die(_("--strategy requires --merge or --interactive"));
@@ -1776,7 +1775,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.type == REBASE_MERGE &&
 	    !options.strategy &&
 	    getenv("GIT_TEST_MERGE_ALGORITHM"))
-		options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
+		options.strategy = getenv("GIT_TEST_MERGE_ALGORITHM");
 
 	switch (options.type) {
 	case REBASE_MERGE:
-- 
2.26.2

Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[Phillip Wood]

Hi Andrzej

Thanks for working on removing memory leaks from git.

On 20/06/2021 16:12, andrzej@ahunt.org wrote:
> From: Andrzej Hunt <ajrhunt@google.com>
> 
> This change:
> - xstrdup()'s all string being used for replace_opts.strategy, to

I think you mean replay_opts rather than replace_opts.

>    guarantee that replace_opts owns these strings. This is needed because
>    sequencer_remove_state() will free replace_opts.strategy, and it's
>    usually called as part of the usage of replace_opts.
> - Removes xstrdup()'s being used to populate options.strategy in
>    cmd_rebase(), which avoids leaking options.strategy, even in the
>    case where strategy is never moved/copied into replace_opts.


> These changes are needed because:
> - We would always create a new string for options.strategy if we either
>    get a strategy via options (OPT_STRING(...strategy...), or via
>    GIT_TEST_MERGE_ALGORITHM.
> - But only sometimes is this string copied into replace_opts - in which
>    case it did get free()'d in sequencer_remove_state().
> - The rest of the time, the newly allocated string would remain unused,
>    causing a leak. But we can't just add a free because that can result
>    in a double-free in those cases where replace_opts was populated.
> 
> An alternative approach would be to set options.strategy to NULL when
> moving the pointer to replace_opts.strategy, combined with always
> free()'ing options.strategy, but that seems like a more
> complicated and wasteful approach.

read_basic_state() contains
	if (file_exists(state_dir_path("strategy", opts))) {
		strbuf_reset(&buf);
		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
				   READ_ONELINER_WARN_MISSING))
			return -1;
		free(opts->strategy);
		opts->strategy = xstrdup(buf.buf);
	}

So we do try to free opts->strategy when reading the state from disc and 
we allocate a new string. I suspect that opts->strategy is actually NULL 
in when this function is called but I haven't checked. Given that we are 
allocating a copy above I think maybe your alternative approach of 
always freeing opts->strategy would be better.

Best Wishes

Phillip

> This was first seen when running t0021 with LSAN, but t2012 helped catch
> the fact that we can't just free(options.strategy) at the end of
> cmd_rebase (as that can cause a double-free). LSAN output from t0021:
> 
> LSAN output from t0021:
> 
> Direct leak of 4 byte(s) in 1 object(s) allocated from:
>      #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>      #1 0xa71eb8 in xstrdup wrapper.c:29:14
>      #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22
>      #3 0x4ce83e in run_builtin git.c:475:11
>      #4 0x4ccafe in handle_builtin git.c:729:3
>      #5 0x4cb01c in run_argv git.c:818:4
>      #6 0x4cb01c in cmd_main git.c:949:19
>      #7 0x6b3fad in main common-main.c:52:11
>      #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)
> 
> SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).
> 
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>   builtin/rebase.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 12f093121d..9d81db0f3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   	replay.ignore_date = opts->ignore_date;
>   	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>   	if (opts->strategy)
> -		replay.strategy = opts->strategy;
> +		replay.strategy = xstrdup_or_null(opts->strategy);
>   	else if (!replay.strategy && replay.default_strategy) {
>   		replay.strategy = replay.default_strategy;
>   		replay.default_strategy = NULL;
> @@ -1723,7 +1723,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	}
>   
>   	if (options.strategy) {
> -		options.strategy = xstrdup(options.strategy);
>   		switch (options.type) {
>   		case REBASE_APPLY:
>   			die(_("--strategy requires --merge or --interactive"));
> @@ -1776,7 +1775,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.type == REBASE_MERGE &&
>   	    !options.strategy &&
>   	    getenv("GIT_TEST_MERGE_ALGORITHM"))
> -		options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
> +		options.strategy = getenv("GIT_TEST_MERGE_ALGORITHM");
>   
>   	switch (options.type) {
>   	case REBASE_MERGE:
> 

Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[Elijah Newren]

On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Andrzej
>
> Thanks for working on removing memory leaks from git.
>
> On 20/06/2021 16:12, andrzej@ahunt.org wrote:
> > From: Andrzej Hunt <ajrhunt@google.com>
> >
> > This change:
> > - xstrdup()'s all string being used for replace_opts.strategy, to
>
> I think you mean replay_opts rather than replace_opts.
>
> >    guarantee that replace_opts owns these strings. This is needed because
> >    sequencer_remove_state() will free replace_opts.strategy, and it's
> >    usually called as part of the usage of replace_opts.
> > - Removes xstrdup()'s being used to populate options.strategy in
> >    cmd_rebase(), which avoids leaking options.strategy, even in the
> >    case where strategy is never moved/copied into replace_opts.
>
>
> > These changes are needed because:
> > - We would always create a new string for options.strategy if we either
> >    get a strategy via options (OPT_STRING(...strategy...), or via
> >    GIT_TEST_MERGE_ALGORITHM.
> > - But only sometimes is this string copied into replace_opts - in which
> >    case it did get free()'d in sequencer_remove_state().
> > - The rest of the time, the newly allocated string would remain unused,
> >    causing a leak. But we can't just add a free because that can result
> >    in a double-free in those cases where replace_opts was populated.
> >
> > An alternative approach would be to set options.strategy to NULL when
> > moving the pointer to replace_opts.strategy, combined with always
> > free()'ing options.strategy, but that seems like a more
> > complicated and wasteful approach.
>
> read_basic_state() contains
>         if (file_exists(state_dir_path("strategy", opts))) {
>                 strbuf_reset(&buf);
>                 if (!read_oneliner(&buf, state_dir_path("strategy", opts),
>                                    READ_ONELINER_WARN_MISSING))
>                         return -1;
>                 free(opts->strategy);
>                 opts->strategy = xstrdup(buf.buf);
>         }
>
> So we do try to free opts->strategy when reading the state from disc and
> we allocate a new string. I suspect that opts->strategy is actually NULL
> in when this function is called but I haven't checked. Given that we are
> allocating a copy above I think maybe your alternative approach of
> always freeing opts->strategy would be better.

Good catches.  sequencer_remove_state() in sequencer.c also has a
free(opts->strategy) call.

To make things even more muddy, we have code like
    replay.strategy = replay.default_strategy;
or
    opts->strategy = opts->default_strategy;
which both will probably work really poorly with the calls to
    free(opts->default_strategy);
    free(opts->strategy);
from sequencer_remove_state().  I suspect we've got a few bugs here...

Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[Phillip Wood]

Hi Elijah

On 21/06/2021 22:39, Elijah Newren wrote:
> On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Andrzej
>>
>> Thanks for working on removing memory leaks from git.
>>
>> On 20/06/2021 16:12, andrzej@ahunt.org wrote:
>>> From: Andrzej Hunt <ajrhunt@google.com>
>>>
>>> This change:
>>> - xstrdup()'s all string being used for replace_opts.strategy, to
>>
>> I think you mean replay_opts rather than replace_opts.
>>
>>>     guarantee that replace_opts owns these strings. This is needed because
>>>     sequencer_remove_state() will free replace_opts.strategy, and it's
>>>     usually called as part of the usage of replace_opts.
>>> - Removes xstrdup()'s being used to populate options.strategy in
>>>     cmd_rebase(), which avoids leaking options.strategy, even in the
>>>     case where strategy is never moved/copied into replace_opts.
>>
>>
>>> These changes are needed because:
>>> - We would always create a new string for options.strategy if we either
>>>     get a strategy via options (OPT_STRING(...strategy...), or via
>>>     GIT_TEST_MERGE_ALGORITHM.
>>> - But only sometimes is this string copied into replace_opts - in which
>>>     case it did get free()'d in sequencer_remove_state().
>>> - The rest of the time, the newly allocated string would remain unused,
>>>     causing a leak. But we can't just add a free because that can result
>>>     in a double-free in those cases where replace_opts was populated.
>>>
>>> An alternative approach would be to set options.strategy to NULL when
>>> moving the pointer to replace_opts.strategy, combined with always
>>> free()'ing options.strategy, but that seems like a more
>>> complicated and wasteful approach.
>>
>> read_basic_state() contains
>>          if (file_exists(state_dir_path("strategy", opts))) {
>>                  strbuf_reset(&buf);
>>                  if (!read_oneliner(&buf, state_dir_path("strategy", opts),
>>                                     READ_ONELINER_WARN_MISSING))
>>                          return -1;
>>                  free(opts->strategy);
>>                  opts->strategy = xstrdup(buf.buf);
>>          }
>>
>> So we do try to free opts->strategy when reading the state from disc and
>> we allocate a new string. I suspect that opts->strategy is actually NULL
>> in when this function is called but I haven't checked. Given that we are
>> allocating a copy above I think maybe your alternative approach of
>> always freeing opts->strategy would be better.
> 
> Good catches.  sequencer_remove_state() in sequencer.c also has a
> free(opts->strategy) call.
> 
> To make things even more muddy, we have code like
>      replay.strategy = replay.default_strategy;
> or
>      opts->strategy = opts->default_strategy;
> which both will probably work really poorly with the calls to
>      free(opts->default_strategy);
>      free(opts->strategy);
> from sequencer_remove_state().  I suspect we've got a few bugs here...

It's not immediately obvious but I think those are actually safe. 
opts->default_strategy is allocated by sequencer_init_config() so it is 
correct to free it and when we assign it in rebase.c we do

	else if (!replay.strategy && replay.default_strategy) {
		replay.strategy = replay.default_strategy;
		replay.default_strategy = NULL;
	}

so there is no double free. There is similar code in builtin/revert.c 
which I think is where your other example came from. I think there is a 
leak in builtin/revert.c though

	if (!opts->strategy && opts->default_strategy) {
		opts->strategy = opts->default_strategy;
		opts->default_strategy = NULL;
	}

	/* do some other stuff */

	/* These option values will be free()d */
	opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
	opts->strategy = xstrdup_or_null(opts->strategy);

So we copy the default strategy, leaking the original copy from 
sequencer_init_options() if --strategy isn't given on the command line. 
I think it would be simple to fix this by making the copy earlier.

	if (!opts->strategy && opts->default_strategy) {
		opts->strategy = opts->default_strategy;
		opts->default_strategy = NULL;
	} else if (opts->strategy) {
	/* This option will be free()d in sequencer_remove_state() */
		opts->strategy = xstrdup(opts->strategy);
	}

I'm going offline for a week or so in a couple of days but I'll have 
look at making a proper patch when I get back.

Best Wishes

Phillip

Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[Andrzej Hunt]

On 22/06/2021 11:02, Phillip Wood wrote:
> Hi Elijah
> 
> On 21/06/2021 22:39, Elijah Newren wrote:
>> On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood 
>> <phillip.wood123@gmail.com> wrote:
>>>
>>> Hi Andrzej
>>>
>>> Thanks for working on removing memory leaks from git.
>>>
>>> On 20/06/2021 16:12, andrzej@ahunt.org wrote:
>>>> From: Andrzej Hunt <ajrhunt@google.com>
>>>>
>>>> This change:
>>>> - xstrdup()'s all string being used for replace_opts.strategy, to
>>>
>>> I think you mean replay_opts rather than replace_opts.
>>>
>>>>     guarantee that replace_opts owns these strings. This is needed 
>>>> because
>>>>     sequencer_remove_state() will free replace_opts.strategy, and it's
>>>>     usually called as part of the usage of replace_opts.
>>>> - Removes xstrdup()'s being used to populate options.strategy in
>>>>     cmd_rebase(), which avoids leaking options.strategy, even in the
>>>>     case where strategy is never moved/copied into replace_opts.
>>>
>>>
>>>> These changes are needed because:
>>>> - We would always create a new string for options.strategy if we either
>>>>     get a strategy via options (OPT_STRING(...strategy...), or via
>>>>     GIT_TEST_MERGE_ALGORITHM.
>>>> - But only sometimes is this string copied into replace_opts - in which
>>>>     case it did get free()'d in sequencer_remove_state().
>>>> - The rest of the time, the newly allocated string would remain unused,
>>>>     causing a leak. But we can't just add a free because that can 
>>>> result
>>>>     in a double-free in those cases where replace_opts was populated.
>>>>
>>>> An alternative approach would be to set options.strategy to NULL when
>>>> moving the pointer to replace_opts.strategy, combined with always
>>>> free()'ing options.strategy, but that seems like a more
>>>> complicated and wasteful approach.
>>>
>>> read_basic_state() contains
>>>          if (file_exists(state_dir_path("strategy", opts))) {
>>>                  strbuf_reset(&buf);
>>>                  if (!read_oneliner(&buf, state_dir_path("strategy", 
>>> opts),
>>>                                     READ_ONELINER_WARN_MISSING))
>>>                          return -1;
>>>                  free(opts->strategy);
>>>                  opts->strategy = xstrdup(buf.buf);
>>>          }
>>>
>>> So we do try to free opts->strategy when reading the state from disc and
>>> we allocate a new string. I suspect that opts->strategy is actually NULL
>>> in when this function is called but I haven't checked. 

Thank you for noticing this. I think you're right - running an ASAN 
build past the whole test suite also didn't catch any double-frees which 
mostly confirms that opts->strategy is indeed always NULL here. But 
that's not a good reason for taking the risk.

>>> Given that we are
>>> allocating a copy above I think maybe your alternative approach of
>>> always freeing opts->strategy would be better.

I will go down this route for V2. Although on further thought: instead 
of my original idea of moving the string to replay_opts (and NULL'ing 
out rebase_options->strategy), I think it's better to create a new copy 
when populating replay_opts. The move/NULL approach I suggested in V1 
happens to work OK, but I think it's non-obvious and could break if we 
ever wanted to use get_replay_opts() more than once - creating separate 
copies reduces the number of surprises.

>>
>> Good catches.  sequencer_remove_state() in sequencer.c also has a
>> free(opts->strategy) call.
>>
>> To make things even more muddy, we have code like
>>      replay.strategy = replay.default_strategy;
>> or
>>      opts->strategy = opts->default_strategy;
>> which both will probably work really poorly with the calls to
>>      free(opts->default_strategy);
>>      free(opts->strategy);
>> from sequencer_remove_state().  I suspect we've got a few bugs here...
> 
> It's not immediately obvious but I think those are actually safe. 
> opts->default_strategy is allocated by sequencer_init_config() so it is 
> correct to free it and when we assign it in rebase.c we do
> 
>      else if (!replay.strategy && replay.default_strategy) {
>          replay.strategy = replay.default_strategy;
>          replay.default_strategy = NULL;
>      }
> 
> so there is no double free.

As mentioned above, ASAN isn't catching any double-frees here (but I 
guess that depends on whether or not you trust the test suite to be 
reasonably testing all permutations).

But it's still good to take note of sequencer_remove_state() free'ing 
opts->strategy, because I almost did manage to add a double free when I 
added a free(options.strategy) to cmd_rebase without also xstrdup'ing 
strategy in get_replay_opts().

> There is similar code in builtin/revert.c 
> which I think is where your other example came from. I think there is a 
> leak in builtin/revert.c though
> 
>      if (!opts->strategy && opts->default_strategy) {
>          opts->strategy = opts->default_strategy;
>          opts->default_strategy = NULL;
>      }
> 
>      /* do some other stuff */
> 
>      /* These option values will be free()d */
>      opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
>      opts->strategy = xstrdup_or_null(opts->strategy);
> 
> So we copy the default strategy, leaking the original copy from 
> sequencer_init_options() if --strategy isn't given on the command line. 
> I think it would be simple to fix this by making the copy earlier.
> 
>      if (!opts->strategy && opts->default_strategy) {
>          opts->strategy = opts->default_strategy;
>          opts->default_strategy = NULL;
>      } else if (opts->strategy) {
>      /* This option will be free()d in sequencer_remove_state() */
>          opts->strategy = xstrdup(opts->strategy);
>      }
> 

Nice find. I'm noticing a lot of interesting leaks in git's options 
handling, and those leaks also tend to be the trickiest ones to fix (as 
my blunder in the original version of this patch demonstrates :) ).

ATB,

   Andrzej

Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

[Phillip Wood]

Hi Andrzej

On 25/07/2021 14:03, Andrzej Hunt wrote:
> [...]
>>>> Given that we are
>>>> allocating a copy above I think maybe your alternative approach of
>>>> always freeing opts->strategy would be better.
> 
> I will go down this route for V2. Although on further thought: instead 
> of my original idea of moving the string to replay_opts (and NULL'ing 
> out rebase_options->strategy), I think it's better to create a new copy 
> when populating replay_opts. The move/NULL approach I suggested in V1 
> happens to work OK, but I think it's non-obvious and could break if we 
> ever wanted to use get_replay_opts() more than once - creating separate 
> copies reduces the number of surprises.

Copying the string sounds like a good approach. I've looked at the V2 
patch and it looks fine to me.

Thanks

Phillip

[PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

setup_unpack_trees_porcelain() populates various fields on
unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to
avoid leaking them. Specifically, we used to leak
unpack_tree_opts.msgs_to_free.

We have to do this in leave_reset_head because there are multiple
scenarios where unpack_tree_opts has already been configured, followed
by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'
prior to having initialised unpack_tree_opts via memset(..., 0, ...).
Therefore we also move unpack_tree_opts initialisation to the start of
reset_head(), and convert it to use brace initialisation - which
guarantees that we can never clear an unitialised unpack_tree_opts.
clear_unpack_tree_opts() is always safe to call as long as
unpack_tree_opts is at least zero-initialised, i.e. it does not depend
on a previous call to setup_unpack_trees_porcelain().

LSAN output from t0021:

Direct leak of 192 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9f7861 in strvec_push_nodup strvec.c:19:2
    #3 0x9f7861 in strvec_pushf strvec.c:39:2
    #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
    #5 0x97e011 in reset_head reset.c:53:2
    #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #7 0x4ce83e in run_builtin git.c:475:11
    #8 0x4ccafe in handle_builtin git.c:729:3
    #9 0x4cb01c in run_argv git.c:818:4
    #10 0x4cb01c in cmd_main git.c:949:19
    #11 0x6b3f3d in main common-main.c:52:11
    #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 147 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 134 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 130 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 reset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/reset.c b/reset.c
index 4bea758053..79310ae071 100644
--- a/reset.c
+++ b/reset.c
@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;
-	struct unpack_trees_options unpack_tree_opts;
+	struct unpack_trees_options unpack_tree_opts = { 0 };
 	struct tree *tree;
 	const char *reflog_action;
 	struct strbuf msg = STRBUF_INIT;
@@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	if (refs_only)
 		goto reset_head_refs;
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;
@@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 leave_reset_head:
 	strbuf_release(&msg);
 	rollback_lock_file(&lock);
+	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	while (nr)
 		free((void *)desc[--nr].buffer);
 	return ret;
-- 
2.26.2

Re: [PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> setup_unpack_trees_porcelain() populates various fields on
> unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to
> avoid leaking them. Specifically, we used to leak
> unpack_tree_opts.msgs_to_free.
>
> We have to do this in leave_reset_head because there are multiple
> scenarios where unpack_tree_opts has already been configured, followed
> by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'
> prior to having initialised unpack_tree_opts via memset(..., 0, ...).
> Therefore we also move unpack_tree_opts initialisation to the start of
> reset_head(), and convert it to use brace initialisation - which
> guarantees that we can never clear an unitialised unpack_tree_opts.

I think you mean either "uninitialized" or "uninitialised" (missing an
'in' in the spelling)

> clear_unpack_tree_opts() is always safe to call as long as
> unpack_tree_opts is at least zero-initialised, i.e. it does not depend
> on a previous call to setup_unpack_trees_porcelain().
>
> LSAN output from t0021:
>
> Direct leak of 192 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa721e5 in xrealloc wrapper.c:126:8
>     #2 0x9f7861 in strvec_push_nodup strvec.c:19:2
>     #3 0x9f7861 in strvec_pushf strvec.c:39:2
>     #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
>     #5 0x97e011 in reset_head reset.c:53:2
>     #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
>     #7 0x4ce83e in run_builtin git.c:475:11
>     #8 0x4ccafe in handle_builtin git.c:729:3
>     #9 0x4cb01c in run_argv git.c:818:4
>     #10 0x4cb01c in cmd_main git.c:949:19
>     #11 0x6b3f3d in main common-main.c:52:11
>     #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Indirect leak of 147 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa721e5 in xrealloc wrapper.c:126:8
>     #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
>     #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
>     #4 0x9f7774 in strvec_pushf strvec.c:36:2
>     #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
>     #6 0x97e011 in reset_head reset.c:53:2
>     #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
>     #8 0x4ce83e in run_builtin git.c:475:11
>     #9 0x4ccafe in handle_builtin git.c:729:3
>     #10 0x4cb01c in run_argv git.c:818:4
>     #11 0x4cb01c in cmd_main git.c:949:19
>     #12 0x6b3f3d in main common-main.c:52:11
>     #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Indirect leak of 134 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa721e5 in xrealloc wrapper.c:126:8
>     #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
>     #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
>     #4 0x9f7774 in strvec_pushf strvec.c:36:2
>     #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3
>     #6 0x97e011 in reset_head reset.c:53:2
>     #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
>     #8 0x4ce83e in run_builtin git.c:475:11
>     #9 0x4ccafe in handle_builtin git.c:729:3
>     #10 0x4cb01c in run_argv git.c:818:4
>     #11 0x4cb01c in cmd_main git.c:949:19
>     #12 0x6b3f3d in main common-main.c:52:11
>     #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Indirect leak of 130 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa721e5 in xrealloc wrapper.c:126:8
>     #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
>     #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
>     #4 0x9f7774 in strvec_pushf strvec.c:36:2
>     #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3
>     #6 0x97e011 in reset_head reset.c:53:2
>     #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
>     #8 0x4ce83e in run_builtin git.c:475:11
>     #9 0x4ccafe in handle_builtin git.c:729:3
>     #10 0x4cb01c in run_argv git.c:818:4
>     #11 0x4cb01c in cmd_main git.c:949:19
>     #12 0x6b3f3d in main common-main.c:52:11
>     #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  reset.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/reset.c b/reset.c
> index 4bea758053..79310ae071 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>         struct object_id head_oid;
>         struct tree_desc desc[2] = { { NULL }, { NULL } };
>         struct lock_file lock = LOCK_INIT;
> -       struct unpack_trees_options unpack_tree_opts;
> +       struct unpack_trees_options unpack_tree_opts = { 0 };
>         struct tree *tree;
>         const char *reflog_action;
>         struct strbuf msg = STRBUF_INIT;
> @@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>         if (refs_only)
>                 goto reset_head_refs;
>
> -       memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
>         setup_unpack_trees_porcelain(&unpack_tree_opts, action);
>         unpack_tree_opts.head_idx = 1;
>         unpack_tree_opts.src_index = r->index;
> @@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>  leave_reset_head:
>         strbuf_release(&msg);
>         rollback_lock_file(&lock);
> +       clear_unpack_trees_porcelain(&unpack_tree_opts);
>         while (nr)
>                 free((void *)desc[--nr].buffer);
>         return ret;
> --
> 2.26.2

Nice catch, and nice explanation.  I think we probably have several
similar problems throughout the code base; a quick grep (`git grep -e
struct.unpack_trees_options -e clear_unpack_trees_porcelain`) suggests
there are several places that clear_unpack_trees_porcelain() is
probably missing and which could likely use your struct initialization
trick as well.

Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2

[Elijah Newren]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <andrzej@ahunt.org>
>
> This series plugs more of the leaks that were found while running
> t0002-t0099 with LSAN.
>
> See also the first series (already merged) at [1]. I'm currently
> expecting at least another 2 series before t0002-t0099 run leak free.
> I'm not being particularly systematic about the order of patches -
> although I am trying to send out "real" (if mostly small) leaks first,
> before sending out the more boring patches that add free()/UNLEAK() to
> cmd_* and direct helpers thereof.

I've read over the series.  It provides some good clear fixes.  I
noted on patches 2, 6, and 12 that a some greps suggested that leaks
similar to the ones being fixed likely also affect other places of the
codebase.  Those other places don't need to be fixed as part of this
series, but they might be good items for #leftoverbits or GSoC early
tasks (cc: Christian in case he wants to record those somewhere).

I cc'ed Stolee on patch 4 because he suggested he wanted to read it in
an earlier discussion.

Phillip noted some issues with patch 11, and I added a couple more.
The ownership of opts->strategy appears to be pretty messy and in need
of cleanup.

All the patches other than 11 look good to me.

Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2

[Andrzej Hunt]

On 21/06/2021 23:54, Elijah Newren wrote:
> On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>>
>> From: Andrzej Hunt <andrzej@ahunt.org>
>>
>> This series plugs more of the leaks that were found while running
>> t0002-t0099 with LSAN.
>>
>> See also the first series (already merged) at [1]. I'm currently
>> expecting at least another 2 series before t0002-t0099 run leak free.
>> I'm not being particularly systematic about the order of patches -
>> although I am trying to send out "real" (if mostly small) leaks first,
>> before sending out the more boring patches that add free()/UNLEAK() to
>> cmd_* and direct helpers thereof.
> 
> I've read over the series.  It provides some good clear fixes.  I
> noted on patches 2, 6, and 12 that a some greps suggested that leaks
> similar to the ones being fixed likely also affect other places of the
> codebase.  Those other places don't need to be fixed as part of this
> series, but they might be good items for #leftoverbits or GSoC early
> tasks (cc: Christian in case he wants to record those somewhere).
> 
> I cc'ed Stolee on patch 4 because he suggested he wanted to read it in
> an earlier discussion.
> 
> Phillip noted some issues with patch 11, and I added a couple more.
> The ownership of opts->strategy appears to be pretty messy and in need
> of cleanup.
> 
> All the patches other than 11 look good to me.
> 

Thank you for the careful reviews - and especially for pointing out when 
a given pattern does occur elsewhere in the codebase!

As suggested I will skip the additional locations that you've found 
while reviewing this series - but I'm starting a separate series where I 
can address those. I have been focused on getting tests to pass 
leak-free one-by-one, but spotting and fixing patterns is probably more 
efficient (since author and reviewer already have the right context) and 
in some cases might fix leaks that aren't occurring during the tests.

ATB,

   Andrzej

Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2

[Christian Couder]

On Mon, Jun 21, 2021 at 11:54 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
> >
> > From: Andrzej Hunt <andrzej@ahunt.org>
> >
> > This series plugs more of the leaks that were found while running
> > t0002-t0099 with LSAN.
> >
> > See also the first series (already merged) at [1]. I'm currently
> > expecting at least another 2 series before t0002-t0099 run leak free.
> > I'm not being particularly systematic about the order of patches -
> > although I am trying to send out "real" (if mostly small) leaks first,
> > before sending out the more boring patches that add free()/UNLEAK() to
> > cmd_* and direct helpers thereof.
>
> I've read over the series.  It provides some good clear fixes.  I
> noted on patches 2, 6, and 12 that a some greps suggested that leaks
> similar to the ones being fixed likely also affect other places of the
> codebase.  Those other places don't need to be fixed as part of this
> series, but they might be good items for #leftoverbits or GSoC early
> tasks (cc: Christian in case he wants to record those somewhere).

Yeah, thanks for letting me know!

[PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 2

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

V2 fixes patch 11/12 (rebase_options.strategy lifecycle) as per review
discussion. Many thanks to Phillip and Elijah for spotting the issues there!

ATB,

  Andrzej

Andrzej Hunt (12):
  fmt-merge-msg: free newly allocated temporary strings when done
  environment: move strbuf into block to plug leak
  builtin/submodule--helper: release unused strbuf to avoid leak
  builtin/for-each-repo: remove unnecessary argv copy to plug leak
  diffcore-rename: move old_dir/new_dir definition to plug leak
  ref-filter: also free head for ATOM_HEAD to avoid leak
  read-cache: call diff_setup_done to avoid leak
  convert: release strbuf to avoid leak
  builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv
  builtin/merge: free found_ref when done
  builtin/rebase: fix options.strategy memory lifecycle
  reset: clear_unpack_trees_porcelain to plug leak

 builtin/for-each-repo.c     | 14 ++++----------
 builtin/merge.c             |  3 ++-
 builtin/mv.c                |  5 +++++
 builtin/rebase.c            |  3 ++-
 builtin/submodule--helper.c |  6 ++++--
 convert.c                   |  2 ++
 diffcore-rename.c           | 10 +++++++---
 environment.c               |  7 +++----
 fmt-merge-msg.c             |  6 ++++--
 read-cache.c                |  1 +
 ref-filter.c                |  8 ++++++--
 reset.c                     |  4 ++--
 12 files changed, 42 insertions(+), 27 deletions(-)

Interdiff against v1:
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9d81db0f3a..33e0961900 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1723,6 +1723,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if (options.strategy) {
+		options.strategy = xstrdup(options.strategy);
 		switch (options.type) {
 		case REBASE_APPLY:
 			die(_("--strategy requires --merge or --interactive"));
@@ -1775,7 +1776,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.type == REBASE_MERGE &&
 	    !options.strategy &&
 	    getenv("GIT_TEST_MERGE_ALGORITHM"))
-		options.strategy = getenv("GIT_TEST_MERGE_ALGORITHM");
+		options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
 
 	switch (options.type) {
 	case REBASE_MERGE:
@@ -2108,6 +2109,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.head_name);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
+	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	return ret;
-- 
2.26.2

[PATCH v2 01/12] fmt-merge-msg: free newly allocated temporary strings when done

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

origin starts off pointing to somewhere within line, which is owned by
the caller. Later we might allocate a new string using xmemdupz() or
xstrfmt(). To avoid leaking these new strings, we introduce a to_free
pointer - which allows us to safely free the newly allocated string when
we're done (we cannot just free origin directly as it might still be
pointing to line).

LSAN output from t0090:

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa71f49 in do_xmalloc wrapper.c:41:8
    #2 0xa720b0 in do_xmallocz wrapper.c:75:8
    #3 0xa720b0 in xmallocz wrapper.c:83:9
    #4 0xa720b0 in xmemdupz wrapper.c:99:16
    #5 0x8092ba in handle_line fmt-merge-msg.c:187:23
    #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7
    #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2
    #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3
    #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6b3fad in main common-main.c:52:11
    #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 fmt-merge-msg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 0f66818e0f..b969dc6ebb 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -108,6 +108,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 	struct origin_data *origin_data;
 	char *src;
 	const char *origin, *tag_name;
+	char *to_free = NULL;
 	struct src_data *src_data;
 	struct string_list_item *item;
 	int pulling_head = 0;
@@ -183,12 +184,13 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 	if (!strcmp(".", src) || !strcmp(src, origin)) {
 		int len = strlen(origin);
 		if (origin[0] == '\'' && origin[len - 1] == '\'')
-			origin = xmemdupz(origin + 1, len - 2);
+			origin = to_free = xmemdupz(origin + 1, len - 2);
 	} else
-		origin = xstrfmt("%s of %s", origin, src);
+		origin = to_free = xstrfmt("%s of %s", origin, src);
 	if (strcmp(".", src))
 		origin_data->is_local_branch = 0;
 	string_list_append(&origins, origin)->util = origin_data;
+	free(to_free);
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH v2 01/12] fmt-merge-msg: free newly allocated temporary strings when done

[Junio C Hamano]

andrzej@ahunt.org writes:

> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index 0f66818e0f..b969dc6ebb 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -108,6 +108,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
>  	struct origin_data *origin_data;
>  	char *src;
>  	const char *origin, *tag_name;
> +	char *to_free = NULL;
>  	struct src_data *src_data;
>  	struct string_list_item *item;
>  	int pulling_head = 0;
> @@ -183,12 +184,13 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
>  	if (!strcmp(".", src) || !strcmp(src, origin)) {
>  		int len = strlen(origin);
>  		if (origin[0] == '\'' && origin[len - 1] == '\'')
> -			origin = xmemdupz(origin + 1, len - 2);
> +			origin = to_free = xmemdupz(origin + 1, len - 2);
>  	} else
> -		origin = xstrfmt("%s of %s", origin, src);
> +		origin = to_free = xstrfmt("%s of %s", origin, src);
>  	if (strcmp(".", src))
>  		origin_data->is_local_branch = 0;
>  	string_list_append(&origins, origin)->util = origin_data;
> +	free(to_free);
>  	return 0;
>  }

Thanks; obviously correct.

[PATCH v2 02/12] environment: move strbuf into block to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

realpath is only populated if we execute the git_work_tree_initialized
block. However that block also causes us to return early, meaning we
never actually release the strbuf in the case where we populated it.
Therefore we move all strbuf related code into the block to guarantee
that we can't leak it.

LSAN output from t0095:

Direct leak of 129 byte(s) in 1 object(s) allocated from:
    #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x78f585 in xrealloc wrapper.c:126:8
    #2 0x713ff4 in strbuf_grow strbuf.c:98:2
    #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3
    #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7
    #5 0x5ae4a4 in set_git_work_tree environment.c:259:3
    #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2
    #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12
    #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2
    #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3
    #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11
    #11 0x4caded in main common-main.c:52:11
    #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).

It looks like this leak has existed since realpath was first added to
set_git_work_tree() in:
  3d7747e318 (real_path: remove unsafe API, 2020-03-10)

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 environment.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/environment.c b/environment.c
index 2f27008424..d6b22ede7e 100644
--- a/environment.c
+++ b/environment.c
@@ -253,21 +253,20 @@ static int git_work_tree_initialized;
  */
 void set_git_work_tree(const char *new_work_tree)
 {
-	struct strbuf realpath = STRBUF_INIT;
-
 	if (git_work_tree_initialized) {
+		struct strbuf realpath = STRBUF_INIT;
+
 		strbuf_realpath(&realpath, new_work_tree, 1);
 		new_work_tree = realpath.buf;
 		if (strcmp(new_work_tree, the_repository->worktree))
 			die("internal error: work tree has already been set\n"
 			    "Current worktree: %s\nNew worktree: %s",
 			    the_repository->worktree, new_work_tree);
+		strbuf_release(&realpath);
 		return;
 	}
 	git_work_tree_initialized = 1;
 	repo_set_worktree(the_repository, new_work_tree);
-
-	strbuf_release(&realpath);
 }
 
 const char *get_git_work_tree(void)
-- 
2.26.2

[PATCH v2 03/12] builtin/submodule--helper: release unused strbuf to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

relative_url() populates sb. In the normal return path, its buffer is
detached using strbuf_detach(). However the early return path does
nothing with sb, which means that sb's memory is leaked - therefore
we add a release to avoid this leak.

The reset is also only necessary for the normal return path, hence we
move it down to after the early-return to avoid unnecessary work.

LSAN output from t0060:

Direct leak of 121 byte(s) in 1 object(s) allocated from:
    #0 0x7f31246f28b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
    #1 0x98d7d6 in xrealloc wrapper.c:126
    #2 0x909a60 in strbuf_grow strbuf.c:98
    #3 0x90bf00 in strbuf_vaddf strbuf.c:401
    #4 0x90c321 in strbuf_addf strbuf.c:335
    #5 0x5cb78d in relative_url builtin/submodule--helper.c:182
    #6 0x5cbe46 in resolve_relative_url_test builtin/submodule--helper.c:248
    #7 0x410dcd in run_builtin git.c:475
    #8 0x410dcd in handle_builtin git.c:729
    #9 0x414087 in run_argv git.c:818
    #10 0x414087 in cmd_main git.c:949
    #11 0x40e9ec in main common-main.c:52
    #12 0x7f3123c41349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 121 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/submodule--helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 414fcb63ea..528a78eed8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -187,11 +187,13 @@ static char *relative_url(const char *remote_url,
 		out = xstrdup(sb.buf + 2);
 	else
 		out = xstrdup(sb.buf);
-	strbuf_reset(&sb);
 
-	if (!up_path || !is_relative)
+	if (!up_path || !is_relative) {
+		strbuf_release(&sb);
 		return out;
+	}
 
+	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s%s", up_path, out);
 	free(out);
 	return strbuf_detach(&sb, NULL);
-- 
2.26.2

[PATCH v2 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

cmd_for_each_repo() copies argv into args (a strvec), which is later
passed into run_command_on_repo(), which in turn copies that strvec onto
the end of child.args. The initial copy is unnecessary (we never modify
args). We therefore choose to just pass argv directly into
run_command_on_repo(), which lets us avoid the copy and fixes the leak.

LSAN output from t0068:

Direct leak of 192 byte(s) in 1 object(s) allocated from:
    #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
    #1 0x98d7e6 in xrealloc wrapper.c:126
    #2 0x916914 in strvec_push_nodup strvec.c:19
    #3 0x916a6e in strvec_push strvec.c:26
    #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
    #5 0x410dcd in run_builtin git.c:475
    #6 0x410dcd in handle_builtin git.c:729
    #7 0x414087 in run_argv git.c:818
    #8 0x414087 in cmd_main git.c:949
    #9 0x40e9ec in main common-main.c:52
    #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 22 byte(s) in 2 object(s) allocated from:
    #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)
    #1 0x98d698 in xstrdup wrapper.c:29
    #2 0x916a63 in strvec_push strvec.c:26
    #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
    #4 0x410dcd in run_builtin git.c:475
    #5 0x410dcd in handle_builtin git.c:729
    #6 0x414087 in run_argv git.c:818
    #7 0x414087 in cmd_main git.c:949
    #8 0x40e9ec in main common-main.c:52
    #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)

See also discussion about the original implementation below - this code
appears to have evolved from a callback explaining the double-strvec-copy
pattern, but there's no strong reason to keep that now:
  https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/for-each-repo.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 52be64a437..fd86e5a861 100644
--- a/builtin/for-each-repo.c
+++ b/builtin/for-each-repo.c
@@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {
 	NULL
 };
 
-static int run_command_on_repo(const char *path,
-			       void *cbdata)
+static int run_command_on_repo(const char *path, int argc, const char ** argv)
 {
 	int i;
 	struct child_process child = CHILD_PROCESS_INIT;
-	struct strvec *args = (struct strvec *)cbdata;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "-C", path, NULL);
 
-	for (i = 0; i < args->nr; i++)
-		strvec_push(&child.args, args->v[i]);
+	for (i = 0; i < argc; i++)
+		strvec_push(&child.args, argv[i]);
 
 	return run_command(&child);
 }
@@ -31,7 +29,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
 	static const char *config_key = NULL;
 	int i, result = 0;
 	const struct string_list *values;
-	struct strvec args = STRVEC_INIT;
 
 	const struct option options[] = {
 		OPT_STRING(0, "config", &config_key, N_("config"),
@@ -45,9 +42,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
 	if (!config_key)
 		die(_("missing --config=<config>"));
 
-	for (i = 0; i < argc; i++)
-		strvec_push(&args, argv[i]);
-
 	values = repo_config_get_value_multi(the_repository,
 					     config_key);
 
@@ -59,7 +53,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
 		return 0;
 
 	for (i = 0; !result && i < values->nr; i++)
-		result = run_command_on_repo(values->items[i].string, &args);
+		result = run_command_on_repo(values->items[i].string, argc, argv);
 
 	return result;
 }
-- 
2.26.2

Re: [PATCH v2 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> cmd_for_each_repo() copies argv into args (a strvec), which is later
> passed into run_command_on_repo(), which in turn copies that strvec onto
> the end of child.args. The initial copy is unnecessary (we never modify
> args). We therefore choose to just pass argv directly into
> run_command_on_repo(), which lets us avoid the copy and fixes the leak.

Makes sense.  There is no reason to make these copies.

>
> LSAN output from t0068:
>
> Direct leak of 192 byte(s) in 1 object(s) allocated from:
>     #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
>     #1 0x98d7e6 in xrealloc wrapper.c:126
>     #2 0x916914 in strvec_push_nodup strvec.c:19
>     #3 0x916a6e in strvec_push strvec.c:26
>     #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
>     #5 0x410dcd in run_builtin git.c:475
>     #6 0x410dcd in handle_builtin git.c:729
>     #7 0x414087 in run_argv git.c:818
>     #8 0x414087 in cmd_main git.c:949
>     #9 0x40e9ec in main common-main.c:52
>     #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> Indirect leak of 22 byte(s) in 2 object(s) allocated from:
>     #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)
>     #1 0x98d698 in xstrdup wrapper.c:29
>     #2 0x916a63 in strvec_push strvec.c:26
>     #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
>     #4 0x410dcd in run_builtin git.c:475
>     #5 0x410dcd in handle_builtin git.c:729
>     #6 0x414087 in run_argv git.c:818
>     #7 0x414087 in cmd_main git.c:949
>     #8 0x40e9ec in main common-main.c:52
>     #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> See also discussion about the original implementation below - this code
> appears to have evolved from a callback explaining the double-strvec-copy
> pattern, but there's no strong reason to keep that now:
>   https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  builtin/for-each-repo.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> index 52be64a437..fd86e5a861 100644
> --- a/builtin/for-each-repo.c
> +++ b/builtin/for-each-repo.c
> @@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {
>  	NULL
>  };
>  
> -static int run_command_on_repo(const char *path,
> -			       void *cbdata)
> +static int run_command_on_repo(const char *path, int argc, const char ** argv)
>  {
>  	int i;
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	struct strvec *args = (struct strvec *)cbdata;
>  
>  	child.git_cmd = 1;
>  	strvec_pushl(&child.args, "-C", path, NULL);
>  
> -	for (i = 0; i < args->nr; i++)
> -		strvec_push(&child.args, args->v[i]);
> +	for (i = 0; i < argc; i++)
> +		strvec_push(&child.args, argv[i]);
>  
>  	return run_command(&child);
>  }
> @@ -31,7 +29,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>  	static const char *config_key = NULL;
>  	int i, result = 0;
>  	const struct string_list *values;
> -	struct strvec args = STRVEC_INIT;
>  
>  	const struct option options[] = {
>  		OPT_STRING(0, "config", &config_key, N_("config"),
> @@ -45,9 +42,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>  	if (!config_key)
>  		die(_("missing --config=<config>"));
>  
> -	for (i = 0; i < argc; i++)
> -		strvec_push(&args, argv[i]);
> -
>  	values = repo_config_get_value_multi(the_repository,
>  					     config_key);
>  
> @@ -59,7 +53,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>  		return 0;
>  
>  	for (i = 0; !result && i < values->nr; i++)
> -		result = run_command_on_repo(values->items[i].string, &args);
> +		result = run_command_on_repo(values->items[i].string, argc, argv);
>  
>  	return result;
>  }

[PATCH v2 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

old_dir/new_dir are free()'d at the end of update_dir_rename_counts,
however if we return early we'll never free those strings. Therefore
we should move all new allocations after the possible early return,
avoiding a leak.

This seems like a fairly recent leak, that started happening since the
early-return was added in:
  1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)

LSAN output from t0022:

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71e48 in xstrdup wrapper.c:29:14
    #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12
    #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
    #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
    #5 0x7b4cfc in diffcore_std diff.c:6705:4
    #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
    #7 0x856574 in log_tree_diff log-tree.c:955:3
    #8 0x856574 in log_tree_commit log-tree.c:986:10
    #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
    #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6b3f3d in main common-main.c:52:11
    #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71e48 in xstrdup wrapper.c:29:14
    #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12
    #3 0x7db6ae in find_renames diffcore-rename.c:1062:3
    #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18
    #5 0x7b4cfc in diffcore_std diff.c:6705:4
    #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2
    #7 0x856574 in log_tree_diff log-tree.c:955:3
    #8 0x856574 in log_tree_commit log-tree.c:986:10
    #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7
    #10 0x52e623 in cmd_commit builtin/commit.c:1862:3
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6b3f3d in main common-main.c:52:11
    #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 diffcore-rename.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 2618bb07c1..c95857b51f 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -448,9 +448,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
 				     const char *oldname,
 				     const char *newname)
 {
-	char *old_dir = xstrdup(oldname);
-	char *new_dir = xstrdup(newname);
-	char new_dir_first_char = new_dir[0];
+	char *old_dir;
+	char *new_dir;
+	const char new_dir_first_char = newname[0];
 	int first_time_in_loop = 1;
 
 	if (!info->setup)
@@ -475,6 +475,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
 		 */
 		return;
 
+
+	old_dir = xstrdup(oldname);
+	new_dir = xstrdup(newname);
+
 	while (1) {
 		int drd_flag = NOT_RELEVANT;
 
-- 
2.26.2

Re: [PATCH v2 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> old_dir/new_dir are free()'d at the end of update_dir_rename_counts,
> however if we return early we'll never free those strings. Therefore
> we should move all new allocations after the possible early return,
> avoiding a leak.
>
> This seems like a fairly recent leak, that started happening since the
> early-return was added in:
>   1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)

Yup.  It is not surprising to have issues in younger parts of the
code.  Thanks.

[PATCH v2 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

u.head is populated using resolve_refdup(), which returns a newly
allocated string - hence we also need to free() it.

Found while running t0041 with LSAN:

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa8be98 in xstrdup wrapper.c:29:14
    #2 0x9481db in head_atom_parser ref-filter.c:549:17
    #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30
    #4 0x9400e3 in verify_ref_format ref-filter.c:974:8
    #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6
    #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3
    #7 0x4ce83e in run_builtin git.c:475:11
    #8 0x4ccafe in handle_builtin git.c:729:3
    #9 0x4cb01c in run_argv git.c:818:4
    #10 0x4cb01c in cmd_main git.c:949:19
    #11 0x6bdc2d in main common-main.c:52:11
    #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 ref-filter.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f45d3a1b26..0cfef7b719 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2226,8 +2226,12 @@ void ref_array_clear(struct ref_array *array)
 	FREE_AND_NULL(array->items);
 	array->nr = array->alloc = 0;
 
-	for (i = 0; i < used_atom_cnt; i++)
-		free((char *)used_atom[i].name);
+	for (i = 0; i < used_atom_cnt; i++) {
+		struct used_atom *atom = &used_atom[i];
+		if (atom->atom_type == ATOM_HEAD)
+			free(atom->u.head);
+		free((char *)atom->name);
+	}
 	FREE_AND_NULL(used_atom);
 	used_atom_cnt = 0;
 
-- 
2.26.2

Re: [PATCH v2 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> u.head is populated using resolve_refdup(), which returns a newly
> allocated string - hence we also need to free() it.

Correct.  The solution makes me wonder if this approach scales as we
add more and more members to u.* union that need deallocating, but
for now, this is perfectly adequate.

Thanks.

>
> Found while running t0041 with LSAN:
>
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0xa8be98 in xstrdup wrapper.c:29:14
>     #2 0x9481db in head_atom_parser ref-filter.c:549:17
>     #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30
>     #4 0x9400e3 in verify_ref_format ref-filter.c:974:8
>     #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6
>     #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3
>     #7 0x4ce83e in run_builtin git.c:475:11
>     #8 0x4ccafe in handle_builtin git.c:729:3
>     #9 0x4cb01c in run_argv git.c:818:4
>     #10 0x4cb01c in cmd_main git.c:949:19
>     #11 0x6bdc2d in main common-main.c:52:11
>     #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  ref-filter.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index f45d3a1b26..0cfef7b719 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2226,8 +2226,12 @@ void ref_array_clear(struct ref_array *array)
>  	FREE_AND_NULL(array->items);
>  	array->nr = array->alloc = 0;
>  
> -	for (i = 0; i < used_atom_cnt; i++)
> -		free((char *)used_atom[i].name);
> +	for (i = 0; i < used_atom_cnt; i++) {
> +		struct used_atom *atom = &used_atom[i];
> +		if (atom->atom_type == ATOM_HEAD)
> +			free(atom->u.head);
> +		free((char *)atom->name);
> +	}
>  	FREE_AND_NULL(used_atom);
>  	used_atom_cnt = 0;

[PATCH v2 07/12] read-cache: call diff_setup_done to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

repo_diff_setup() calls through to diff.c's static prep_parse_options(),
which in  turn allocates a new array into diff_opts.parseopts.
diff_setup_done() is responsible for freeing that array, and has the
benefit of verifying diff_opts too - hence we add a call to
diff_setup_done() to avoid leaking parseopts.

Output from the leak as found while running t0090 with LSAN:

Direct leak of 7120 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
    #2 0x7a7bae in prep_parse_options diff.c:5636:2
    #3 0x7a7bae in repo_diff_setup diff.c:4611:2
    #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
    #5 0x872233 in unclean merge-ort-wrappers.c:12:14
    #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
    #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
    #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
    #9 0x4ce83e in run_builtin git.c:475:11
    #10 0x4ccafe in handle_builtin git.c:729:3
    #11 0x4cb01c in run_argv git.c:818:4
    #12 0x4cb01c in cmd_main git.c:949:19
    #13 0x6bdc2d in main common-main.c:52:11
    #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/read-cache.c b/read-cache.c
index 46ccd66f34..83d1817ad0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2505,6 +2505,7 @@ int repo_index_has_changes(struct repository *repo,
 		opt.flags.exit_with_status = 1;
 		if (!sb)
 			opt.flags.quick = 1;
+		diff_setup_done(&opt);
 		do_diff_cache(&cmp, &opt);
 		diffcore_std(&opt);
 		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
-- 
2.26.2

Re: [PATCH v2 07/12] read-cache: call diff_setup_done to avoid leak

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> repo_diff_setup() calls through to diff.c's static prep_parse_options(),
> which in  turn allocates a new array into diff_opts.parseopts.
> diff_setup_done() is responsible for freeing that array, and has the
> benefit of verifying diff_opts too - hence we add a call to
> diff_setup_done() to avoid leaking parseopts.

Right.  Thanks.

>
> Output from the leak as found while running t0090 with LSAN:
>
> Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>     #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>     #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
>     #2 0x7a7bae in prep_parse_options diff.c:5636:2
>     #3 0x7a7bae in repo_diff_setup diff.c:4611:2
>     #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
>     #5 0x872233 in unclean merge-ort-wrappers.c:12:14
>     #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
>     #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
>     #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
>     #9 0x4ce83e in run_builtin git.c:475:11
>     #10 0x4ccafe in handle_builtin git.c:729:3
>     #11 0x4cb01c in run_argv git.c:818:4
>     #12 0x4cb01c in cmd_main git.c:949:19
>     #13 0x6bdc2d in main common-main.c:52:11
>     #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  read-cache.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 46ccd66f34..83d1817ad0 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2505,6 +2505,7 @@ int repo_index_has_changes(struct repository *repo,
>  		opt.flags.exit_with_status = 1;
>  		if (!sb)
>  			opt.flags.quick = 1;
> +		diff_setup_done(&opt);
>  		do_diff_cache(&cmp, &opt);
>  		diffcore_std(&opt);
>  		for (i = 0; sb && i < diff_queued_diff.nr; i++) {

[PATCH v2 08/12] convert: release strbuf to avoid leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

apply_multi_file_filter and async_query_available_blobs both query
subprocess output using subprocess_read_status, which writes data into
the identically named filter_status strbuf. We add a strbuf_release to
avoid leaking their contents.

Leak output seen when running t0021 with LSAN:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c2b5 in xrealloc wrapper.c:126:8
    #2 0x9ff99d in strbuf_grow strbuf.c:98:2
    #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2
    #4 0xa101d6 in subprocess_read_status sub-process.c:45:5
    #5 0x77793c in apply_multi_file_filter convert.c:886:8
    #6 0x77793c in apply_filter convert.c:1042:10
    #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7
    #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2
    #9 0x8b48cd in index_fd object-file.c:2248:9
    #10 0x597411 in hash_fd builtin/hash-object.c:43:9
    #11 0x596be1 in hash_object builtin/hash-object.c:59:2
    #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3
    #13 0x4ce83e in run_builtin git.c:475:11
    #14 0x4ccafe in handle_builtin git.c:729:3
    #15 0x4cb01c in run_argv git.c:818:4
    #16 0x4cb01c in cmd_main git.c:949:19
    #17 0x6bdc2d in main common-main.c:52:11
    #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

Direct leak of 120 byte(s) in 5 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c295 in xrealloc wrapper.c:126:8
    #2 0x9ff97d in strbuf_grow strbuf.c:98:2
    #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2
    #4 0xa101b6 in subprocess_read_status sub-process.c:45:5
    #5 0x775c73 in async_query_available_blobs convert.c:960:8
    #6 0x80029d in finish_delayed_checkout entry.c:183:9
    #7 0xa65d1e in check_updates unpack-trees.c:493:10
    #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8
    #9 0x525971 in checkout builtin/clone.c:815:6
    #10 0x525971 in cmd_clone builtin/clone.c:1409:8
    #11 0x4ce83e in run_builtin git.c:475:11
    #12 0x4ccafe in handle_builtin git.c:729:3
    #13 0x4cb01c in run_argv git.c:818:4
    #14 0x4cb01c in cmd_main git.c:949:19
    #15 0x6bdc2d in main common-main.c:52:11
    #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 convert.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/convert.c b/convert.c
index fd9c84b025..0d6fb3410a 100644
--- a/convert.c
+++ b/convert.c
@@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	else
 		strbuf_swap(dst, &nbuf);
 	strbuf_release(&nbuf);
+	strbuf_release(&filter_status);
 	return !err;
 }
 
@@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
 
 	if (err)
 		handle_filter_error(&filter_status, entry, 0);
+	strbuf_release(&filter_status);
 	return !err;
 }
 
-- 
2.26.2

Re: [PATCH v2 08/12] convert: release strbuf to avoid leak

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> apply_multi_file_filter and async_query_available_blobs both query
> subprocess output using subprocess_read_status, which writes data into
> the identically named filter_status strbuf. We add a strbuf_release to
> avoid leaking their contents.
>
> Leak output seen when running t0021 with LSAN:

Obviously correct.  Thanks.

>
> Direct leak of 24 byte(s) in 1 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa8c2b5 in xrealloc wrapper.c:126:8
>     #2 0x9ff99d in strbuf_grow strbuf.c:98:2
>     #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2
>     #4 0xa101d6 in subprocess_read_status sub-process.c:45:5
>     #5 0x77793c in apply_multi_file_filter convert.c:886:8
>     #6 0x77793c in apply_filter convert.c:1042:10
>     #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7
>     #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2
>     #9 0x8b48cd in index_fd object-file.c:2248:9
>     #10 0x597411 in hash_fd builtin/hash-object.c:43:9
>     #11 0x596be1 in hash_object builtin/hash-object.c:59:2
>     #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3
>     #13 0x4ce83e in run_builtin git.c:475:11
>     #14 0x4ccafe in handle_builtin git.c:729:3
>     #15 0x4cb01c in run_argv git.c:818:4
>     #16 0x4cb01c in cmd_main git.c:949:19
>     #17 0x6bdc2d in main common-main.c:52:11
>     #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).
>
> Direct leak of 120 byte(s) in 5 object(s) allocated from:
>     #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
>     #1 0xa8c295 in xrealloc wrapper.c:126:8
>     #2 0x9ff97d in strbuf_grow strbuf.c:98:2
>     #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2
>     #4 0xa101b6 in subprocess_read_status sub-process.c:45:5
>     #5 0x775c73 in async_query_available_blobs convert.c:960:8
>     #6 0x80029d in finish_delayed_checkout entry.c:183:9
>     #7 0xa65d1e in check_updates unpack-trees.c:493:10
>     #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8
>     #9 0x525971 in checkout builtin/clone.c:815:6
>     #10 0x525971 in cmd_clone builtin/clone.c:1409:8
>     #11 0x4ce83e in run_builtin git.c:475:11
>     #12 0x4ccafe in handle_builtin git.c:729:3
>     #13 0x4cb01c in run_argv git.c:818:4
>     #14 0x4cb01c in cmd_main git.c:949:19
>     #15 0x6bdc2d in main common-main.c:52:11
>     #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  convert.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/convert.c b/convert.c
> index fd9c84b025..0d6fb3410a 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>  	else
>  		strbuf_swap(dst, &nbuf);
>  	strbuf_release(&nbuf);
> +	strbuf_release(&filter_status);
>  	return !err;
>  }
>  
> @@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p
>  
>  	if (err)
>  		handle_filter_error(&filter_status, entry, 0);
> +	strbuf_release(&filter_status);
>  	return !err;
>  }

[PATCH v2 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

These leaks all happen at the end of cmd_mv, hence don't matter in any
way. But we still fix the easy ones and squash the rest to get us closer
to being able to run tests without leaks.

LSAN output from t0050:

Direct leak of 384 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa0a7e1 in add_entry string-list.c:44:2
    #3 0xa0a7e1 in string_list_insert string-list.c:58:14
    #4 0x5dac03 in cmd_mv builtin/mv.c:248:4
    #5 0x4ce83e in run_builtin git.c:475:11
    #6 0x4ccafe in handle_builtin git.c:729:3
    #7 0x4cb01c in run_argv git.c:818:4
    #8 0x4cb01c in cmd_main git.c:949:19
    #9 0x6bd9ad in main common-main.c:52:11
    #10 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bd09 in do_xmalloc wrapper.c:41:8
    #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
    #3 0x5da575 in cmd_mv builtin/mv.c:158:14
    #4 0x4ce83e in run_builtin git.c:475:11
    #5 0x4ccafe in handle_builtin git.c:729:3
    #6 0x4cb01c in run_argv git.c:818:4
    #7 0x4cb01c in cmd_main git.c:949:19
    #8 0x6bd9ad in main common-main.c:52:11
    #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bd09 in do_xmalloc wrapper.c:41:8
    #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
    #3 0x5da4e4 in cmd_mv builtin/mv.c:148:11
    #4 0x4ce83e in run_builtin git.c:475:11
    #5 0x4ccafe in handle_builtin git.c:729:3
    #6 0x4cb01c in run_argv git.c:818:4
    #7 0x4cb01c in cmd_main git.c:949:19
    #8 0x6bd9ad in main common-main.c:52:11
    #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xa8c119 in xcalloc wrapper.c:140:8
    #2 0x5da585 in cmd_mv builtin/mv.c:159:22
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6bd9ad in main common-main.c:52:11
    #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xa8c119 in xcalloc wrapper.c:140:8
    #2 0x5da4f8 in cmd_mv builtin/mv.c:149:10
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6bd9ad in main common-main.c:52:11
    #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 65 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa00226 in strbuf_grow strbuf.c:98:2
    #3 0xa00226 in strbuf_vaddf strbuf.c:394:3
    #4 0xa065c7 in xstrvfmt strbuf.c:981:2
    #5 0xa065c7 in xstrfmt strbuf.c:991:8
    #6 0x9e7ce7 in prefix_path_gently setup.c:115:15
    #7 0x9e7fa6 in prefix_path setup.c:128:12
    #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
    #9 0x5da575 in cmd_mv builtin/mv.c:158:14
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6bd9ad in main common-main.c:52:11
    #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 65 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa8c015 in xrealloc wrapper.c:126:8
    #2 0xa00226 in strbuf_grow strbuf.c:98:2
    #3 0xa00226 in strbuf_vaddf strbuf.c:394:3
    #4 0xa065c7 in xstrvfmt strbuf.c:981:2
    #5 0xa065c7 in xstrfmt strbuf.c:991:8
    #6 0x9e7ce7 in prefix_path_gently setup.c:115:15
    #7 0x9e7fa6 in prefix_path setup.c:128:12
    #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
    #9 0x5da4e4 in cmd_mv builtin/mv.c:148:11
    #10 0x4ce83e in run_builtin git.c:475:11
    #11 0x4ccafe in handle_builtin git.c:729:3
    #12 0x4cb01c in run_argv git.c:818:4
    #13 0x4cb01c in cmd_main git.c:949:19
    #14 0x6bd9ad in main common-main.c:52:11
    #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 558 byte(s) leaked in 7 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/mv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 3fccdcb645..c2f96c8e89 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -303,5 +303,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("Unable to write new index file"));
 
+	string_list_clear(&src_for_dst, 0);
+	UNLEAK(source);
+	UNLEAK(dest_path);
+	free(submodule_gitfile);
+	free(modes);
 	return 0;
 }
-- 
2.26.2

[PATCH v2 10/12] builtin/merge: free found_ref when done

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

merge_name() calls dwim_ref(), which allocates a new string into
found_ref. Therefore add a free() to avoid leaking found_ref.

LSAN output from t0021:

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa8beb8 in xstrdup wrapper.c:29:14
    #2 0x954054 in expand_ref refs.c:671:12
    #3 0x953cb6 in repo_dwim_ref refs.c:644:22
    #4 0x5d3759 in dwim_ref refs.h:162:9
    #5 0x5d3759 in merge_name builtin/merge.c:517:6
    #6 0x5d3759 in collect_parents builtin/merge.c:1214:5
    #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6bdbfd in main common-main.c:52:11
    #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/merge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..7ad85c044a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	struct strbuf bname = STRBUF_INIT;
 	struct merge_remote_desc *desc;
 	const char *ptr;
-	char *found_ref;
+	char *found_ref = NULL;
 	int len, early;
 
 	strbuf_branchname(&bname, remote, 0);
@@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	strbuf_addf(msg, "%s\t\tcommit '%s'\n",
 		oid_to_hex(&remote_head->object.oid), remote);
 cleanup:
+	free(found_ref);
 	strbuf_release(&buf);
 	strbuf_release(&bname);
 }
-- 
2.26.2

[PATCH v2 11/12] builtin/rebase: fix options.strategy memory lifecycle

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

- cmd_rebase populates rebase_options.strategy with newly allocated
  strings, hence we need to free those strings at the end of cmd_rebase
  to avoid a leak.
- In some cases: get_replay_opts() is called, which prepares replay_opts
  using data from rebase_options. We used to simply copy the pointer
  from rebase_options.strategy,  however that would now result in a
  double-free because sequencer_remove_state() is eventually used to
  free replay_opts.strategy. To avoid this we xstrdup() strategy when
  adding it to replay_opts.

The original leak happens because we always populate
rebase_options.strategy, but we don't always enter the path that calls
get_replay_opts() and later sequencer_remove_state() - in  other words
we'd always allocate a new string into rebase_options.strategy but
only sometimes did we free it. We now make sure that rebase_options
and replay_opts both own their own copies of strategy, and each copy
is free'd independently.

This was first seen when running t0021 with LSAN, but t2012 helped catch
the fact that we can't just free(options.strategy) at the end of
cmd_rebase (as that can cause a double-free). LSAN output from t0021:

LSAN output from t0021:

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xa71eb8 in xstrdup wrapper.c:29:14
    #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22
    #3 0x4ce83e in run_builtin git.c:475:11
    #4 0x4ccafe in handle_builtin git.c:729:3
    #5 0x4cb01c in run_argv git.c:818:4
    #6 0x4cb01c in cmd_main git.c:949:19
    #7 0x6b3fad in main common-main.c:52:11
    #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/rebase.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d..33e0961900 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	replay.ignore_date = opts->ignore_date;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
 	if (opts->strategy)
-		replay.strategy = opts->strategy;
+		replay.strategy = xstrdup_or_null(opts->strategy);
 	else if (!replay.strategy && replay.default_strategy) {
 		replay.strategy = replay.default_strategy;
 		replay.default_strategy = NULL;
@@ -2109,6 +2109,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.head_name);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
+	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	return ret;
-- 
2.26.2

[PATCH v2 12/12] reset: clear_unpack_trees_porcelain to plug leak

[andrzej]

From: Andrzej Hunt <ajrhunt@google.com>

setup_unpack_trees_porcelain() populates various fields on
unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to
avoid leaking them. Specifically, we used to leak
unpack_tree_opts.msgs_to_free.

We have to do this in leave_reset_head because there are multiple
scenarios where unpack_tree_opts has already been configured, followed
by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'
prior to having initialised unpack_tree_opts via memset(..., 0, ...).
Therefore we also move unpack_tree_opts initialisation to the start of
reset_head(), and convert it to use brace initialisation - which
guarantees that we can never clear an uninitialised unpack_tree_opts.
clear_unpack_tree_opts() is always safe to call as long as
unpack_tree_opts is at least zero-initialised, i.e. it does not depend
on a previous call to setup_unpack_trees_porcelain().

LSAN output from t0021:

Direct leak of 192 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9f7861 in strvec_push_nodup strvec.c:19:2
    #3 0x9f7861 in strvec_pushf strvec.c:39:2
    #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
    #5 0x97e011 in reset_head reset.c:53:2
    #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #7 0x4ce83e in run_builtin git.c:475:11
    #8 0x4ccafe in handle_builtin git.c:729:3
    #9 0x4cb01c in run_argv git.c:818:4
    #10 0x4cb01c in cmd_main git.c:949:19
    #11 0x6b3f3d in main common-main.c:52:11
    #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 147 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 134 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Indirect leak of 130 byte(s) in 1 object(s) allocated from:
    #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xa721e5 in xrealloc wrapper.c:126:8
    #2 0x9e8d54 in strbuf_grow strbuf.c:98:2
    #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
    #4 0x9f7774 in strvec_pushf strvec.c:36:2
    #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3
    #6 0x97e011 in reset_head reset.c:53:2
    #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
    #8 0x4ce83e in run_builtin git.c:475:11
    #9 0x4ccafe in handle_builtin git.c:729:3
    #10 0x4cb01c in run_argv git.c:818:4
    #11 0x4cb01c in cmd_main git.c:949:19
    #12 0x6b3f3d in main common-main.c:52:11
    #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 reset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/reset.c b/reset.c
index 4bea758053..79310ae071 100644
--- a/reset.c
+++ b/reset.c
@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;
-	struct unpack_trees_options unpack_tree_opts;
+	struct unpack_trees_options unpack_tree_opts = { 0 };
 	struct tree *tree;
 	const char *reflog_action;
 	struct strbuf msg = STRBUF_INIT;
@@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	if (refs_only)
 		goto reset_head_refs;
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;
@@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 leave_reset_head:
 	strbuf_release(&msg);
 	rollback_lock_file(&lock);
+	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	while (nr)
 		free((void *)desc[--nr].buffer);
 	return ret;
-- 
2.26.2

Re: [PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 2

[Junio C Hamano]

andrzej@ahunt.org writes:

> From: Andrzej Hunt <ajrhunt@google.com>
>
> V2 fixes patch 11/12 (rebase_options.strategy lifecycle) as per review
> discussion. Many thanks to Phillip and Elijah for spotting the issues there!

Thanks.  Looking good.

Will queue.