@matheusbsbs / matheus-bagliano / commits / 63c9bd372e

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>

Patrick Steinhardt committed Jun 11, 2024 at 11:20 UTC 63c9bd372e388f5fed77be56771d5ad972f37f8e

16 files changed +59 -37

builtin/am.c

@@ -1718,6 +1718,7 @@ static void do_commit(const struct am_state *state)

1718

1719 run_hooks("post-applypatch");

1720

1721 + free_commit_list(parents);

1722 strbuf_release(&sb);

1723 }

1724

builtin/commit-tree.c

+8 -3

@@ -111,6 +111,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)

111 N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

112 OPT_END()

113 };

114 + int ret;

115

116 git_config(git_default_config, NULL);

117

@@ -132,11 +133,15 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)

133

134 if (commit_tree(buffer.buf, buffer.len, &tree_oid, parents, &commit_oid,

135 NULL, sign_commit)) {

135 - strbuf_release(&buffer);

136 - return 1;

136 + ret = 1;

137 + goto out;

138 }

139

140 printf("%s\n", oid_to_hex(&commit_oid));

141 + ret = 0;

142 +

143 +out:

144 + free_commit_list(parents);

145 strbuf_release(&buffer);

141 - return 0;

146 + return ret;

147 }

builtin/commit.c

+2 -1

@@ -1848,7 +1848,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1848 rollback_index_files();

1849 die(_("failed to write commit object"));

1850 }

1851 - free_commit_extra_headers(extra);

1851

1852 if (update_head_with_reflog(current_head, &oid, reflog_msg, &sb,

1853 &err)) {

@@ -1890,6 +1889,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1889 apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");

1890

1891 cleanup:

1892 + free_commit_extra_headers(extra);

1893 + free_commit_list(parents);

1894 strbuf_release(&author_ident);

1895 strbuf_release(&err);

1896 strbuf_release(&sb);

builtin/merge.c

+5 -1

@@ -895,7 +895,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)

895 static int merge_trivial(struct commit *head, struct commit_list *remoteheads)

896 {

897 struct object_id result_tree, result_commit;

898 - struct commit_list *parents, **pptr = &parents;

898 + struct commit_list *parents = NULL, **pptr = &parents;

899

900 if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET,

901 SKIP_IF_UNCHANGED, 0, NULL, NULL,

@@ -911,7 +911,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)

911 &result_commit, NULL, sign_commit))

912 die(_("failed to write commit object"));

913 finish(head, remoteheads, &result_commit, "In-index merge");

914 +

915 remove_merge_branch_state(the_repository);

916 + free_commit_list(parents);

917 return 0;

918 }

919

@@ -937,8 +939,10 @@ static int finish_automerge(struct commit *head,

939 die(_("failed to write commit object"));

940 strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);

941 finish(head, remoteheads, &result_commit, buf.buf);

942 +

943 strbuf_release(&buf);

944 remove_merge_branch_state(the_repository);

945 + free_commit_list(parents);

946 return 0;

947 }

948

builtin/replay.c

+9 -5

@@ -52,11 +52,11 @@ static struct commit *create_commit(struct tree *tree,

52 struct commit *parent)

53 {

54 struct object_id ret;

55 - struct object *obj;

55 + struct object *obj = NULL;

56 struct commit_list *parents = NULL;

57 char *author;

58 char *sign_commit = NULL; /* FIXME: cli users might want to sign again */

59 - struct commit_extra_header *extra;

59 + struct commit_extra_header *extra = NULL;

60 struct strbuf msg = STRBUF_INIT;

61 const char *out_enc = get_commit_output_encoding();

62 const char *message = repo_logmsg_reencode(the_repository, based_on,

@@ -73,12 +73,16 @@ static struct commit *create_commit(struct tree *tree,

73 if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,

74 &ret, author, NULL, sign_commit, extra)) {

75 error(_("failed to write commit object"));

76 - return NULL;

76 + goto out;

77 }

78 - free(author);

79 - strbuf_release(&msg);

79 obj = parse_object(the_repository, &ret);

80 +

81 +out:

82 + free_commit_extra_headers(extra);

83 + free_commit_list(parents);

84 + strbuf_release(&msg);

85 + free(author);

86 return (struct commit *)obj;

87 }

builtin/stash.c

+4 -5

@@ -1416,6 +1416,9 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b

1416 goto done;

1417 }

1418

1419 + free_commit_list(parents);

1420 + parents = NULL;

1421 +

