commit: fix leaking parents when calling `commit_tree_extended()`

When creating commits via `commit_tree_extended()`, the caller passes in
a string list of parents. This call implicitly transfers ownership of
that list to the function, which is quite surprising to begin with. But
to make matters worse, `commit_tree_extended()` doesn't even bother to
free the list of parents in error cases. The result is a memory leak,
and one that the caller cannot fix by themselves because they do not
know whether parts of the string list have already been released.

Refactor the code such that callers can keep ownership of the list of
parents, which is getting indicated by parameter being a constant
pointer now. Free the lists at the calling site and add a common exit
path to those sites as required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/am.c b/builtin/am.c
index 4ba44e2d70619b7e017f75e50374f42b33119ffc..faccc45b13611c2052d57157d93faf54e5251fce 100644 (file)
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1718,6 +1718,7 @@ static void do_commit(const struct am_state *state)
 
        run_hooks("post-applypatch");
 
+       free_commit_list(parents);
        strbuf_release(&sb);
 }
 

diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 1bb78198392e9e13db117e87a4d421e17d10b26a..84bb4502229af0ef20365d329ae72865d4285f34 100644 (file)
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -111,6 +111,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
                        N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
                OPT_END()
        };
+       int ret;
 
        git_config(git_default_config, NULL);
 
@@ -132,11 +133,15 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 
        if (commit_tree(buffer.buf, buffer.len, &tree_oid, parents, &commit_oid,
                        NULL, sign_commit)) {
-               strbuf_release(&buffer);
-               return 1;
+               ret = 1;
+               goto out;
        }
 
        printf("%s\n", oid_to_hex(&commit_oid));
+       ret = 0;
+
+out:
+       free_commit_list(parents);
        strbuf_release(&buffer);
-       return 0;
+       return ret;
 }

diff --git a/builtin/commit.c b/builtin/commit.c
index dcaf4efa035abc8f16e2c46c1b00f13f1183d453..d5713455e5fc23b25d7133e34889a2bd5cea322f 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1848,7 +1848,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
                rollback_index_files();
                die(_("failed to write commit object"));
        }
-       free_commit_extra_headers(extra);
 
        if (update_head_with_reflog(current_head, &oid, reflog_msg, &sb,
                                    &err)) {
@@ -1890,6 +1889,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
        apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 
 cleanup:
+       free_commit_extra_headers(extra);
+       free_commit_list(parents);
        strbuf_release(&author_ident);
        strbuf_release(&err);
        strbuf_release(&sb);

diff --git a/builtin/merge.c b/builtin/merge.c
index daed2d4e1e2beb5da58f4d418c2b10e63d5ad36b..50b0c87a95f0e8ad7ceed355aabc6e7c7b7b6bc5 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -895,7 +895,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
        struct object_id result_tree, result_commit;
-       struct commit_list *parents, **pptr = &parents;
+       struct commit_list *parents = NULL, **pptr = &parents;
 
        if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET,
                                         SKIP_IF_UNCHANGED, 0, NULL, NULL,
@@ -911,7 +911,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
                        &result_commit, NULL, sign_commit))
                die(_("failed to write commit object"));
        finish(head, remoteheads, &result_commit, "In-index merge");
+
        remove_merge_branch_state(the_repository);
+       free_commit_list(parents);
        return 0;
 }
 
@@ -937,8 +939,10 @@ static int finish_automerge(struct commit *head,
                die(_("failed to write commit object"));
        strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
        finish(head, remoteheads, &result_commit, buf.buf);
+
        strbuf_release(&buf);
        remove_merge_branch_state(the_repository);
+       free_commit_list(parents);
        return 0;
 }
 

