| Message ID | cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | unpack-trees: memory-leak fixes | expand |
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.
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()