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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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