@matheusbsbs / matheus-bagliano / commits / 0c52cf8e00

sequencer: add a "goto cleanup" to do_reset()

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Oct 13, 2021 at 15:23 UTC 0c52cf8e00f65bb198800a08caaadc95eaf4419a

1 file changed +13 -19

sequencer.c

+13 -19

@@ -3644,7 +3644,7 @@ static int do_reset(struct repository *r,

3644 struct strbuf ref_name = STRBUF_INIT;

3645 struct object_id oid;

3646 struct lock_file lock = LOCK_INIT;

3647 - struct tree_desc desc;

3647 + struct tree_desc desc = { 0 };

3648 struct tree *tree;

3649 struct unpack_trees_options unpack_tree_opts;

3650 int ret = 0;

@@ -3678,10 +3678,8 @@ static int do_reset(struct repository *r,

3678 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);

3679 if (get_oid(ref_name.buf, &oid) &&

3680 get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {

3681 - error(_("could not read '%s'"), ref_name.buf);

3682 - rollback_lock_file(&lock);

3683 - strbuf_release(&ref_name);

3684 - return -1;

3681 + ret = error(_("could not read '%s'"), ref_name.buf);

3682 + goto cleanup;

3683 }

3684 }

3685

@@ -3696,24 +3694,18 @@ static int do_reset(struct repository *r,

3694 init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);

3695

3696 if (repo_read_index_unmerged(r)) {

3699 - rollback_lock_file(&lock);

3700 - strbuf_release(&ref_name);

3701 - return error_resolve_conflict(_(action_name(opts)));

3697 + ret = error_resolve_conflict(_(action_name(opts)));

3698 + goto cleanup;

3699 }

3700

3701 if (!fill_tree_descriptor(r, &desc, &oid)) {

3705 - error(_("failed to find tree of %s"), oid_to_hex(&oid));

3706 - rollback_lock_file(&lock);

3707 - free((void *)desc.buffer);

3708 - strbuf_release(&ref_name);

3709 - return -1;

3702 + ret = error(_("failed to find tree of %s"), oid_to_hex(&oid));

3703 + goto cleanup;

3704 }

3705

3706 if (unpack_trees(1, &desc, &unpack_tree_opts)) {

3713 - rollback_lock_file(&lock);

3714 - free((void *)desc.buffer);

3715 - strbuf_release(&ref_name);

3716 - return -1;

3707 + ret = -1;

3708 + goto cleanup;

3709 }

3710

3711 tree = parse_tree_indirect(&oid);

@@ -3721,13 +3713,15 @@ static int do_reset(struct repository *r,

3713

3714 if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)

3715 ret = error(_("could not write index"));

3724 - free((void *)desc.buffer);

3716

3717 if (!ret)

3718 ret = update_ref(reflog_message(opts, "reset", "'%.*s'",

3719 len, name), "HEAD", &oid,

3720 NULL, 0, UPDATE_REFS_MSG_ON_ERR);

3730 -

3721 +cleanup:

3722 + free((void *)desc.buffer);

3723 + if (ret < 0)

3724 + rollback_lock_file(&lock);

3725 strbuf_release(&ref_name);

3726 return ret;

3727 }