diff --git a/builtin/replay.c b/builtin/replay.c
index 6bf0691f15d44e1cac28701edfa9035f37d483e6..04483266367bb677d8459e700e0de8a2d133c6c5 100644 (file)
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -52,11 +52,11 @@ static struct commit *create_commit(struct tree *tree,
                                    struct commit *parent)
 {
        struct object_id ret;
-       struct object *obj;
+       struct object *obj = NULL;
        struct commit_list *parents = NULL;
        char *author;
        char *sign_commit = NULL; /* FIXME: cli users might want to sign again */
-       struct commit_extra_header *extra;
+       struct commit_extra_header *extra = NULL;
        struct strbuf msg = STRBUF_INIT;
        const char *out_enc = get_commit_output_encoding();
        const char *message = repo_logmsg_reencode(the_repository, based_on,
@@ -73,12 +73,16 @@ static struct commit *create_commit(struct tree *tree,
        if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
                                 &ret, author, NULL, sign_commit, extra)) {
                error(_("failed to write commit object"));
-               return NULL;
+               goto out;
        }
-       free(author);
-       strbuf_release(&msg);
 
        obj = parse_object(the_repository, &ret);
+
+out:
+       free_commit_extra_headers(extra);
+       free_commit_list(parents);
+       strbuf_release(&msg);
+       free(author);
        return (struct commit *)obj;
 }
 

diff --git a/builtin/stash.c b/builtin/stash.c
index 1ed0a9a5d967fd6b372e41fae7d0c168b2f0b258..46b981c4dd3cd998aa443d5f7c7652aa9a628dd2 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1416,6 +1416,9 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
                goto done;
        }
 
