{"thread":{"id":"56653","subject":"[PATCH 0/2] dir & unpak-trees: memory-leak fixes","startedAt":"2021-10-06T09:41:04Z","lastAt":"2021-10-13T13:24:07Z","messageCount":17,"participants":["Ævar Arnfjörð Bjarmason","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438029","messageId":"cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com","threadId":"56653","inReplyTo":null,"subject":"[PATCH 0/2] dir & unpak-trees: memory-leak fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-06T09:40:46Z","receivedAt":"2021-10-06T09:41:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"These couple of patches fix some remaining memory leaks in\nunpack-trees.c, which mostly happen via a lack of calls to dir_clear()\nfixed in 2/2.\n\nThis goes on top of ab/sanitize-leak-ci (but not\nen/removing-untracked-fixes). In 1/2 I mark a test as passing under\nSANITIZE=leak, which assumes that an environment variable added by\nab/sanitize-leak-ci is understood. Without ab/sanitize-leak-ci it'll\nbe silently ignored.\n\nElijah has parallel work in fixing leaks in dir.c, but without his\nab/sanitize-leak-ci that test will also pass, it's not one of the ones\nthat needed something like his leak fixes to {dir,unpack-trees}.c to\npass.\n\nÆvar Arnfjörð Bjarmason (2):\n unpack-trees: don't leak memory in verify_clean_subdirectory()\n built-ins & lib: plug memory leaks with unpack_trees_options_release()\n\n archive.c | 11 ++++++++---\n builtin/am.c | 17 ++++++++++++-----\n builtin/checkout.c | 9 +++++++--\n builtin/clone.c | 1 +\n builtin/commit.c | 6 +++++-\n builtin/merge.c | 6 ++++--\n builtin/read-tree.c | 14 ++++++++++----\n builtin/reset.c | 13 +++++++++----\n builtin/stash.c | 14 ++++++++++----\n diff-lib.c | 5 ++++-\n sequencer.c | 2 ++\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 13 files changed, 76 insertions(+), 27 deletions(-)\n\n-- \n2.33.0.1441.gbbcdb4c3c66\n\n"},{"id":"438030","messageId":"patch-1.2-e5ef1be2aa9-20211006T093405Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com","subject":"[PATCH 1/2] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-06T09:40:47Z","receivedAt":"2021-10-06T09:41:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix two different but related memory leaks in\nverify_clean_subdirectory(). We leaked both the \"pathbuf\" if\nread_directory() returned non-zero, and we never cleaned up our own\n\"struct dir_struct\" either.\n\n * \"pathbuf\": When the read_directory() call followed by the\n free(pathbuf) was added in c81935348be (Fix switching to a branch\n with D/F when current branch has file D., 2007-03-15) we didn't\n bother to free() before we called die().\n\n But when this code was later libified in 203a2fe1170 (Allow callers\n of unpack_trees() to handle failure, 2008-02-07) we started to leak\n as we returned data to the caller. This fixes that memory leak,\n which can be observed under SANITIZE=leak with e.g. the\n \"t1001-read-tree-m-2way.sh\" test.\n\n * \"struct dir_struct\": We've leaked the dir_struct ever since this\n code was added back in c81935348be.\n\n When that commit was written there wasn't an equivalent of\n dir_clear(). Since it was added in 270be816049 (dir.c: provide\n clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n we've omitted freeing the memory allocated here.\n\n This memory leak could also be observed under SANITIZE=leak and the\n \"t1001-read-tree-m-2way.sh\" test.\n\nThis makes all the test in \"t1001-read-tree-m-2way.sh\" pass under\n\"GIT_TEST_PASSING_SANITIZE_LEAK=true\", we'd previously die in tests\n25, 26 & 28.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 1057a96b249..d1115528cb9 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -20,6 +20,8 @@ In the test, these paths are used:\n \trezrov - in H, deleted in M\n \tyomin - not in H or M\n '\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a7e1712d236..89ca95ce90b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \tif (o->dir)\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n+\tdir_clear(&d);\n+\tfree(pathbuf);\n \tif (i)\n \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n-\tfree(pathbuf);\n \treturn cnt;\n }\n \n-- \n2.33.0.1441.gbbcdb4c3c66\n\n"},{"id":"438031","messageId":"patch-2.2-21f9da06b46-20211006T093405Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com","subject":"[PATCH 2/2] built-ins & lib: plug memory leaks with unpack_trees_options_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-06T09:40:48Z","receivedAt":"2021-10-06T09:41:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Plug memory leaks in various built-ins and the \"diff-lib.c\" and\n\"sequencer.c\" libraries that were missing\nunpack_trees_options_release() calls.\n\nIn the case of \"git archive\" we'll need to memset() the \"struct\nunpack_trees_options\" first, to avoid having to call\nclear_unpack_trees_porcelain() twice within the\n\"!args->worktree_attributes\" branch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n archive.c | 11 ++++++++---\n builtin/am.c | 17 ++++++++++++-----\n builtin/checkout.c | 9 +++++++--\n builtin/clone.c | 1 +\n builtin/commit.c | 6 +++++-\n builtin/merge.c | 6 ++++--\n builtin/read-tree.c | 14 ++++++++++----\n builtin/reset.c | 13 +++++++++----\n builtin/stash.c | 14 ++++++++++----\n diff-lib.c | 5 ++++-\n sequencer.c | 2 ++\n 11 files changed, 72 insertions(+), 26 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex a3bbb091256..b0acc5a8b88 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -299,16 +299,18 @@ int write_archive_entries(struct archiver_args *args,\n \t/*\n \t * Setup index and instruct attr to read index only\n \t */\n+\tmemset(&opts, 0, sizeof(opts));\n \tif (!args->worktree_attributes) {\n-\t\tmemset(&opts, 0, sizeof(opts));\n \t\topts.index_only = 1;\n \t\topts.head_idx = -1;\n \t\topts.src_index = args->repo->index;\n \t\topts.dst_index = args->repo->index;\n \t\topts.fn = oneway_merge;\n \t\tinit_tree_desc(&t, args->tree->buffer, args->tree->size);\n-\t\tif (unpack_trees(1, &t, &opts))\n-\t\t\treturn -1;\n+\t\tif (unpack_trees(1, &t, &opts)) {\n+\t\t\terr = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tgit_attr_set_direction(GIT_ATTR_INDEX);\n \t}\n \n@@ -347,8 +349,11 @@ int write_archive_entries(struct archiver_args *args,\n \t\tif (err)\n \t\t\tbreak;\n \t}\n+\n+cleanup:\n \tstrbuf_release(&path_in_archive);\n \tstrbuf_release(&content);\n+\tclear_unpack_trees_porcelain(&opts);\n \n \treturn err;\n }\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f296226e95f..0126ec2669b 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[2];\n+\tint ret = 0;\n \n \tif (parse_tree(head) || parse_tree(remote))\n \t\treturn -1;\n@@ -1925,13 +1926,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \n \tif (unpack_trees(2, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n-\n-\treturn 0;\n+cleanup:\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n /**\n@@ -1943,6 +1946,7 @@ static int merge_tree(struct tree *tree)\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[1];\n+\tint ret = 0;\n \n \tif (parse_tree(tree))\n \t\treturn -1;\n@@ -1959,13 +1963,16 @@ static int merge_tree(struct tree *tree)\n \n \tif (unpack_trees(1, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\treturn 0;\n+cleanup:\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n /**\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex cbf73b8c9f6..7b74380528b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n+\tint ret;\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n@@ -670,10 +671,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \t\t */\n \t\t/* fallthrough */\n \tcase 0:\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tbreak;\n \tdefault:\n-\t\treturn 128;\n+\t\tret = 128;\n \t}\n+\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n static void setup_branch_path(struct branch_info *branch)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 559acf9e036..c489a9bda10 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -699,6 +699,7 @@ static int checkout(int submodule_progress)\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\n+\tclear_unpack_trees_porcelain(&opts);\n \n \tfree(head);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e7320f66f95..55bb178289f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t;\n+\tint exit_early = 0;\n \n \tif (!current_head) {\n \t\tdiscard_cache();\n@@ -325,7 +326,10 @@ static void create_base_index(const struct commit *current_head)\n \tparse_tree(tree);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n-\t\texit(128); /* We've already reported the error, finish dying */\n+\t\texit_early = 1; /* We've already reported the error, finish dying */\n+\tclear_unpack_trees_porcelain(&opts);\n+\tif (exit_early)\n+\t\texit(128);\n }\n \n static void refresh_cache_or_die(int refresh_flags)\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0ccd5e1ac83..b4bdba2faf6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n \tstruct tree *trees[MAX_UNPACK_TREES];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n+\tint ret = 0;\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 2;\n@@ -697,8 +698,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn -1;\n-\treturn 0;\n+\t\tret = -1;\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n static void write_tree_trivial(struct object_id *oid)\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 2109c4c9e5c..060daa3913f 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -149,6 +149,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n \t\tOPT_END()\n \t};\n+\tint ret = 0;\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \t\tparse_tree(tree);\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn 128;\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tret = 128;\n+\t\tgoto cleanup;\n+\t}\n \n \tif (opts.debug_unpack || opts.dry_run)\n-\t\treturn 0; /* do not write the index out */\n+\t\tgoto cleanup; /* do not write the index out */\n \n \t/*\n \t * When reading only one tree (either the most basic form,\n@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"unable to write new index file\");\n-\treturn 0;\n+\n+cleanup:\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 73935953494..8fb9bee1a98 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -85,10 +85,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == KEEP) {\n \t\tstruct object_id head_oid;\n-\t\tif (get_oid(\"HEAD\", &head_oid))\n-\t\t\treturn error(_(\"You do not have a valid HEAD.\"));\n-\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))\n-\t\t\treturn error(_(\"Failed to find tree of HEAD.\"));\n+\t\tif (get_oid(\"HEAD\", &head_oid)) {\n+\t\t\terror(_(\"You do not have a valid HEAD.\"));\n+\t\t\tgoto out;\n+\t\t}\n+\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {\n+\t\t\terror(_(\"Failed to find tree of HEAD.\"));\n+\t\t\tgoto out;\n+\t\t}\n \t\tnr++;\n \t\topts.fn = twoway_merge;\n \t}\n@@ -110,6 +114,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tret = 0;\n \n out:\n+\tclear_unpack_trees_porcelain(&opts);\n \tfor (i = 0; i < nr; i++)\n \t\tfree((void *)desc[i].buffer);\n \treturn ret;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex cc93ace4223..6bde00393fe 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct tree *tree;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tint ret = 0;\n \n \tread_cache_preload(NULL);\n \tif (refresh_cache(REFRESH_QUIET))\n@@ -262,13 +263,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n \t\topts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n \topts.fn = oneway_merge;\n \n-\tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn -1;\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tret = -1;\n+\t\tgoto cleanup;\n+\t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\treturn error(_(\"unable to write new index file\"));\n+\t\tret = error(_(\"unable to write new index file\"));\n \n-\treturn 0;\n+cleanup:\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n@@ -833,6 +838,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n \n \tif (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))\n \t\tdie(_(\"failed to unpack trees\"));\n+\tclear_unpack_trees_porcelain(&unpack_tree_opt);\n \n \tdo_diff_cache(&info->b_commit, diff_opt);\n }\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ca085a03efc..ddfbcf22abf 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,\n \tstruct tree *tree;\n \tstruct tree_desc t;\n \tstruct unpack_trees_options opts;\n+\tint ret;\n \n \ttree = parse_tree_indirect(tree_oid);\n \tif (!tree)\n@@ -546,7 +547,9 @@ static int diff_cache(struct rev_info *revs,\n \topts.pathspec->recursive = 1;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\treturn unpack_trees(1, &t, &opts);\n+\tret = unpack_trees(1, &t, &opts);\n+\tclear_unpack_trees_porcelain(&opts);\n+\treturn ret;\n }\n \n void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\ndiff --git a/sequencer.c b/sequencer.c\nindex 6872b7b00a4..74d478d67d3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3720,6 +3720,7 @@ static int do_reset(struct repository *r,\n \t\trollback_lock_file(&lock);\n \t\tfree((void *)desc.buffer);\n \t\tstrbuf_release(&ref_name);\n+\t\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \t\treturn -1;\n \t}\n \n@@ -3736,6 +3737,7 @@ static int do_reset(struct repository *r,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n \n \tstrbuf_release(&ref_name);\n+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \treturn ret;\n }\n \n-- \n2.33.0.1441.gbbcdb4c3c66\n\n"},{"id":"438071","messageId":"CABPp-BH0ca=kx85kfqh7GGRqEzUpzd4dUr58M5WrUmYXQs0e4w@mail.gmail.com","threadId":"56653","inReplyTo":"patch-1.2-e5ef1be2aa9-20211006T093405Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-06T15:58:44Z","receivedAt":"2021-10-06T15:58:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Oct 6, 2021 at 2:40 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Fix two different but related memory leaks in\n> verify_clean_subdirectory(). We leaked both the \"pathbuf\" if\n> read_directory() returned non-zero, and we never cleaned up our own\n> \"struct dir_struct\" either.\n>\n> * \"pathbuf\": When the read_directory() call followed by the\n> free(pathbuf) was added in c81935348be (Fix switching to a branch\n> with D/F when current branch has file D., 2007-03-15) we didn't\n> bother to free() before we called die().\n>\n> But when this code was later libified in 203a2fe1170 (Allow callers\n> of unpack_trees() to handle failure, 2008-02-07) we started to leak\n> as we returned data to the caller. This fixes that memory leak,\n> which can be observed under SANITIZE=leak with e.g. the\n> \"t1001-read-tree-m-2way.sh\" test.\n>\n> * \"struct dir_struct\": We've leaked the dir_struct ever since this\n> code was added back in c81935348be.\n>\n> When that commit was written there wasn't an equivalent of\n> dir_clear(). Since it was added in 270be816049 (dir.c: provide\n> clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n> we've omitted freeing the memory allocated here.\n>\n> This memory leak could also be observed under SANITIZE=leak and the\n> \"t1001-read-tree-m-2way.sh\" test.\n>\n> This makes all the test in \"t1001-read-tree-m-2way.sh\" pass under\n> \"GIT_TEST_PASSING_SANITIZE_LEAK=true\", we'd previously die in tests\n> 25, 26 & 28.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t1001-read-tree-m-2way.sh | 2 ++\n> unpack-trees.c | 3 ++-\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\n> index 1057a96b249..d1115528cb9 100755\n> --- a/t/t1001-read-tree-m-2way.sh\n> +++ b/t/t1001-read-tree-m-2way.sh\n> @@ -20,6 +20,8 @@ In the test, these paths are used:\n> rezrov - in H, deleted in M\n> yomin - not in H or M\n> '\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index a7e1712d236..89ca95ce90b 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n> if (o->dir)\n> d.exclude_per_dir = o->dir->exclude_per_dir;\n> i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n> + dir_clear(&d);\n> + free(pathbuf);\n> if (i)\n> return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n> - free(pathbuf);\n> return cnt;\n> }\n>\n> --\n> 2.33.0.1441.gbbcdb4c3c66\n\nNicely explained; and nice clean patch.\n"},{"id":"438072","messageId":"CABPp-BGrGgjUgK1c+0NezY6t2M0yNMb25XHnwaTyXH9ZeAXKcA@mail.gmail.com","threadId":"56653","inReplyTo":"patch-2.2-21f9da06b46-20211006T093405Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] built-ins & lib: plug memory leaks with unpack_trees_options_release()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-06T16:12:35Z","receivedAt":"2021-10-06T16:12:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Oct 6, 2021 at 2:41 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Plug memory leaks in various built-ins and the \"diff-lib.c\" and\n> \"sequencer.c\" libraries that were missing\n> unpack_trees_options_release() calls.\n\nSince you've rebased these changes, unpack_trees_options_release()\ndoesn't exist. You correctly refer to it below as\nclear_unpack_trees_porcelain().\n\nHowever, I'm not sure about most of these changes.\nclear_unpack_trees_porcelain() currently only cleans up memory\nallocated by setup_unpack_trees_porcelain(). Your other series\nrenamed the cleanup function and added a dir_clear() to it, but since\nmy series moved the dir_clear() into unpack_trees(), we don't have\nthat other bit of cleanup to do. So, I don't think there's a leak,\nunless setup_unpack_trees_porcelain() has been called.\n\nGranted, it is safe to call clear_unpack_trees_porcelain() even if\nsetup_unpack_trees_porcelain() isn't called, but does it make sense?\nThe names suggest they are a pair, so if we want to add these (which\nmay be nice to prevent future additions to unpack_trees_options from\ncausing leaks), then we may want to rename\nclear_unpack_trees_porcelain() at the same time.\n\nI'll comment more below...\n\n> In the case of \"git archive\" we'll need to memset() the \"struct\n> unpack_trees_options\" first, to avoid having to call\n> clear_unpack_trees_porcelain() twice within the\n> \"!args->worktree_attributes\" branch.\n\nThis was slightly confusing, though the code is clear. It suggested\nto me that unpack_trees_options was being used uninitialized. Perhaps\n\"...we'll need to move the memset() of struct unpack_trees_options\nearlier to avoid...\" ?\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> archive.c | 11 ++++++++---\n> builtin/am.c | 17 ++++++++++++-----\n> builtin/checkout.c | 9 +++++++--\n> builtin/clone.c | 1 +\n> builtin/commit.c | 6 +++++-\n> builtin/merge.c | 6 ++++--\n> builtin/read-tree.c | 14 ++++++++++----\n> builtin/reset.c | 13 +++++++++----\n> builtin/stash.c | 14 ++++++++++----\n> diff-lib.c | 5 ++++-\n> sequencer.c | 2 ++\n> 11 files changed, 72 insertions(+), 26 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index a3bbb091256..b0acc5a8b88 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -299,16 +299,18 @@ int write_archive_entries(struct archiver_args *args,\n> /*\n> * Setup index and instruct attr to read index only\n> */\n> + memset(&opts, 0, sizeof(opts));\n> if (!args->worktree_attributes) {\n> - memset(&opts, 0, sizeof(opts));\n> opts.index_only = 1;\n> opts.head_idx = -1;\n> opts.src_index = args->repo->index;\n> opts.dst_index = args->repo->index;\n> opts.fn = oneway_merge;\n> init_tree_desc(&t, args->tree->buffer, args->tree->size);\n> - if (unpack_trees(1, &t, &opts))\n> - return -1;\n> + if (unpack_trees(1, &t, &opts)) {\n> + err = -1;\n> + goto cleanup;\n> + }\n> git_attr_set_direction(GIT_ATTR_INDEX);\n> }\n>\n> @@ -347,8 +349,11 @@ int write_archive_entries(struct archiver_args *args,\n> if (err)\n> break;\n> }\n> +\n> +cleanup:\n> strbuf_release(&path_in_archive);\n> strbuf_release(&content);\n> + clear_unpack_trees_porcelain(&opts);\n>\n> return err;\n> }\n> diff --git a/builtin/am.c b/builtin/am.c\n> index f296226e95f..0126ec2669b 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n> struct lock_file lock_file = LOCK_INIT;\n> struct unpack_trees_options opts;\n> struct tree_desc t[2];\n> + int ret = 0;\n>\n> if (parse_tree(head) || parse_tree(remote))\n> return -1;\n> @@ -1925,13 +1926,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n>\n> if (unpack_trees(2, t, &opts)) {\n> rollback_lock_file(&lock_file);\n> - return -1;\n> + ret = -1;\n> + goto cleanup;\n> }\n>\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(_(\"unable to write new index file\"));\n> -\n> - return 0;\n> +cleanup:\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> /**\n> @@ -1943,6 +1946,7 @@ static int merge_tree(struct tree *tree)\n> struct lock_file lock_file = LOCK_INIT;\n> struct unpack_trees_options opts;\n> struct tree_desc t[1];\n> + int ret = 0;\n>\n> if (parse_tree(tree))\n> return -1;\n> @@ -1959,13 +1963,16 @@ static int merge_tree(struct tree *tree)\n>\n> if (unpack_trees(1, t, &opts)) {\n> rollback_lock_file(&lock_file);\n> - return -1;\n> + ret = -1;\n> + goto cleanup;\n> }\n>\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(_(\"unable to write new index file\"));\n>\n> - return 0;\n> +cleanup:\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> /**\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index cbf73b8c9f6..7b74380528b 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n> {\n> struct unpack_trees_options opts;\n> struct tree_desc tree_desc;\n> + int ret;\n>\n> memset(&opts, 0, sizeof(opts));\n> opts.head_idx = -1;\n> @@ -670,10 +671,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n> */\n> /* fallthrough */\n> case 0:\n> - return 0;\n> + ret = 0;\n> + break;\n> default:\n> - return 128;\n> + ret = 128;\n> }\n> +\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> static void setup_branch_path(struct branch_info *branch)\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 559acf9e036..c489a9bda10 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -699,6 +699,7 @@ static int checkout(int submodule_progress)\n> init_tree_desc(&t, tree->buffer, tree->size);\n> if (unpack_trees(1, &t, &opts) < 0)\n> die(_(\"unable to checkout working tree\"));\n> + clear_unpack_trees_porcelain(&opts);\n>\n> free(head);\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e7320f66f95..55bb178289f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)\n> struct tree *tree;\n> struct unpack_trees_options opts;\n> struct tree_desc t;\n> + int exit_early = 0;\n>\n> if (!current_head) {\n> discard_cache();\n> @@ -325,7 +326,10 @@ static void create_base_index(const struct commit *current_head)\n> parse_tree(tree);\n> init_tree_desc(&t, tree->buffer, tree->size);\n> if (unpack_trees(1, &t, &opts))\n> - exit(128); /* We've already reported the error, finish dying */\n> + exit_early = 1; /* We've already reported the error, finish dying */\n> + clear_unpack_trees_porcelain(&opts);\n> + if (exit_early)\n> + exit(128);\n> }\n>\n> static void refresh_cache_or_die(int refresh_flags)\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 0ccd5e1ac83..b4bdba2faf6 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n> struct tree *trees[MAX_UNPACK_TREES];\n> struct tree_desc t[MAX_UNPACK_TREES];\n> struct unpack_trees_options opts;\n> + int ret = 0;\n>\n> memset(&opts, 0, sizeof(opts));\n> opts.head_idx = 2;\n> @@ -697,8 +698,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n> init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n> }\n> if (unpack_trees(nr_trees, t, &opts))\n> - return -1;\n> - return 0;\n> + ret = -1;\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> static void write_tree_trivial(struct object_id *oid)\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 2109c4c9e5c..060daa3913f 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -149,6 +149,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n> OPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n> OPT_END()\n> };\n> + int ret = 0;\n>\n> memset(&opts, 0, sizeof(opts));\n> opts.head_idx = -1;\n> @@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n> parse_tree(tree);\n> init_tree_desc(t+i, tree->buffer, tree->size);\n> }\n> - if (unpack_trees(nr_trees, t, &opts))\n> - return 128;\n> + if (unpack_trees(nr_trees, t, &opts)) {\n> + ret = 128;\n> + goto cleanup;\n> + }\n>\n> if (opts.debug_unpack || opts.dry_run)\n> - return 0; /* do not write the index out */\n> + goto cleanup; /* do not write the index out */\n>\n> /*\n> * When reading only one tree (either the most basic form,\n> @@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n>\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> die(\"unable to write new index file\");\n> - return 0;\n> +\n> +cleanup:\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 73935953494..8fb9bee1a98 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -85,10 +85,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n>\n> if (reset_type == KEEP) {\n> struct object_id head_oid;\n> - if (get_oid(\"HEAD\", &head_oid))\n> - return error(_(\"You do not have a valid HEAD.\"));\n> - if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))\n> - return error(_(\"Failed to find tree of HEAD.\"));\n> + if (get_oid(\"HEAD\", &head_oid)) {\n> + error(_(\"You do not have a valid HEAD.\"));\n> + goto out;\n> + }\n> + if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {\n> + error(_(\"Failed to find tree of HEAD.\"));\n> + goto out;\n> + }\n> nr++;\n> opts.fn = twoway_merge;\n> }\n> @@ -110,6 +114,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n> ret = 0;\n>\n> out:\n> + clear_unpack_trees_porcelain(&opts);\n> for (i = 0; i < nr; i++)\n> free((void *)desc[i].buffer);\n> return ret;\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index cc93ace4223..6bde00393fe 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n> struct tree_desc t[MAX_UNPACK_TREES];\n> struct tree *tree;\n> struct lock_file lock_file = LOCK_INIT;\n> + int ret = 0;\n>\n> read_cache_preload(NULL);\n> if (refresh_cache(REFRESH_QUIET))\n> @@ -262,13 +263,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n> opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n> opts.fn = oneway_merge;\n>\n> - if (unpack_trees(nr_trees, t, &opts))\n> - return -1;\n> + if (unpack_trees(nr_trees, t, &opts)) {\n> + ret = -1;\n> + goto cleanup;\n> + }\n>\n> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> - return error(_(\"unable to write new index file\"));\n> + ret = error(_(\"unable to write new index file\"));\n>\n> - return 0;\n> +cleanup:\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n> @@ -833,6 +838,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n>\n> if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))\n> die(_(\"failed to unpack trees\"));\n> + clear_unpack_trees_porcelain(&unpack_tree_opt);\n>\n> do_diff_cache(&info->b_commit, diff_opt);\n> }\n> diff --git a/diff-lib.c b/diff-lib.c\n> index ca085a03efc..ddfbcf22abf 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,\n> struct tree *tree;\n> struct tree_desc t;\n> struct unpack_trees_options opts;\n> + int ret;\n>\n> tree = parse_tree_indirect(tree_oid);\n> if (!tree)\n> @@ -546,7 +547,9 @@ static int diff_cache(struct rev_info *revs,\n> opts.pathspec->recursive = 1;\n>\n> init_tree_desc(&t, tree->buffer, tree->size);\n> - return unpack_trees(1, &t, &opts);\n> + ret = unpack_trees(1, &t, &opts);\n> + clear_unpack_trees_porcelain(&opts);\n> + return ret;\n> }\n>\n> void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n\nI think none of the above are cases where\nsetup_unpack_trees_porcelain() were called, and thus aren't fixing\ncurrent leaks (but might be helping us prevent future leaks, as\ndiscussed above)\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 6872b7b00a4..74d478d67d3 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3720,6 +3720,7 @@ static int do_reset(struct repository *r,\n> rollback_lock_file(&lock);\n> free((void *)desc.buffer);\n> strbuf_release(&ref_name);\n> + clear_unpack_trees_porcelain(&unpack_tree_opts);\n> return -1;\n> }\n>\n> @@ -3736,6 +3737,7 @@ static int do_reset(struct repository *r,\n> NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n>\n> strbuf_release(&ref_name);\n> + clear_unpack_trees_porcelain(&unpack_tree_opts);\n> return ret;\n> }\n\nThis one is a leak fix, but I think it's incomplete. I count four\nreturn statements in do_reset(), after the\nsetup_unpack_trees_porcelain() call, but here you only add\nclear_unpack_trees_porcelain() calls before two of them. I think you\nneed to add calls for the other two as well. Or restructure the\nreturns to use a \"goto cleanup\" or something like that.\n"},{"id":"438075","messageId":"CABPp-BGsJOtffyLqk85Z65chJkPr+PsZieK=eX57ccKawdVAVQ@mail.gmail.com","threadId":"56653","inReplyTo":"cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] dir & unpak-trees: memory-leak fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-06T16:33:50Z","receivedAt":"2021-10-06T16:34:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Oct 6, 2021 at 2:40 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> These couple of patches fix some remaining memory leaks in\n> unpack-trees.c, which mostly happen via a lack of calls to dir_clear()\n> fixed in 2/2.\n\nThere is no added dir_clear() in 2/2 (the equivalent patch in the\nprevious series you took that patch from did have one, so I think this\nwas just an incomplete edit-or-commenting-after-rebasing case). More\ncomments on that patch. The idea for 2/2 is probably good, but should\nprobably be accompanied with another tweak, and the commit message and\ncover letter about it need some updating.\n\nPatch 1/2 is a clear memory leak fix.\n\n> This goes on top of ab/sanitize-leak-ci (but not\n> en/removing-untracked-fixes). In 1/2 I mark a test as passing under\n> SANITIZE=leak, which assumes that an environment variable added by\n> ab/sanitize-leak-ci is understood. Without ab/sanitize-leak-ci it'll\n> be silently ignored.\n>\n> Elijah has parallel work in fixing leaks in dir.c, but without his\n> ab/sanitize-leak-ci that test will also pass, it's not one of the ones\n> that needed something like his leak fixes to {dir,unpack-trees}.c to\n> pass.\n>\n> Ævar Arnfjörð Bjarmason (2):\n> unpack-trees: don't leak memory in verify_clean_subdirectory()\n> built-ins & lib: plug memory leaks with unpack_trees_options_release()\n>\n> archive.c | 11 ++++++++---\n> builtin/am.c | 17 ++++++++++++-----\n> builtin/checkout.c | 9 +++++++--\n> builtin/clone.c | 1 +\n> builtin/commit.c | 6 +++++-\n> builtin/merge.c | 6 ++++--\n> builtin/read-tree.c | 14 ++++++++++----\n> builtin/reset.c | 13 +++++++++----\n> builtin/stash.c | 14 ++++++++++----\n> diff-lib.c | 5 ++++-\n> sequencer.c | 2 ++\n> t/t1001-read-tree-m-2way.sh | 2 ++\n> unpack-trees.c | 3 ++-\n> 13 files changed, 76 insertions(+), 27 deletions(-)\n>\n> --\n> 2.33.0.1441.gbbcdb4c3c66\n"},{"id":"438128","messageId":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com","subject":"[PATCH v2 0/3] unpack-trees: memory-leak fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-07T09:46:08Z","receivedAt":"2021-10-07T09:46:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"These patches fix a couple of stray memory leaks in unpack-trees.c.\n\nThis goes on top of ab/sanitize-leak-ci (but not\nen/removing-untracked-fixes, although they combine to fix more leaks\nin the area).\n\nChanges since v1[1]:\n\nIn rebasing v1 from some earlier patches I came up with something that\ndidn't make sense with related fixes in Elijah's\nen/removing-untracked-fixes. This hopefully makes sense:\n\n * The old 3/3 is gone, but there's a new 2-3/3 which fix the only\n actual leak that was left, i.e. the one in sequencer.c.\n\n * We might want something like the 3/3 from v1 of this series where\n we call clear_unpack_trees_porcelain() everywhere (and rename it to\n unpack_trees_options_release()) just for good measure and in case\n we'd ever add something to the struct that needs unconditional\n freeing.\n\n But let's punt on that here and just keep the current\n setup_unpack_trees_porcelain()/clear_unpack_trees_porcelain()\n behavior, callers who don't use setup_unpack_trees_porcelain() but\n use \"struct unpack_trees_options\" don't need to call any free-like\n function at the end.\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n unpack-trees: don't leak memory in verify_clean_subdirectory()\n sequencer: add a \"goto cleanup\" to do_reset()\n sequencer: fix a memory leak in do_reset()\n\n sequencer.c | 36 +++++++++++++++---------------------\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 3 files changed, 19 insertions(+), 22 deletions(-)\n\nRange-diff against v1:\n1: e5ef1be2aa9 = 1: e5ef1be2aa9 unpack-trees: don't leak memory in verify_clean_subdirectory()\n2: 21f9da06b46 ! 2: 1d5f5e9fff0 built-ins & lib: plug memory leaks with unpack_trees_options_release()\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - built-ins & lib: plug memory leaks with unpack_trees_options_release()\n + sequencer: add a \"goto cleanup\" to do_reset()\n \n - Plug memory leaks in various built-ins and the \"diff-lib.c\" and\n - \"sequencer.c\" libraries that were missing\n - unpack_trees_options_release() calls.\n + Restructure code that's mostly added in 9055e401dd6 (sequencer:\n + introduce new commands to reset the revision, 2018-04-25) to avoid\n + code duplication, and to make freeing other resources easier in a\n + subsequent commit.\n \n - In the case of \"git archive\" we'll need to memset() the \"struct\n - unpack_trees_options\" first, to avoid having to call\n - clear_unpack_trees_porcelain() twice within the\n - \"!args->worktree_attributes\" branch.\n + It's safe to initialize \"tree_desc\" to be zero'd out in order to\n + unconditionally free desc.buffer, it won't be initialized on the first\n + couple of \"goto\"'s.\n +\n + There are three earlier \"return\"'s in this function that I'm not\n + bothering to covert, those don't need to rollback anything, or free\n + any resources, so let's leave, even though they could safely \"goto\n + cleanup\" as well.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## archive.c ##\n -@@ archive.c: int write_archive_entries(struct archiver_args *args,\n - \t/*\n - \t * Setup index and instruct attr to read index only\n - \t */\n -+\tmemset(&opts, 0, sizeof(opts));\n - \tif (!args->worktree_attributes) {\n --\t\tmemset(&opts, 0, sizeof(opts));\n - \t\topts.index_only = 1;\n - \t\topts.head_idx = -1;\n - \t\topts.src_index = args->repo->index;\n - \t\topts.dst_index = args->repo->index;\n - \t\topts.fn = oneway_merge;\n - \t\tinit_tree_desc(&t, args->tree->buffer, args->tree->size);\n --\t\tif (unpack_trees(1, &t, &opts))\n + ## sequencer.c ##\n +@@ sequencer.c: static int do_reset(struct repository *r,\n + \tstruct strbuf ref_name = STRBUF_INIT;\n + \tstruct object_id oid;\n + \tstruct lock_file lock = LOCK_INIT;\n +-\tstruct tree_desc desc;\n ++\tstruct tree_desc desc = { 0 };\n + \tstruct tree *tree;\n + \tstruct unpack_trees_options unpack_tree_opts;\n + \tint ret = 0;\n +@@ sequencer.c: static int do_reset(struct repository *r,\n + \t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n + \t\tif (get_oid(ref_name.buf, &oid) &&\n + \t\t get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n +-\t\t\terror(_(\"could not read '%s'\"), ref_name.buf);\n +-\t\t\trollback_lock_file(&lock);\n +-\t\t\tstrbuf_release(&ref_name);\n -\t\t\treturn -1;\n -+\t\tif (unpack_trees(1, &t, &opts)) {\n -+\t\t\terr = -1;\n ++\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n +\t\t\tgoto cleanup;\n -+\t\t}\n - \t\tgit_attr_set_direction(GIT_ATTR_INDEX);\n + \t\t}\n \t}\n \n -@@ archive.c: int write_archive_entries(struct archiver_args *args,\n - \t\tif (err)\n - \t\t\tbreak;\n - \t}\n -+\n -+cleanup:\n - \tstrbuf_release(&path_in_archive);\n - \tstrbuf_release(&content);\n -+\tclear_unpack_trees_porcelain(&opts);\n - \n - \treturn err;\n - }\n -\n - ## builtin/am.c ##\n -@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n - \tstruct lock_file lock_file = LOCK_INIT;\n - \tstruct unpack_trees_options opts;\n - \tstruct tree_desc t[2];\n -+\tint ret = 0;\n - \n - \tif (parse_tree(head) || parse_tree(remote))\n - \t\treturn -1;\n -@@ builtin/am.c: static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n +@@ sequencer.c: static int do_reset(struct repository *r,\n + \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n - \tif (unpack_trees(2, t, &opts)) {\n - \t\trollback_lock_file(&lock_file);\n --\t\treturn -1;\n -+\t\tret = -1;\n + \tif (repo_read_index_unmerged(r)) {\n +-\t\trollback_lock_file(&lock);\n +-\t\tstrbuf_release(&ref_name);\n +-\t\treturn error_resolve_conflict(_(action_name(opts)));\n ++\t\tret = error_resolve_conflict(_(action_name(opts)));\n +\t\tgoto cleanup;\n \t}\n \n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n - \t\tdie(_(\"unable to write new index file\"));\n --\n --\treturn 0;\n -+cleanup:\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - /**\n -@@ builtin/am.c: static int merge_tree(struct tree *tree)\n - \tstruct lock_file lock_file = LOCK_INIT;\n - \tstruct unpack_trees_options opts;\n - \tstruct tree_desc t[1];\n -+\tint ret = 0;\n - \n - \tif (parse_tree(tree))\n - \t\treturn -1;\n -@@ builtin/am.c: static int merge_tree(struct tree *tree)\n - \n - \tif (unpack_trees(1, t, &opts)) {\n - \t\trollback_lock_file(&lock_file);\n + \tif (!fill_tree_descriptor(r, &desc, &oid)) {\n +-\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n +-\t\trollback_lock_file(&lock);\n +-\t\tfree((void *)desc.buffer);\n +-\t\tstrbuf_release(&ref_name);\n -\t\treturn -1;\n -+\t\tret = -1;\n ++\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n +\t\tgoto cleanup;\n \t}\n \n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n - \t\tdie(_(\"unable to write new index file\"));\n - \n --\treturn 0;\n -+cleanup:\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - /**\n -\n - ## builtin/checkout.c ##\n -@@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n - {\n - \tstruct unpack_trees_options opts;\n - \tstruct tree_desc tree_desc;\n -+\tint ret;\n - \n - \tmemset(&opts, 0, sizeof(opts));\n - \topts.head_idx = -1;\n -@@ builtin/checkout.c: static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n - \t\t */\n - \t\t/* fallthrough */\n - \tcase 0:\n --\t\treturn 0;\n -+\t\tret = 0;\n -+\t\tbreak;\n - \tdefault:\n --\t\treturn 128;\n -+\t\tret = 128;\n - \t}\n -+\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - static void setup_branch_path(struct branch_info *branch)\n -\n - ## builtin/clone.c ##\n -@@ builtin/clone.c: static int checkout(int submodule_progress)\n - \tinit_tree_desc(&t, tree->buffer, tree->size);\n - \tif (unpack_trees(1, &t, &opts) < 0)\n - \t\tdie(_(\"unable to checkout working tree\"));\n -+\tclear_unpack_trees_porcelain(&opts);\n - \n - \tfree(head);\n - \n -\n - ## builtin/commit.c ##\n -@@ builtin/commit.c: static void create_base_index(const struct commit *current_head)\n - \tstruct tree *tree;\n - \tstruct unpack_trees_options opts;\n - \tstruct tree_desc t;\n -+\tint exit_early = 0;\n - \n - \tif (!current_head) {\n - \t\tdiscard_cache();\n -@@ builtin/commit.c: static void create_base_index(const struct commit *current_head)\n - \tparse_tree(tree);\n - \tinit_tree_desc(&t, tree->buffer, tree->size);\n - \tif (unpack_trees(1, &t, &opts))\n --\t\texit(128); /* We've already reported the error, finish dying */\n -+\t\texit_early = 1; /* We've already reported the error, finish dying */\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\tif (exit_early)\n -+\t\texit(128);\n - }\n - \n - static void refresh_cache_or_die(int refresh_flags)\n -\n - ## builtin/merge.c ##\n -@@ builtin/merge.c: static int read_tree_trivial(struct object_id *common, struct object_id *head,\n - \tstruct tree *trees[MAX_UNPACK_TREES];\n - \tstruct tree_desc t[MAX_UNPACK_TREES];\n - \tstruct unpack_trees_options opts;\n -+\tint ret = 0;\n - \n - \tmemset(&opts, 0, sizeof(opts));\n - \topts.head_idx = 2;\n -@@ builtin/merge.c: static int read_tree_trivial(struct object_id *common, struct object_id *head,\n - \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n - \t}\n - \tif (unpack_trees(nr_trees, t, &opts))\n + \tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n +-\t\trollback_lock_file(&lock);\n +-\t\tfree((void *)desc.buffer);\n +-\t\tstrbuf_release(&ref_name);\n -\t\treturn -1;\n --\treturn 0;\n +\t\tret = -1;\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - static void write_tree_trivial(struct object_id *oid)\n -\n - ## builtin/read-tree.c ##\n -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n - \t\tOPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n - \t\tOPT_END()\n - \t};\n -+\tint ret = 0;\n - \n - \tmemset(&opts, 0, sizeof(opts));\n - \topts.head_idx = -1;\n -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n - \t\tparse_tree(tree);\n - \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n - \t}\n --\tif (unpack_trees(nr_trees, t, &opts))\n --\t\treturn 128;\n -+\tif (unpack_trees(nr_trees, t, &opts)) {\n -+\t\tret = 128;\n +\t\tgoto cleanup;\n -+\t}\n - \n - \tif (opts.debug_unpack || opts.dry_run)\n --\t\treturn 0; /* do not write the index out */\n -+\t\tgoto cleanup; /* do not write the index out */\n - \n - \t/*\n - \t * When reading only one tree (either the most basic form,\n -@@ builtin/read-tree.c: int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n - \n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n - \t\tdie(\"unable to write new index file\");\n --\treturn 0;\n -+\n -+cleanup:\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n -\n - ## builtin/reset.c ##\n -@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n - \n - \tif (reset_type == KEEP) {\n - \t\tstruct object_id head_oid;\n --\t\tif (get_oid(\"HEAD\", &head_oid))\n --\t\t\treturn error(_(\"You do not have a valid HEAD.\"));\n --\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))\n --\t\t\treturn error(_(\"Failed to find tree of HEAD.\"));\n -+\t\tif (get_oid(\"HEAD\", &head_oid)) {\n -+\t\t\terror(_(\"You do not have a valid HEAD.\"));\n -+\t\t\tgoto out;\n -+\t\t}\n -+\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {\n -+\t\t\terror(_(\"Failed to find tree of HEAD.\"));\n -+\t\t\tgoto out;\n -+\t\t}\n - \t\tnr++;\n - \t\topts.fn = twoway_merge;\n \t}\n -@@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n - \tret = 0;\n - \n - out:\n -+\tclear_unpack_trees_porcelain(&opts);\n - \tfor (i = 0; i < nr; i++)\n - \t\tfree((void *)desc[i].buffer);\n - \treturn ret;\n -\n - ## builtin/stash.c ##\n -@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)\n - \tstruct tree_desc t[MAX_UNPACK_TREES];\n - \tstruct tree *tree;\n - \tstruct lock_file lock_file = LOCK_INIT;\n -+\tint ret = 0;\n - \n - \tread_cache_preload(NULL);\n - \tif (refresh_cache(REFRESH_QUIET))\n -@@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int reset)\n - \t\topts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n - \topts.fn = oneway_merge;\n - \n --\tif (unpack_trees(nr_trees, t, &opts))\n --\t\treturn -1;\n -+\tif (unpack_trees(nr_trees, t, &opts)) {\n -+\t\tret = -1;\n -+\t\tgoto cleanup;\n -+\t}\n - \n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n --\t\treturn error(_(\"unable to write new index file\"));\n -+\t\tret = error(_(\"unable to write new index file\"));\n \n --\treturn 0;\n -+cleanup:\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n -@@ builtin/stash.c: static void diff_include_untracked(const struct stash_info *info, struct diff_op\n - \n - \tif (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))\n - \t\tdie(_(\"failed to unpack trees\"));\n -+\tclear_unpack_trees_porcelain(&unpack_tree_opt);\n - \n - \tdo_diff_cache(&info->b_commit, diff_opt);\n - }\n -\n - ## diff-lib.c ##\n -@@ diff-lib.c: static int diff_cache(struct rev_info *revs,\n - \tstruct tree *tree;\n - \tstruct tree_desc t;\n - \tstruct unpack_trees_options opts;\n -+\tint ret;\n - \n - \ttree = parse_tree_indirect(tree_oid);\n - \tif (!tree)\n -@@ diff-lib.c: static int diff_cache(struct rev_info *revs,\n - \topts.pathspec->recursive = 1;\n - \n - \tinit_tree_desc(&t, tree->buffer, tree->size);\n --\treturn unpack_trees(1, &t, &opts);\n -+\tret = unpack_trees(1, &t, &opts);\n -+\tclear_unpack_trees_porcelain(&opts);\n -+\treturn ret;\n - }\n - \n - void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n -\n - ## sequencer.c ##\n + \ttree = parse_tree_indirect(&oid);\n @@ sequencer.c: static int do_reset(struct repository *r,\n - \t\trollback_lock_file(&lock);\n - \t\tfree((void *)desc.buffer);\n - \t\tstrbuf_release(&ref_name);\n -+\t\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n - \t\treturn -1;\n - \t}\n \n -@@ sequencer.c: static int do_reset(struct repository *r,\n - \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n + \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n + \t\tret = error(_(\"could not write index\"));\n +-\tfree((void *)desc.buffer);\n \n + \tif (!ret)\n + \t\tret = update_ref(reflog_message(opts, \"reset\", \"'%.*s'\",\n + \t\t\t\t\t\tlen, name), \"HEAD\", &oid,\n + \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n +-\n ++cleanup:\n ++\tfree((void *)desc.buffer);\n ++\tif (ret < 0)\n ++\t\trollback_lock_file(&lock);\n \tstrbuf_release(&ref_name);\n -+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \treturn ret;\n }\n - \n-: ----------- > 3: 66ae63db8fd sequencer: fix a memory leak in do_reset()\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438129","messageId":"patch-v2-2.3-1d5f5e9fff0-20211007T094019Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","subject":"[PATCH v2 2/3] sequencer: add a \"goto cleanup\" to do_reset()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-07T09:46:10Z","receivedAt":"2021-10-07T09:46:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Restructure code that's mostly added in 9055e401dd6 (sequencer:\nintroduce new commands to reset the revision, 2018-04-25) to avoid\ncode duplication, and to make freeing other resources easier in a\nsubsequent commit.\n\nIt's safe to initialize \"tree_desc\" to be zero'd out in order to\nunconditionally free desc.buffer, it won't be initialized on the first\ncouple of \"goto\"'s.\n\nThere are three earlier \"return\"'s in this function that I'm not\nbothering to covert, those don't need to rollback anything, or free\nany resources, so let's leave, even though they could safely \"goto\ncleanup\" as well.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 32 +++++++++++++-------------------\n 1 file changed, 13 insertions(+), 19 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 6872b7b00a4..457eba4ab10 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3650,7 +3650,7 @@ static int do_reset(struct repository *r,\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct object_id oid;\n \tstruct lock_file lock = LOCK_INIT;\n-\tstruct tree_desc desc;\n+\tstruct tree_desc desc = { 0 };\n \tstruct tree *tree;\n \tstruct unpack_trees_options unpack_tree_opts;\n \tint ret = 0;\n@@ -3684,10 +3684,8 @@ static int do_reset(struct repository *r,\n \t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \t\tif (get_oid(ref_name.buf, &oid) &&\n \t\t get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n-\t\t\terror(_(\"could not read '%s'\"), ref_name.buf);\n-\t\t\trollback_lock_file(&lock);\n-\t\t\tstrbuf_release(&ref_name);\n-\t\t\treturn -1;\n+\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n+\t\t\tgoto cleanup;\n \t\t}\n \t}\n \n@@ -3703,24 +3701,18 @@ static int do_reset(struct repository *r,\n \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n \tif (repo_read_index_unmerged(r)) {\n-\t\trollback_lock_file(&lock);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn error_resolve_conflict(_(action_name(opts)));\n+\t\tret = error_resolve_conflict(_(action_name(opts)));\n+\t\tgoto cleanup;\n \t}\n \n \tif (!fill_tree_descriptor(r, &desc, &oid)) {\n-\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn -1;\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n+\t\tgoto cleanup;\n \t}\n \n \tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n@@ -3728,13 +3720,15 @@ static int do_reset(struct repository *r,\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n \t\tret = error(_(\"could not write index\"));\n-\tfree((void *)desc.buffer);\n \n \tif (!ret)\n \t\tret = update_ref(reflog_message(opts, \"reset\", \"'%.*s'\",\n \t\t\t\t\t\tlen, name), \"HEAD\", &oid,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n-\n+cleanup:\n+\tfree((void *)desc.buffer);\n+\tif (ret < 0)\n+\t\trollback_lock_file(&lock);\n \tstrbuf_release(&ref_name);\n \treturn ret;\n }\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438130","messageId":"patch-v2-1.3-e5ef1be2aa9-20211007T094019Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","subject":"[PATCH v2 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-07T09:46:09Z","receivedAt":"2021-10-07T09:46:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix two different but related memory leaks in\nverify_clean_subdirectory(). We leaked both the \"pathbuf\" if\nread_directory() returned non-zero, and we never cleaned up our own\n\"struct dir_struct\" either.\n\n * \"pathbuf\": When the read_directory() call followed by the\n free(pathbuf) was added in c81935348be (Fix switching to a branch\n with D/F when current branch has file D., 2007-03-15) we didn't\n bother to free() before we called die().\n\n But when this code was later libified in 203a2fe1170 (Allow callers\n of unpack_trees() to handle failure, 2008-02-07) we started to leak\n as we returned data to the caller. This fixes that memory leak,\n which can be observed under SANITIZE=leak with e.g. the\n \"t1001-read-tree-m-2way.sh\" test.\n\n * \"struct dir_struct\": We've leaked the dir_struct ever since this\n code was added back in c81935348be.\n\n When that commit was written there wasn't an equivalent of\n dir_clear(). Since it was added in 270be816049 (dir.c: provide\n clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n we've omitted freeing the memory allocated here.\n\n This memory leak could also be observed under SANITIZE=leak and the\n \"t1001-read-tree-m-2way.sh\" test.\n\nThis makes all the test in \"t1001-read-tree-m-2way.sh\" pass under\n\"GIT_TEST_PASSING_SANITIZE_LEAK=true\", we'd previously die in tests\n25, 26 & 28.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 1057a96b249..d1115528cb9 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -20,6 +20,8 @@ In the test, these paths are used:\n \trezrov - in H, deleted in M\n \tyomin - not in H or M\n '\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a7e1712d236..89ca95ce90b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \tif (o->dir)\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n+\tdir_clear(&d);\n+\tfree(pathbuf);\n \tif (i)\n \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n-\tfree(pathbuf);\n \treturn cnt;\n }\n \n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438131","messageId":"patch-v2-3.3-66ae63db8fd-20211007T094019Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","subject":"[PATCH v2 3/3] sequencer: fix a memory leak in do_reset()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-07T09:46:11Z","receivedAt":"2021-10-07T09:46:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak introduced in 9055e401dd6 (sequencer: introduce new\ncommands to reset the revision, 2018-04-25), which called\nsetup_unpack_trees_porcelain() without a corresponding call to\nclear_unpack_trees_porcelain().\n\nThis inches us closer to passing various tests in\n\"t34*.sh\" (e.g. \"t3434-rebase-i18n.sh\"), but because they have so many\nother memory leaks in revisions.c this doesn't make any test file or\neven a single test pass.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 457eba4ab10..fefe5a601f4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3652,7 +3652,7 @@ static int do_reset(struct repository *r,\n \tstruct lock_file lock = LOCK_INIT;\n \tstruct tree_desc desc = { 0 };\n \tstruct tree *tree;\n-\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct unpack_trees_options unpack_tree_opts = { 0 };\n \tint ret = 0;\n \n \tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)\n@@ -3689,7 +3689,6 @@ static int do_reset(struct repository *r,\n \t\t}\n \t}\n \n-\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = r->index;\n@@ -3730,6 +3729,7 @@ static int do_reset(struct repository *r,\n \tif (ret < 0)\n \t\trollback_lock_file(&lock);\n \tstrbuf_release(&ref_name);\n+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \treturn ret;\n }\n \n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438175","messageId":"CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com","threadId":"56653","inReplyTo":"patch-v2-2.3-1d5f5e9fff0-20211007T094019Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/3] sequencer: add a \"goto cleanup\" to do_reset()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-07T16:06:48Z","receivedAt":"2021-10-07T16:07:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Oct 7, 2021 at 2:46 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Restructure code that's mostly added in 9055e401dd6 (sequencer:\n> introduce new commands to reset the revision, 2018-04-25) to avoid\n> code duplication, and to make freeing other resources easier in a\n> subsequent commit.\n\nSince the first goto you add precedes the call to\nsetup_unpack_trees_porcelain(), the next commit will introduce a case\nwhere some code calls clear_unpack_trees_porcelain() without having\nfirst called setup_unpack_trees_porcelain(). With the current code\nthat is fine since you do initialize unpack_trees_opts to { 0 } first,\nand clear_unpack_trees_porcelain() doesn't rely on any special setup,\nbut I wonder if the naming similarity of the two functions might lead\nto future authors to presume they are always called in pairs.\n\nI wonder if we should rename clear_unpack_trees_porcelain() to e.g.\nunpack_trees_clear() just to avoid this problem, and then include the\nother bits of your previous round to just make all callers of\nunpack_trees() call unpack_trees_clear() when they are done.\n\nOr, maybe we could just make unpack_trees() call unpack_trees_clear()\nautomatically and remove that responsibility from callers...except\nthat merge-recursive is weird and special and would need a new\nfunction (unpack_trees_without_automatic_cleanup() or something like\nthat?) since it's the one caller that needs to use\nunpack_trees_options data after calling unpack_trees().\n\nSo, multiple competing ideas here, and all for theoretical future code\nsafety. As such, if any of it sounds like too much of a pain, I'm\nfine with punting on it.\n\n> It's safe to initialize \"tree_desc\" to be zero'd out in order to\n> unconditionally free desc.buffer, it won't be initialized on the first\n> couple of \"goto\"'s.\n>\n> There are three earlier \"return\"'s in this function that I'm not\n> bothering to covert,\n\ns/covert/convert/\n\n> those don't need to rollback anything,\n\nThey aren't currently rolling back the lockfile right now, but I think\nthey should. Those error cases are pretty unlikely to happen (hard\ndisk full?), but if they did, they'd leave the lock file in place.\n\nGranted, that's a pre-existing problem rather than a problem\nintroduced by your patch, so it doesn't need to be part of this\nseries, but the commit message should at least be tweaked.\n\n> or free any resources\n\nTrue.\n\n> , so let's leave, even though they could safely \"goto\n> cleanup\" as well.\n\n\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> sequencer.c | 32 +++++++++++++-------------------\n> 1 file changed, 13 insertions(+), 19 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 6872b7b00a4..457eba4ab10 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3650,7 +3650,7 @@ static int do_reset(struct repository *r,\n> struct strbuf ref_name = STRBUF_INIT;\n> struct object_id oid;\n> struct lock_file lock = LOCK_INIT;\n> - struct tree_desc desc;\n> + struct tree_desc desc = { 0 };\n> struct tree *tree;\n> struct unpack_trees_options unpack_tree_opts;\n> int ret = 0;\n> @@ -3684,10 +3684,8 @@ static int do_reset(struct repository *r,\n> strbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n> if (get_oid(ref_name.buf, &oid) &&\n> get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n> - error(_(\"could not read '%s'\"), ref_name.buf);\n> - rollback_lock_file(&lock);\n> - strbuf_release(&ref_name);\n> - return -1;\n> + ret = error(_(\"could not read '%s'\"), ref_name.buf);\n> + goto cleanup;\n> }\n> }\n>\n> @@ -3703,24 +3701,18 @@ static int do_reset(struct repository *r,\n> init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n>\n> if (repo_read_index_unmerged(r)) {\n> - rollback_lock_file(&lock);\n> - strbuf_release(&ref_name);\n> - return error_resolve_conflict(_(action_name(opts)));\n> + ret = error_resolve_conflict(_(action_name(opts)));\n> + goto cleanup;\n> }\n>\n> if (!fill_tree_descriptor(r, &desc, &oid)) {\n> - error(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n> - rollback_lock_file(&lock);\n> - free((void *)desc.buffer);\n> - strbuf_release(&ref_name);\n> - return -1;\n> + ret = error(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n> + goto cleanup;\n> }\n>\n> if (unpack_trees(1, &desc, &unpack_tree_opts)) {\n> - rollback_lock_file(&lock);\n> - free((void *)desc.buffer);\n> - strbuf_release(&ref_name);\n> - return -1;\n> + ret = -1;\n> + goto cleanup;\n> }\n>\n> tree = parse_tree_indirect(&oid);\n> @@ -3728,13 +3720,15 @@ static int do_reset(struct repository *r,\n>\n> if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n> ret = error(_(\"could not write index\"));\n> - free((void *)desc.buffer);\n>\n> if (!ret)\n> ret = update_ref(reflog_message(opts, \"reset\", \"'%.*s'\",\n> len, name), \"HEAD\", &oid,\n> NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n> -\n> +cleanup:\n> + free((void *)desc.buffer);\n> + if (ret < 0)\n> + rollback_lock_file(&lock);\n> strbuf_release(&ref_name);\n> return ret;\n> }\n> --\n> 2.33.0.1446.g6af949f83bd\n>\n"},{"id":"438176","messageId":"CABPp-BGb2kkwRgu-c6DDBOTDCgJYWW+zmhV_ZgOogYqyVpGdLw@mail.gmail.com","threadId":"56653","inReplyTo":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/3] unpack-trees: memory-leak fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-07T16:10:43Z","receivedAt":"2021-10-07T16:10:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Oct 7, 2021 at 2:46 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> These patches fix a couple of stray memory leaks in unpack-trees.c.\n>\n> This goes on top of ab/sanitize-leak-ci (but not\n> en/removing-untracked-fixes, although they combine to fix more leaks\n> in the area).\n>\n> Changes since v1[1]:\n>\n> In rebasing v1 from some earlier patches I came up with something that\n> didn't make sense with related fixes in Elijah's\n> en/removing-untracked-fixes. This hopefully makes sense:\n>\n> * The old 3/3 is gone, but there's a new 2-3/3 which fix the only\n> actual leak that was left, i.e. the one in sequencer.c.\n>\n> * We might want something like the 3/3 from v1 of this series where\n> we call clear_unpack_trees_porcelain() everywhere (and rename it to\n> unpack_trees_options_release()) just for good measure and in case\n> we'd ever add something to the struct that needs unconditional\n> freeing.\n>\n> But let's punt on that here and just keep the current\n> setup_unpack_trees_porcelain()/clear_unpack_trees_porcelain()\n> behavior, callers who don't use setup_unpack_trees_porcelain() but\n> use \"struct unpack_trees_options\" don't need to call any free-like\n> function at the end.\n>\n> 1. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com/\n>\n> Ævar Arnfjörð Bjarmason (3):\n> unpack-trees: don't leak memory in verify_clean_subdirectory()\n> sequencer: add a \"goto cleanup\" to do_reset()\n> sequencer: fix a memory leak in do_reset()\n\nLeft a bunch of comments on patch 2. The upshot is anywhere from \"the\ncommit message just needs a few tweaks\" to \"well, maybe we should\nclean up these other things too, and we could restructure some related\ncode...\". I'll let you pick where in that spectrum you want to take\nthings, because the other possible changes I mention beyond fixing the\ncommit message don't need to hold up this series.\n"},{"id":"438242","messageId":"xmqqzgrko4gs.fsf@gitster.g","threadId":"56653","inReplyTo":"patch-v2-1.3-e5ef1be2aa9-20211007T094019Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-07T22:27:47Z","receivedAt":"2021-10-07T22:28:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index a7e1712d236..89ca95ce90b 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n> \tif (o->dir)\n> \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n> \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n> +\tdir_clear(&d);\n> +\tfree(pathbuf);\n> \tif (i)\n> \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n> -\tfree(pathbuf);\n> \treturn cnt;\n> }\n\nLooks trivially correct.\n\n"},{"id":"438651","messageId":"cover-v3-0.3-00000000000-20211013T132223Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v2-0.3-00000000000-20211007T094019Z-avarab@gmail.com","subject":"[PATCH v3 0/3] unpack-trees: memory-leak fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T13:23:52Z","receivedAt":"2021-10-13T13:24:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This commit-message-only-change to v2 should address the comments\nElijah had on v2 (see link in the range-diff). I.e. there's probably\nbug in adjacent code in leaving a stale lockfile, but I'm punting on\nthat and just narrowly fixing a memory leak here.\n\nÆvar Arnfjörð Bjarmason (3):\n unpack-trees: don't leak memory in verify_clean_subdirectory()\n sequencer: add a \"goto cleanup\" to do_reset()\n sequencer: fix a memory leak in do_reset()\n\n sequencer.c | 36 +++++++++++++++---------------------\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 3 files changed, 19 insertions(+), 22 deletions(-)\n\nRange-diff against v2:\n1: e5ef1be2aa9 = 1: 0ab1e74f50d unpack-trees: don't leak memory in verify_clean_subdirectory()\n2: 1d5f5e9fff0 ! 2: 393937e8a98 sequencer: add a \"goto cleanup\" to do_reset()\n @@ Commit message\n unconditionally free desc.buffer, it won't be initialized on the first\n couple of \"goto\"'s.\n \n - There are three earlier \"return\"'s in this function that I'm not\n - bothering to covert, those don't need to rollback anything, or free\n - any resources, so let's leave, even though they could safely \"goto\n - cleanup\" as well.\n + There are three earlier \"return\"'s in this function which should\n + probably be made to use this new \"cleanup\" too, per [1] it looks like\n + they're leaving behind stale locks. But let's not try to fix every\n + potential bug here now, I'm just trying to narrowly plug a memory\n + leak.\n +\n + 1. https://lore.kernel.org/git/CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n3: 66ae63db8fd ! 3: 00b6b469a8c sequencer: fix a memory leak in do_reset()\n @@ Commit message\n setup_unpack_trees_porcelain() without a corresponding call to\n clear_unpack_trees_porcelain().\n \n + This introduces a change in behavior in that we now start calling\n + clear_unpack_trees_porcelain() even without having called the\n + setup_unpack_trees_porcelain(). That's OK, that clear function, like\n + most others, will accept a zero'd out struct.\n +\n This inches us closer to passing various tests in\n \"t34*.sh\" (e.g. \"t3434-rebase-i18n.sh\"), but because they have so many\n other memory leaks in revisions.c this doesn't make any test file or\n-- \n2.33.0.1569.gd2dc77f7abf\n\n"},{"id":"438652","messageId":"patch-v3-1.3-0ab1e74f50d-20211013T132223Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v3-0.3-00000000000-20211013T132223Z-avarab@gmail.com","subject":"[PATCH v3 1/3] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T13:23:53Z","receivedAt":"2021-10-13T13:24:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix two different but related memory leaks in\nverify_clean_subdirectory(). We leaked both the \"pathbuf\" if\nread_directory() returned non-zero, and we never cleaned up our own\n\"struct dir_struct\" either.\n\n * \"pathbuf\": When the read_directory() call followed by the\n free(pathbuf) was added in c81935348be (Fix switching to a branch\n with D/F when current branch has file D., 2007-03-15) we didn't\n bother to free() before we called die().\n\n But when this code was later libified in 203a2fe1170 (Allow callers\n of unpack_trees() to handle failure, 2008-02-07) we started to leak\n as we returned data to the caller. This fixes that memory leak,\n which can be observed under SANITIZE=leak with e.g. the\n \"t1001-read-tree-m-2way.sh\" test.\n\n * \"struct dir_struct\": We've leaked the dir_struct ever since this\n code was added back in c81935348be.\n\n When that commit was written there wasn't an equivalent of\n dir_clear(). Since it was added in 270be816049 (dir.c: provide\n clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n we've omitted freeing the memory allocated here.\n\n This memory leak could also be observed under SANITIZE=leak and the\n \"t1001-read-tree-m-2way.sh\" test.\n\nThis makes all the test in \"t1001-read-tree-m-2way.sh\" pass under\n\"GIT_TEST_PASSING_SANITIZE_LEAK=true\", we'd previously die in tests\n25, 26 & 28.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1001-read-tree-m-2way.sh | 2 ++\n unpack-trees.c | 3 ++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 1057a96b249..d1115528cb9 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -20,6 +20,8 @@ In the test, these paths are used:\n \trezrov - in H, deleted in M\n \tyomin - not in H or M\n '\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-read-tree.sh\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a7e1712d236..89ca95ce90b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2156,9 +2156,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \tif (o->dir)\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n+\tdir_clear(&d);\n+\tfree(pathbuf);\n \tif (i)\n \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n-\tfree(pathbuf);\n \treturn cnt;\n }\n \n-- \n2.33.0.1569.gd2dc77f7abf\n\n"},{"id":"438653","messageId":"patch-v3-2.3-393937e8a98-20211013T132223Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v3-0.3-00000000000-20211013T132223Z-avarab@gmail.com","subject":"[PATCH v3 2/3] sequencer: add a \"goto cleanup\" to do_reset()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T13:23:54Z","receivedAt":"2021-10-13T13:24:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Restructure code that's mostly added in 9055e401dd6 (sequencer:\nintroduce new commands to reset the revision, 2018-04-25) to avoid\ncode duplication, and to make freeing other resources easier in a\nsubsequent commit.\n\nIt's safe to initialize \"tree_desc\" to be zero'd out in order to\nunconditionally free desc.buffer, it won't be initialized on the first\ncouple of \"goto\"'s.\n\nThere are three earlier \"return\"'s in this function which should\nprobably be made to use this new \"cleanup\" too, per [1] it looks like\nthey're leaving behind stale locks. But let's not try to fix every\npotential bug here now, I'm just trying to narrowly plug a memory\nleak.\n\n1. https://lore.kernel.org/git/CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 32 +++++++++++++-------------------\n 1 file changed, 13 insertions(+), 19 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b92dae12166..14c37c4e25b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3644,7 +3644,7 @@ static int do_reset(struct repository *r,\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct object_id oid;\n \tstruct lock_file lock = LOCK_INIT;\n-\tstruct tree_desc desc;\n+\tstruct tree_desc desc = { 0 };\n \tstruct tree *tree;\n \tstruct unpack_trees_options unpack_tree_opts;\n \tint ret = 0;\n@@ -3678,10 +3678,8 @@ static int do_reset(struct repository *r,\n \t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \t\tif (get_oid(ref_name.buf, &oid) &&\n \t\t get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n-\t\t\terror(_(\"could not read '%s'\"), ref_name.buf);\n-\t\t\trollback_lock_file(&lock);\n-\t\t\tstrbuf_release(&ref_name);\n-\t\t\treturn -1;\n+\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n+\t\t\tgoto cleanup;\n \t\t}\n \t}\n \n@@ -3697,24 +3695,18 @@ static int do_reset(struct repository *r,\n \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n \tif (repo_read_index_unmerged(r)) {\n-\t\trollback_lock_file(&lock);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn error_resolve_conflict(_(action_name(opts)));\n+\t\tret = error_resolve_conflict(_(action_name(opts)));\n+\t\tgoto cleanup;\n \t}\n \n \tif (!fill_tree_descriptor(r, &desc, &oid)) {\n-\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn -1;\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(&oid));\n+\t\tgoto cleanup;\n \t}\n \n \tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\tstrbuf_release(&ref_name);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n@@ -3722,13 +3714,15 @@ static int do_reset(struct repository *r,\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n \t\tret = error(_(\"could not write index\"));\n-\tfree((void *)desc.buffer);\n \n \tif (!ret)\n \t\tret = update_ref(reflog_message(opts, \"reset\", \"'%.*s'\",\n \t\t\t\t\t\tlen, name), \"HEAD\", &oid,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n-\n+cleanup:\n+\tfree((void *)desc.buffer);\n+\tif (ret < 0)\n+\t\trollback_lock_file(&lock);\n \tstrbuf_release(&ref_name);\n \treturn ret;\n }\n-- \n2.33.0.1569.gd2dc77f7abf\n\n"},{"id":"438654","messageId":"patch-v3-3.3-00b6b469a8c-20211013T132223Z-avarab@gmail.com","threadId":"56653","inReplyTo":"cover-v3-0.3-00000000000-20211013T132223Z-avarab@gmail.com","subject":"[PATCH v3 3/3] sequencer: fix a memory leak in do_reset()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T13:23:55Z","receivedAt":"2021-10-13T13:24:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak introduced in 9055e401dd6 (sequencer: introduce new\ncommands to reset the revision, 2018-04-25), which called\nsetup_unpack_trees_porcelain() without a corresponding call to\nclear_unpack_trees_porcelain().\n\nThis introduces a change in behavior in that we now start calling\nclear_unpack_trees_porcelain() even without having called the\nsetup_unpack_trees_porcelain(). That's OK, that clear function, like\nmost others, will accept a zero'd out struct.\n\nThis inches us closer to passing various tests in\n\"t34*.sh\" (e.g. \"t3434-rebase-i18n.sh\"), but because they have so many\nother memory leaks in revisions.c this doesn't make any test file or\neven a single test pass.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 14c37c4e25b..98181b3186e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3646,7 +3646,7 @@ static int do_reset(struct repository *r,\n \tstruct lock_file lock = LOCK_INIT;\n \tstruct tree_desc desc = { 0 };\n \tstruct tree *tree;\n-\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct unpack_trees_options unpack_tree_opts = { 0 };\n \tint ret = 0;\n \n \tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)\n@@ -3683,7 +3683,6 @@ static int do_reset(struct repository *r,\n \t\t}\n \t}\n \n-\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = r->index;\n@@ -3724,6 +3723,7 @@ static int do_reset(struct repository *r,\n \tif (ret < 0)\n \t\trollback_lock_file(&lock);\n \tstrbuf_release(&ref_name);\n+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \treturn ret;\n }\n \n-- \n2.33.0.1569.gd2dc77f7abf\n\n"}]}