[PATCH 0/2] dir & unpak-trees: memory-leak fixes

[PATCH 0/2] dir & unpak-trees: memory-leak fixes

[Ævar Arnfjörð Bjarmason]

These couple of patches fix some remaining memory leaks in
unpack-trees.c, which mostly happen via a lack of calls to dir_clear()
fixed in 2/2.

This goes on top of ab/sanitize-leak-ci (but not
en/removing-untracked-fixes). In 1/2 I mark a test as passing under
SANITIZE=leak, which assumes that an environment variable added by
ab/sanitize-leak-ci is understood. Without ab/sanitize-leak-ci it'll
be silently ignored.

Elijah has parallel work in fixing leaks in dir.c, but without his
ab/sanitize-leak-ci that test will also pass, it's not one of the ones
that needed something like his leak fixes to {dir,unpack-trees}.c to
pass.

Ævar Arnfjörð Bjarmason (2):
  unpack-trees: don't leak memory in verify_clean_subdirectory()
  built-ins & lib: plug memory leaks with unpack_trees_options_release()

 archive.c                   | 11 ++++++++---
 builtin/am.c                | 17 ++++++++++++-----
 builtin/checkout.c          |  9 +++++++--
 builtin/clone.c             |  1 +
 builtin/commit.c            |  6 +++++-
 builtin/merge.c             |  6 ++++--
 builtin/read-tree.c         | 14 ++++++++++----
 builtin/reset.c             | 13 +++++++++----
 builtin/stash.c             | 14 ++++++++++----
 diff-lib.c                  |  5 ++++-
 sequencer.c                 |  2 ++
 t/t1001-read-tree-m-2way.sh |  2 ++
 unpack-trees.c              |  3 ++-
 13 files changed, 76 insertions(+), 27 deletions(-)

-- 
2.33.0.1441.gbbcdb4c3c66

[PATCH 1/2] unpack-trees: don't leak memory in verify_clean_subdirectory()

[Ævar Arnfjörð Bjarmason]

Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.

 * "pathbuf": When the read_directory() call followed by the
   free(pathbuf) was added in c81935348be (Fix switching to a branch
   with D/F when current branch has file D., 2007-03-15) we didn't
   bother to free() before we called die().

   But when this code was later libified in 203a2fe1170 (Allow callers
   of unpack_trees() to handle failure, 2008-02-07) we started to leak
   as we returned data to the caller. This fixes that memory leak,
   which can be observed under SANITIZE=leak with e.g. the
   "t1001-read-tree-m-2way.sh" test.

 * "struct dir_struct": We've leaked the dir_struct ever since this
   code was added back in c81935348be.

   When that commit was written there wasn't an equivalent of
   dir_clear(). Since it was added in 270be816049 (dir.c: provide
   clear_directory() for reclaiming dir_struct memory, 2013-01-06)
   we've omitted freeing the memory allocated here.

   This memory leak could also be observed under SANITIZE=leak and the
   "t1001-read-tree-m-2way.sh" test.

This makes all the test in "t1001-read-tree-m-2way.sh" pass under
"GIT_TEST_PASSING_SANITIZE_LEAK=true", we'd previously die in tests
25, 26 & 28.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1001-read-tree-m-2way.sh | 2 ++
 unpack-trees.c              | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 1057a96b249..d1115528cb9 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -20,6 +20,8 @@ In the test, these paths are used:
 	rezrov  - in H, deleted in M
 	yomin   - not in H or M
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
diff --git a/unpack-trees.c b/unpack-trees.c
index a7e1712d236..89ca95ce90b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	if (o->dir)
 		d.exclude_per_dir = o->dir->exclude_per_dir;
 	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
+	dir_clear(&d);
+	free(pathbuf);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
-	free(pathbuf);
 	return cnt;
 }
 
-- 
2.33.0.1441.gbbcdb4c3c66

Re: [PATCH 1/2] unpack-trees: don't leak memory in verify_clean_subdirectory()

[Elijah Newren]

On Wed, Oct 6, 2021 at 2:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Fix two different but related memory leaks in
> verify_clean_subdirectory(). We leaked both the "pathbuf" if
> read_directory() returned non-zero, and we never cleaned up our own
> "struct dir_struct" either.
>
>  * "pathbuf": When the read_directory() call followed by the
>    free(pathbuf) was added in c81935348be (Fix switching to a branch
>    with D/F when current branch has file D., 2007-03-15) we didn't
>    bother to free() before we called die().
>
>    But when this code was later libified in 203a2fe1170 (Allow callers
>    of unpack_trees() to handle failure, 2008-02-07) we started to leak
>    as we returned data to the caller. This fixes that memory leak,
>    which can be observed under SANITIZE=leak with e.g. the
>    "t1001-read-tree-m-2way.sh" test.
>
>  * "struct dir_struct": We've leaked the dir_struct ever since this
>    code was added back in c81935348be.
>
>    When that commit was written there wasn't an equivalent of
>    dir_clear(). Since it was added in 270be816049 (dir.c: provide
>    clear_directory() for reclaiming dir_struct memory, 2013-01-06)
>    we've omitted freeing the memory allocated here.
>
>    This memory leak could also be observed under SANITIZE=leak and the
>    "t1001-read-tree-m-2way.sh" test.
>
> This makes all the test in "t1001-read-tree-m-2way.sh" pass under
> "GIT_TEST_PASSING_SANITIZE_LEAK=true", we'd previously die in tests
> 25, 26 & 28.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t1001-read-tree-m-2way.sh | 2 ++
>  unpack-trees.c              | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
> index 1057a96b249..d1115528cb9 100755
> --- a/t/t1001-read-tree-m-2way.sh
> +++ b/t/t1001-read-tree-m-2way.sh
> @@ -20,6 +20,8 @@ In the test, these paths are used:
>         rezrov  - in H, deleted in M
>         yomin   - not in H or M
>  '
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-read-tree.sh
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index a7e1712d236..89ca95ce90b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>         if (o->dir)
>                 d.exclude_per_dir = o->dir->exclude_per_dir;
>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> +       dir_clear(&d);
> +       free(pathbuf);
>         if (i)
>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> -       free(pathbuf);
>         return cnt;
>  }
>
> --
> 2.33.0.1441.gbbcdb4c3c66

Nicely explained; and nice clean patch.

[PATCH 2/2] built-ins & lib: plug memory leaks with unpack_trees_options_release()

[Ævar Arnfjörð Bjarmason]

Plug memory leaks in various built-ins and the "diff-lib.c" and
"sequencer.c" libraries that were missing
unpack_trees_options_release() calls.

In the case of "git archive" we'll need to memset() the "struct
unpack_trees_options" first, to avoid having to call
clear_unpack_trees_porcelain() twice within the
"!args->worktree_attributes" branch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 archive.c           | 11 ++++++++---
 builtin/am.c        | 17 ++++++++++++-----
 builtin/checkout.c  |  9 +++++++--
 builtin/clone.c     |  1 +
 builtin/commit.c    |  6 +++++-
 builtin/merge.c     |  6 ++++--
 builtin/read-tree.c | 14 ++++++++++----
 builtin/reset.c     | 13 +++++++++----
 builtin/stash.c     | 14 ++++++++++----
 diff-lib.c          |  5 ++++-
 sequencer.c         |  2 ++
 11 files changed, 72 insertions(+), 26 deletions(-)

diff --git a/archive.c b/archive.c
index a3bbb091256..b0acc5a8b88 100644
--- a/archive.c
+++ b/archive.c
@@ -299,16 +299,18 @@ int write_archive_entries(struct archiver_args *args,
 	/*
 	 * Setup index and instruct attr to read index only
 	 */
+	memset(&opts, 0, sizeof(opts));
 	if (!args->worktree_attributes) {
-		memset(&opts, 0, sizeof(opts));
 		opts.index_only = 1;
 		opts.head_idx = -1;
 		opts.src_index = args->repo->index;
 		opts.dst_index = args->repo->index;
 		opts.fn = oneway_merge;
 		init_tree_desc(&t, args->tree->buffer, args->tree->size);
-		if (unpack_trees(1, &t, &opts))
-			return -1;
+		if (unpack_trees(1, &t, &opts)) {
+			err = -1;
+			goto cleanup;
+		}
 		git_attr_set_direction(GIT_ATTR_INDEX);
 	}
 
@@ -347,8 +349,11 @@ int write_archive_entries(struct archiver_args *args,
 		if (err)
 			break;
 	}
+
+cleanup:
 	strbuf_release(&path_in_archive);
 	strbuf_release(&content);
+	clear_unpack_trees_porcelain(&opts);
 
 	return err;
 }