+       free_commit_list(parents);
+       parents = NULL;
+
        if (include_untracked) {
                if (save_untracked_files(info, &msg, untracked_files)) {
                        if (!quiet)
@@ -1461,11 +1464,6 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
        else
                strbuf_insertf(stash_msg_buf, 0, "On %s: ", branch_name);
 
-       /*
-        * `parents` will be empty after calling `commit_tree()`, so there is
-        * no need to call `free_commit_list()`
-        */
-       parents = NULL;
        if (untracked_commit_option)
                commit_list_insert(lookup_commit(the_repository,
                                                 &info->u_commit),
@@ -1487,6 +1485,7 @@ done:
        strbuf_release(&commit_tree_label);
        strbuf_release(&msg);
        strbuf_release(&untracked_files);
+       free_commit_list(parents);
        return ret;
 }
 

diff --git a/commit.c b/commit.c
index 1d08951007bcd9c3722737ef7b4b6ba3ada2f5f5..f674eca32051581db5daf89914da444644a93e92 100644 (file)
--- a/commit.c
+++ b/commit.c
@@ -1262,7 +1262,7 @@ int remove_signature(struct strbuf *buf)
        return sigs[0].start != NULL;
 }
 
-static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)
+static void handle_signed_tag(const struct commit *parent, struct commit_extra_header ***tail)
 {
        struct merge_remote_desc *desc;
        struct commit_extra_header *mergetag;
@@ -1359,17 +1359,17 @@ void verify_merge_signature(struct commit *commit, int verbosity,
        signature_check_clear(&signature_check);
 }
 
-void append_merge_tag_headers(struct commit_list *parents,
+void append_merge_tag_headers(const struct commit_list *parents,
                              struct commit_extra_header ***tail)
 {
        while (parents) {
-               struct commit *parent = parents->item;
+               const struct commit *parent = parents->item;
                handle_signed_tag(parent, tail);
                parents = parents->next;
        }
 }
 
-static int convert_commit_extra_headers(struct commit_extra_header *orig,
+static int convert_commit_extra_headers(const struct commit_extra_header *orig,
                                        struct commit_extra_header **result)
 {
        const struct git_hash_algo *compat = the_repository->compat_hash_algo;
@@ -1403,7 +1403,7 @@ static int convert_commit_extra_headers(struct commit_extra_header *orig,
 }
 
 static void add_extra_header(struct strbuf *buffer,
-                            struct commit_extra_header *extra)
+                            const struct commit_extra_header *extra)
 {
        strbuf_addstr(buffer, extra->key);
        if (extra->len)
@@ -1517,7 +1517,7 @@ void free_commit_extra_headers(struct commit_extra_header *extra)
 }
 
 int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,
-               struct commit_list *parents, struct object_id *ret,
+               const struct commit_list *parents, struct object_id *ret,
                const char *author, const char *sign_commit)
 {
        struct commit_extra_header *extra = NULL, **tail = &extra;
@@ -1649,7 +1649,7 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg
                              const struct object_id *tree,
                              const struct object_id *parents, size_t parents_len,
                              const char *author, const char *committer,
-                             struct commit_extra_header *extra)
+                             const struct commit_extra_header *extra)
 {
        int encoding_is_utf8;
        size_t i;
@@ -1690,10 +1690,10 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg
 
 int commit_tree_extended(const char *msg, size_t msg_len,
                         const struct object_id *tree,
-                        struct commit_list *parents, struct object_id *ret,
+                        const struct commit_list *parents, struct object_id *ret,
                         const char *author, const char *committer,
                         const char *sign_commit,
-                        struct commit_extra_header *extra)
+                        const struct commit_extra_header *extra)
 {
        struct repository *r = the_repository;
        int result = 0;
@@ -1715,10 +1715,8 @@ int commit_tree_extended(const char *msg, size_t msg_len,
        nparents = commit_list_count(parents);
        CALLOC_ARRAY(parent_buf, nparents);
        i = 0;
-       while (parents) {
-               struct commit *parent = pop_commit(&parents);
-               oidcpy(&parent_buf[i++], &parent->object.oid);
-       }
+       for (const struct commit_list *p = parents; p; p = p->next)
+               oidcpy(&parent_buf[i++], &p->item->object.oid);
 
        write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra);
        if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) {
@@ -1814,7 +1812,7 @@ out:
 define_commit_slab(merge_desc_slab, struct merge_remote_desc *);
 static struct merge_desc_slab merge_desc_slab = COMMIT_SLAB_INIT(1, merge_desc_slab);
 
-struct merge_remote_desc *merge_remote_util(struct commit *commit)
+struct merge_remote_desc *merge_remote_util(const struct commit *commit)
 {
        return *merge_desc_slab_at(&merge_desc_slab, commit);
 }

diff --git a/commit.h b/commit.h
index 62fe0d77a70738048858e4087533b045fcc40613..442e50ff2450a111bfbb9819943a0aca76338d67 100644 (file)
--- a/commit.h
+++ b/commit.h
@@ -260,19 +260,19 @@ struct commit_extra_header {
        size_t len;
 };
 
-void append_merge_tag_headers(struct commit_list *parents,
+void append_merge_tag_headers(const struct commit_list *parents,
                              struct commit_extra_header ***tail);
 
 int commit_tree(const char *msg, size_t msg_len,
                const struct object_id *tree,
-               struct commit_list *parents, struct object_id *ret,
+               const struct commit_list *parents, struct object_id *ret,
                const char *author, const char *sign_commit);
 
 int commit_tree_extended(const char *msg, size_t msg_len,
                         const struct object_id *tree,
-                        struct commit_list *parents, struct object_id *ret,
+                        const struct commit_list *parents, struct object_id *ret,
                         const char *author, const char *committer,
-                        const char *sign_commit, struct commit_extra_header *);
+                        const char *sign_commit, const struct commit_extra_header *);
 
 struct commit_extra_header *read_commit_extra_headers(struct commit *, const char **);
 
@@ -306,7 +306,7 @@ struct merge_remote_desc {
        struct object *obj; /* the named object, could be a tag */
        char name[FLEX_ARRAY];
 };
-struct merge_remote_desc *merge_remote_util(struct commit *);
+struct merge_remote_desc *merge_remote_util(const struct commit *);
 void set_merge_remote_desc(struct commit *commit,
                           const char *name, struct object *obj);
 

diff --git a/notes-merge.c b/notes-merge.c
index 6a9a139b123f23e9c331fc1bbcad75cbb1f97adb..f3cc84f45d441ca7da72e9a729d05bdc165d8ffa 100644 (file)
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -661,6 +661,7 @@ int notes_merge(struct notes_merge_options *o,
                commit_list_insert(local, &parents);
                create_notes_commit(o->repo, local_tree, parents, o->commit_msg.buf,
                                    o->commit_msg.len, result_oid);
+               free_commit_list(parents);
        }
 
 found_result:

diff --git a/notes-utils.c b/notes-utils.c
index 671d1969b1f3ab0e5d24b8de996bd97c013263e4..3198c14e4dff5920c783d86d0d9ab09861a7428b 100644 (file)
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -9,10 +9,11 @@
 
 void create_notes_commit(struct repository *r,
                         struct notes_tree *t,
-                        struct commit_list *parents,
+                        const struct commit_list *parents,
                         const char *msg, size_t msg_len,
                         struct object_id *result_oid)
 {
+       struct commit_list *parents_to_free = NULL;
        struct object_id tree_oid;
 
        assert(t->initialized);
@@ -27,7 +28,8 @@ void create_notes_commit(struct repository *r,
                        struct commit *parent = lookup_commit(r, &parent_oid);
                        if (repo_parse_commit(r, parent))
                                die("Failed to find/parse commit %s", t->ref);
-                       commit_list_insert(parent, &parents);
+                       commit_list_insert(parent, &parents_to_free);
+                       parents = parents_to_free;
                }
                /* else: t->ref points to nothing, assume root/orphan commit */
        }
@@ -35,6 +37,8 @@ void create_notes_commit(struct repository *r,
        if (commit_tree(msg, msg_len, &tree_oid, parents, result_oid, NULL,
                        NULL))
                die("Failed to commit notes tree to database");
+
+       free_commit_list(parents_to_free);
 }
 
 void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)

diff --git a/notes-utils.h b/notes-utils.h
index d9b3c09eaf09819f9444cfcf68ca5443aa918b78..c54b1fe141f4bfa42c01e609444de4495a014d94 100644 (file)
--- a/notes-utils.h
+++ b/notes-utils.h
@@ -20,7 +20,7 @@ struct repository;
  */
 void create_notes_commit(struct repository *r,
                         struct notes_tree *t,
-                        struct commit_list *parents,
+                        const struct commit_list *parents,
                         const char *msg, size_t msg_len,
                         struct object_id *result_oid);
 

diff --git a/sequencer.c b/sequencer.c
index 30513e87bfb9e6c07e61b0b8d36896a5637e72db..475646671a8fdaf6a6056b51461895b606ddfafd 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -1711,6 +1711,7 @@ static int try_to_commit(struct repository *r,
 
 out:
        free_commit_extra_headers(extra);
+       free_commit_list(parents);
        strbuf_release(&err);
        strbuf_release(&commit_msg);
        free(amend_author);

diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index a1911c4a9d60e71f99d66ef6d7f30728ab8732df..4f1d6e8ea642e1a2f6b624ad333385e11036dcba 100755 (executable)
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -8,6 +8,7 @@ test_description='git rebase --merge --skip tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh

diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index 1ee6b00fd57726e055b95903e09f97c1e24ebcd2..515c949ae37269d056e9bf5b0fb88f409eba5c02 100755 (executable)
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase of commits that start or become empty'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup test repository' '

diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 9748443530cd713c8c9d06e6eaa67552a69e978b..ead3fb46807ef33427172dde75a45f5cfa01bfb0 100755 (executable)
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -5,6 +5,7 @@ test_description='test cherry-picking an empty commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 2128142a61c60da918381f6b6f74ca612ac9227b..b88383df9e48e19f76d2f494151a3bd0e7c0fa61 100755 (executable)
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -5,6 +5,7 @@ test_description='prepare-commit-msg hook'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up commits for rebasing' '