1422 if (include_untracked) {

1423 if (save_untracked_files(info, &msg, untracked_files)) {

1424 if (!quiet)

@@ -1461,11 +1464,6 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b

1464 else

1465 strbuf_insertf(stash_msg_buf, 0, "On %s: ", branch_name);

1466

1464 - /*

1465 - * `parents` will be empty after calling `commit_tree()`, so there is

1466 - * no need to call `free_commit_list()`

1467 - */

1468 - parents = NULL;

1467 if (untracked_commit_option)

1468 commit_list_insert(lookup_commit(the_repository,

1469 &info->u_commit),

@@ -1487,6 +1485,7 @@ done:

1485 strbuf_release(&commit_tree_label);

1486 strbuf_release(&msg);

1487 strbuf_release(&untracked_files);

1488 + free_commit_list(parents);

1489 return ret;

1490 }

1491

commit.c

+12 -14

@@ -1262,7 +1262,7 @@ int remove_signature(struct strbuf *buf)

1262 return sigs[0].start != NULL;

1263 }

1264

1265 -static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)

1265 +static void handle_signed_tag(const struct commit *parent, struct commit_extra_header ***tail)

1266 {

1267 struct merge_remote_desc *desc;

1268 struct commit_extra_header *mergetag;

@@ -1359,17 +1359,17 @@ void verify_merge_signature(struct commit *commit, int verbosity,

1359 signature_check_clear(&signature_check);

1360 }

1361

1362 -void append_merge_tag_headers(struct commit_list *parents,

1362 +void append_merge_tag_headers(const struct commit_list *parents,

1363 struct commit_extra_header ***tail)

1364 {

1365 while (parents) {

1366 - struct commit *parent = parents->item;

1366 + const struct commit *parent = parents->item;

1367 handle_signed_tag(parent, tail);

1368 parents = parents->next;

1369 }

1370 }

1371

1372 -static int convert_commit_extra_headers(struct commit_extra_header *orig,

1372 +static int convert_commit_extra_headers(const struct commit_extra_header *orig,

1373 struct commit_extra_header **result)

1374 {

1375 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,

1403 }

1404

1405 static void add_extra_header(struct strbuf *buffer,

1406 - struct commit_extra_header *extra)

1406 + const struct commit_extra_header *extra)

1407 {

1408 strbuf_addstr(buffer, extra->key);

1409 if (extra->len)

@@ -1517,7 +1517,7 @@ void free_commit_extra_headers(struct commit_extra_header *extra)

1517 }

1518

1519 int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,

1520 - struct commit_list *parents, struct object_id *ret,

1520 + const struct commit_list *parents, struct object_id *ret,

1521 const char *author, const char *sign_commit)

1522 {

1523 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

1649 const struct object_id *tree,

1650 const struct object_id *parents, size_t parents_len,

1651 const char *author, const char *committer,

1652 - struct commit_extra_header *extra)

1652 + const struct commit_extra_header *extra)

1653 {

1654 int encoding_is_utf8;

1655 size_t i;

@@ -1690,10 +1690,10 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg

1690

1691 int commit_tree_extended(const char *msg, size_t msg_len,

1692 const struct object_id *tree,

1693 - struct commit_list *parents, struct object_id *ret,

1693 + const struct commit_list *parents, struct object_id *ret,

1694 const char *author, const char *committer,

1695 const char *sign_commit,

1696 - struct commit_extra_header *extra)

1696 + const struct commit_extra_header *extra)