diff --git a/builtin/am.c b/builtin/am.c
index f296226e95f..0126ec2669b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	struct lock_file lock_file = LOCK_INIT;
 	struct unpack_trees_options opts;
 	struct tree_desc t[2];
+	int ret = 0;
 
 	if (parse_tree(head) || parse_tree(remote))
 		return -1;
@@ -1925,13 +1926,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 
 	if (unpack_trees(2, t, &opts)) {
 		rollback_lock_file(&lock_file);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
-
-	return 0;
+cleanup:
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 /**
@@ -1943,6 +1946,7 @@ static int merge_tree(struct tree *tree)
 	struct lock_file lock_file = LOCK_INIT;
 	struct unpack_trees_options opts;
 	struct tree_desc t[1];
+	int ret = 0;
 
 	if (parse_tree(tree))
 		return -1;
@@ -1959,13 +1963,16 @@ static int merge_tree(struct tree *tree)
 
 	if (unpack_trees(1, t, &opts)) {
 		rollback_lock_file(&lock_file);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
-	return 0;
+cleanup:
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 /**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index cbf73b8c9f6..7b74380528b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;
+	int ret;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
@@ -670,10 +671,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 		 */
 		/* fallthrough */
 	case 0:
-		return 0;
+		ret = 0;
+		break;
 	default:
-		return 128;
+		ret = 128;
 	}
+
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 static void setup_branch_path(struct branch_info *branch)
diff --git a/builtin/clone.c b/builtin/clone.c
index 559acf9e036..c489a9bda10 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -699,6 +699,7 @@ static int checkout(int submodule_progress)
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));
+	clear_unpack_trees_porcelain(&opts);
 
 	free(head);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index e7320f66f95..55bb178289f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)
 	struct tree *tree;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
+	int exit_early = 0;
 
 	if (!current_head) {
 		discard_cache();
@@ -325,7 +326,10 @@ static void create_base_index(const struct commit *current_head)
 	parse_tree(tree);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
-		exit(128); /* We've already reported the error, finish dying */
+		exit_early = 1; /* We've already reported the error, finish dying */
+	clear_unpack_trees_porcelain(&opts);
+	if (exit_early)
+		exit(128);
 }
 
 static void refresh_cache_or_die(int refresh_flags)
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ccd5e1ac83..b4bdba2faf6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 	struct tree *trees[MAX_UNPACK_TREES];
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
+	int ret = 0;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 2;
@@ -697,8 +698,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))
-		return -1;
-	return 0;
+		ret = -1;
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 static void write_tree_trivial(struct object_id *oid)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 2109c4c9e5c..060daa3913f 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -149,6 +149,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
 		OPT_END()
 	};
+	int ret = 0;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 		parse_tree(tree);
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
-		return 128;
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = 128;
+		goto cleanup;
+	}
 
 	if (opts.debug_unpack || opts.dry_run)
-		return 0; /* do not write the index out */
+		goto cleanup; /* do not write the index out */
 
 	/*
 	 * When reading only one tree (either the most basic form,
@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die("unable to write new index file");
-	return 0;
+
+cleanup:
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 73935953494..8fb9bee1a98 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -85,10 +85,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == KEEP) {
 		struct object_id head_oid;
-		if (get_oid("HEAD", &head_oid))
-			return error(_("You do not have a valid HEAD."));
-		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
-			return error(_("Failed to find tree of HEAD."));
+		if (get_oid("HEAD", &head_oid)) {
+			error(_("You do not have a valid HEAD."));
+			goto out;
+		}
+		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
+			error(_("Failed to find tree of HEAD."));
+			goto out;
+		}
 		nr++;
 		opts.fn = twoway_merge;
 	}
@@ -110,6 +114,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 	ret = 0;
 
 out:
+	clear_unpack_trees_porcelain(&opts);
 	for (i = 0; i < nr; i++)
 		free((void *)desc[i].buffer);
 	return ret;
diff --git a/builtin/stash.c b/builtin/stash.c
index cc93ace4223..6bde00393fe 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct tree *tree;
 	struct lock_file lock_file = LOCK_INIT;
+	int ret = 0;
 
 	read_cache_preload(NULL);
 	if (refresh_cache(REFRESH_QUIET))
@@ -262,13 +263,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = oneway_merge;
 
-	if (unpack_trees(nr_trees, t, &opts))
-		return -1;
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = -1;
+		goto cleanup;
+	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		return error(_("unable to write new index file"));
+		ret = error(_("unable to write new index file"));
 
-	return 0;
+cleanup:
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
@@ -833,6 +838,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
 
 	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
 		die(_("failed to unpack trees"));
+	clear_unpack_trees_porcelain(&unpack_tree_opt);
 
 	do_diff_cache(&info->b_commit, diff_opt);
 }
diff --git a/diff-lib.c b/diff-lib.c
index ca085a03efc..ddfbcf22abf 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,
 	struct tree *tree;
 	struct tree_desc t;
 	struct unpack_trees_options opts;
+	int ret;
 
 	tree = parse_tree_indirect(tree_oid);
 	if (!tree)
@@ -546,7 +547,9 @@ static int diff_cache(struct rev_info *revs,
 	opts.pathspec->recursive = 1;
 
 	init_tree_desc(&t, tree->buffer, tree->size);
-	return unpack_trees(1, &t, &opts);
+	ret = unpack_trees(1, &t, &opts);
+	clear_unpack_trees_porcelain(&opts);
+	return ret;
 }
 
 void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
diff --git a/sequencer.c b/sequencer.c
index 6872b7b00a4..74d478d67d3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3720,6 +3720,7 @@ static int do_reset(struct repository *r,
 		rollback_lock_file(&lock);
 		free((void *)desc.buffer);
 		strbuf_release(&ref_name);
+		clear_unpack_trees_porcelain(&unpack_tree_opts);
 		return -1;
 	}
 
@@ -3736,6 +3737,7 @@ static int do_reset(struct repository *r,
 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
 
 	strbuf_release(&ref_name);
+	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	return ret;
 }
 
-- 
2.33.0.1441.gbbcdb4c3c66

Re: [PATCH 2/2] built-ins & lib: plug memory leaks with unpack_trees_options_release()

[Elijah Newren]

On Wed, Oct 6, 2021 at 2:41 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Plug memory leaks in various built-ins and the "diff-lib.c" and
> "sequencer.c" libraries that were missing
> unpack_trees_options_release() calls.

Since you've rebased these changes, unpack_trees_options_release()
doesn't exist.  You correctly refer to it below as
clear_unpack_trees_porcelain().

However, I'm not sure about most of these changes.
clear_unpack_trees_porcelain() currently only cleans up memory
allocated by setup_unpack_trees_porcelain().  Your other series
renamed the cleanup function and added a dir_clear() to it, but since
my series moved the dir_clear() into unpack_trees(), we don't have
that other bit of cleanup to do.  So, I don't think there's a leak,
unless setup_unpack_trees_porcelain() has been called.

Granted, it is safe to call clear_unpack_trees_porcelain() even if
setup_unpack_trees_porcelain() isn't called, but does it make sense?
The names suggest they are a pair, so if we want to add these (which
may be nice to prevent future additions to unpack_trees_options from
causing leaks), then we may want to rename
clear_unpack_trees_porcelain() at the same time.

I'll comment more below...

> In the case of "git archive" we'll need to memset() the "struct
> unpack_trees_options" first, to avoid having to call
> clear_unpack_trees_porcelain() twice within the
> "!args->worktree_attributes" branch.

This was slightly confusing, though the code is clear.  It suggested
to me that unpack_trees_options was being used uninitialized.  Perhaps
"...we'll need to move the memset() of struct unpack_trees_options
earlier to avoid..." ?

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  archive.c           | 11 ++++++++---
>  builtin/am.c        | 17 ++++++++++++-----
>  builtin/checkout.c  |  9 +++++++--
>  builtin/clone.c     |  1 +
>  builtin/commit.c    |  6 +++++-
>  builtin/merge.c     |  6 ++++--
>  builtin/read-tree.c | 14 ++++++++++----
>  builtin/reset.c     | 13 +++++++++----
>  builtin/stash.c     | 14 ++++++++++----
>  diff-lib.c          |  5 ++++-
>  sequencer.c         |  2 ++
>  11 files changed, 72 insertions(+), 26 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index a3bbb091256..b0acc5a8b88 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -299,16 +299,18 @@ int write_archive_entries(struct archiver_args *args,
>         /*
>          * Setup index and instruct attr to read index only
>          */
> +       memset(&opts, 0, sizeof(opts));
>         if (!args->worktree_attributes) {
> -               memset(&opts, 0, sizeof(opts));
>                 opts.index_only = 1;
>                 opts.head_idx = -1;
>                 opts.src_index = args->repo->index;
>                 opts.dst_index = args->repo->index;
>                 opts.fn = oneway_merge;
>                 init_tree_desc(&t, args->tree->buffer, args->tree->size);
> -               if (unpack_trees(1, &t, &opts))
> -                       return -1;
> +               if (unpack_trees(1, &t, &opts)) {
> +                       err = -1;
> +                       goto cleanup;
> +               }
>                 git_attr_set_direction(GIT_ATTR_INDEX);
>         }
>
> @@ -347,8 +349,11 @@ int write_archive_entries(struct archiver_args *args,
>                 if (err)
>                         break;
>         }
> +
> +cleanup:
>         strbuf_release(&path_in_archive);
>         strbuf_release(&content);
> +       clear_unpack_trees_porcelain(&opts);
>
>         return err;
>  }
> diff --git a/builtin/am.c b/builtin/am.c
> index f296226e95f..0126ec2669b 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
>         struct lock_file lock_file = LOCK_INIT;
>         struct unpack_trees_options opts;
>         struct tree_desc t[2];
> +       int ret = 0;
>
>         if (parse_tree(head) || parse_tree(remote))
>                 return -1;
> @@ -1925,13 +1926,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
>
>         if (unpack_trees(2, t, &opts)) {
>                 rollback_lock_file(&lock_file);
> -               return -1;
> +               ret = -1;
> +               goto cleanup;
>         }
>
>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>                 die(_("unable to write new index file"));
> -
> -       return 0;
> +cleanup:
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  /**
> @@ -1943,6 +1946,7 @@ static int merge_tree(struct tree *tree)
>         struct lock_file lock_file = LOCK_INIT;
>         struct unpack_trees_options opts;
>         struct tree_desc t[1];
> +       int ret = 0;
>
>         if (parse_tree(tree))
>                 return -1;
> @@ -1959,13 +1963,16 @@ static int merge_tree(struct tree *tree)
>
>         if (unpack_trees(1, t, &opts)) {
>                 rollback_lock_file(&lock_file);
> -               return -1;
> +               ret = -1;
> +               goto cleanup;
>         }
>
>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>                 die(_("unable to write new index file"));
>
> -       return 0;
> +cleanup:
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  /**
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index cbf73b8c9f6..7b74380528b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>  {
>         struct unpack_trees_options opts;
>         struct tree_desc tree_desc;
> +       int ret;
>
>         memset(&opts, 0, sizeof(opts));
>         opts.head_idx = -1;
> @@ -670,10 +671,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>                  */
>                 /* fallthrough */
>         case 0:
> -               return 0;
> +               ret = 0;
> +               break;
>         default:
> -               return 128;
> +               ret = 128;
>         }
> +
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  static void setup_branch_path(struct branch_info *branch)
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 559acf9e036..c489a9bda10 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -699,6 +699,7 @@ static int checkout(int submodule_progress)
>         init_tree_desc(&t, tree->buffer, tree->size);
>         if (unpack_trees(1, &t, &opts) < 0)
>                 die(_("unable to checkout working tree"));
> +       clear_unpack_trees_porcelain(&opts);
>
>         free(head);
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e7320f66f95..55bb178289f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)
>         struct tree *tree;
>         struct unpack_trees_options opts;
>         struct tree_desc t;
> +       int exit_early = 0;
>
>         if (!current_head) {
>                 discard_cache();
> @@ -325,7 +326,10 @@ static void create_base_index(const struct commit *current_head)
>         parse_tree(tree);
>         init_tree_desc(&t, tree->buffer, tree->size);
>         if (unpack_trees(1, &t, &opts))
> -               exit(128); /* We've already reported the error, finish dying */
> +               exit_early = 1; /* We've already reported the error, finish dying */
> +       clear_unpack_trees_porcelain(&opts);
> +       if (exit_early)
> +               exit(128);
>  }
>
>  static void refresh_cache_or_die(int refresh_flags)
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0ccd5e1ac83..b4bdba2faf6 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
>         struct tree *trees[MAX_UNPACK_TREES];
>         struct tree_desc t[MAX_UNPACK_TREES];
>         struct unpack_trees_options opts;
> +       int ret = 0;
>
>         memset(&opts, 0, sizeof(opts));
>         opts.head_idx = 2;
> @@ -697,8 +698,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
>                 init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
>         }
>         if (unpack_trees(nr_trees, t, &opts))
> -               return -1;
> -       return 0;
> +               ret = -1;
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  static void write_tree_trivial(struct object_id *oid)
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 2109c4c9e5c..060daa3913f 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -149,6 +149,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>                 OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
>                 OPT_END()
>         };
> +       int ret = 0;
>
>         memset(&opts, 0, sizeof(opts));
>         opts.head_idx = -1;
> @@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>                 parse_tree(tree);
>                 init_tree_desc(t+i, tree->buffer, tree->size);
>         }
> -       if (unpack_trees(nr_trees, t, &opts))
> -               return 128;
> +       if (unpack_trees(nr_trees, t, &opts)) {
> +               ret = 128;
> +               goto cleanup;
> +       }
>
>         if (opts.debug_unpack || opts.dry_run)
> -               return 0; /* do not write the index out */
> +               goto cleanup; /* do not write the index out */
>
>         /*
>          * When reading only one tree (either the most basic form,
> @@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>
>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>                 die("unable to write new index file");
> -       return 0;
> +
> +cleanup:
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 73935953494..8fb9bee1a98 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -85,10 +85,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>
>         if (reset_type == KEEP) {
>                 struct object_id head_oid;
> -               if (get_oid("HEAD", &head_oid))
> -                       return error(_("You do not have a valid HEAD."));
> -               if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
> -                       return error(_("Failed to find tree of HEAD."));
> +               if (get_oid("HEAD", &head_oid)) {
> +                       error(_("You do not have a valid HEAD."));
> +                       goto out;
> +               }
> +               if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
> +                       error(_("Failed to find tree of HEAD."));
> +                       goto out;
> +               }
>                 nr++;
>                 opts.fn = twoway_merge;
>         }
> @@ -110,6 +114,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>         ret = 0;
>
>  out:
> +       clear_unpack_trees_porcelain(&opts);
>         for (i = 0; i < nr; i++)
>                 free((void *)desc[i].buffer);
>         return ret;
> diff --git a/builtin/stash.c b/builtin/stash.c
> index cc93ace4223..6bde00393fe 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>         struct tree_desc t[MAX_UNPACK_TREES];
>         struct tree *tree;
>         struct lock_file lock_file = LOCK_INIT;
> +       int ret = 0;
>
>         read_cache_preload(NULL);
>         if (refresh_cache(REFRESH_QUIET))
> @@ -262,13 +263,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>                 opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
>         opts.fn = oneway_merge;
>
> -       if (unpack_trees(nr_trees, t, &opts))
> -               return -1;
> +       if (unpack_trees(nr_trees, t, &opts)) {
> +               ret = -1;
> +               goto cleanup;
> +       }
>
>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> -               return error(_("unable to write new index file"));
> +               ret = error(_("unable to write new index file"));
>
> -       return 0;
> +cleanup:
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
> @@ -833,6 +838,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
>
>         if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
>                 die(_("failed to unpack trees"));
> +       clear_unpack_trees_porcelain(&unpack_tree_opt);
>
>         do_diff_cache(&info->b_commit, diff_opt);
>  }
> diff --git a/diff-lib.c b/diff-lib.c
> index ca085a03efc..ddfbcf22abf 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,
>         struct tree *tree;
>         struct tree_desc t;
>         struct unpack_trees_options opts;
> +       int ret;
>
>         tree = parse_tree_indirect(tree_oid);
>         if (!tree)
> @@ -546,7 +547,9 @@ static int diff_cache(struct rev_info *revs,
>         opts.pathspec->recursive = 1;
>
>         init_tree_desc(&t, tree->buffer, tree->size);
> -       return unpack_trees(1, &t, &opts);
> +       ret = unpack_trees(1, &t, &opts);
> +       clear_unpack_trees_porcelain(&opts);
> +       return ret;
>  }
>
>  void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)

I think none of the above are cases where
setup_unpack_trees_porcelain() were called, and thus aren't fixing
current leaks (but might be helping us prevent future leaks, as
discussed above)

> diff --git a/sequencer.c b/sequencer.c
> index 6872b7b00a4..74d478d67d3 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3720,6 +3720,7 @@ static int do_reset(struct repository *r,
>                 rollback_lock_file(&lock);
>                 free((void *)desc.buffer);
>                 strbuf_release(&ref_name);
> +               clear_unpack_trees_porcelain(&unpack_tree_opts);
>                 return -1;
>         }
>
> @@ -3736,6 +3737,7 @@ static int do_reset(struct repository *r,
>                                  NULL, 0, UPDATE_REFS_MSG_ON_ERR);
>
>         strbuf_release(&ref_name);
> +       clear_unpack_trees_porcelain(&unpack_tree_opts);
>         return ret;
>  }

This one is a leak fix, but I think it's incomplete.  I count four
return statements in do_reset(), after the
setup_unpack_trees_porcelain() call, but here you only add
clear_unpack_trees_porcelain() calls before two of them.  I think you
need to add calls for the other two as well.  Or restructure the
returns to use a "goto cleanup" or something like that.

Re: [PATCH 0/2] dir & unpak-trees: memory-leak fixes

[Elijah Newren]

On Wed, Oct 6, 2021 at 2:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> These couple of patches fix some remaining memory leaks in
> unpack-trees.c, which mostly happen via a lack of calls to dir_clear()
> fixed in 2/2.

There is no added dir_clear() in 2/2 (the equivalent patch in the
previous series you took that patch from did have one, so I think this
was just an incomplete edit-or-commenting-after-rebasing case).  More
comments on that patch.  The idea for 2/2 is probably good, but should
probably be accompanied with another tweak, and the commit message and
cover letter about it need some updating.

Patch 1/2 is a clear memory leak fix.

> This goes on top of ab/sanitize-leak-ci (but not
> en/removing-untracked-fixes). In 1/2 I mark a test as passing under
> SANITIZE=leak, which assumes that an environment variable added by
> ab/sanitize-leak-ci is understood. Without ab/sanitize-leak-ci it'll
> be silently ignored.
>
> Elijah has parallel work in fixing leaks in dir.c, but without his
> ab/sanitize-leak-ci that test will also pass, it's not one of the ones
> that needed something like his leak fixes to {dir,unpack-trees}.c to
> pass.
>
> Ævar Arnfjörð Bjarmason (2):
>   unpack-trees: don't leak memory in verify_clean_subdirectory()
>   built-ins & lib: plug memory leaks with unpack_trees_options_release()
>
>  archive.c                   | 11 ++++++++---
>  builtin/am.c                | 17 ++++++++++++-----
>  builtin/checkout.c          |  9 +++++++--
>  builtin/clone.c             |  1 +
>  builtin/commit.c            |  6 +++++-
>  builtin/merge.c             |  6 ++++--
>  builtin/read-tree.c         | 14 ++++++++++----
>  builtin/reset.c             | 13 +++++++++----
>  builtin/stash.c             | 14 ++++++++++----
>  diff-lib.c                  |  5 ++++-
>  sequencer.c                 |  2 ++
>  t/t1001-read-tree-m-2way.sh |  2 ++
>  unpack-trees.c              |  3 ++-
>  13 files changed, 76 insertions(+), 27 deletions(-)
>
> --
> 2.33.0.1441.gbbcdb4c3c66

[PATCH v2 0/3] unpack-trees: memory-leak fixes

[Ævar Arnfjörð Bjarmason]

These patches fix a couple of stray memory leaks in unpack-trees.c.

This goes on top of ab/sanitize-leak-ci (but not
en/removing-untracked-fixes, although they combine to fix more leaks
in the area).

Changes since v1[1]:

In rebasing v1 from some earlier patches I came up with something that
didn't make sense with related fixes in Elijah's
en/removing-untracked-fixes. This hopefully makes sense:

 * The old 3/3 is gone, but there's a new 2-3/3 which fix the only
   actual leak that was left, i.e. the one in sequencer.c.

 * We might want something like the 3/3 from v1 of this series where
   we call clear_unpack_trees_porcelain() everywhere (and rename it to
   unpack_trees_options_release()) just for good measure and in case
   we'd ever add something to the struct that needs unconditional
   freeing.

   But let's punt on that here and just keep the current
   setup_unpack_trees_porcelain()/clear_unpack_trees_porcelain()
   behavior, callers who don't use setup_unpack_trees_porcelain() but
   use "struct unpack_trees_options" don't need to call any free-like
   function at the end.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  unpack-trees: don't leak memory in verify_clean_subdirectory()
  sequencer: add a "goto cleanup" to do_reset()
  sequencer: fix a memory leak in do_reset()

 sequencer.c                 | 36 +++++++++++++++---------------------
 t/t1001-read-tree-m-2way.sh |  2 ++
 unpack-trees.c              |  3 ++-
 3 files changed, 19 insertions(+), 22 deletions(-)

Range-diff against v1:
1:  e5ef1be2aa9 = 1:  e5ef1be2aa9 unpack-trees: don't leak memory in verify_clean_subdirectory()
2:  21f9da06b46 ! 2:  1d5f5e9fff0 built-ins & lib: plug memory leaks with unpack_trees_options_release()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    built-ins & lib: plug memory leaks with unpack_trees_options_release()
    +    sequencer: add a "goto cleanup" to do_reset()
     
    -    Plug memory leaks in various built-ins and the "diff-lib.c" and
    -    "sequencer.c" libraries that were missing
    -    unpack_trees_options_release() calls.
    +    Restructure code that's mostly added in 9055e401dd6 (sequencer:
    +    introduce new commands to reset the revision, 2018-04-25) to avoid
    +    code duplication, and to make freeing other resources easier in a
    +    subsequent commit.
     
    -    In the case of "git archive" we'll need to memset() the "struct
    -    unpack_trees_options" first, to avoid having to call
    -    clear_unpack_trees_porcelain() twice within the
    -    "!args->worktree_attributes" branch.
    +    It's safe to initialize "tree_desc" to be zero'd out in order to
    +    unconditionally free desc.buffer, it won't be initialized on the first
    +    couple of "goto"'s.
    +
    +    There are three earlier "return"'s in this function that I'm not
    +    bothering to covert, those don't need to rollback anything, or free
    +    any resources, so let's leave, even though they could safely "goto
    +    cleanup" as well.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## archive.c ##
    -@@ archive.c: int write_archive_entries(struct archiver_args *args,
    - 	/*
    - 	 * Setup index and instruct attr to read index only
    - 	 */
    -+	memset(&opts, 0, sizeof(opts));
    - 	if (!args->worktree_attributes) {
    --		memset(&opts, 0, sizeof(opts));
    - 		opts.index_only = 1;
    - 		opts.head_idx = -1;
    - 		opts.src_index = args->repo->index;
    - 		opts.dst_index = args->repo->index;
    - 		opts.fn = oneway_merge;
    - 		init_tree_desc(&t, args->tree->buffer, args->tree->size);
    --		if (unpack_trees(1, &t, &opts))
    + ## sequencer.c ##
    +@@ sequencer.c: static int do_reset(struct repository *r,
    + 	struct strbuf ref_name = STRBUF_INIT;
    + 	struct object_id oid;
    + 	struct lock_file lock = LOCK_INIT;
    +-	struct tree_desc desc;
    ++	struct tree_desc desc = { 0 };
    + 	struct tree *tree;
    + 	struct unpack_trees_options unpack_tree_opts;
    + 	int ret = 0;
    +@@ sequencer.c: static int do_reset(struct repository *r,
    + 		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
    + 		if (get_oid(ref_name.buf, &oid) &&
    + 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
    +-			error(_("could not read '%s'"), ref_name.buf);
    +-			rollback_lock_file(&lock);
    +-			strbuf_release(&ref_name);
     -			return -1;
    -+		if (unpack_trees(1, &t, &opts)) {
    -+			err = -1;
    ++			ret = error(_("could not read '%s'"), ref_name.buf);
     +			goto cleanup;
    -+		}
    - 		git_attr_set_direction(GIT_ATTR_INDEX);
    + 		}
      	}
      
    -@@ archive.c: int write_archive_entries(struct archiver_args *args,
    - 		if (err)
    - 			break;
    - 	}
    -+
    -+cleanup:
    - 	strbuf_release(&path_in_archive);
    - 	strbuf_release(&content);
    -+	clear_unpack_trees_porcelain(&opts);
    - 
    - 	return err;
    - }
    -
    - ## builtin/am.c ##
    -@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
    - 	struct lock_file lock_file = LOCK_INIT;
    - 	struct unpack_trees_options opts;
    - 	struct tree_desc t[2];
    -+	int ret = 0;
    - 
    - 	if (parse_tree(head) || parse_tree(remote))
    - 		return -1;
    -@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
    +@@ sequencer.c: static int do_reset(struct repository *r,
    + 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
      
    - 	if (unpack_trees(2, t, &opts)) {
    - 		rollback_lock_file(&lock_file);
    --		return -1;
    -+		ret = -1;
    + 	if (repo_read_index_unmerged(r)) {
    +-		rollback_lock_file(&lock);
    +-		strbuf_release(&ref_name);
    +-		return error_resolve_conflict(_(action_name(opts)));
    ++		ret = error_resolve_conflict(_(action_name(opts)));
     +		goto cleanup;
      	}
      
    - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
    - 		die(_("unable to write new index file"));
    --
    --	return 0;
    -+cleanup:
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - /**
    -@@ builtin/am.c: static int merge_tree(struct tree *tree)
    - 	struct lock_file lock_file = LOCK_INIT;
    - 	struct unpack_trees_options opts;
    - 	struct tree_desc t[1];
    -+	int ret = 0;
    - 
    - 	if (parse_tree(tree))
    - 		return -1;
    -@@ builtin/am.c: static int merge_tree(struct tree *tree)
    - 
    - 	if (unpack_trees(1, t, &opts)) {
    - 		rollback_lock_file(&lock_file);
    + 	if (!fill_tree_descriptor(r, &desc, &oid)) {
    +-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
    +-		rollback_lock_file(&lock);
    +-		free((void *)desc.buffer);
    +-		strbuf_release(&ref_name);
     -		return -1;
    -+		ret = -1;
    ++		ret = error(_("failed to find tree of %s"), oid_to_hex(&oid));
     +		goto cleanup;
      	}
      
    - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
    - 		die(_("unable to write new index file"));
    - 
    --	return 0;
    -+cleanup:
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - /**
    -
    - ## builtin/checkout.c ##
    -@@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,
    - {
    - 	struct unpack_trees_options opts;
    - 	struct tree_desc tree_desc;
    -+	int ret;
    - 
    - 	memset(&opts, 0, sizeof(opts));
    - 	opts.head_idx = -1;
    -@@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,
    - 		 */
    - 		/* fallthrough */
    - 	case 0:
    --		return 0;
    -+		ret = 0;
    -+		break;
    - 	default:
    --		return 128;
    -+		ret = 128;
    - 	}
    -+
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - static void setup_branch_path(struct branch_info *branch)
    -
    - ## builtin/clone.c ##
    -@@ builtin/clone.c: static int checkout(int submodule_progress)
    - 	init_tree_desc(&t, tree->buffer, tree->size);
    - 	if (unpack_trees(1, &t, &opts) < 0)
    - 		die(_("unable to checkout working tree"));
    -+	clear_unpack_trees_porcelain(&opts);
    - 
    - 	free(head);
    - 
    -
    - ## builtin/commit.c ##
    -@@ builtin/commit.c: static void create_base_index(const struct commit *current_head)
    - 	struct tree *tree;
    - 	struct unpack_trees_options opts;
    - 	struct tree_desc t;
    -+	int exit_early = 0;
    - 
    - 	if (!current_head) {
    - 		discard_cache();
    -@@ builtin/commit.c: static void create_base_index(const struct commit *current_head)
    - 	parse_tree(tree);
    - 	init_tree_desc(&t, tree->buffer, tree->size);
    - 	if (unpack_trees(1, &t, &opts))
    --		exit(128); /* We've already reported the error, finish dying */
    -+		exit_early = 1; /* We've already reported the error, finish dying */
    -+	clear_unpack_trees_porcelain(&opts);
    -+	if (exit_early)
    -+		exit(128);
    - }
    - 
    - static void refresh_cache_or_die(int refresh_flags)
    -
    - ## builtin/merge.c ##
    -@@ builtin/merge.c: static int read_tree_trivial(struct object_id *common, struct object_id *head,
    - 	struct tree *trees[MAX_UNPACK_TREES];
    - 	struct tree_desc t[MAX_UNPACK_TREES];
    - 	struct unpack_trees_options opts;
    -+	int ret = 0;
    - 
    - 	memset(&opts, 0, sizeof(opts));
    - 	opts.head_idx = 2;
    -@@ builtin/merge.c: static int read_tree_trivial(struct object_id *common, struct object_id *head,
    - 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
    - 	}
    - 	if (unpack_trees(nr_trees, t, &opts))
    + 	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
    +-		rollback_lock_file(&lock);
    +-		free((void *)desc.buffer);
    +-		strbuf_release(&ref_name);
     -		return -1;
    --	return 0;
     +		ret = -1;
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - static void write_tree_trivial(struct object_id *oid)
    -
    - ## builtin/read-tree.c ##
    -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
    - 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
    - 		OPT_END()
    - 	};
    -+	int ret = 0;
    - 
    - 	memset(&opts, 0, sizeof(opts));
    - 	opts.head_idx = -1;
    -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
    - 		parse_tree(tree);
    - 		init_tree_desc(t+i, tree->buffer, tree->size);
    - 	}
    --	if (unpack_trees(nr_trees, t, &opts))
    --		return 128;
    -+	if (unpack_trees(nr_trees, t, &opts)) {
    -+		ret = 128;
     +		goto cleanup;
    -+	}
    - 
    - 	if (opts.debug_unpack || opts.dry_run)
    --		return 0; /* do not write the index out */
    -+		goto cleanup; /* do not write the index out */
    - 
    - 	/*
    - 	 * When reading only one tree (either the most basic form,
    -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
    - 
    - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
    - 		die("unable to write new index file");
    --	return 0;
    -+
    -+cleanup:
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    -
    - ## builtin/reset.c ##
    -@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t
    - 
    - 	if (reset_type == KEEP) {
    - 		struct object_id head_oid;
    --		if (get_oid("HEAD", &head_oid))
    --			return error(_("You do not have a valid HEAD."));
    --		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
    --			return error(_("Failed to find tree of HEAD."));
    -+		if (get_oid("HEAD", &head_oid)) {
    -+			error(_("You do not have a valid HEAD."));
    -+			goto out;
    -+		}
    -+		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
    -+			error(_("Failed to find tree of HEAD."));
    -+			goto out;
    -+		}
    - 		nr++;
    - 		opts.fn = twoway_merge;
      	}
    -@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t
    - 	ret = 0;
    - 
    - out:
    -+	clear_unpack_trees_porcelain(&opts);
    - 	for (i = 0; i < nr; i++)
    - 		free((void *)desc[i].buffer);
    - 	return ret;
    -
    - ## builtin/stash.c ##
    -@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)
    - 	struct tree_desc t[MAX_UNPACK_TREES];
    - 	struct tree *tree;
    - 	struct lock_file lock_file = LOCK_INIT;
    -+	int ret = 0;
    - 
    - 	read_cache_preload(NULL);
    - 	if (refresh_cache(REFRESH_QUIET))
    -@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)
    - 		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
    - 	opts.fn = oneway_merge;
    - 
    --	if (unpack_trees(nr_trees, t, &opts))
    --		return -1;
    -+	if (unpack_trees(nr_trees, t, &opts)) {
    -+		ret = -1;
    -+		goto cleanup;
    -+	}
    - 
    - 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
    --		return error(_("unable to write new index file"));
    -+		ret = error(_("unable to write new index file"));
      
    --	return 0;
    -+cleanup:
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
    -@@ builtin/stash.c: static void diff_include_untracked(const struct stash_info *info, struct diff_op
    - 
    - 	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
    - 		die(_("failed to unpack trees"));
    -+	clear_unpack_trees_porcelain(&unpack_tree_opt);
    - 
    - 	do_diff_cache(&info->b_commit, diff_opt);
    - }
    -
    - ## diff-lib.c ##
    -@@ diff-lib.c: static int diff_cache(struct rev_info *revs,
    - 	struct tree *tree;
    - 	struct tree_desc t;
    - 	struct unpack_trees_options opts;
    -+	int ret;
    - 
    - 	tree = parse_tree_indirect(tree_oid);
    - 	if (!tree)
    -@@ diff-lib.c: static int diff_cache(struct rev_info *revs,
    - 	opts.pathspec->recursive = 1;
    - 
    - 	init_tree_desc(&t, tree->buffer, tree->size);
    --	return unpack_trees(1, &t, &opts);
    -+	ret = unpack_trees(1, &t, &opts);
    -+	clear_unpack_trees_porcelain(&opts);
    -+	return ret;
    - }
    - 
    - void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
    -
    - ## sequencer.c ##
    + 	tree = parse_tree_indirect(&oid);
     @@ sequencer.c: static int do_reset(struct repository *r,
    - 		rollback_lock_file(&lock);
    - 		free((void *)desc.buffer);
    - 		strbuf_release(&ref_name);
    -+		clear_unpack_trees_porcelain(&unpack_tree_opts);
    - 		return -1;
    - 	}
      
    -@@ sequencer.c: static int do_reset(struct repository *r,
    - 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
    + 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
    + 		ret = error(_("could not write index"));
    +-	free((void *)desc.buffer);
      
    + 	if (!ret)
    + 		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
    + 						len, name), "HEAD", &oid,
    + 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
    +-
    ++cleanup:
    ++	free((void *)desc.buffer);
    ++	if (ret < 0)
    ++		rollback_lock_file(&lock);
      	strbuf_release(&ref_name);
    -+	clear_unpack_trees_porcelain(&unpack_tree_opts);
      	return ret;
      }
    - 
-:  ----------- > 3:  66ae63db8fd sequencer: fix a memory leak in do_reset()
-- 
2.33.0.1446.g6af949f83bd

[PATCH v2 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()

[Ævar Arnfjörð Bjarmason]

Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.

 * "pathbuf": When the read_directory() call followed by the
   free(pathbuf) was added in c81935348be (Fix switching to a branch
   with D/F when current branch has file D., 2007-03-15) we didn't
   bother to free() before we called die().

   But when this code was later libified in 203a2fe1170 (Allow callers
   of unpack_trees() to handle failure, 2008-02-07) we started to leak
   as we returned data to the caller. This fixes that memory leak,
   which can be observed under SANITIZE=leak with e.g. the
   "t1001-read-tree-m-2way.sh" test.

 * "struct dir_struct": We've leaked the dir_struct ever since this
   code was added back in c81935348be.

   When that commit was written there wasn't an equivalent of
   dir_clear(). Since it was added in 270be816049 (dir.c: provide
   clear_directory() for reclaiming dir_struct memory, 2013-01-06)
   we've omitted freeing the memory allocated here.

   This memory leak could also be observed under SANITIZE=leak and the
   "t1001-read-tree-m-2way.sh" test.

This makes all the test in "t1001-read-tree-m-2way.sh" pass under
"GIT_TEST_PASSING_SANITIZE_LEAK=true", we'd previously die in tests
25, 26 & 28.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1001-read-tree-m-2way.sh | 2 ++
 unpack-trees.c              | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 1057a96b249..d1115528cb9 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -20,6 +20,8 @@ In the test, these paths are used:
 	rezrov  - in H, deleted in M
 	yomin   - not in H or M
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
diff --git a/unpack-trees.c b/unpack-trees.c
index a7e1712d236..89ca95ce90b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	if (o->dir)
 		d.exclude_per_dir = o->dir->exclude_per_dir;
 	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
+	dir_clear(&d);
+	free(pathbuf);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
-	free(pathbuf);
 	return cnt;
 }
 
-- 
2.33.0.1446.g6af949f83bd

Re: [PATCH v2 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index a7e1712d236..89ca95ce90b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>  	if (o->dir)
>  		d.exclude_per_dir = o->dir->exclude_per_dir;
>  	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> +	dir_clear(&d);
> +	free(pathbuf);
>  	if (i)
>  		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> -	free(pathbuf);
>  	return cnt;
>  }

Looks trivially correct.

[PATCH v2 2/3] sequencer: add a "goto cleanup" to do_reset()

[Ævar Arnfjörð Bjarmason]

Restructure code that's mostly added in 9055e401dd6 (sequencer:
introduce new commands to reset the revision, 2018-04-25) to avoid
code duplication, and to make freeing other resources easier in a
subsequent commit.

It's safe to initialize "tree_desc" to be zero'd out in order to
unconditionally free desc.buffer, it won't be initialized on the first
couple of "goto"'s.

There are three earlier "return"'s in this function that I'm not
bothering to covert, those don't need to rollback anything, or free
any resources, so let's leave, even though they could safely "goto
cleanup" as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 6872b7b00a4..457eba4ab10 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3650,7 +3650,7 @@ static int do_reset(struct repository *r,
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
 	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
+	struct tree_desc desc = { 0 };
 	struct tree *tree;
 	struct unpack_trees_options unpack_tree_opts;
 	int ret = 0;
@@ -3684,10 +3684,8 @@ static int do_reset(struct repository *r,
 		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
-			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
-			strbuf_release(&ref_name);
-			return -1;
+			ret = error(_("could not read '%s'"), ref_name.buf);
+			goto cleanup;
 		}
 	}
 
@@ -3703,24 +3701,18 @@ static int do_reset(struct repository *r,
 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
 
 	if (repo_read_index_unmerged(r)) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
+		ret = error_resolve_conflict(_(action_name(opts)));
+		goto cleanup;
 	}
 
 	if (!fill_tree_descriptor(r, &desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
+		ret = error(_("failed to find tree of %s"), oid_to_hex(&oid));
+		goto cleanup;
 	}
 
 	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	tree = parse_tree_indirect(&oid);
@@ -3728,13 +3720,15 @@ static int do_reset(struct repository *r,
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
 		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
 
 	if (!ret)
 		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
 						len, name), "HEAD", &oid,
 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
+cleanup:
+	free((void *)desc.buffer);
+	if (ret < 0)
+		rollback_lock_file(&lock);
 	strbuf_release(&ref_name);
 	return ret;
 }
-- 
2.33.0.1446.g6af949f83bd

Re: [PATCH v2 2/3] sequencer: add a "goto cleanup" to do_reset()

[Elijah Newren]

On Thu, Oct 7, 2021 at 2:46 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Restructure code that's mostly added in 9055e401dd6 (sequencer:
> introduce new commands to reset the revision, 2018-04-25) to avoid
> code duplication, and to make freeing other resources easier in a
> subsequent commit.

Since the first goto you add precedes the call to
setup_unpack_trees_porcelain(), the next commit will introduce a case
where some code calls clear_unpack_trees_porcelain() without having
first called setup_unpack_trees_porcelain().  With the current code
that is fine since you do initialize unpack_trees_opts to { 0 } first,
and clear_unpack_trees_porcelain() doesn't rely on any special setup,
but I wonder if the naming similarity of the two functions might lead
to future authors to presume they are always called in pairs.

I wonder if we should rename clear_unpack_trees_porcelain() to e.g.
unpack_trees_clear() just to avoid this problem, and then include the
other bits of your previous round to just make all callers of
unpack_trees() call unpack_trees_clear() when they are done.

Or, maybe we could just make unpack_trees() call unpack_trees_clear()
automatically and remove that responsibility from callers...except
that merge-recursive is weird and special and would need a new
function (unpack_trees_without_automatic_cleanup() or something like
that?) since it's the one caller that needs to use
unpack_trees_options data after calling unpack_trees().

So, multiple competing ideas here, and all for theoretical future code
safety.  As such, if any of it sounds like too much of a pain, I'm
fine with punting on it.

> It's safe to initialize "tree_desc" to be zero'd out in order to
> unconditionally free desc.buffer, it won't be initialized on the first
> couple of "goto"'s.
>
> There are three earlier "return"'s in this function that I'm not
> bothering to covert,

s/covert/convert/

> those don't need to rollback anything,

They aren't currently rolling back the lockfile right now, but I think
they should.  Those error cases are pretty unlikely to happen (hard
disk full?), but if they did, they'd leave the lock file in place.

Granted, that's a pre-existing problem rather than a problem
introduced by your patch, so it doesn't need to be part of this
series, but the commit message should at least be tweaked.

> or free any resources

True.

> , so let's leave, even though they could safely "goto
> cleanup" as well.



>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  sequencer.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 6872b7b00a4..457eba4ab10 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3650,7 +3650,7 @@ static int do_reset(struct repository *r,
>         struct strbuf ref_name = STRBUF_INIT;
>         struct object_id oid;
>         struct lock_file lock = LOCK_INIT;
> -       struct tree_desc desc;
> +       struct tree_desc desc = { 0 };
>         struct tree *tree;
>         struct unpack_trees_options unpack_tree_opts;
>         int ret = 0;
> @@ -3684,10 +3684,8 @@ static int do_reset(struct repository *r,
>                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>                 if (get_oid(ref_name.buf, &oid) &&
>                     get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
> -                       error(_("could not read '%s'"), ref_name.buf);
> -                       rollback_lock_file(&lock);
> -                       strbuf_release(&ref_name);
> -                       return -1;
> +                       ret = error(_("could not read '%s'"), ref_name.buf);
> +                       goto cleanup;
>                 }
>         }
>
> @@ -3703,24 +3701,18 @@ static int do_reset(struct repository *r,
>         init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
>
>         if (repo_read_index_unmerged(r)) {
> -               rollback_lock_file(&lock);
> -               strbuf_release(&ref_name);
> -               return error_resolve_conflict(_(action_name(opts)));
> +               ret = error_resolve_conflict(_(action_name(opts)));
> +               goto cleanup;
>         }
>
>         if (!fill_tree_descriptor(r, &desc, &oid)) {
> -               error(_("failed to find tree of %s"), oid_to_hex(&oid));
> -               rollback_lock_file(&lock);
> -               free((void *)desc.buffer);
> -               strbuf_release(&ref_name);
> -               return -1;
> +               ret = error(_("failed to find tree of %s"), oid_to_hex(&oid));
> +               goto cleanup;
>         }
>
>         if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> -               rollback_lock_file(&lock);
> -               free((void *)desc.buffer);
> -               strbuf_release(&ref_name);
> -               return -1;
> +               ret = -1;
> +               goto cleanup;
>         }
>
>         tree = parse_tree_indirect(&oid);
> @@ -3728,13 +3720,15 @@ static int do_reset(struct repository *r,
>
>         if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
>                 ret = error(_("could not write index"));
> -       free((void *)desc.buffer);
>
>         if (!ret)
>                 ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
>                                                 len, name), "HEAD", &oid,
>                                  NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> -
> +cleanup:
> +       free((void *)desc.buffer);
> +       if (ret < 0)
> +               rollback_lock_file(&lock);
>         strbuf_release(&ref_name);
>         return ret;
>  }
> --
> 2.33.0.1446.g6af949f83bd
>

[PATCH v2 3/3] sequencer: fix a memory leak in do_reset()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 9055e401dd6 (sequencer: introduce new
commands to reset the revision, 2018-04-25), which called
setup_unpack_trees_porcelain() without a corresponding call to
clear_unpack_trees_porcelain().

This inches us closer to passing various tests in
"t34*.sh" (e.g. "t3434-rebase-i18n.sh"), but because they have so many
other memory leaks in revisions.c this doesn't make any test file or
even a single test pass.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 457eba4ab10..fefe5a601f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3652,7 +3652,7 @@ static int do_reset(struct repository *r,
 	struct lock_file lock = LOCK_INIT;
 	struct tree_desc desc = { 0 };
 	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
+	struct unpack_trees_options unpack_tree_opts = { 0 };
 	int ret = 0;
 
 	if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)
@@ -3689,7 +3689,6 @@ static int do_reset(struct repository *r,
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;
@@ -3730,6 +3729,7 @@ static int do_reset(struct repository *r,
 	if (ret < 0)
 		rollback_lock_file(&lock);
 	strbuf_release(&ref_name);
+	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	return ret;
 }
 
-- 
2.33.0.1446.g6af949f83bd

Re: [PATCH v2 0/3] unpack-trees: memory-leak fixes

[Elijah Newren]

On Thu, Oct 7, 2021 at 2:46 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> These patches fix a couple of stray memory leaks in unpack-trees.c.
>
> This goes on top of ab/sanitize-leak-ci (but not
> en/removing-untracked-fixes, although they combine to fix more leaks
> in the area).
>
> Changes since v1[1]:
>
> In rebasing v1 from some earlier patches I came up with something that
> didn't make sense with related fixes in Elijah's
> en/removing-untracked-fixes. This hopefully makes sense:
>
>  * The old 3/3 is gone, but there's a new 2-3/3 which fix the only
>    actual leak that was left, i.e. the one in sequencer.c.
>
>  * We might want something like the 3/3 from v1 of this series where
>    we call clear_unpack_trees_porcelain() everywhere (and rename it to
>    unpack_trees_options_release()) just for good measure and in case
>    we'd ever add something to the struct that needs unconditional
>    freeing.
>
>    But let's punt on that here and just keep the current
>    setup_unpack_trees_porcelain()/clear_unpack_trees_porcelain()
>    behavior, callers who don't use setup_unpack_trees_porcelain() but
>    use "struct unpack_trees_options" don't need to call any free-like
>    function at the end.
>
> 1. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com/
>
> Ævar Arnfjörð Bjarmason (3):
>   unpack-trees: don't leak memory in verify_clean_subdirectory()
>   sequencer: add a "goto cleanup" to do_reset()
>   sequencer: fix a memory leak in do_reset()

Left a bunch of comments on patch 2.  The upshot is anywhere from "the
commit message just needs a few tweaks" to "well, maybe we should
clean up these other things too, and we could restructure some related
code...".  I'll let you pick where in that spectrum you want to take
things, because the other possible changes I mention beyond fixing the
commit message don't need to hold up this series.

[PATCH v3 0/3] unpack-trees: memory-leak fixes

[Ævar Arnfjörð Bjarmason]

This commit-message-only-change to v2 should address the comments
Elijah had on v2 (see link in the range-diff). I.e. there's probably
bug in adjacent code in leaving a stale lockfile, but I'm punting on
that and just narrowly fixing a memory leak here.

Ævar Arnfjörð Bjarmason (3):
  unpack-trees: don't leak memory in verify_clean_subdirectory()
  sequencer: add a "goto cleanup" to do_reset()
  sequencer: fix a memory leak in do_reset()

 sequencer.c                 | 36 +++++++++++++++---------------------
 t/t1001-read-tree-m-2way.sh |  2 ++
 unpack-trees.c              |  3 ++-
 3 files changed, 19 insertions(+), 22 deletions(-)

Range-diff against v2:
1:  e5ef1be2aa9 = 1:  0ab1e74f50d unpack-trees: don't leak memory in verify_clean_subdirectory()
2:  1d5f5e9fff0 ! 2:  393937e8a98 sequencer: add a "goto cleanup" to do_reset()
    @@ Commit message
         unconditionally free desc.buffer, it won't be initialized on the first
         couple of "goto"'s.
     
    -    There are three earlier "return"'s in this function that I'm not
    -    bothering to covert, those don't need to rollback anything, or free
    -    any resources, so let's leave, even though they could safely "goto
    -    cleanup" as well.
    +    There are three earlier "return"'s in this function which should
    +    probably be made to use this new "cleanup" too, per [1] it looks like
    +    they're leaving behind stale locks. But let's not try to fix every
    +    potential bug here now, I'm just trying to narrowly plug a memory
    +    leak.
    +
    +    1. https://lore.kernel.org/git/CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
3:  66ae63db8fd ! 3:  00b6b469a8c sequencer: fix a memory leak in do_reset()
    @@ Commit message
         setup_unpack_trees_porcelain() without a corresponding call to
         clear_unpack_trees_porcelain().
     
    +    This introduces a change in behavior in that we now start calling
    +    clear_unpack_trees_porcelain() even without having called the
    +    setup_unpack_trees_porcelain(). That's OK, that clear function, like
    +    most others, will accept a zero'd out struct.
    +
         This inches us closer to passing various tests in
         "t34*.sh" (e.g. "t3434-rebase-i18n.sh"), but because they have so many
         other memory leaks in revisions.c this doesn't make any test file or
-- 
2.33.0.1569.gd2dc77f7abf

[PATCH v3 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()

[Ævar Arnfjörð Bjarmason]

Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.

 * "pathbuf": When the read_directory() call followed by the
   free(pathbuf) was added in c81935348be (Fix switching to a branch
   with D/F when current branch has file D., 2007-03-15) we didn't
   bother to free() before we called die().

   But when this code was later libified in 203a2fe1170 (Allow callers
   of unpack_trees() to handle failure, 2008-02-07) we started to leak
   as we returned data to the caller. This fixes that memory leak,
   which can be observed under SANITIZE=leak with e.g. the
   "t1001-read-tree-m-2way.sh" test.

 * "struct dir_struct": We've leaked the dir_struct ever since this
   code was added back in c81935348be.

   When that commit was written there wasn't an equivalent of
   dir_clear(). Since it was added in 270be816049 (dir.c: provide
   clear_directory() for reclaiming dir_struct memory, 2013-01-06)
   we've omitted freeing the memory allocated here.

   This memory leak could also be observed under SANITIZE=leak and the
   "t1001-read-tree-m-2way.sh" test.

This makes all the test in "t1001-read-tree-m-2way.sh" pass under
"GIT_TEST_PASSING_SANITIZE_LEAK=true", we'd previously die in tests
25, 26 & 28.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1001-read-tree-m-2way.sh | 2 ++
 unpack-trees.c              | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 1057a96b249..d1115528cb9 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -20,6 +20,8 @@ In the test, these paths are used:
 	rezrov  - in H, deleted in M
 	yomin   - not in H or M
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
diff --git a/unpack-trees.c b/unpack-trees.c
index a7e1712d236..89ca95ce90b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	if (o->dir)
 		d.exclude_per_dir = o->dir->exclude_per_dir;
 	i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
+	dir_clear(&d);
+	free(pathbuf);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
-	free(pathbuf);
 	return cnt;
 }
 
-- 
2.33.0.1569.gd2dc77f7abf

[PATCH v3 2/3] sequencer: add a "goto cleanup" to do_reset()

[Ævar Arnfjörð Bjarmason]

Restructure code that's mostly added in 9055e401dd6 (sequencer:
introduce new commands to reset the revision, 2018-04-25) to avoid
code duplication, and to make freeing other resources easier in a
subsequent commit.

It's safe to initialize "tree_desc" to be zero'd out in order to
unconditionally free desc.buffer, it won't be initialized on the first
couple of "goto"'s.

There are three earlier "return"'s in this function which should
probably be made to use this new "cleanup" too, per [1] it looks like
they're leaving behind stale locks. But let's not try to fix every
potential bug here now, I'm just trying to narrowly plug a memory
leak.

1. https://lore.kernel.org/git/CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b92dae12166..14c37c4e25b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3644,7 +3644,7 @@ static int do_reset(struct repository *r,
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
 	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
+	struct tree_desc desc = { 0 };
 	struct tree *tree;
 	struct unpack_trees_options unpack_tree_opts;
 	int ret = 0;
@@ -3678,10 +3678,8 @@ static int do_reset(struct repository *r,
 		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
-			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
-			strbuf_release(&ref_name);
-			return -1;
+			ret = error(_("could not read '%s'"), ref_name.buf);
+			goto cleanup;
 		}
 	}
 
@@ -3697,24 +3695,18 @@ static int do_reset(struct repository *r,
 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
 
 	if (repo_read_index_unmerged(r)) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
+		ret = error_resolve_conflict(_(action_name(opts)));
+		goto cleanup;
 	}
 
 	if (!fill_tree_descriptor(r, &desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
+		ret = error(_("failed to find tree of %s"), oid_to_hex(&oid));
+		goto cleanup;
 	}
 
 	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	tree = parse_tree_indirect(&oid);
@@ -3722,13 +3714,15 @@ static int do_reset(struct repository *r,
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
 		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
 
 	if (!ret)
 		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
 						len, name), "HEAD", &oid,
 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
+cleanup:
+	free((void *)desc.buffer);
+	if (ret < 0)
+		rollback_lock_file(&lock);
 	strbuf_release(&ref_name);
 	return ret;
 }
-- 
2.33.0.1569.gd2dc77f7abf

[PATCH v3 3/3] sequencer: fix a memory leak in do_reset()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak introduced in 9055e401dd6 (sequencer: introduce new
commands to reset the revision, 2018-04-25), which called
setup_unpack_trees_porcelain() without a corresponding call to
clear_unpack_trees_porcelain().

This introduces a change in behavior in that we now start calling
clear_unpack_trees_porcelain() even without having called the
setup_unpack_trees_porcelain(). That's OK, that clear function, like
most others, will accept a zero'd out struct.

This inches us closer to passing various tests in
"t34*.sh" (e.g. "t3434-rebase-i18n.sh"), but because they have so many
other memory leaks in revisions.c this doesn't make any test file or
even a single test pass.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 14c37c4e25b..98181b3186e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3646,7 +3646,7 @@ static int do_reset(struct repository *r,
 	struct lock_file lock = LOCK_INIT;
 	struct tree_desc desc = { 0 };
 	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
+	struct unpack_trees_options unpack_tree_opts = { 0 };
 	int ret = 0;
 
 	if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)
@@ -3683,7 +3683,6 @@ static int do_reset(struct repository *r,
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;
@@ -3724,6 +3723,7 @@ static int do_reset(struct repository *r,
 	if (ret < 0)
 		rollback_lock_file(&lock);
 	strbuf_release(&ref_name);
+	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	return ret;
 }
 
-- 
2.33.0.1569.gd2dc77f7abf