1697 {

1698 struct repository *r = the_repository;

1699 int result = 0;

@@ -1715,10 +1715,8 @@ int commit_tree_extended(const char *msg, size_t msg_len,

1715 nparents = commit_list_count(parents);

1716 CALLOC_ARRAY(parent_buf, nparents);

1717 i = 0;

1718 - while (parents) {

1719 - struct commit *parent = pop_commit(&parents);

1720 - oidcpy(&parent_buf[i++], &parent->object.oid);

1721 - }

1718 + for (const struct commit_list *p = parents; p; p = p->next)

1719 + oidcpy(&parent_buf[i++], &p->item->object.oid);

1720

1721 write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra);

1722 if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) {

@@ -1814,7 +1812,7 @@ out:

1812 define_commit_slab(merge_desc_slab, struct merge_remote_desc *);

1813 static struct merge_desc_slab merge_desc_slab = COMMIT_SLAB_INIT(1, merge_desc_slab);

1814

1817 -struct merge_remote_desc *merge_remote_util(struct commit *commit)

1815 +struct merge_remote_desc *merge_remote_util(const struct commit *commit)

1816 {

1817 return *merge_desc_slab_at(&merge_desc_slab, commit);

1818 }

commit.h

+5 -5

@@ -260,19 +260,19 @@ struct commit_extra_header {

260 size_t len;

261 };

262

263 -void append_merge_tag_headers(struct commit_list *parents,

263 +void append_merge_tag_headers(const struct commit_list *parents,

264 struct commit_extra_header ***tail);

265

266 int commit_tree(const char *msg, size_t msg_len,

267 const struct object_id *tree,

268 - struct commit_list *parents, struct object_id *ret,

268 + const struct commit_list *parents, struct object_id *ret,

269 const char *author, const char *sign_commit);

270

271 int commit_tree_extended(const char *msg, size_t msg_len,

272 const struct object_id *tree,

273 - struct commit_list *parents, struct object_id *ret,

273 + const struct commit_list *parents, struct object_id *ret,

274 const char *author, const char *committer,

275 - const char *sign_commit, struct commit_extra_header *);

275 + const char *sign_commit, const struct commit_extra_header *);

276

277 struct commit_extra_header *read_commit_extra_headers(struct commit *, const char **);

278

@@ -306,7 +306,7 @@ struct merge_remote_desc {

306 struct object *obj; /* the named object, could be a tag */

307 char name[FLEX_ARRAY];

308 };

309 -struct merge_remote_desc *merge_remote_util(struct commit *);

309 +struct merge_remote_desc *merge_remote_util(const struct commit *);

310 void set_merge_remote_desc(struct commit *commit,

311 const char *name, struct object *obj);

312

notes-merge.c

@@ -661,6 +661,7 @@ int notes_merge(struct notes_merge_options *o,

661 commit_list_insert(local, &parents);

662 create_notes_commit(o->repo, local_tree, parents, o->commit_msg.buf,

663 o->commit_msg.len, result_oid);

664 + free_commit_list(parents);

665 }

666

667 found_result:

notes-utils.c

+6 -2

@@ -9,10 +9,11 @@

10 void create_notes_commit(struct repository *r,

11 struct notes_tree *t,

12 - struct commit_list *parents,

12 + const struct commit_list *parents,

13 const char *msg, size_t msg_len,

14 struct object_id *result_oid)

15 {

16 + struct commit_list *parents_to_free = NULL;

17 struct object_id tree_oid;

19 assert(t->initialized);

@@ -27,7 +28,8 @@ void create_notes_commit(struct repository *r,

28 struct commit *parent = lookup_commit(r, &parent_oid);

29 if (repo_parse_commit(r, parent))

30 die("Failed to find/parse commit %s", t->ref);

30 - commit_list_insert(parent, &parents);

31 + commit_list_insert(parent, &parents_to_free);

32 + parents = parents_to_free;

33 }

34 /* else: t->ref points to nothing, assume root/orphan commit */

35 }

@@ -35,6 +37,8 @@ void create_notes_commit(struct repository *r,

37 if (commit_tree(msg, msg_len, &tree_oid, parents, result_oid, NULL,

38 NULL))

39 die("Failed to commit notes tree to database");

40 +

41 + free_commit_list(parents_to_free);

42 }

44 void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)

notes-utils.h

+1 -1

@@ -20,7 +20,7 @@ struct repository;

20 */

21 void create_notes_commit(struct repository *r,

22 struct notes_tree *t,

23 - struct commit_list *parents,

23 + const struct commit_list *parents,

24 const char *msg, size_t msg_len,

25 struct object_id *result_oid);

sequencer.c

@@ -1711,6 +1711,7 @@ static int try_to_commit(struct repository *r,

1711

1712 out:

1713 free_commit_extra_headers(extra);

1714 + free_commit_list(parents);

1715 strbuf_release(&err);

1716 strbuf_release(&commit_msg);

1717 free(amend_author);

t/t3403-rebase-skip.sh

@@ -8,6 +8,7 @@ test_description='git rebase --merge --skip tests'

8 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

9 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

11 +TEST_PASSES_SANITIZE_LEAK=true

12 . ./test-lib.sh

14 . "$TEST_DIRECTORY"/lib-rebase.sh

t/t3424-rebase-empty.sh

@@ -2,6 +2,7 @@

3 test_description='git rebase of commits that start or become empty'

5 +TEST_PASSES_SANITIZE_LEAK=true

6 . ./test-lib.sh

8 test_expect_success 'setup test repository' '

t/t3505-cherry-pick-empty.sh

@@ -5,6 +5,7 @@ test_description='test cherry-picking an empty commit'

5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

8 +TEST_PASSES_SANITIZE_LEAK=true

9 . ./test-lib.sh

11 test_expect_success setup '

t/t7505-prepare-commit-msg-hook.sh

@@ -5,6 +5,7 @@ test_description='prepare-commit-msg hook'

5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

8 +TEST_PASSES_SANITIZE_LEAK=true

9 . ./test-lib.sh

11 test_expect_success 'set up commits for rebasing' '