{"thread":{"id":"55969","subject":"[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2","startedAt":"2021-06-20T15:12:13Z","lastAt":"2021-07-27T19:34:43Z","messageCount":51,"participants":["andrzej@ahunt.org","Phillip Wood","Elijah Newren","René Scharfe","Andrzej Hunt","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"427975","messageId":"20210620151204.19260-1-andrzej@ahunt.org","threadId":"55969","inReplyTo":null,"subject":"[PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:52Z","receivedAt":"2021-06-20T15:12:13Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nThis series plugs more of the leaks that were found while running\nt0002-t0099 with LSAN.\n\nSee also the first series (already merged) at [1]. I'm currently\nexpecting at least another 2 series before t0002-t0099 run leak free.\nI'm not being particularly systematic about the order of patches -\nalthough I am trying to send out \"real\" (if mostly small) leaks first,\nbefore sending out the more boring patches that add free()/UNLEAK() to\ncmd_* and direct helpers thereof.\n\nATB,\n\nAndrzej\n\n[1] https://lore.kernel.org/git/pull.929.git.1617994052.gitgitgadget@gmail.com/\n\nAndrzej Hunt (12):\n fmt-merge-msg: free newly allocated temporary strings when done\n environment: move strbuf into block to plug leak\n builtin/submodule--helper: release unused strbuf to avoid leak\n for-each-repo: remove unnecessary argv copy to plug leak\n diffcore-rename: move old_dir/new_dir definition to plug leak\n ref-filter: also free head for ATOM_HEAD to avoid leak\n read-cache: call diff_setup_done to avoid leak\n convert: release strbuf to avoid leak\n builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv\n builtin/merge: free found_ref when done\n builtin/rebase: fix options.strategy memory lifecycle\n reset: clear_unpack_trees_porcelain to plug leak\n\n builtin/for-each-repo.c | 14 ++++----------\n builtin/merge.c | 3 ++-\n builtin/mv.c | 5 +++++\n builtin/rebase.c | 5 ++---\n builtin/submodule--helper.c | 6 ++++--\n convert.c | 2 ++\n diffcore-rename.c | 10 +++++++---\n environment.c | 7 +++----\n fmt-merge-msg.c | 6 ++++--\n read-cache.c | 1 +\n ref-filter.c | 8 ++++++--\n reset.c | 4 ++--\n 12 files changed, 42 insertions(+), 29 deletions(-)\n\n-- \n2.26.2\n\n"},{"id":"427976","messageId":"20210620151204.19260-2-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:53Z","receivedAt":"2021-06-20T15:12:20Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\norigin starts off pointing to somewhere within line, which is owned by\nthe caller. Later we might allocate a new string using xmemdupz() or\nxstrfmt(). To avoid leaking these new strings, we introduce a to_free\npointer - which allows us to safely free the newly allocated string when\nwe're done (we cannot just free origin directly as it might still be\npointing to line).\n\nLSAN output from t0090:\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa71f49 in do_xmalloc wrapper.c:41:8\n #2 0xa720b0 in do_xmallocz wrapper.c:75:8\n #3 0xa720b0 in xmallocz wrapper.c:83:9\n #4 0xa720b0 in xmemdupz wrapper.c:99:16\n #5 0x8092ba in handle_line fmt-merge-msg.c:187:23\n #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7\n #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2\n #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3\n #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6b3fad in main common-main.c:52:11\n #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n fmt-merge-msg.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\nindex 0f66818e0f..b969dc6ebb 100644\n--- a/fmt-merge-msg.c\n+++ b/fmt-merge-msg.c\n@@ -105,90 +105,92 @@ static void add_merge_parent(struct merge_parents *table,\n static int handle_line(char *line, struct merge_parents *merge_parents)\n {\n \tint i, len = strlen(line);\n \tstruct origin_data *origin_data;\n \tchar *src;\n \tconst char *origin, *tag_name;\n+\tchar *to_free = NULL;\n \tstruct src_data *src_data;\n \tstruct string_list_item *item;\n \tint pulling_head = 0;\n \tstruct object_id oid;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n \tif (len < hexsz + 3 || line[hexsz] != '\\t')\n \t\treturn 1;\n \n \tif (starts_with(line + hexsz + 1, \"not-for-merge\"))\n \t\treturn 0;\n \n \tif (line[hexsz + 1] != '\\t')\n \t\treturn 2;\n \n \ti = get_oid_hex(line, &oid);\n \tif (i)\n \t\treturn 3;\n \n \tif (!find_merge_parent(merge_parents, &oid, NULL))\n \t\treturn 0; /* subsumed by other parents */\n \n \tCALLOC_ARRAY(origin_data, 1);\n \toidcpy(&origin_data->oid, &oid);\n \n \tif (line[len - 1] == '\\n')\n \t\tline[len - 1] = 0;\n \tline += hexsz + 2;\n \n \t/*\n \t * At this point, line points at the beginning of comment e.g.\n \t * \"branch 'frotz' of git://that/repository.git\".\n \t * Find the repository name and point it with src.\n \t */\n \tsrc = strstr(line, \" of \");\n \tif (src) {\n \t\t*src = 0;\n \t\tsrc += 4;\n \t\tpulling_head = 0;\n \t} else {\n \t\tsrc = line;\n \t\tpulling_head = 1;\n \t}\n \n \titem = unsorted_string_list_lookup(&srcs, src);\n \tif (!item) {\n \t\titem = string_list_append(&srcs, src);\n \t\titem->util = xcalloc(1, sizeof(struct src_data));\n \t\tinit_src_data(item->util);\n \t}\n \tsrc_data = item->util;\n \n \tif (pulling_head) {\n \t\torigin = src;\n \t\tsrc_data->head_status |= 1;\n \t} else if (skip_prefix(line, \"branch \", &origin)) {\n \t\torigin_data->is_local_branch = 1;\n \t\tstring_list_append(&src_data->branch, origin);\n \t\tsrc_data->head_status |= 2;\n \t} else if (skip_prefix(line, \"tag \", &tag_name)) {\n \t\torigin = line;\n \t\tstring_list_append(&src_data->tag, tag_name);\n \t\tsrc_data->head_status |= 2;\n \t} else if (skip_prefix(line, \"remote-tracking branch \", &origin)) {\n \t\tstring_list_append(&src_data->r_branch, origin);\n \t\tsrc_data->head_status |= 2;\n \t} else {\n \t\torigin = src;\n \t\tstring_list_append(&src_data->generic, line);\n \t\tsrc_data->head_status |= 2;\n \t}\n \n \tif (!strcmp(\".\", src) || !strcmp(src, origin)) {\n \t\tint len = strlen(origin);\n \t\tif (origin[0] == '\\'' && origin[len - 1] == '\\'')\n-\t\t\torigin = xmemdupz(origin + 1, len - 2);\n+\t\t\torigin = to_free = xmemdupz(origin + 1, len - 2);\n \t} else\n-\t\torigin = xstrfmt(\"%s of %s\", origin, src);\n+\t\torigin = to_free = xstrfmt(\"%s of %s\", origin, src);\n \tif (strcmp(\".\", src))\n \t\torigin_data->is_local_branch = 0;\n \tstring_list_append(&origins, origin)->util = origin_data;\n+\tfree(to_free);\n \treturn 0;\n }\n \n-- \n2.26.2\n\n"},{"id":"427977","messageId":"20210620151204.19260-3-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 02/12] environment: move strbuf into block to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:54Z","receivedAt":"2021-06-20T15:12:21Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrealpath is only populated if we execute the git_work_tree_initialized\nblock. However that block also causes us to return early, meaning we\nnever actually release the strbuf in the case where we populated it.\nTherefore we move all strbuf related code into the block to guarantee\nthat we can't leak it.\n\nLSAN output from t0095:\n\nDirect leak of 129 byte(s) in 1 object(s) allocated from:\n #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x78f585 in xrealloc wrapper.c:126:8\n #2 0x713ff4 in strbuf_grow strbuf.c:98:2\n #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3\n #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7\n #5 0x5ae4a4 in set_git_work_tree environment.c:259:3\n #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2\n #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12\n #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2\n #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3\n #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11\n #11 0x4caded in main common-main.c:52:11\n #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).\n\nIt looks like this leak has existed since realpath was first added to\nset_git_work_tree() in:\n 3d7747e318 (real_path: remove unsafe API, 2020-03-10)\n\nSigned-off-by: Andrzej Hunt \n---\n environment.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 2f27008424..d6b22ede7e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -249,25 +249,24 @@ static int git_work_tree_initialized;\n /*\n * Note. This works only before you used a work tree. This was added\n * primarily to support git-clone to work in a new repository it just\n * created, and is not meant to flip between different work trees.\n */\n void set_git_work_tree(const char *new_work_tree)\n {\n-\tstruct strbuf realpath = STRBUF_INIT;\n-\n \tif (git_work_tree_initialized) {\n+\t\tstruct strbuf realpath = STRBUF_INIT;\n+\n \t\tstrbuf_realpath(&realpath, new_work_tree, 1);\n \t\tnew_work_tree = realpath.buf;\n \t\tif (strcmp(new_work_tree, the_repository->worktree))\n \t\t\tdie(\"internal error: work tree has already been set\\n\"\n \t\t\t \"Current worktree: %s\\nNew worktree: %s\",\n \t\t\t the_repository->worktree, new_work_tree);\n+\t\tstrbuf_release(&realpath);\n \t\treturn;\n \t}\n \tgit_work_tree_initialized = 1;\n \trepo_set_worktree(the_repository, new_work_tree);\n-\n-\tstrbuf_release(&realpath);\n }\n \n const char *get_git_work_tree(void)\n-- \n2.26.2\n\n"},{"id":"427978","messageId":"20210620151204.19260-4-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 03/12] builtin/submodule--helper: release unused strbuf to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:55Z","receivedAt":"2021-06-20T15:12:21Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrelative_url() populates sb. In the normal return path, its buffer is\ndetached using strbuf_detach(). However the early return path does\nnothing with sb, which means that sb's memory is leaked - therefore\nwe add a release to avoid this leak.\n\nThe reset is also only necessary for the normal return path, hence we\nmove it down to after the early-return to avoid unnecessary work.\n\nLSAN output from t0060:\n\nDirect leak of 121 byte(s) in 1 object(s) allocated from:\n #0 0x7f31246f28b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n #1 0x98d7d6 in xrealloc wrapper.c:126\n #2 0x909a60 in strbuf_grow strbuf.c:98\n #3 0x90bf00 in strbuf_vaddf strbuf.c:401\n #4 0x90c321 in strbuf_addf strbuf.c:335\n #5 0x5cb78d in relative_url builtin/submodule--helper.c:182\n #6 0x5cbe46 in resolve_relative_url_test builtin/submodule--helper.c:248\n #7 0x410dcd in run_builtin git.c:475\n #8 0x410dcd in handle_builtin git.c:729\n #9 0x414087 in run_argv git.c:818\n #10 0x414087 in cmd_main git.c:949\n #11 0x40e9ec in main common-main.c:52\n #12 0x7f3123c41349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 121 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/submodule--helper.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ae6174ab05..4015d114b3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -188,11 +188,13 @@ static char *relative_url(const char *remote_url,\n \t\tout = xstrdup(sb.buf + 2);\n \telse\n \t\tout = xstrdup(sb.buf);\n-\tstrbuf_reset(&sb);\n \n-\tif (!up_path || !is_relative)\n+\tif (!up_path || !is_relative) {\n+\t\tstrbuf_release(&sb);\n \t\treturn out;\n+\t}\n \n+\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n \tfree(out);\n \treturn strbuf_detach(&sb, NULL);\n-- \n2.26.2\n\n"},{"id":"427979","messageId":"20210620151204.19260-5-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:56Z","receivedAt":"2021-06-20T15:12:23Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncmd_for_each_repo() copies argv into args (a strvec), which is later\npassed into run_command_on_repo(), which in turn copies that strvec onto\nthe end of child.args. The initial copy is unnecessary (we never modify\nargs). We therefore choose to just pass argv directly into\nrun_command_on_repo(), which lets us avoid the copy and fixes the leak.\n\nLSAN output from t0068:\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n #1 0x98d7e6 in xrealloc wrapper.c:126\n #2 0x916914 in strvec_push_nodup strvec.c:19\n #3 0x916a6e in strvec_push strvec.c:26\n #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n #5 0x410dcd in run_builtin git.c:475\n #6 0x410dcd in handle_builtin git.c:729\n #7 0x414087 in run_argv git.c:818\n #8 0x414087 in cmd_main git.c:949\n #9 0x40e9ec in main common-main.c:52\n #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 22 byte(s) in 2 object(s) allocated from:\n #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)\n #1 0x98d698 in xstrdup wrapper.c:29\n #2 0x916a63 in strvec_push strvec.c:26\n #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n #4 0x410dcd in run_builtin git.c:475\n #5 0x410dcd in handle_builtin git.c:729\n #6 0x414087 in run_argv git.c:818\n #7 0x414087 in cmd_main git.c:949\n #8 0x40e9ec in main common-main.c:52\n #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSee also discussion about the original implementation below - this code\nappears to have evolved from a callback explaining the double-strvec-copy\npattern, but there's no strong reason to keep that now:\n https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/for-each-repo.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex 52be64a437..fd86e5a861 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {\n \tNULL\n };\n \n-static int run_command_on_repo(const char *path,\n-\t\t\t void *cbdata)\n+static int run_command_on_repo(const char *path, int argc, const char ** argv)\n {\n \tint i;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tstruct strvec *args = (struct strvec *)cbdata;\n \n \tchild.git_cmd = 1;\n \tstrvec_pushl(&child.args, \"-C\", path, NULL);\n \n-\tfor (i = 0; i < args->nr; i++)\n-\t\tstrvec_push(&child.args, args->v[i]);\n+\tfor (i = 0; i < argc; i++)\n+\t\tstrvec_push(&child.args, argv[i]);\n \n \treturn run_command(&child);\n }\n@@ -29,37 +27,33 @@ static int run_command_on_repo(const char *path,\n int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n {\n \tstatic const char *config_key = NULL;\n \tint i, result = 0;\n \tconst struct string_list *values;\n-\tstruct strvec args = STRVEC_INIT;\n \n \tconst struct option options[] = {\n \t\tOPT_STRING(0, \"config\", &config_key, N_(\"config\"),\n \t\t\t N_(\"config key storing a list of repository paths\")),\n \t\tOPT_END()\n \t};\n \n \targc = parse_options(argc, argv, prefix, options, for_each_repo_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n \tif (!config_key)\n \t\tdie(_(\"missing --config=\"));\n \n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&args, argv[i]);\n-\n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t config_key);\n \n \t/*\n \t * Do nothing on an empty list, which is equivalent to the case\n \t * where the config variable does not exist at all.\n \t */\n \tif (!values)\n \t\treturn 0;\n \n \tfor (i = 0; !result && i < values->nr; i++)\n-\t\tresult = run_command_on_repo(values->items[i].string, &args);\n+\t\tresult = run_command_on_repo(values->items[i].string, argc, argv);\n \n \treturn result;\n }\n-- \n2.26.2\n\n"},{"id":"427980","messageId":"20210620151204.19260-6-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:57Z","receivedAt":"2021-06-20T15:12:28Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nold_dir/new_dir are free()'d at the end of update_dir_rename_counts,\nhowever if we return early we'll never free those strings. Therefore\nwe should move all new allocations after the possible early return,\navoiding a leak.\n\nThis seems like a fairly recent leak, that started happening since the\nearly-return was added in:\n 1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)\n\nLSAN output from t0022:\n\nDirect leak of 7 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71e48 in xstrdup wrapper.c:29:14\n #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12\n #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n #5 0x7b4cfc in diffcore_std diff.c:6705:4\n #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n #7 0x856574 in log_tree_diff log-tree.c:955:3\n #8 0x856574 in log_tree_commit log-tree.c:986:10\n #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6b3f3d in main common-main.c:52:11\n #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 7 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71e48 in xstrdup wrapper.c:29:14\n #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12\n #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n #5 0x7b4cfc in diffcore_std diff.c:6705:4\n #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n #7 0x856574 in log_tree_diff log-tree.c:955:3\n #8 0x856574 in log_tree_commit log-tree.c:986:10\n #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6b3f3d in main common-main.c:52:11\n #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n diffcore-rename.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 3375e24659..f7c728fe47 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t\t\t const char *oldname,\n \t\t\t\t const char *newname)\n {\n-\tchar *old_dir = xstrdup(oldname);\n-\tchar *new_dir = xstrdup(newname);\n-\tchar new_dir_first_char = new_dir[0];\n+\tchar *old_dir;\n+\tchar *new_dir;\n+\tconst char new_dir_first_char = newname[0];\n \tint first_time_in_loop = 1;\n \n \tif (!info->setup)\n@@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t */\n \t\treturn;\n \n+\n+\told_dir = xstrdup(oldname);\n+\tnew_dir = xstrdup(newname);\n+\n \twhile (1) {\n \t\tint drd_flag = NOT_RELEVANT;\n \n-- \n2.26.2\n\n"},{"id":"427981","messageId":"20210620151204.19260-7-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:58Z","receivedAt":"2021-06-20T15:12:32Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nu.head is populated using resolve_refdup(), which returns a newly\nallocated string - hence we also need to free() it.\n\nFound while running t0041 with LSAN:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa8be98 in xstrdup wrapper.c:29:14\n #2 0x9481db in head_atom_parser ref-filter.c:549:17\n #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30\n #4 0x9400e3 in verify_ref_format ref-filter.c:974:8\n #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6\n #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3\n #7 0x4ce83e in run_builtin git.c:475:11\n #8 0x4ccafe in handle_builtin git.c:729:3\n #9 0x4cb01c in run_argv git.c:818:4\n #10 0x4cb01c in cmd_main git.c:949:19\n #11 0x6bdc2d in main common-main.c:52:11\n #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n ref-filter.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4db0e40ff4..f8bfd25ae4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2225,8 +2225,12 @@ void ref_array_clear(struct ref_array *array)\n \tFREE_AND_NULL(array->items);\n \tarray->nr = array->alloc = 0;\n \n-\tfor (i = 0; i < used_atom_cnt; i++)\n-\t\tfree((char *)used_atom[i].name);\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tif (atom->atom_type == ATOM_HEAD)\n+\t\t\tfree(atom->u.head);\n+\t\tfree((char *)atom->name);\n+\t}\n \tFREE_AND_NULL(used_atom);\n \tused_atom_cnt = 0;\n \n-- \n2.26.2\n\n"},{"id":"427982","messageId":"20210620151204.19260-8-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 07/12] read-cache: call diff_setup_done to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:11:59Z","receivedAt":"2021-06-20T15:12:41Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrepo_diff_setup() calls through to diff.c's static prep_parse_options(),\nwhich in turn allocates a new array into diff_opts.parseopts.\ndiff_setup_done() is responsible for freeing that array, and has the\nbenefit of verifying diff_opts too - hence we add a call to\ndiff_setup_done() to avoid leaking parseopts.\n\nOutput from the leak as found while running t0090 with LSAN:\n\nDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bf89 in do_xmalloc wrapper.c:41:8\n #2 0x7a7bae in prep_parse_options diff.c:5636:2\n #3 0x7a7bae in repo_diff_setup diff.c:4611:2\n #4 0x93716c in repo_index_has_changes read-cache.c:2518:3\n #5 0x872233 in unclean merge-ort-wrappers.c:12:14\n #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6\n #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12\n #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9\n #9 0x4ce83e in run_builtin git.c:475:11\n #10 0x4ccafe in handle_builtin git.c:729:3\n #11 0x4cb01c in run_argv git.c:818:4\n #12 0x4cb01c in cmd_main git.c:949:19\n #13 0x6bdc2d in main common-main.c:52:11\n #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)\n\nSigned-off-by: Andrzej Hunt \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 77961a3885..212d604dd3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)\n int repo_index_has_changes(struct repository *repo,\n \t\t\t struct tree *tree,\n \t\t\t struct strbuf *sb)\n {\n \tstruct index_state *istate = repo->index;\n \tstruct object_id cmp;\n \tint i;\n \n \tif (tree)\n \t\tcmp = tree->object.oid;\n \tif (tree || !get_oid_tree(\"HEAD\", &cmp)) {\n \t\tstruct diff_options opt;\n \n \t\trepo_diff_setup(repo, &opt);\n \t\topt.flags.exit_with_status = 1;\n \t\tif (!sb)\n \t\t\topt.flags.quick = 1;\n+\t\tdiff_setup_done(&opt);\n \t\tdo_diff_cache(&cmp, &opt);\n \t\tdiffcore_std(&opt);\n \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n \t\t\tif (i)\n \t\t\t\tstrbuf_addch(sb, ' ');\n \t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n \t\t}\n \t\tdiff_flush(&opt);\n \t\treturn opt.flags.has_changes != 0;\n \t} else {\n \t\t/* TODO: audit for interaction with sparse-index. */\n \t\tensure_full_index(istate);\n \t\tfor (i = 0; sb && i < istate->cache_nr; i++) {\n \t\t\tif (i)\n \t\t\t\tstrbuf_addch(sb, ' ');\n \t\t\tstrbuf_addstr(sb, istate->cache[i]->name);\n \t\t}\n \t\treturn !!istate->cache_nr;\n \t}\n }\n-- \n2.26.2\n\n"},{"id":"427983","messageId":"20210620151204.19260-9-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 08/12] convert: release strbuf to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:12:00Z","receivedAt":"2021-06-20T15:12:54Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\napply_multi_file_filter and async_query_available_blobs both query\nsubprocess output using subprocess_read_status, which writes data into\nthe identically named filter_status strbuf. We add a strbuf_release to\navoid leaking their contents.\n\nLeak output seen when running t0021 with LSAN:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c2b5 in xrealloc wrapper.c:126:8\n #2 0x9ff99d in strbuf_grow strbuf.c:98:2\n #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2\n #4 0xa101d6 in subprocess_read_status sub-process.c:45:5\n #5 0x77793c in apply_multi_file_filter convert.c:886:8\n #6 0x77793c in apply_filter convert.c:1042:10\n #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7\n #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2\n #9 0x8b48cd in index_fd object-file.c:2248:9\n #10 0x597411 in hash_fd builtin/hash-object.c:43:9\n #11 0x596be1 in hash_object builtin/hash-object.c:59:2\n #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3\n #13 0x4ce83e in run_builtin git.c:475:11\n #14 0x4ccafe in handle_builtin git.c:729:3\n #15 0x4cb01c in run_argv git.c:818:4\n #16 0x4cb01c in cmd_main git.c:949:19\n #17 0x6bdc2d in main common-main.c:52:11\n #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n\nDirect leak of 120 byte(s) in 5 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c295 in xrealloc wrapper.c:126:8\n #2 0x9ff97d in strbuf_grow strbuf.c:98:2\n #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2\n #4 0xa101b6 in subprocess_read_status sub-process.c:45:5\n #5 0x775c73 in async_query_available_blobs convert.c:960:8\n #6 0x80029d in finish_delayed_checkout entry.c:183:9\n #7 0xa65d1e in check_updates unpack-trees.c:493:10\n #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8\n #9 0x525971 in checkout builtin/clone.c:815:6\n #10 0x525971 in cmd_clone builtin/clone.c:1409:8\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6bdc2d in main common-main.c:52:11\n #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n convert.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b025..0d6fb3410a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \telse\n \t\tstrbuf_swap(dst, &nbuf);\n \tstrbuf_release(&nbuf);\n+\tstrbuf_release(&filter_status);\n \treturn !err;\n }\n \n@@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p\n \n \tif (err)\n \t\thandle_filter_error(&filter_status, entry, 0);\n+\tstrbuf_release(&filter_status);\n \treturn !err;\n }\n \n-- \n2.26.2\n\n"},{"id":"427984","messageId":"20210620151204.19260-10-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:12:01Z","receivedAt":"2021-06-20T15:12:56Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nThese leaks all happen at the end of cmd_mv, hence don't matter in any\nway. But we still fix the easy ones and squash the rest to get us closer\nto being able to run tests without leaks.\n\nLSAN output from t0050:\n\nDirect leak of 384 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa0a7e1 in add_entry string-list.c:44:2\n #3 0xa0a7e1 in string_list_insert string-list.c:58:14\n #4 0x5dac03 in cmd_mv builtin/mv.c:248:4\n #5 0x4ce83e in run_builtin git.c:475:11\n #6 0x4ccafe in handle_builtin git.c:729:3\n #7 0x4cb01c in run_argv git.c:818:4\n #8 0x4cb01c in cmd_main git.c:949:19\n #9 0x6bd9ad in main common-main.c:52:11\n #10 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bd09 in do_xmalloc wrapper.c:41:8\n #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2\n #3 0x5da575 in cmd_mv builtin/mv.c:158:14\n #4 0x4ce83e in run_builtin git.c:475:11\n #5 0x4ccafe in handle_builtin git.c:729:3\n #6 0x4cb01c in run_argv git.c:818:4\n #7 0x4cb01c in cmd_main git.c:949:19\n #8 0x6bd9ad in main common-main.c:52:11\n #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bd09 in do_xmalloc wrapper.c:41:8\n #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2\n #3 0x5da4e4 in cmd_mv builtin/mv.c:148:11\n #4 0x4ce83e in run_builtin git.c:475:11\n #5 0x4ccafe in handle_builtin git.c:729:3\n #6 0x4cb01c in run_argv git.c:818:4\n #7 0x4cb01c in cmd_main git.c:949:19\n #8 0x6bd9ad in main common-main.c:52:11\n #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0xa8c119 in xcalloc wrapper.c:140:8\n #2 0x5da585 in cmd_mv builtin/mv.c:159:22\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6bd9ad in main common-main.c:52:11\n #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 4 byte(s) in 1 object(s) allocated from:\n #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0xa8c119 in xcalloc wrapper.c:140:8\n #2 0x5da4f8 in cmd_mv builtin/mv.c:149:10\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6bd9ad in main common-main.c:52:11\n #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa00226 in strbuf_grow strbuf.c:98:2\n #3 0xa00226 in strbuf_vaddf strbuf.c:394:3\n #4 0xa065c7 in xstrvfmt strbuf.c:981:2\n #5 0xa065c7 in xstrfmt strbuf.c:991:8\n #6 0x9e7ce7 in prefix_path_gently setup.c:115:15\n #7 0x9e7fa6 in prefix_path setup.c:128:12\n #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23\n #9 0x5da575 in cmd_mv builtin/mv.c:158:14\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6bd9ad in main common-main.c:52:11\n #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa00226 in strbuf_grow strbuf.c:98:2\n #3 0xa00226 in strbuf_vaddf strbuf.c:394:3\n #4 0xa065c7 in xstrvfmt strbuf.c:981:2\n #5 0xa065c7 in xstrfmt strbuf.c:991:8\n #6 0x9e7ce7 in prefix_path_gently setup.c:115:15\n #7 0x9e7fa6 in prefix_path setup.c:128:12\n #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23\n #9 0x5da4e4 in cmd_mv builtin/mv.c:148:11\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6bd9ad in main common-main.c:52:11\n #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 558 byte(s) leaked in 7 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/mv.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 3fccdcb645..c2f96c8e89 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -303,5 +303,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+\tstring_list_clear(&src_for_dst, 0);\n+\tUNLEAK(source);\n+\tUNLEAK(dest_path);\n+\tfree(submodule_gitfile);\n+\tfree(modes);\n \treturn 0;\n }\n-- \n2.26.2\n\n"},{"id":"427985","messageId":"20210620151204.19260-11-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 10/12] builtin/merge: free found_ref when done","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:12:02Z","receivedAt":"2021-06-20T15:12:56Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nmerge_name() calls dwim_ref(), which allocates a new string into\nfound_ref. Therefore add a free() to avoid leaking found_ref.\n\nLSAN output from t0021:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa8beb8 in xstrdup wrapper.c:29:14\n #2 0x954054 in expand_ref refs.c:671:12\n #3 0x953cb6 in repo_dwim_ref refs.c:644:22\n #4 0x5d3759 in dwim_ref refs.h:162:9\n #5 0x5d3759 in merge_name builtin/merge.c:517:6\n #6 0x5d3759 in collect_parents builtin/merge.c:1214:5\n #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6bdbfd in main common-main.c:52:11\n #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/merge.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8a843b1f5..7ad85c044a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n-\tchar *found_ref;\n+\tchar *found_ref = NULL;\n \tint len, early;\n \n \tstrbuf_branchname(&bname, remote, 0);\n@@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \tstrbuf_addf(msg, \"%s\\t\\tcommit '%s'\\n\",\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n+\tfree(found_ref);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n-- \n2.26.2\n\n"},{"id":"427986","messageId":"20210620151204.19260-12-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:12:03Z","receivedAt":"2021-06-20T15:12:59Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nThis change:\n- xstrdup()'s all string being used for replace_opts.strategy, to\n guarantee that replace_opts owns these strings. This is needed because\n sequencer_remove_state() will free replace_opts.strategy, and it's\n usually called as part of the usage of replace_opts.\n- Removes xstrdup()'s being used to populate options.strategy in\n cmd_rebase(), which avoids leaking options.strategy, even in the\n case where strategy is never moved/copied into replace_opts.\n\nThese changes are needed because:\n- We would always create a new string for options.strategy if we either\n get a strategy via options (OPT_STRING(...strategy...), or via\n GIT_TEST_MERGE_ALGORITHM.\n- But only sometimes is this string copied into replace_opts - in which\n case it did get free()'d in sequencer_remove_state().\n- The rest of the time, the newly allocated string would remain unused,\n causing a leak. But we can't just add a free because that can result\n in a double-free in those cases where replace_opts was populated.\n\nAn alternative approach would be to set options.strategy to NULL when\nmoving the pointer to replace_opts.strategy, combined with always\nfree()'ing options.strategy, but that seems like a more\ncomplicated and wasteful approach.\n\nThis was first seen when running t0021 with LSAN, but t2012 helped catch\nthe fact that we can't just free(options.strategy) at the end of\ncmd_rebase (as that can cause a double-free). LSAN output from t0021:\n\nLSAN output from t0021:\n\nDirect leak of 4 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71eb8 in xstrdup wrapper.c:29:14\n #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6b3fad in main common-main.c:52:11\n #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rebase.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d..9d81db0f3a 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \treplay.ignore_date = opts->ignore_date;\n \treplay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);\n \tif (opts->strategy)\n-\t\treplay.strategy = opts->strategy;\n+\t\treplay.strategy = xstrdup_or_null(opts->strategy);\n \telse if (!replay.strategy && replay.default_strategy) {\n \t\treplay.strategy = replay.default_strategy;\n \t\treplay.default_strategy = NULL;\n@@ -1723,7 +1723,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (options.strategy) {\n-\t\toptions.strategy = xstrdup(options.strategy);\n \t\tswitch (options.type) {\n \t\tcase REBASE_APPLY:\n \t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n@@ -1776,7 +1775,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.type == REBASE_MERGE &&\n \t !options.strategy &&\n \t getenv(\"GIT_TEST_MERGE_ALGORITHM\"))\n-\t\toptions.strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n+\t\toptions.strategy = getenv(\"GIT_TEST_MERGE_ALGORITHM\");\n \n \tswitch (options.type) {\n \tcase REBASE_MERGE:\n-- \n2.26.2\n\n"},{"id":"427987","messageId":"20210620151204.19260-13-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-06-20T15:12:04Z","receivedAt":"2021-06-20T15:13:05Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nsetup_unpack_trees_porcelain() populates various fields on\nunpack_tree_opts, we need to call clear_unpack_trees_porcelain() to\navoid leaking them. Specifically, we used to leak\nunpack_tree_opts.msgs_to_free.\n\nWe have to do this in leave_reset_head because there are multiple\nscenarios where unpack_tree_opts has already been configured, followed\nby a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'\nprior to having initialised unpack_tree_opts via memset(..., 0, ...).\nTherefore we also move unpack_tree_opts initialisation to the start of\nreset_head(), and convert it to use brace initialisation - which\nguarantees that we can never clear an unitialised unpack_tree_opts.\nclear_unpack_tree_opts() is always safe to call as long as\nunpack_tree_opts is at least zero-initialised, i.e. it does not depend\non a previous call to setup_unpack_trees_porcelain().\n\nLSAN output from t0021:\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9f7861 in strvec_push_nodup strvec.c:19:2\n #3 0x9f7861 in strvec_pushf strvec.c:39:2\n #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n #5 0x97e011 in reset_head reset.c:53:2\n #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #7 0x4ce83e in run_builtin git.c:475:11\n #8 0x4ccafe in handle_builtin git.c:729:3\n #9 0x4cb01c in run_argv git.c:818:4\n #10 0x4cb01c in cmd_main git.c:949:19\n #11 0x6b3f3d in main common-main.c:52:11\n #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 147 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 134 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 130 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n reset.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reset.c b/reset.c\nindex 4bea758053..79310ae071 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n-\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct unpack_trees_options unpack_tree_opts = { 0 };\n \tstruct tree *tree;\n \tconst char *reflog_action;\n \tstruct strbuf msg = STRBUF_INIT;\n@@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \tif (refs_only)\n \t\tgoto reset_head_refs;\n \n-\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, action);\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = r->index;\n@@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n leave_reset_head:\n \tstrbuf_release(&msg);\n \trollback_lock_file(&lock);\n+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \twhile (nr)\n \t\tfree((void *)desc[--nr].buffer);\n \treturn ret;\n-- \n2.26.2\n\n"},{"id":"427991","messageId":"6e02fc85-42a4-8b19-1fe7-3527c2308a24@gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-12-andrzej@ahunt.org","subject":"Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-20T18:14:36Z","receivedAt":"2021-06-20T18:14:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Andrzej\n\nThanks for working on removing memory leaks from git.\n\nOn 20/06/2021 16:12, andrzej@ahunt.org wrote:\n> From: Andrzej Hunt \n> \n> This change:\n> - xstrdup()'s all string being used for replace_opts.strategy, to\n\nI think you mean replay_opts rather than replace_opts.\n\n> guarantee that replace_opts owns these strings. This is needed because\n> sequencer_remove_state() will free replace_opts.strategy, and it's\n> usually called as part of the usage of replace_opts.\n> - Removes xstrdup()'s being used to populate options.strategy in\n> cmd_rebase(), which avoids leaking options.strategy, even in the\n> case where strategy is never moved/copied into replace_opts.\n\n\n> These changes are needed because:\n> - We would always create a new string for options.strategy if we either\n> get a strategy via options (OPT_STRING(...strategy...), or via\n> GIT_TEST_MERGE_ALGORITHM.\n> - But only sometimes is this string copied into replace_opts - in which\n> case it did get free()'d in sequencer_remove_state().\n> - The rest of the time, the newly allocated string would remain unused,\n> causing a leak. But we can't just add a free because that can result\n> in a double-free in those cases where replace_opts was populated.\n> \n> An alternative approach would be to set options.strategy to NULL when\n> moving the pointer to replace_opts.strategy, combined with always\n> free()'ing options.strategy, but that seems like a more\n> complicated and wasteful approach.\n\nread_basic_state() contains\n\tif (file_exists(state_dir_path(\"strategy\", opts))) {\n\t\tstrbuf_reset(&buf);\n\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n\t\t\t\t READ_ONELINER_WARN_MISSING))\n\t\t\treturn -1;\n\t\tfree(opts->strategy);\n\t\topts->strategy = xstrdup(buf.buf);\n\t}\n\nSo we do try to free opts->strategy when reading the state from disc and \nwe allocate a new string. I suspect that opts->strategy is actually NULL \nin when this function is called but I haven't checked. Given that we are \nallocating a copy above I think maybe your alternative approach of \nalways freeing opts->strategy would be better.\n\nBest Wishes\n\nPhillip\n\n> This was first seen when running t0021 with LSAN, but t2012 helped catch\n> the fact that we can't just free(options.strategy) at the end of\n> cmd_rebase (as that can cause a double-free). LSAN output from t0021:\n> \n> LSAN output from t0021:\n> \n> Direct leak of 4 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa71eb8 in xstrdup wrapper.c:29:14\n> #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22\n> #3 0x4ce83e in run_builtin git.c:475:11\n> #4 0x4ccafe in handle_builtin git.c:729:3\n> #5 0x4cb01c in run_argv git.c:818:4\n> #6 0x4cb01c in cmd_main git.c:949:19\n> #7 0x6b3fad in main common-main.c:52:11\n> #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n> \n> SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).\n> \n> Signed-off-by: Andrzej Hunt \n> ---\n> builtin/rebase.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 12f093121d..9d81db0f3a 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \treplay.ignore_date = opts->ignore_date;\n> \treplay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);\n> \tif (opts->strategy)\n> -\t\treplay.strategy = opts->strategy;\n> +\t\treplay.strategy = xstrdup_or_null(opts->strategy);\n> \telse if (!replay.strategy && replay.default_strategy) {\n> \t\treplay.strategy = replay.default_strategy;\n> \t\treplay.default_strategy = NULL;\n> @@ -1723,7 +1723,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tif (options.strategy) {\n> -\t\toptions.strategy = xstrdup(options.strategy);\n> \t\tswitch (options.type) {\n> \t\tcase REBASE_APPLY:\n> \t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n> @@ -1776,7 +1775,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.type == REBASE_MERGE &&\n> \t !options.strategy &&\n> \t getenv(\"GIT_TEST_MERGE_ALGORITHM\"))\n> -\t\toptions.strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n> +\t\toptions.strategy = getenv(\"GIT_TEST_MERGE_ALGORITHM\");\n> \n> \tswitch (options.type) {\n> \tcase REBASE_MERGE:\n> \n\n"},{"id":"428036","messageId":"CABPp-BHvr1Y0XxXfEavhOKkrSf9s6S5H8dXBZy0kR5CTohmuUQ@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-6-andrzej@ahunt.org","subject":"Re: [PATCH 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T14:01:20Z","receivedAt":"2021-06-21T14:01:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> old_dir/new_dir are free()'d at the end of update_dir_rename_counts,\n> however if we return early we'll never free those strings. Therefore\n> we should move all new allocations after the possible early return,\n> avoiding a leak.\n>\n> This seems like a fairly recent leak, that started happening since the\n> early-return was added in:\n> 1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)\n\nThe entire function was added relatively recently, just a few commits\nbefore that one at\n 0c4fd732f0 (\"Move computation of dir_rename_count from merge-ort to\ndiffcore-rename\", 2021-02-27)\n\n> LSAN output from t0022:\n>\n> Direct leak of 7 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa71e48 in xstrdup wrapper.c:29:14\n> #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12\n> #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n> #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n> #5 0x7b4cfc in diffcore_std diff.c:6705:4\n> #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n> #7 0x856574 in log_tree_diff log-tree.c:955:3\n> #8 0x856574 in log_tree_commit log-tree.c:986:10\n> #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n> #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n> #11 0x4ce83e in run_builtin git.c:475:11\n> #12 0x4ccafe in handle_builtin git.c:729:3\n> #13 0x4cb01c in run_argv git.c:818:4\n> #14 0x4cb01c in cmd_main git.c:949:19\n> #15 0x6b3f3d in main common-main.c:52:11\n> #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Direct leak of 7 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa71e48 in xstrdup wrapper.c:29:14\n> #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12\n> #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n> #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n> #5 0x7b4cfc in diffcore_std diff.c:6705:4\n> #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n> #7 0x856574 in log_tree_diff log-tree.c:955:3\n> #8 0x856574 in log_tree_commit log-tree.c:986:10\n> #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n> #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n> #11 0x4ce83e in run_builtin git.c:475:11\n> #12 0x4ccafe in handle_builtin git.c:729:3\n> #13 0x4cb01c in run_argv git.c:818:4\n> #14 0x4cb01c in cmd_main git.c:949:19\n> #15 0x6b3f3d in main common-main.c:52:11\n> #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).\n\nI ran this code under valgrind with specific tests, but apparently not\nunder enough different cases. Thanks for catching this.\n\n> Signed-off-by: Andrzej Hunt \n> ---\n> diffcore-rename.c | 10 +++++++---\n> 1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index 3375e24659..f7c728fe47 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n> const char *oldname,\n> const char *newname)\n> {\n> - char *old_dir = xstrdup(oldname);\n> - char *new_dir = xstrdup(newname);\n> - char new_dir_first_char = new_dir[0];\n> + char *old_dir;\n> + char *new_dir;\n> + const char new_dir_first_char = newname[0];\n> int first_time_in_loop = 1;\n>\n> if (!info->setup)\n> @@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n> */\n> return;\n>\n> +\n> + old_dir = xstrdup(oldname);\n> + new_dir = xstrdup(newname);\n> +\n> while (1) {\n> int drd_flag = NOT_RELEVANT;\n>\n> --\n> 2.26.2\n\nThe patch is correct.\n"},{"id":"428101","messageId":"CABPp-BE=1bNxm9eVUBaCwDqXrFCXccWJ2yHftvCUQ+4Rr8seYw@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-9-andrzej@ahunt.org","subject":"Re: [PATCH 08/12] convert: release strbuf to avoid leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T20:31:28Z","receivedAt":"2021-06-21T20:31:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:15 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> apply_multi_file_filter and async_query_available_blobs both query\n> subprocess output using subprocess_read_status, which writes data into\n> the identically named filter_status strbuf. We add a strbuf_release to\n> avoid leaking their contents.\n>\n> Leak output seen when running t0021 with LSAN:\n>\n> Direct leak of 24 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa8c2b5 in xrealloc wrapper.c:126:8\n> #2 0x9ff99d in strbuf_grow strbuf.c:98:2\n> #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2\n> #4 0xa101d6 in subprocess_read_status sub-process.c:45:5\n> #5 0x77793c in apply_multi_file_filter convert.c:886:8\n> #6 0x77793c in apply_filter convert.c:1042:10\n> #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7\n> #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2\n> #9 0x8b48cd in index_fd object-file.c:2248:9\n> #10 0x597411 in hash_fd builtin/hash-object.c:43:9\n> #11 0x596be1 in hash_object builtin/hash-object.c:59:2\n> #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3\n> #13 0x4ce83e in run_builtin git.c:475:11\n> #14 0x4ccafe in handle_builtin git.c:729:3\n> #15 0x4cb01c in run_argv git.c:818:4\n> #16 0x4cb01c in cmd_main git.c:949:19\n> #17 0x6bdc2d in main common-main.c:52:11\n> #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n>\n> Direct leak of 120 byte(s) in 5 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa8c295 in xrealloc wrapper.c:126:8\n> #2 0x9ff97d in strbuf_grow strbuf.c:98:2\n> #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2\n> #4 0xa101b6 in subprocess_read_status sub-process.c:45:5\n> #5 0x775c73 in async_query_available_blobs convert.c:960:8\n> #6 0x80029d in finish_delayed_checkout entry.c:183:9\n> #7 0xa65d1e in check_updates unpack-trees.c:493:10\n> #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8\n> #9 0x525971 in checkout builtin/clone.c:815:6\n> #10 0x525971 in cmd_clone builtin/clone.c:1409:8\n> #11 0x4ce83e in run_builtin git.c:475:11\n> #12 0x4ccafe in handle_builtin git.c:729:3\n> #13 0x4cb01c in run_argv git.c:818:4\n> #14 0x4cb01c in cmd_main git.c:949:19\n> #15 0x6bdc2d in main common-main.c:52:11\n> #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> convert.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/convert.c b/convert.c\n> index fd9c84b025..0d6fb3410a 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> else\n> strbuf_swap(dst, &nbuf);\n> strbuf_release(&nbuf);\n> + strbuf_release(&filter_status);\n> return !err;\n> }\n>\n> @@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p\n>\n> if (err)\n> handle_filter_error(&filter_status, entry, 0);\n> + strbuf_release(&filter_status);\n> return !err;\n> }\n>\n> --\n> 2.26.2\n\nMakes sense; a `git grep -e STRBUF_INIT -e strbuf_release -e\nstrbuf_init convert.c` does a fairly good job of highlighting that\nthese appear to be the only two strbuf leaks in this file.\n"},{"id":"428104","messageId":"CABPp-BH_35UkaXhaBeo_SVPcyRk=OuENpGr+L3Jkycz6RNh1LQ@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-2-andrzej@ahunt.org","subject":"Re: [PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T20:34:30Z","receivedAt":"2021-06-21T20:34:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> origin starts off pointing to somewhere within line, which is owned by\n> the caller. Later we might allocate a new string using xmemdupz() or\n> xstrfmt(). To avoid leaking these new strings, we introduce a to_free\n> pointer - which allows us to safely free the newly allocated string when\n> we're done (we cannot just free origin directly as it might still be\n> pointing to line).\n>\n> LSAN output from t0090:\n>\n> Direct leak of 8 byte(s) in 1 object(s) allocated from:\n> #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0xa71f49 in do_xmalloc wrapper.c:41:8\n> #2 0xa720b0 in do_xmallocz wrapper.c:75:8\n> #3 0xa720b0 in xmallocz wrapper.c:83:9\n> #4 0xa720b0 in xmemdupz wrapper.c:99:16\n> #5 0x8092ba in handle_line fmt-merge-msg.c:187:23\n> #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7\n> #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2\n> #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3\n> #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16\n> #10 0x4ce83e in run_builtin git.c:475:11\n> #11 0x4ccafe in handle_builtin git.c:729:3\n> #12 0x4cb01c in run_argv git.c:818:4\n> #13 0x4cb01c in cmd_main git.c:949:19\n> #14 0x6b3fad in main common-main.c:52:11\n> #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> fmt-merge-msg.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\n> index 0f66818e0f..b969dc6ebb 100644\n> --- a/fmt-merge-msg.c\n> +++ b/fmt-merge-msg.c\n> @@ -105,90 +105,92 @@ static void add_merge_parent(struct merge_parents *table,\n> static int handle_line(char *line, struct merge_parents *merge_parents)\n> {\n> int i, len = strlen(line);\n> struct origin_data *origin_data;\n> char *src;\n> const char *origin, *tag_name;\n> + char *to_free = NULL;\n> struct src_data *src_data;\n> struct string_list_item *item;\n> int pulling_head = 0;\n> struct object_id oid;\n> const unsigned hexsz = the_hash_algo->hexsz;\n>\n> if (len < hexsz + 3 || line[hexsz] != '\\t')\n> return 1;\n>\n> if (starts_with(line + hexsz + 1, \"not-for-merge\"))\n> return 0;\n>\n> if (line[hexsz + 1] != '\\t')\n> return 2;\n>\n> i = get_oid_hex(line, &oid);\n> if (i)\n> return 3;\n>\n> if (!find_merge_parent(merge_parents, &oid, NULL))\n> return 0; /* subsumed by other parents */\n>\n> CALLOC_ARRAY(origin_data, 1);\n> oidcpy(&origin_data->oid, &oid);\n>\n> if (line[len - 1] == '\\n')\n> line[len - 1] = 0;\n> line += hexsz + 2;\n>\n> /*\n> * At this point, line points at the beginning of comment e.g.\n> * \"branch 'frotz' of git://that/repository.git\".\n> * Find the repository name and point it with src.\n> */\n> src = strstr(line, \" of \");\n> if (src) {\n> *src = 0;\n> src += 4;\n> pulling_head = 0;\n> } else {\n> src = line;\n> pulling_head = 1;\n> }\n>\n> item = unsorted_string_list_lookup(&srcs, src);\n> if (!item) {\n> item = string_list_append(&srcs, src);\n> item->util = xcalloc(1, sizeof(struct src_data));\n> init_src_data(item->util);\n> }\n> src_data = item->util;\n>\n> if (pulling_head) {\n> origin = src;\n> src_data->head_status |= 1;\n> } else if (skip_prefix(line, \"branch \", &origin)) {\n> origin_data->is_local_branch = 1;\n> string_list_append(&src_data->branch, origin);\n> src_data->head_status |= 2;\n> } else if (skip_prefix(line, \"tag \", &tag_name)) {\n> origin = line;\n> string_list_append(&src_data->tag, tag_name);\n> src_data->head_status |= 2;\n> } else if (skip_prefix(line, \"remote-tracking branch \", &origin)) {\n> string_list_append(&src_data->r_branch, origin);\n> src_data->head_status |= 2;\n> } else {\n> origin = src;\n> string_list_append(&src_data->generic, line);\n> src_data->head_status |= 2;\n> }\n>\n> if (!strcmp(\".\", src) || !strcmp(src, origin)) {\n> int len = strlen(origin);\n> if (origin[0] == '\\'' && origin[len - 1] == '\\'')\n> - origin = xmemdupz(origin + 1, len - 2);\n> + origin = to_free = xmemdupz(origin + 1, len - 2);\n> } else\n> - origin = xstrfmt(\"%s of %s\", origin, src);\n> + origin = to_free = xstrfmt(\"%s of %s\", origin, src);\n> if (strcmp(\".\", src))\n> origin_data->is_local_branch = 0;\n> string_list_append(&origins, origin)->util = origin_data;\n> + free(to_free);\n> return 0;\n> }\n>\n> --\n> 2.26.2\n\nMakes sense. The extended diff context makes this patch easier to\nread and verify too; thanks.\n"},{"id":"428106","messageId":"CABPp-BGFH787gsw-yd3BpLt_rDe2zDoSFP6mMx6PSQfy1Ct4vw@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-3-andrzej@ahunt.org","subject":"Re: [PATCH 02/12] environment: move strbuf into block to plug leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T20:49:04Z","receivedAt":"2021-06-21T20:49:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> realpath is only populated if we execute the git_work_tree_initialized\n> block. However that block also causes us to return early, meaning we\n> never actually release the strbuf in the case where we populated it.\n> Therefore we move all strbuf related code into the block to guarantee\n> that we can't leak it.\n>\n> LSAN output from t0095:\n>\n> Direct leak of 129 byte(s) in 1 object(s) allocated from:\n> #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0x78f585 in xrealloc wrapper.c:126:8\n> #2 0x713ff4 in strbuf_grow strbuf.c:98:2\n> #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3\n> #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7\n> #5 0x5ae4a4 in set_git_work_tree environment.c:259:3\n> #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2\n> #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12\n> #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2\n> #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3\n> #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11\n> #11 0x4caded in main common-main.c:52:11\n> #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).\n>\n> It looks like this leak has existed since realpath was first added to\n> set_git_work_tree() in:\n> 3d7747e318 (real_path: remove unsafe API, 2020-03-10)\n\nLooking at that commit, it appears to have introduced other problems.\nFor example, the documentation for read_gitfile_gently() claims it\nreturns a value from a shared buffer, but that commit got rid of the\nshared buffer so the documentation is no longer accurate. The thing\nthat is returned is either the path that was passed in, or some newly\nallocated path that differs, in which case the caller would be\nresponsible to free() it, but it looks like the callers aren't doing\nso. There may be others; as I didn't read the whole old patch, but it\nlooks like even this example could get messy.\n\nI don't think you need to address the whole mess, fixing one of the\nissues from it is fine and...\n\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> environment.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/environment.c b/environment.c\n> index 2f27008424..d6b22ede7e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -249,25 +249,24 @@ static int git_work_tree_initialized;\n> /*\n> * Note. This works only before you used a work tree. This was added\n> * primarily to support git-clone to work in a new repository it just\n> * created, and is not meant to flip between different work trees.\n> */\n> void set_git_work_tree(const char *new_work_tree)\n> {\n> - struct strbuf realpath = STRBUF_INIT;\n> -\n> if (git_work_tree_initialized) {\n> + struct strbuf realpath = STRBUF_INIT;\n> +\n> strbuf_realpath(&realpath, new_work_tree, 1);\n> new_work_tree = realpath.buf;\n> if (strcmp(new_work_tree, the_repository->worktree))\n> die(\"internal error: work tree has already been set\\n\"\n> \"Current worktree: %s\\nNew worktree: %s\",\n> the_repository->worktree, new_work_tree);\n> + strbuf_release(&realpath);\n> return;\n> }\n> git_work_tree_initialized = 1;\n> repo_set_worktree(the_repository, new_work_tree);\n> -\n> - strbuf_release(&realpath);\n> }\n>\n> const char *get_git_work_tree(void)\n> --\n> 2.26.2\n\nThis patch looks simple and correct.\n"},{"id":"428108","messageId":"CABPp-BGBKC5y-8Kk4Et9O8LMOwypQfxDxDZ3BEMNLaya30jkxg@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-5-andrzej@ahunt.org","subject":"Re: [PATCH 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T20:55:34Z","receivedAt":"2021-06-21T20:55:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> cmd_for_each_repo() copies argv into args (a strvec), which is later\n> passed into run_command_on_repo(), which in turn copies that strvec onto\n> the end of child.args. The initial copy is unnecessary (we never modify\n> args). We therefore choose to just pass argv directly into\n> run_command_on_repo(), which lets us avoid the copy and fixes the leak.\n>\n> LSAN output from t0068:\n>\n> Direct leak of 192 byte(s) in 1 object(s) allocated from:\n> #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n> #1 0x98d7e6 in xrealloc wrapper.c:126\n> #2 0x916914 in strvec_push_nodup strvec.c:19\n> #3 0x916a6e in strvec_push strvec.c:26\n> #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n> #5 0x410dcd in run_builtin git.c:475\n> #6 0x410dcd in handle_builtin git.c:729\n> #7 0x414087 in run_argv git.c:818\n> #8 0x414087 in cmd_main git.c:949\n> #9 0x40e9ec in main common-main.c:52\n> #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 22 byte(s) in 2 object(s) allocated from:\n> #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)\n> #1 0x98d698 in xstrdup wrapper.c:29\n> #2 0x916a63 in strvec_push strvec.c:26\n> #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n> #4 0x410dcd in run_builtin git.c:475\n> #5 0x410dcd in handle_builtin git.c:729\n> #6 0x414087 in run_argv git.c:818\n> #7 0x414087 in cmd_main git.c:949\n> #8 0x40e9ec in main common-main.c:52\n> #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> See also discussion about the original implementation below - this code\n> appears to have evolved from a callback explaining the double-strvec-copy\n> pattern, but there's no strong reason to keep that now:\n> https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/\n\nThe link you give shows that Stolee was looking forward to reviewing\nyour patch to fix this issue, so cc'ing him so he can do so.\n\nIn general, it helps to cc folks who were involved in earlier\nconversations about an area being updated, or who know the code area\nwell.\n\n> Signed-off-by: Andrzej Hunt \n> ---\n> builtin/for-each-repo.c | 14 ++++----------\n> 1 file changed, 4 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\n> index 52be64a437..fd86e5a861 100644\n> --- a/builtin/for-each-repo.c\n> +++ b/builtin/for-each-repo.c\n> @@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {\n> NULL\n> };\n>\n> -static int run_command_on_repo(const char *path,\n> - void *cbdata)\n> +static int run_command_on_repo(const char *path, int argc, const char ** argv)\n> {\n> int i;\n> struct child_process child = CHILD_PROCESS_INIT;\n> - struct strvec *args = (struct strvec *)cbdata;\n>\n> child.git_cmd = 1;\n> strvec_pushl(&child.args, \"-C\", path, NULL);\n>\n> - for (i = 0; i < args->nr; i++)\n> - strvec_push(&child.args, args->v[i]);\n> + for (i = 0; i < argc; i++)\n> + strvec_push(&child.args, argv[i]);\n>\n> return run_command(&child);\n> }\n> @@ -29,37 +27,33 @@ static int run_command_on_repo(const char *path,\n> int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n> {\n> static const char *config_key = NULL;\n> int i, result = 0;\n> const struct string_list *values;\n> - struct strvec args = STRVEC_INIT;\n>\n> const struct option options[] = {\n> OPT_STRING(0, \"config\", &config_key, N_(\"config\"),\n> N_(\"config key storing a list of repository paths\")),\n> OPT_END()\n> };\n>\n> argc = parse_options(argc, argv, prefix, options, for_each_repo_usage,\n> PARSE_OPT_STOP_AT_NON_OPTION);\n>\n> if (!config_key)\n> die(_(\"missing --config=\"));\n>\n> - for (i = 0; i < argc; i++)\n> - strvec_push(&args, argv[i]);\n> -\n> values = repo_config_get_value_multi(the_repository,\n> config_key);\n>\n> /*\n> * Do nothing on an empty list, which is equivalent to the case\n> * where the config variable does not exist at all.\n> */\n> if (!values)\n> return 0;\n>\n> for (i = 0; !result && i < values->nr; i++)\n> - result = run_command_on_repo(values->items[i].string, &args);\n> + result = run_command_on_repo(values->items[i].string, argc, argv);\n>\n> return result;\n> }\n> --\n> 2.26.2\n\nPatch makes sense to me\n"},{"id":"428109","messageId":"CABPp-BFwufgAc6C5b-_jM1cXnCWXm9KGBEr_B0cDwBSbeCFy8g@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-7-andrzej@ahunt.org","subject":"Re: [PATCH 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:10:12Z","receivedAt":"2021-06-21T21:10:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> u.head is populated using resolve_refdup(), which returns a newly\n> allocated string - hence we also need to free() it.\n>\n> Found while running t0041 with LSAN:\n>\n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa8be98 in xstrdup wrapper.c:29:14\n> #2 0x9481db in head_atom_parser ref-filter.c:549:17\n> #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30\n> #4 0x9400e3 in verify_ref_format ref-filter.c:974:8\n> #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6\n> #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3\n> #7 0x4ce83e in run_builtin git.c:475:11\n> #8 0x4ccafe in handle_builtin git.c:729:3\n> #9 0x4cb01c in run_argv git.c:818:4\n> #10 0x4cb01c in cmd_main git.c:949:19\n> #11 0x6bdc2d in main common-main.c:52:11\n> #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> ref-filter.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 4db0e40ff4..f8bfd25ae4 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2225,8 +2225,12 @@ void ref_array_clear(struct ref_array *array)\n> FREE_AND_NULL(array->items);\n> array->nr = array->alloc = 0;\n>\n> - for (i = 0; i < used_atom_cnt; i++)\n> - free((char *)used_atom[i].name);\n> + for (i = 0; i < used_atom_cnt; i++) {\n> + struct used_atom *atom = &used_atom[i];\n> + if (atom->atom_type == ATOM_HEAD)\n> + free(atom->u.head);\n> + free((char *)atom->name);\n> + }\n> FREE_AND_NULL(used_atom);\n> used_atom_cnt = 0;\n>\n> --\n> 2.26.2\n\nMakes sense. I think builtin/branch.c and builtin/show-branch.c may\nhave similar problems with resolve_refdup() calls from a few greps.\nYou don't need to include those in this series, but if you want to\nalso tackle those, it would be nice.\n"},{"id":"428110","messageId":"CABPp-BGBk8qT+ApEkaDMF4zqK5ZeW07fTjQUuNz6c6z=oQd2eQ@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-8-andrzej@ahunt.org","subject":"Re: [PATCH 07/12] read-cache: call diff_setup_done to avoid leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:17:23Z","receivedAt":"2021-06-21T21:17:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:15 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> repo_diff_setup() calls through to diff.c's static prep_parse_options(),\n> which in turn allocates a new array into diff_opts.parseopts.\n> diff_setup_done() is responsible for freeing that array, and has the\n> benefit of verifying diff_opts too - hence we add a call to\n> diff_setup_done() to avoid leaking parseopts.\n\nShould the documentation near the top of diff.h also point out that\npart of the purpose of diff_setup_done() is to free some memory?\n\n> Output from the leak as found while running t0090 with LSAN:\n>\n> Direct leak of 7120 byte(s) in 1 object(s) allocated from:\n> #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0xa8bf89 in do_xmalloc wrapper.c:41:8\n> #2 0x7a7bae in prep_parse_options diff.c:5636:2\n> #3 0x7a7bae in repo_diff_setup diff.c:4611:2\n> #4 0x93716c in repo_index_has_changes read-cache.c:2518:3\n> #5 0x872233 in unclean merge-ort-wrappers.c:12:14\n> #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6\n> #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12\n> #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9\n> #9 0x4ce83e in run_builtin git.c:475:11\n> #10 0x4ccafe in handle_builtin git.c:729:3\n> #11 0x4cb01c in run_argv git.c:818:4\n> #12 0x4cb01c in cmd_main git.c:949:19\n> #13 0x6bdc2d in main common-main.c:52:11\n> #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> read-cache.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 77961a3885..212d604dd3 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)\n> int repo_index_has_changes(struct repository *repo,\n> struct tree *tree,\n> struct strbuf *sb)\n> {\n> struct index_state *istate = repo->index;\n> struct object_id cmp;\n> int i;\n>\n> if (tree)\n> cmp = tree->object.oid;\n> if (tree || !get_oid_tree(\"HEAD\", &cmp)) {\n> struct diff_options opt;\n>\n> repo_diff_setup(repo, &opt);\n> opt.flags.exit_with_status = 1;\n> if (!sb)\n> opt.flags.quick = 1;\n> + diff_setup_done(&opt);\n> do_diff_cache(&cmp, &opt);\n> diffcore_std(&opt);\n> for (i = 0; sb && i < diff_queued_diff.nr; i++) {\n> if (i)\n> strbuf_addch(sb, ' ');\n> strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n> }\n> diff_flush(&opt);\n> return opt.flags.has_changes != 0;\n> } else {\n> /* TODO: audit for interaction with sparse-index. */\n> ensure_full_index(istate);\n> for (i = 0; sb && i < istate->cache_nr; i++) {\n> if (i)\n> strbuf_addch(sb, ' ');\n> strbuf_addstr(sb, istate->cache[i]->name);\n> }\n> return !!istate->cache_nr;\n> }\n> }\n> --\n> 2.26.2\n\nPatch makes sense; a quick `git grep -e repo_diff_setup -e\ndiff_setup_done` doesn't flag any other areas of the code as having\nthe same bug.\n"},{"id":"428111","messageId":"CABPp-BHkgJKUMHBGLQ_1z8w09wY28i55h37YKchJo46nqw=LXQ@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-11-andrzej@ahunt.org","subject":"Re: [PATCH 10/12] builtin/merge: free found_ref when done","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:27:07Z","receivedAt":"2021-06-21T21:27:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:15 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> merge_name() calls dwim_ref(), which allocates a new string into\n> found_ref. Therefore add a free() to avoid leaking found_ref.\n>\n> LSAN output from t0021:\n>\n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa8beb8 in xstrdup wrapper.c:29:14\n> #2 0x954054 in expand_ref refs.c:671:12\n> #3 0x953cb6 in repo_dwim_ref refs.c:644:22\n> #4 0x5d3759 in dwim_ref refs.h:162:9\n> #5 0x5d3759 in merge_name builtin/merge.c:517:6\n> #6 0x5d3759 in collect_parents builtin/merge.c:1214:5\n> #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16\n> #8 0x4ce83e in run_builtin git.c:475:11\n> #9 0x4ccafe in handle_builtin git.c:729:3\n> #10 0x4cb01c in run_argv git.c:818:4\n> #11 0x4cb01c in cmd_main git.c:949:19\n> #12 0x6bdbfd in main common-main.c:52:11\n> #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> builtin/merge.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a8a843b1f5..7ad85c044a 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n> struct strbuf bname = STRBUF_INIT;\n> struct merge_remote_desc *desc;\n> const char *ptr;\n> - char *found_ref;\n> + char *found_ref = NULL;\n> int len, early;\n>\n> strbuf_branchname(&bname, remote, 0);\n> @@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n> strbuf_addf(msg, \"%s\\t\\tcommit '%s'\\n\",\n> oid_to_hex(&remote_head->object.oid), remote);\n> cleanup:\n> + free(found_ref);\n> strbuf_release(&buf);\n> strbuf_release(&bname);\n> }\n> --\n> 2.26.2\n\nMakes sense, and a quick grep through the code doesn't suggest any\nother obvious leaks from using dwim_ref().\n"},{"id":"428112","messageId":"CABPp-BEQkUQLt-ZbwdO+ecd2rumttBUKUmh3=7LaKRxwXCkB+g@mail.gmail.com","threadId":"55969","inReplyTo":"6e02fc85-42a4-8b19-1fe7-3527c2308a24@gmail.com","subject":"Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:39:56Z","receivedAt":"2021-06-21T21:40:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood wrote:\n>\n> Hi Andrzej\n>\n> Thanks for working on removing memory leaks from git.\n>\n> On 20/06/2021 16:12, andrzej@ahunt.org wrote:\n> > From: Andrzej Hunt \n> >\n> > This change:\n> > - xstrdup()'s all string being used for replace_opts.strategy, to\n>\n> I think you mean replay_opts rather than replace_opts.\n>\n> > guarantee that replace_opts owns these strings. This is needed because\n> > sequencer_remove_state() will free replace_opts.strategy, and it's\n> > usually called as part of the usage of replace_opts.\n> > - Removes xstrdup()'s being used to populate options.strategy in\n> > cmd_rebase(), which avoids leaking options.strategy, even in the\n> > case where strategy is never moved/copied into replace_opts.\n>\n>\n> > These changes are needed because:\n> > - We would always create a new string for options.strategy if we either\n> > get a strategy via options (OPT_STRING(...strategy...), or via\n> > GIT_TEST_MERGE_ALGORITHM.\n> > - But only sometimes is this string copied into replace_opts - in which\n> > case it did get free()'d in sequencer_remove_state().\n> > - The rest of the time, the newly allocated string would remain unused,\n> > causing a leak. But we can't just add a free because that can result\n> > in a double-free in those cases where replace_opts was populated.\n> >\n> > An alternative approach would be to set options.strategy to NULL when\n> > moving the pointer to replace_opts.strategy, combined with always\n> > free()'ing options.strategy, but that seems like a more\n> > complicated and wasteful approach.\n>\n> read_basic_state() contains\n> if (file_exists(state_dir_path(\"strategy\", opts))) {\n> strbuf_reset(&buf);\n> if (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n> READ_ONELINER_WARN_MISSING))\n> return -1;\n> free(opts->strategy);\n> opts->strategy = xstrdup(buf.buf);\n> }\n>\n> So we do try to free opts->strategy when reading the state from disc and\n> we allocate a new string. I suspect that opts->strategy is actually NULL\n> in when this function is called but I haven't checked. Given that we are\n> allocating a copy above I think maybe your alternative approach of\n> always freeing opts->strategy would be better.\n\nGood catches. sequencer_remove_state() in sequencer.c also has a\nfree(opts->strategy) call.\n\nTo make things even more muddy, we have code like\n replay.strategy = replay.default_strategy;\nor\n opts->strategy = opts->default_strategy;\nwhich both will probably work really poorly with the calls to\n free(opts->default_strategy);\n free(opts->strategy);\nfrom sequencer_remove_state(). I suspect we've got a few bugs here...\n"},{"id":"428113","messageId":"CABPp-BH23-c9cFvMw==WUukcoswne7Qc9H65Nuk5Km5Coco2vA@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-13-andrzej@ahunt.org","subject":"Re: [PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:44:58Z","receivedAt":"2021-06-21T21:45:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:15 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> setup_unpack_trees_porcelain() populates various fields on\n> unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to\n> avoid leaking them. Specifically, we used to leak\n> unpack_tree_opts.msgs_to_free.\n>\n> We have to do this in leave_reset_head because there are multiple\n> scenarios where unpack_tree_opts has already been configured, followed\n> by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'\n> prior to having initialised unpack_tree_opts via memset(..., 0, ...).\n> Therefore we also move unpack_tree_opts initialisation to the start of\n> reset_head(), and convert it to use brace initialisation - which\n> guarantees that we can never clear an unitialised unpack_tree_opts.\n\nI think you mean either \"uninitialized\" or \"uninitialised\" (missing an\n'in' in the spelling)\n\n> clear_unpack_tree_opts() is always safe to call as long as\n> unpack_tree_opts is at least zero-initialised, i.e. it does not depend\n> on a previous call to setup_unpack_trees_porcelain().\n>\n> LSAN output from t0021:\n>\n> Direct leak of 192 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa721e5 in xrealloc wrapper.c:126:8\n> #2 0x9f7861 in strvec_push_nodup strvec.c:19:2\n> #3 0x9f7861 in strvec_pushf strvec.c:39:2\n> #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n> #5 0x97e011 in reset_head reset.c:53:2\n> #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n> #7 0x4ce83e in run_builtin git.c:475:11\n> #8 0x4ccafe in handle_builtin git.c:729:3\n> #9 0x4cb01c in run_argv git.c:818:4\n> #10 0x4cb01c in cmd_main git.c:949:19\n> #11 0x6b3f3d in main common-main.c:52:11\n> #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 147 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa721e5 in xrealloc wrapper.c:126:8\n> #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n> #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n> #4 0x9f7774 in strvec_pushf strvec.c:36:2\n> #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n> #6 0x97e011 in reset_head reset.c:53:2\n> #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n> #8 0x4ce83e in run_builtin git.c:475:11\n> #9 0x4ccafe in handle_builtin git.c:729:3\n> #10 0x4cb01c in run_argv git.c:818:4\n> #11 0x4cb01c in cmd_main git.c:949:19\n> #12 0x6b3f3d in main common-main.c:52:11\n> #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 134 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa721e5 in xrealloc wrapper.c:126:8\n> #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n> #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n> #4 0x9f7774 in strvec_pushf strvec.c:36:2\n> #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3\n> #6 0x97e011 in reset_head reset.c:53:2\n> #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n> #8 0x4ce83e in run_builtin git.c:475:11\n> #9 0x4ccafe in handle_builtin git.c:729:3\n> #10 0x4cb01c in run_argv git.c:818:4\n> #11 0x4cb01c in cmd_main git.c:949:19\n> #12 0x6b3f3d in main common-main.c:52:11\n> #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 130 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa721e5 in xrealloc wrapper.c:126:8\n> #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n> #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n> #4 0x9f7774 in strvec_pushf strvec.c:36:2\n> #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3\n> #6 0x97e011 in reset_head reset.c:53:2\n> #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n> #8 0x4ce83e in run_builtin git.c:475:11\n> #9 0x4ccafe in handle_builtin git.c:729:3\n> #10 0x4cb01c in run_argv git.c:818:4\n> #11 0x4cb01c in cmd_main git.c:949:19\n> #12 0x6b3f3d in main common-main.c:52:11\n> #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> reset.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/reset.c b/reset.c\n> index 4bea758053..79310ae071 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n> struct object_id head_oid;\n> struct tree_desc desc[2] = { { NULL }, { NULL } };\n> struct lock_file lock = LOCK_INIT;\n> - struct unpack_trees_options unpack_tree_opts;\n> + struct unpack_trees_options unpack_tree_opts = { 0 };\n> struct tree *tree;\n> const char *reflog_action;\n> struct strbuf msg = STRBUF_INIT;\n> @@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n> if (refs_only)\n> goto reset_head_refs;\n>\n> - memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n> setup_unpack_trees_porcelain(&unpack_tree_opts, action);\n> unpack_tree_opts.head_idx = 1;\n> unpack_tree_opts.src_index = r->index;\n> @@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n> leave_reset_head:\n> strbuf_release(&msg);\n> rollback_lock_file(&lock);\n> + clear_unpack_trees_porcelain(&unpack_tree_opts);\n> while (nr)\n> free((void *)desc[--nr].buffer);\n> return ret;\n> --\n> 2.26.2\n\nNice catch, and nice explanation. I think we probably have several\nsimilar problems throughout the code base; a quick grep (`git grep -e\nstruct.unpack_trees_options -e clear_unpack_trees_porcelain`) suggests\nthere are several places that clear_unpack_trees_porcelain() is\nprobably missing and which could likely use your struct initialization\ntrick as well.\n"},{"id":"428115","messageId":"CABPp-BG0a0OM7s7cmO8yCeyA5TOCD_yOSJJepQE8MFEHct4EQA@mail.gmail.com","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T21:54:03Z","receivedAt":"2021-06-21T21:54:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>\n> From: Andrzej Hunt \n>\n> This series plugs more of the leaks that were found while running\n> t0002-t0099 with LSAN.\n>\n> See also the first series (already merged) at [1]. I'm currently\n> expecting at least another 2 series before t0002-t0099 run leak free.\n> I'm not being particularly systematic about the order of patches -\n> although I am trying to send out \"real\" (if mostly small) leaks first,\n> before sending out the more boring patches that add free()/UNLEAK() to\n> cmd_* and direct helpers thereof.\n\nI've read over the series. It provides some good clear fixes. I\nnoted on patches 2, 6, and 12 that a some greps suggested that leaks\nsimilar to the ones being fixed likely also affect other places of the\ncodebase. Those other places don't need to be fixed as part of this\nseries, but they might be good items for #leftoverbits or GSoC early\ntasks (cc: Christian in case he wants to record those somewhere).\n\nI cc'ed Stolee on patch 4 because he suggested he wanted to read it in\nan earlier discussion.\n\nPhillip noted some issues with patch 11, and I added a couple more.\nThe ownership of opts->strategy appears to be pretty messy and in need\nof cleanup.\n\nAll the patches other than 11 look good to me.\n"},{"id":"428186","messageId":"d1ef45c1-067e-abde-62a2-1df2c12ba3a3@gmail.com","threadId":"55969","inReplyTo":"CABPp-BEQkUQLt-ZbwdO+ecd2rumttBUKUmh3=7LaKRxwXCkB+g@mail.gmail.com","subject":"Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-22T09:02:53Z","receivedAt":"2021-06-22T09:03:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 21/06/2021 22:39, Elijah Newren wrote:\n> On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood wrote:\n>>\n>> Hi Andrzej\n>>\n>> Thanks for working on removing memory leaks from git.\n>>\n>> On 20/06/2021 16:12, andrzej@ahunt.org wrote:\n>>> From: Andrzej Hunt \n>>>\n>>> This change:\n>>> - xstrdup()'s all string being used for replace_opts.strategy, to\n>>\n>> I think you mean replay_opts rather than replace_opts.\n>>\n>>> guarantee that replace_opts owns these strings. This is needed because\n>>> sequencer_remove_state() will free replace_opts.strategy, and it's\n>>> usually called as part of the usage of replace_opts.\n>>> - Removes xstrdup()'s being used to populate options.strategy in\n>>> cmd_rebase(), which avoids leaking options.strategy, even in the\n>>> case where strategy is never moved/copied into replace_opts.\n>>\n>>\n>>> These changes are needed because:\n>>> - We would always create a new string for options.strategy if we either\n>>> get a strategy via options (OPT_STRING(...strategy...), or via\n>>> GIT_TEST_MERGE_ALGORITHM.\n>>> - But only sometimes is this string copied into replace_opts - in which\n>>> case it did get free()'d in sequencer_remove_state().\n>>> - The rest of the time, the newly allocated string would remain unused,\n>>> causing a leak. But we can't just add a free because that can result\n>>> in a double-free in those cases where replace_opts was populated.\n>>>\n>>> An alternative approach would be to set options.strategy to NULL when\n>>> moving the pointer to replace_opts.strategy, combined with always\n>>> free()'ing options.strategy, but that seems like a more\n>>> complicated and wasteful approach.\n>>\n>> read_basic_state() contains\n>> if (file_exists(state_dir_path(\"strategy\", opts))) {\n>> strbuf_reset(&buf);\n>> if (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n>> READ_ONELINER_WARN_MISSING))\n>> return -1;\n>> free(opts->strategy);\n>> opts->strategy = xstrdup(buf.buf);\n>> }\n>>\n>> So we do try to free opts->strategy when reading the state from disc and\n>> we allocate a new string. I suspect that opts->strategy is actually NULL\n>> in when this function is called but I haven't checked. Given that we are\n>> allocating a copy above I think maybe your alternative approach of\n>> always freeing opts->strategy would be better.\n> \n> Good catches. sequencer_remove_state() in sequencer.c also has a\n> free(opts->strategy) call.\n> \n> To make things even more muddy, we have code like\n> replay.strategy = replay.default_strategy;\n> or\n> opts->strategy = opts->default_strategy;\n> which both will probably work really poorly with the calls to\n> free(opts->default_strategy);\n> free(opts->strategy);\n> from sequencer_remove_state(). I suspect we've got a few bugs here...\n\nIt's not immediately obvious but I think those are actually safe. \nopts->default_strategy is allocated by sequencer_init_config() so it is \ncorrect to free it and when we assign it in rebase.c we do\n\n\telse if (!replay.strategy && replay.default_strategy) {\n\t\treplay.strategy = replay.default_strategy;\n\t\treplay.default_strategy = NULL;\n\t}\n\nso there is no double free. There is similar code in builtin/revert.c \nwhich I think is where your other example came from. I think there is a \nleak in builtin/revert.c though\n\n\tif (!opts->strategy && opts->default_strategy) {\n\t\topts->strategy = opts->default_strategy;\n\t\topts->default_strategy = NULL;\n\t}\n\n\t/* do some other stuff */\n\n\t/* These option values will be free()d */\n\topts->gpg_sign = xstrdup_or_null(opts->gpg_sign);\n\topts->strategy = xstrdup_or_null(opts->strategy);\n\nSo we copy the default strategy, leaking the original copy from \nsequencer_init_options() if --strategy isn't given on the command line. \nI think it would be simple to fix this by making the copy earlier.\n\n\tif (!opts->strategy && opts->default_strategy) {\n\t\topts->strategy = opts->default_strategy;\n\t\topts->default_strategy = NULL;\n\t} else if (opts->strategy) {\n\t/* This option will be free()d in sequencer_remove_state() */\n\t\topts->strategy = xstrdup(opts->strategy);\n\t}\n\nI'm going offline for a week or so in a couple of days but I'll have \nlook at making a proper patch when I get back.\n\nBest Wishes\n\nPhillip\n"},{"id":"428515","messageId":"15df4c07-988e-baff-8760-a199ec9aeb1b@web.de","threadId":"55969","inReplyTo":"CABPp-BGFH787gsw-yd3BpLt_rDe2zDoSFP6mMx6PSQfy1Ct4vw@mail.gmail.com","subject":"Re: [PATCH 02/12] environment: move strbuf into block to plug leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-26T08:27:42Z","receivedAt":"2021-06-26T08:27:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.21 um 22:49 schrieb Elijah Newren:\n> On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>> It looks like this leak has existed since realpath was first added to\n>> set_git_work_tree() in:\n>> 3d7747e318 (real_path: remove unsafe API, 2020-03-10)\n>\n> Looking at that commit, it appears to have introduced other problems.\n> For example, the documentation for read_gitfile_gently() claims it\n> returns a value from a shared buffer, but that commit got rid of the\n> shared buffer so the documentation is no longer accurate. The thing\n> that is returned is either the path that was passed in, or some newly\n> allocated path that differs, in which case the caller would be\n> responsible to free() it, but it looks like the callers aren't doing\n> so.\n\nThat comment is still correct. The returned pointer references a shared\nstatic buffer declared in read_gitfile_gently(). The control flow is a\nbit hard to follow; path points to the static buffer if and only if\nerror_code is zero. Using a dedicated variable for the result would\nmake that clearer, I think:\n\ndiff --git a/setup.c b/setup.c\nindex ead2f80cd8..75b0a4bea6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -720,86 +720,87 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n /*\n * Try to read the location of the git directory from the .git file,\n * return path to git directory if found. The return value comes from\n * a shared buffer.\n *\n * On failure, if return_error_code is not NULL, return_error_code\n * will be set to an error code and NULL will be returned. If\n * return_error_code is NULL the function will die instead (for most\n * cases).\n */\n const char *read_gitfile_gently(const char *path, int *return_error_code)\n {\n \tconst int max_file_size = 1 << 20; /* 1MB */\n \tint error_code = 0;\n \tchar *buf = NULL;\n \tchar *dir = NULL;\n \tconst char *slash;\n \tstruct stat st;\n \tint fd;\n \tssize_t len;\n \tstatic struct strbuf realpath = STRBUF_INIT;\n+\tconst char *result = NULL;\n\n \tif (stat(path, &st)) {\n \t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n \t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n \t\tgoto cleanup_return;\n \t}\n \tif (!S_ISREG(st.st_mode)) {\n \t\terror_code = READ_GITFILE_ERR_NOT_A_FILE;\n \t\tgoto cleanup_return;\n \t}\n \tif (st.st_size > max_file_size) {\n \t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\n \t\tgoto cleanup_return;\n \t}\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n \t\terror_code = READ_GITFILE_ERR_OPEN_FAILED;\n \t\tgoto cleanup_return;\n \t}\n \tbuf = xmallocz(st.st_size);\n \tlen = read_in_full(fd, buf, st.st_size);\n \tclose(fd);\n \tif (len != st.st_size) {\n \t\terror_code = READ_GITFILE_ERR_READ_FAILED;\n \t\tgoto cleanup_return;\n \t}\n \tif (!starts_with(buf, \"gitdir: \")) {\n \t\terror_code = READ_GITFILE_ERR_INVALID_FORMAT;\n \t\tgoto cleanup_return;\n \t}\n \twhile (buf[len - 1] == '\\n' || buf[len - 1] == '\\r')\n \t\tlen--;\n \tif (len < 9) {\n \t\terror_code = READ_GITFILE_ERR_NO_PATH;\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n \tdir = buf + 8;\n\n \tif (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {\n \t\tsize_t pathlen = slash+1 - path;\n \t\tdir = xstrfmt(\"%.*s%.*s\", (int)pathlen, path,\n \t\t\t (int)(len - 8), buf + 8);\n \t\tfree(buf);\n \t\tbuf = dir;\n \t}\n \tif (!is_git_directory(dir)) {\n \t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n \t\tgoto cleanup_return;\n \t}\n\n \tstrbuf_realpath(&realpath, dir, 1);\n-\tpath = realpath.buf;\n+\tresult = realpath.buf;\n\n cleanup_return:\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n \telse if (error_code)\n \t\tread_gitfile_error_die(error_code, path, dir);\n\n \tfree(buf);\n-\treturn error_code ? NULL : path;\n+\treturn result;\n }\n\n static const char *setup_explicit_git_dir(const char *gitdirenv,\n"},{"id":"431134","messageId":"9f298c97-07d6-7117-baab-6a44359c44d2@ahunt.org","threadId":"55969","inReplyTo":"d1ef45c1-067e-abde-62a2-1df2c12ba3a3@gmail.com","subject":"Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:03:21Z","receivedAt":"2021-07-25T13:03:29Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 22/06/2021 11:02, Phillip Wood wrote:\n> Hi Elijah\n> \n> On 21/06/2021 22:39, Elijah Newren wrote:\n>> On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood \n>> wrote:\n>>>\n>>> Hi Andrzej\n>>>\n>>> Thanks for working on removing memory leaks from git.\n>>>\n>>> On 20/06/2021 16:12, andrzej@ahunt.org wrote:\n>>>> From: Andrzej Hunt \n>>>>\n>>>> This change:\n>>>> - xstrdup()'s all string being used for replace_opts.strategy, to\n>>>\n>>> I think you mean replay_opts rather than replace_opts.\n>>>\n>>>>     guarantee that replace_opts owns these strings. This is needed \n>>>> because\n>>>>     sequencer_remove_state() will free replace_opts.strategy, and it's\n>>>>     usually called as part of the usage of replace_opts.\n>>>> - Removes xstrdup()'s being used to populate options.strategy in\n>>>>     cmd_rebase(), which avoids leaking options.strategy, even in the\n>>>>     case where strategy is never moved/copied into replace_opts.\n>>>\n>>>\n>>>> These changes are needed because:\n>>>> - We would always create a new string for options.strategy if we either\n>>>>     get a strategy via options (OPT_STRING(...strategy...), or via\n>>>>     GIT_TEST_MERGE_ALGORITHM.\n>>>> - But only sometimes is this string copied into replace_opts - in which\n>>>>     case it did get free()'d in sequencer_remove_state().\n>>>> - The rest of the time, the newly allocated string would remain unused,\n>>>>     causing a leak. But we can't just add a free because that can \n>>>> result\n>>>>     in a double-free in those cases where replace_opts was populated.\n>>>>\n>>>> An alternative approach would be to set options.strategy to NULL when\n>>>> moving the pointer to replace_opts.strategy, combined with always\n>>>> free()'ing options.strategy, but that seems like a more\n>>>> complicated and wasteful approach.\n>>>\n>>> read_basic_state() contains\n>>>          if (file_exists(state_dir_path(\"strategy\", opts))) {\n>>>                  strbuf_reset(&buf);\n>>>                  if (!read_oneliner(&buf, state_dir_path(\"strategy\", \n>>> opts),\n>>>                                     READ_ONELINER_WARN_MISSING))\n>>>                          return -1;\n>>>                  free(opts->strategy);\n>>>                  opts->strategy = xstrdup(buf.buf);\n>>>          }\n>>>\n>>> So we do try to free opts->strategy when reading the state from disc and\n>>> we allocate a new string. I suspect that opts->strategy is actually NULL\n>>> in when this function is called but I haven't checked. \n\nThank you for noticing this. I think you're right - running an ASAN \nbuild past the whole test suite also didn't catch any double-frees which \nmostly confirms that opts->strategy is indeed always NULL here. But \nthat's not a good reason for taking the risk.\n\n>>> Given that we are\n>>> allocating a copy above I think maybe your alternative approach of\n>>> always freeing opts->strategy would be better.\n\nI will go down this route for V2. Although on further thought: instead \nof my original idea of moving the string to replay_opts (and NULL'ing \nout rebase_options->strategy), I think it's better to create a new copy \nwhen populating replay_opts. The move/NULL approach I suggested in V1 \nhappens to work OK, but I think it's non-obvious and could break if we \never wanted to use get_replay_opts() more than once - creating separate \ncopies reduces the number of surprises.\n\n>>\n>> Good catches.  sequencer_remove_state() in sequencer.c also has a\n>> free(opts->strategy) call.\n>>\n>> To make things even more muddy, we have code like\n>>      replay.strategy = replay.default_strategy;\n>> or\n>>      opts->strategy = opts->default_strategy;\n>> which both will probably work really poorly with the calls to\n>>      free(opts->default_strategy);\n>>      free(opts->strategy);\n>> from sequencer_remove_state().  I suspect we've got a few bugs here...\n> \n> It's not immediately obvious but I think those are actually safe. \n> opts->default_strategy is allocated by sequencer_init_config() so it is \n> correct to free it and when we assign it in rebase.c we do\n> \n>     else if (!replay.strategy && replay.default_strategy) {\n>         replay.strategy = replay.default_strategy;\n>         replay.default_strategy = NULL;\n>     }\n> \n> so there is no double free.\n\nAs mentioned above, ASAN isn't catching any double-frees here (but I \nguess that depends on whether or not you trust the test suite to be \nreasonably testing all permutations).\n\nBut it's still good to take note of sequencer_remove_state() free'ing \nopts->strategy, because I almost did manage to add a double free when I \nadded a free(options.strategy) to cmd_rebase without also xstrdup'ing \nstrategy in get_replay_opts().\n\n> There is similar code in builtin/revert.c \n> which I think is where your other example came from. I think there is a \n> leak in builtin/revert.c though\n> \n>     if (!opts->strategy && opts->default_strategy) {\n>         opts->strategy = opts->default_strategy;\n>         opts->default_strategy = NULL;\n>     }\n> \n>     /* do some other stuff */\n> \n>     /* These option values will be free()d */\n>     opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);\n>     opts->strategy = xstrdup_or_null(opts->strategy);\n> \n> So we copy the default strategy, leaking the original copy from \n> sequencer_init_options() if --strategy isn't given on the command line. \n> I think it would be simple to fix this by making the copy earlier.\n> \n>     if (!opts->strategy && opts->default_strategy) {\n>         opts->strategy = opts->default_strategy;\n>         opts->default_strategy = NULL;\n>     } else if (opts->strategy) {\n>     /* This option will be free()d in sequencer_remove_state() */\n>         opts->strategy = xstrdup(opts->strategy);\n>     }\n> \n\nNice find. I'm noticing a lot of interesting leaks in git's options \nhandling, and those leaks also tend to be the trickiest ones to fix (as \nmy blunder in the original version of this patch demonstrates :) ).\n\nATB,\n\n Andrzej\n"},{"id":"431135","messageId":"e83899bb-3d49-68b2-b67b-76ab824adfc3@ahunt.org","threadId":"55969","inReplyTo":"CABPp-BG0a0OM7s7cmO8yCeyA5TOCD_yOSJJepQE8MFEHct4EQA@mail.gmail.com","subject":"Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:05:31Z","receivedAt":"2021-07-25T13:05:48Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 21/06/2021 23:54, Elijah Newren wrote:\n> On Sun, Jun 20, 2021 at 8:14 AM wrote:\n>>\n>> From: Andrzej Hunt \n>>\n>> This series plugs more of the leaks that were found while running\n>> t0002-t0099 with LSAN.\n>>\n>> See also the first series (already merged) at [1]. I'm currently\n>> expecting at least another 2 series before t0002-t0099 run leak free.\n>> I'm not being particularly systematic about the order of patches -\n>> although I am trying to send out \"real\" (if mostly small) leaks first,\n>> before sending out the more boring patches that add free()/UNLEAK() to\n>> cmd_* and direct helpers thereof.\n> \n> I've read over the series. It provides some good clear fixes. I\n> noted on patches 2, 6, and 12 that a some greps suggested that leaks\n> similar to the ones being fixed likely also affect other places of the\n> codebase. Those other places don't need to be fixed as part of this\n> series, but they might be good items for #leftoverbits or GSoC early\n> tasks (cc: Christian in case he wants to record those somewhere).\n> \n> I cc'ed Stolee on patch 4 because he suggested he wanted to read it in\n> an earlier discussion.\n> \n> Phillip noted some issues with patch 11, and I added a couple more.\n> The ownership of opts->strategy appears to be pretty messy and in need\n> of cleanup.\n> \n> All the patches other than 11 look good to me.\n> \n\nThank you for the careful reviews - and especially for pointing out when \na given pattern does occur elsewhere in the codebase!\n\nAs suggested I will skip the additional locations that you've found \nwhile reviewing this series - but I'm starting a separate series where I \ncan address those. I have been focused on getting tests to pass \nleak-free one-by-one, but spotting and fixing patterns is probably more \nefficient (since author and reviewer already have the right context) and \nin some cases might fix leaks that aren't occurring during the tests.\n\nATB,\n\n Andrzej\n"},{"id":"431136","messageId":"20210725130830.5145-1-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210620151204.19260-1-andrzej@ahunt.org","subject":"[PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:18Z","receivedAt":"2021-07-25T13:09:26Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nV2 fixes patch 11/12 (rebase_options.strategy lifecycle) as per review\ndiscussion. Many thanks to Phillip and Elijah for spotting the issues there!\n\nATB,\n\n Andrzej\n\nAndrzej Hunt (12):\n fmt-merge-msg: free newly allocated temporary strings when done\n environment: move strbuf into block to plug leak\n builtin/submodule--helper: release unused strbuf to avoid leak\n builtin/for-each-repo: remove unnecessary argv copy to plug leak\n diffcore-rename: move old_dir/new_dir definition to plug leak\n ref-filter: also free head for ATOM_HEAD to avoid leak\n read-cache: call diff_setup_done to avoid leak\n convert: release strbuf to avoid leak\n builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv\n builtin/merge: free found_ref when done\n builtin/rebase: fix options.strategy memory lifecycle\n reset: clear_unpack_trees_porcelain to plug leak\n\n builtin/for-each-repo.c | 14 ++++----------\n builtin/merge.c | 3 ++-\n builtin/mv.c | 5 +++++\n builtin/rebase.c | 3 ++-\n builtin/submodule--helper.c | 6 ++++--\n convert.c | 2 ++\n diffcore-rename.c | 10 +++++++---\n environment.c | 7 +++----\n fmt-merge-msg.c | 6 ++++--\n read-cache.c | 1 +\n ref-filter.c | 8 ++++++--\n reset.c | 4 ++--\n 12 files changed, 42 insertions(+), 27 deletions(-)\n\nInterdiff against v1:\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 9d81db0f3a..33e0961900 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1723,6 +1723,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (options.strategy) {\n+\t\toptions.strategy = xstrdup(options.strategy);\n \t\tswitch (options.type) {\n \t\tcase REBASE_APPLY:\n \t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n@@ -1775,7 +1776,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.type == REBASE_MERGE &&\n \t !options.strategy &&\n \t getenv(\"GIT_TEST_MERGE_ALGORITHM\"))\n-\t\toptions.strategy = getenv(\"GIT_TEST_MERGE_ALGORITHM\");\n+\t\toptions.strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \n \tswitch (options.type) {\n \tcase REBASE_MERGE:\n@@ -2108,6 +2109,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n+\tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \treturn ret;\n-- \n2.26.2\n\n"},{"id":"431137","messageId":"20210725130830.5145-2-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 01/12] fmt-merge-msg: free newly allocated temporary strings when done","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:19Z","receivedAt":"2021-07-25T13:09:29Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\norigin starts off pointing to somewhere within line, which is owned by\nthe caller. Later we might allocate a new string using xmemdupz() or\nxstrfmt(). To avoid leaking these new strings, we introduce a to_free\npointer - which allows us to safely free the newly allocated string when\nwe're done (we cannot just free origin directly as it might still be\npointing to line).\n\nLSAN output from t0090:\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa71f49 in do_xmalloc wrapper.c:41:8\n #2 0xa720b0 in do_xmallocz wrapper.c:75:8\n #3 0xa720b0 in xmallocz wrapper.c:83:9\n #4 0xa720b0 in xmemdupz wrapper.c:99:16\n #5 0x8092ba in handle_line fmt-merge-msg.c:187:23\n #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7\n #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2\n #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3\n #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6b3fad in main common-main.c:52:11\n #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n fmt-merge-msg.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\nindex 0f66818e0f..b969dc6ebb 100644\n--- a/fmt-merge-msg.c\n+++ b/fmt-merge-msg.c\n@@ -108,6 +108,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n \tstruct origin_data *origin_data;\n \tchar *src;\n \tconst char *origin, *tag_name;\n+\tchar *to_free = NULL;\n \tstruct src_data *src_data;\n \tstruct string_list_item *item;\n \tint pulling_head = 0;\n@@ -183,12 +184,13 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n \tif (!strcmp(\".\", src) || !strcmp(src, origin)) {\n \t\tint len = strlen(origin);\n \t\tif (origin[0] == '\\'' && origin[len - 1] == '\\'')\n-\t\t\torigin = xmemdupz(origin + 1, len - 2);\n+\t\t\torigin = to_free = xmemdupz(origin + 1, len - 2);\n \t} else\n-\t\torigin = xstrfmt(\"%s of %s\", origin, src);\n+\t\torigin = to_free = xstrfmt(\"%s of %s\", origin, src);\n \tif (strcmp(\".\", src))\n \t\torigin_data->is_local_branch = 0;\n \tstring_list_append(&origins, origin)->util = origin_data;\n+\tfree(to_free);\n \treturn 0;\n }\n \n-- \n2.26.2\n\n"},{"id":"431138","messageId":"20210725130830.5145-3-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 02/12] environment: move strbuf into block to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:20Z","receivedAt":"2021-07-25T13:09:31Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrealpath is only populated if we execute the git_work_tree_initialized\nblock. However that block also causes us to return early, meaning we\nnever actually release the strbuf in the case where we populated it.\nTherefore we move all strbuf related code into the block to guarantee\nthat we can't leak it.\n\nLSAN output from t0095:\n\nDirect leak of 129 byte(s) in 1 object(s) allocated from:\n #0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0x78f585 in xrealloc wrapper.c:126:8\n #2 0x713ff4 in strbuf_grow strbuf.c:98:2\n #3 0x713ff4 in strbuf_getcwd strbuf.c:597:3\n #4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7\n #5 0x5ae4a4 in set_git_work_tree environment.c:259:3\n #6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2\n #7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12\n #8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2\n #9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3\n #10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11\n #11 0x4caded in main common-main.c:52:11\n #12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).\n\nIt looks like this leak has existed since realpath was first added to\nset_git_work_tree() in:\n 3d7747e318 (real_path: remove unsafe API, 2020-03-10)\n\nSigned-off-by: Andrzej Hunt \n---\n environment.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 2f27008424..d6b22ede7e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -253,21 +253,20 @@ static int git_work_tree_initialized;\n */\n void set_git_work_tree(const char *new_work_tree)\n {\n-\tstruct strbuf realpath = STRBUF_INIT;\n-\n \tif (git_work_tree_initialized) {\n+\t\tstruct strbuf realpath = STRBUF_INIT;\n+\n \t\tstrbuf_realpath(&realpath, new_work_tree, 1);\n \t\tnew_work_tree = realpath.buf;\n \t\tif (strcmp(new_work_tree, the_repository->worktree))\n \t\t\tdie(\"internal error: work tree has already been set\\n\"\n \t\t\t \"Current worktree: %s\\nNew worktree: %s\",\n \t\t\t the_repository->worktree, new_work_tree);\n+\t\tstrbuf_release(&realpath);\n \t\treturn;\n \t}\n \tgit_work_tree_initialized = 1;\n \trepo_set_worktree(the_repository, new_work_tree);\n-\n-\tstrbuf_release(&realpath);\n }\n \n const char *get_git_work_tree(void)\n-- \n2.26.2\n\n"},{"id":"431139","messageId":"20210725130830.5145-4-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 03/12] builtin/submodule--helper: release unused strbuf to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:21Z","receivedAt":"2021-07-25T13:09:44Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrelative_url() populates sb. In the normal return path, its buffer is\ndetached using strbuf_detach(). However the early return path does\nnothing with sb, which means that sb's memory is leaked - therefore\nwe add a release to avoid this leak.\n\nThe reset is also only necessary for the normal return path, hence we\nmove it down to after the early-return to avoid unnecessary work.\n\nLSAN output from t0060:\n\nDirect leak of 121 byte(s) in 1 object(s) allocated from:\n #0 0x7f31246f28b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n #1 0x98d7d6 in xrealloc wrapper.c:126\n #2 0x909a60 in strbuf_grow strbuf.c:98\n #3 0x90bf00 in strbuf_vaddf strbuf.c:401\n #4 0x90c321 in strbuf_addf strbuf.c:335\n #5 0x5cb78d in relative_url builtin/submodule--helper.c:182\n #6 0x5cbe46 in resolve_relative_url_test builtin/submodule--helper.c:248\n #7 0x410dcd in run_builtin git.c:475\n #8 0x410dcd in handle_builtin git.c:729\n #9 0x414087 in run_argv git.c:818\n #10 0x414087 in cmd_main git.c:949\n #11 0x40e9ec in main common-main.c:52\n #12 0x7f3123c41349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 121 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/submodule--helper.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 414fcb63ea..528a78eed8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -187,11 +187,13 @@ static char *relative_url(const char *remote_url,\n \t\tout = xstrdup(sb.buf + 2);\n \telse\n \t\tout = xstrdup(sb.buf);\n-\tstrbuf_reset(&sb);\n \n-\tif (!up_path || !is_relative)\n+\tif (!up_path || !is_relative) {\n+\t\tstrbuf_release(&sb);\n \t\treturn out;\n+\t}\n \n+\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n \tfree(out);\n \treturn strbuf_detach(&sb, NULL);\n-- \n2.26.2\n\n"},{"id":"431140","messageId":"20210725130830.5145-5-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:22Z","receivedAt":"2021-07-25T13:09:45Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\ncmd_for_each_repo() copies argv into args (a strvec), which is later\npassed into run_command_on_repo(), which in turn copies that strvec onto\nthe end of child.args. The initial copy is unnecessary (we never modify\nargs). We therefore choose to just pass argv directly into\nrun_command_on_repo(), which lets us avoid the copy and fixes the leak.\n\nLSAN output from t0068:\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n #1 0x98d7e6 in xrealloc wrapper.c:126\n #2 0x916914 in strvec_push_nodup strvec.c:19\n #3 0x916a6e in strvec_push strvec.c:26\n #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n #5 0x410dcd in run_builtin git.c:475\n #6 0x410dcd in handle_builtin git.c:729\n #7 0x414087 in run_argv git.c:818\n #8 0x414087 in cmd_main git.c:949\n #9 0x40e9ec in main common-main.c:52\n #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 22 byte(s) in 2 object(s) allocated from:\n #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)\n #1 0x98d698 in xstrdup wrapper.c:29\n #2 0x916a63 in strvec_push strvec.c:26\n #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n #4 0x410dcd in run_builtin git.c:475\n #5 0x410dcd in handle_builtin git.c:729\n #6 0x414087 in run_argv git.c:818\n #7 0x414087 in cmd_main git.c:949\n #8 0x40e9ec in main common-main.c:52\n #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSee also discussion about the original implementation below - this code\nappears to have evolved from a callback explaining the double-strvec-copy\npattern, but there's no strong reason to keep that now:\n https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/for-each-repo.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex 52be64a437..fd86e5a861 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {\n \tNULL\n };\n \n-static int run_command_on_repo(const char *path,\n-\t\t\t void *cbdata)\n+static int run_command_on_repo(const char *path, int argc, const char ** argv)\n {\n \tint i;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tstruct strvec *args = (struct strvec *)cbdata;\n \n \tchild.git_cmd = 1;\n \tstrvec_pushl(&child.args, \"-C\", path, NULL);\n \n-\tfor (i = 0; i < args->nr; i++)\n-\t\tstrvec_push(&child.args, args->v[i]);\n+\tfor (i = 0; i < argc; i++)\n+\t\tstrvec_push(&child.args, argv[i]);\n \n \treturn run_command(&child);\n }\n@@ -31,7 +29,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \tstatic const char *config_key = NULL;\n \tint i, result = 0;\n \tconst struct string_list *values;\n-\tstruct strvec args = STRVEC_INIT;\n \n \tconst struct option options[] = {\n \t\tOPT_STRING(0, \"config\", &config_key, N_(\"config\"),\n@@ -45,9 +42,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \tif (!config_key)\n \t\tdie(_(\"missing --config=\"));\n \n-\tfor (i = 0; i < argc; i++)\n-\t\tstrvec_push(&args, argv[i]);\n-\n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t config_key);\n \n@@ -59,7 +53,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \n \tfor (i = 0; !result && i < values->nr; i++)\n-\t\tresult = run_command_on_repo(values->items[i].string, &args);\n+\t\tresult = run_command_on_repo(values->items[i].string, argc, argv);\n \n \treturn result;\n }\n-- \n2.26.2\n\n"},{"id":"431141","messageId":"20210725130830.5145-7-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:24Z","receivedAt":"2021-07-25T13:09:45Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nu.head is populated using resolve_refdup(), which returns a newly\nallocated string - hence we also need to free() it.\n\nFound while running t0041 with LSAN:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa8be98 in xstrdup wrapper.c:29:14\n #2 0x9481db in head_atom_parser ref-filter.c:549:17\n #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30\n #4 0x9400e3 in verify_ref_format ref-filter.c:974:8\n #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6\n #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3\n #7 0x4ce83e in run_builtin git.c:475:11\n #8 0x4ccafe in handle_builtin git.c:729:3\n #9 0x4cb01c in run_argv git.c:818:4\n #10 0x4cb01c in cmd_main git.c:949:19\n #11 0x6bdc2d in main common-main.c:52:11\n #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n ref-filter.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f45d3a1b26..0cfef7b719 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2226,8 +2226,12 @@ void ref_array_clear(struct ref_array *array)\n \tFREE_AND_NULL(array->items);\n \tarray->nr = array->alloc = 0;\n \n-\tfor (i = 0; i < used_atom_cnt; i++)\n-\t\tfree((char *)used_atom[i].name);\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tif (atom->atom_type == ATOM_HEAD)\n+\t\t\tfree(atom->u.head);\n+\t\tfree((char *)atom->name);\n+\t}\n \tFREE_AND_NULL(used_atom);\n \tused_atom_cnt = 0;\n \n-- \n2.26.2\n\n"},{"id":"431142","messageId":"20210725130830.5145-8-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 07/12] read-cache: call diff_setup_done to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:25Z","receivedAt":"2021-07-25T13:09:47Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nrepo_diff_setup() calls through to diff.c's static prep_parse_options(),\nwhich in turn allocates a new array into diff_opts.parseopts.\ndiff_setup_done() is responsible for freeing that array, and has the\nbenefit of verifying diff_opts too - hence we add a call to\ndiff_setup_done() to avoid leaking parseopts.\n\nOutput from the leak as found while running t0090 with LSAN:\n\nDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bf89 in do_xmalloc wrapper.c:41:8\n #2 0x7a7bae in prep_parse_options diff.c:5636:2\n #3 0x7a7bae in repo_diff_setup diff.c:4611:2\n #4 0x93716c in repo_index_has_changes read-cache.c:2518:3\n #5 0x872233 in unclean merge-ort-wrappers.c:12:14\n #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6\n #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12\n #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9\n #9 0x4ce83e in run_builtin git.c:475:11\n #10 0x4ccafe in handle_builtin git.c:729:3\n #11 0x4cb01c in run_argv git.c:818:4\n #12 0x4cb01c in cmd_main git.c:949:19\n #13 0x6bdc2d in main common-main.c:52:11\n #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)\n\nSigned-off-by: Andrzej Hunt \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 46ccd66f34..83d1817ad0 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2505,6 +2505,7 @@ int repo_index_has_changes(struct repository *repo,\n \t\topt.flags.exit_with_status = 1;\n \t\tif (!sb)\n \t\t\topt.flags.quick = 1;\n+\t\tdiff_setup_done(&opt);\n \t\tdo_diff_cache(&cmp, &opt);\n \t\tdiffcore_std(&opt);\n \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n-- \n2.26.2\n\n"},{"id":"431143","messageId":"20210725130830.5145-6-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:23Z","receivedAt":"2021-07-25T13:09:51Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nold_dir/new_dir are free()'d at the end of update_dir_rename_counts,\nhowever if we return early we'll never free those strings. Therefore\nwe should move all new allocations after the possible early return,\navoiding a leak.\n\nThis seems like a fairly recent leak, that started happening since the\nearly-return was added in:\n 1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)\n\nLSAN output from t0022:\n\nDirect leak of 7 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71e48 in xstrdup wrapper.c:29:14\n #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12\n #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n #5 0x7b4cfc in diffcore_std diff.c:6705:4\n #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n #7 0x856574 in log_tree_diff log-tree.c:955:3\n #8 0x856574 in log_tree_commit log-tree.c:986:10\n #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6b3f3d in main common-main.c:52:11\n #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 7 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71e48 in xstrdup wrapper.c:29:14\n #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12\n #3 0x7db6ae in find_renames diffcore-rename.c:1062:3\n #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18\n #5 0x7b4cfc in diffcore_std diff.c:6705:4\n #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2\n #7 0x856574 in log_tree_diff log-tree.c:955:3\n #8 0x856574 in log_tree_commit log-tree.c:986:10\n #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7\n #10 0x52e623 in cmd_commit builtin/commit.c:1862:3\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6b3f3d in main common-main.c:52:11\n #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n diffcore-rename.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 2618bb07c1..c95857b51f 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -448,9 +448,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t\t\t const char *oldname,\n \t\t\t\t const char *newname)\n {\n-\tchar *old_dir = xstrdup(oldname);\n-\tchar *new_dir = xstrdup(newname);\n-\tchar new_dir_first_char = new_dir[0];\n+\tchar *old_dir;\n+\tchar *new_dir;\n+\tconst char new_dir_first_char = newname[0];\n \tint first_time_in_loop = 1;\n \n \tif (!info->setup)\n@@ -475,6 +475,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info,\n \t\t */\n \t\treturn;\n \n+\n+\told_dir = xstrdup(oldname);\n+\tnew_dir = xstrdup(newname);\n+\n \twhile (1) {\n \t\tint drd_flag = NOT_RELEVANT;\n \n-- \n2.26.2\n\n"},{"id":"431144","messageId":"20210725130830.5145-10-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:27Z","receivedAt":"2021-07-25T13:09:53Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nThese leaks all happen at the end of cmd_mv, hence don't matter in any\nway. But we still fix the easy ones and squash the rest to get us closer\nto being able to run tests without leaks.\n\nLSAN output from t0050:\n\nDirect leak of 384 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa0a7e1 in add_entry string-list.c:44:2\n #3 0xa0a7e1 in string_list_insert string-list.c:58:14\n #4 0x5dac03 in cmd_mv builtin/mv.c:248:4\n #5 0x4ce83e in run_builtin git.c:475:11\n #6 0x4ccafe in handle_builtin git.c:729:3\n #7 0x4cb01c in run_argv git.c:818:4\n #8 0x4cb01c in cmd_main git.c:949:19\n #9 0x6bd9ad in main common-main.c:52:11\n #10 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bd09 in do_xmalloc wrapper.c:41:8\n #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2\n #3 0x5da575 in cmd_mv builtin/mv.c:158:14\n #4 0x4ce83e in run_builtin git.c:475:11\n #5 0x4ccafe in handle_builtin git.c:729:3\n #6 0x4cb01c in run_argv git.c:818:4\n #7 0x4cb01c in cmd_main git.c:949:19\n #8 0x6bd9ad in main common-main.c:52:11\n #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n #1 0xa8bd09 in do_xmalloc wrapper.c:41:8\n #2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2\n #3 0x5da4e4 in cmd_mv builtin/mv.c:148:11\n #4 0x4ce83e in run_builtin git.c:475:11\n #5 0x4ccafe in handle_builtin git.c:729:3\n #6 0x4cb01c in run_argv git.c:818:4\n #7 0x4cb01c in cmd_main git.c:949:19\n #8 0x6bd9ad in main common-main.c:52:11\n #9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 8 byte(s) in 1 object(s) allocated from:\n #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0xa8c119 in xcalloc wrapper.c:140:8\n #2 0x5da585 in cmd_mv builtin/mv.c:159:22\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6bd9ad in main common-main.c:52:11\n #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nDirect leak of 4 byte(s) in 1 object(s) allocated from:\n #0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3\n #1 0xa8c119 in xcalloc wrapper.c:140:8\n #2 0x5da4f8 in cmd_mv builtin/mv.c:149:10\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6bd9ad in main common-main.c:52:11\n #8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa00226 in strbuf_grow strbuf.c:98:2\n #3 0xa00226 in strbuf_vaddf strbuf.c:394:3\n #4 0xa065c7 in xstrvfmt strbuf.c:981:2\n #5 0xa065c7 in xstrfmt strbuf.c:991:8\n #6 0x9e7ce7 in prefix_path_gently setup.c:115:15\n #7 0x9e7fa6 in prefix_path setup.c:128:12\n #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23\n #9 0x5da575 in cmd_mv builtin/mv.c:158:14\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6bd9ad in main common-main.c:52:11\n #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 65 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c015 in xrealloc wrapper.c:126:8\n #2 0xa00226 in strbuf_grow strbuf.c:98:2\n #3 0xa00226 in strbuf_vaddf strbuf.c:394:3\n #4 0xa065c7 in xstrvfmt strbuf.c:981:2\n #5 0xa065c7 in xstrfmt strbuf.c:991:8\n #6 0x9e7ce7 in prefix_path_gently setup.c:115:15\n #7 0x9e7fa6 in prefix_path setup.c:128:12\n #8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23\n #9 0x5da4e4 in cmd_mv builtin/mv.c:148:11\n #10 0x4ce83e in run_builtin git.c:475:11\n #11 0x4ccafe in handle_builtin git.c:729:3\n #12 0x4cb01c in run_argv git.c:818:4\n #13 0x4cb01c in cmd_main git.c:949:19\n #14 0x6bd9ad in main common-main.c:52:11\n #15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 558 byte(s) leaked in 7 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/mv.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 3fccdcb645..c2f96c8e89 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -303,5 +303,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+\tstring_list_clear(&src_for_dst, 0);\n+\tUNLEAK(source);\n+\tUNLEAK(dest_path);\n+\tfree(submodule_gitfile);\n+\tfree(modes);\n \treturn 0;\n }\n-- \n2.26.2\n\n"},{"id":"431145","messageId":"20210725130830.5145-9-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 08/12] convert: release strbuf to avoid leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:26Z","receivedAt":"2021-07-25T13:10:00Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\napply_multi_file_filter and async_query_available_blobs both query\nsubprocess output using subprocess_read_status, which writes data into\nthe identically named filter_status strbuf. We add a strbuf_release to\navoid leaking their contents.\n\nLeak output seen when running t0021 with LSAN:\n\nDirect leak of 24 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c2b5 in xrealloc wrapper.c:126:8\n #2 0x9ff99d in strbuf_grow strbuf.c:98:2\n #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2\n #4 0xa101d6 in subprocess_read_status sub-process.c:45:5\n #5 0x77793c in apply_multi_file_filter convert.c:886:8\n #6 0x77793c in apply_filter convert.c:1042:10\n #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7\n #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2\n #9 0x8b48cd in index_fd object-file.c:2248:9\n #10 0x597411 in hash_fd builtin/hash-object.c:43:9\n #11 0x596be1 in hash_object builtin/hash-object.c:59:2\n #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3\n #13 0x4ce83e in run_builtin git.c:475:11\n #14 0x4ccafe in handle_builtin git.c:729:3\n #15 0x4cb01c in run_argv git.c:818:4\n #16 0x4cb01c in cmd_main git.c:949:19\n #17 0x6bdc2d in main common-main.c:52:11\n #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n\nDirect leak of 120 byte(s) in 5 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa8c295 in xrealloc wrapper.c:126:8\n #2 0x9ff97d in strbuf_grow strbuf.c:98:2\n #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2\n #4 0xa101b6 in subprocess_read_status sub-process.c:45:5\n #5 0x775c73 in async_query_available_blobs convert.c:960:8\n #6 0x80029d in finish_delayed_checkout entry.c:183:9\n #7 0xa65d1e in check_updates unpack-trees.c:493:10\n #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8\n #9 0x525971 in checkout builtin/clone.c:815:6\n #10 0x525971 in cmd_clone builtin/clone.c:1409:8\n #11 0x4ce83e in run_builtin git.c:475:11\n #12 0x4ccafe in handle_builtin git.c:729:3\n #13 0x4cb01c in run_argv git.c:818:4\n #14 0x4cb01c in cmd_main git.c:949:19\n #15 0x6bdc2d in main common-main.c:52:11\n #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n convert.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b025..0d6fb3410a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \telse\n \t\tstrbuf_swap(dst, &nbuf);\n \tstrbuf_release(&nbuf);\n+\tstrbuf_release(&filter_status);\n \treturn !err;\n }\n \n@@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p\n \n \tif (err)\n \t\thandle_filter_error(&filter_status, entry, 0);\n+\tstrbuf_release(&filter_status);\n \treturn !err;\n }\n \n-- \n2.26.2\n\n"},{"id":"431146","messageId":"20210725130830.5145-11-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 10/12] builtin/merge: free found_ref when done","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:28Z","receivedAt":"2021-07-25T13:10:03Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nmerge_name() calls dwim_ref(), which allocates a new string into\nfound_ref. Therefore add a free() to avoid leaking found_ref.\n\nLSAN output from t0021:\n\nDirect leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa8beb8 in xstrdup wrapper.c:29:14\n #2 0x954054 in expand_ref refs.c:671:12\n #3 0x953cb6 in repo_dwim_ref refs.c:644:22\n #4 0x5d3759 in dwim_ref refs.h:162:9\n #5 0x5d3759 in merge_name builtin/merge.c:517:6\n #6 0x5d3759 in collect_parents builtin/merge.c:1214:5\n #7 0x5cf60d in cmd_merge builtin/merge.c:1458:16\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6bdbfd in main common-main.c:52:11\n #13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/merge.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8a843b1f5..7ad85c044a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -503,7 +503,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \tstruct strbuf bname = STRBUF_INIT;\n \tstruct merge_remote_desc *desc;\n \tconst char *ptr;\n-\tchar *found_ref;\n+\tchar *found_ref = NULL;\n \tint len, early;\n \n \tstrbuf_branchname(&bname, remote, 0);\n@@ -586,6 +586,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \tstrbuf_addf(msg, \"%s\\t\\tcommit '%s'\\n\",\n \t\toid_to_hex(&remote_head->object.oid), remote);\n cleanup:\n+\tfree(found_ref);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&bname);\n }\n-- \n2.26.2\n\n"},{"id":"431147","messageId":"20210725130830.5145-13-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 12/12] reset: clear_unpack_trees_porcelain to plug leak","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:30Z","receivedAt":"2021-07-25T13:10:04Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\nsetup_unpack_trees_porcelain() populates various fields on\nunpack_tree_opts, we need to call clear_unpack_trees_porcelain() to\navoid leaking them. Specifically, we used to leak\nunpack_tree_opts.msgs_to_free.\n\nWe have to do this in leave_reset_head because there are multiple\nscenarios where unpack_tree_opts has already been configured, followed\nby a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'\nprior to having initialised unpack_tree_opts via memset(..., 0, ...).\nTherefore we also move unpack_tree_opts initialisation to the start of\nreset_head(), and convert it to use brace initialisation - which\nguarantees that we can never clear an uninitialised unpack_tree_opts.\nclear_unpack_tree_opts() is always safe to call as long as\nunpack_tree_opts is at least zero-initialised, i.e. it does not depend\non a previous call to setup_unpack_trees_porcelain().\n\nLSAN output from t0021:\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9f7861 in strvec_push_nodup strvec.c:19:2\n #3 0x9f7861 in strvec_pushf strvec.c:39:2\n #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n #5 0x97e011 in reset_head reset.c:53:2\n #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #7 0x4ce83e in run_builtin git.c:475:11\n #8 0x4ccafe in handle_builtin git.c:729:3\n #9 0x4cb01c in run_argv git.c:818:4\n #10 0x4cb01c in cmd_main git.c:949:19\n #11 0x6b3f3d in main common-main.c:52:11\n #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 147 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 134 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nIndirect leak of 130 byte(s) in 1 object(s) allocated from:\n #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n #1 0xa721e5 in xrealloc wrapper.c:126:8\n #2 0x9e8d54 in strbuf_grow strbuf.c:98:2\n #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3\n #4 0x9f7774 in strvec_pushf strvec.c:36:2\n #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3\n #6 0x97e011 in reset_head reset.c:53:2\n #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9\n #8 0x4ce83e in run_builtin git.c:475:11\n #9 0x4ccafe in handle_builtin git.c:729:3\n #10 0x4cb01c in run_argv git.c:818:4\n #11 0x4cb01c in cmd_main git.c:949:19\n #12 0x6b3f3d in main common-main.c:52:11\n #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n reset.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reset.c b/reset.c\nindex 4bea758053..79310ae071 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n-\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct unpack_trees_options unpack_tree_opts = { 0 };\n \tstruct tree *tree;\n \tconst char *reflog_action;\n \tstruct strbuf msg = STRBUF_INIT;\n@@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \tif (refs_only)\n \t\tgoto reset_head_refs;\n \n-\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, action);\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = r->index;\n@@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n leave_reset_head:\n \tstrbuf_release(&msg);\n \trollback_lock_file(&lock);\n+\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n \twhile (nr)\n \t\tfree((void *)desc[--nr].buffer);\n \treturn ret;\n-- \n2.26.2\n\n"},{"id":"431148","messageId":"20210725130830.5145-12-andrzej@ahunt.org","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"[PATCH v2 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"","fromEmail":"andrzej@ahunt.org","sentAt":"2021-07-25T13:08:29Z","receivedAt":"2021-07-25T13:10:08Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt \n\n- cmd_rebase populates rebase_options.strategy with newly allocated\n strings, hence we need to free those strings at the end of cmd_rebase\n to avoid a leak.\n- In some cases: get_replay_opts() is called, which prepares replay_opts\n using data from rebase_options. We used to simply copy the pointer\n from rebase_options.strategy, however that would now result in a\n double-free because sequencer_remove_state() is eventually used to\n free replay_opts.strategy. To avoid this we xstrdup() strategy when\n adding it to replay_opts.\n\nThe original leak happens because we always populate\nrebase_options.strategy, but we don't always enter the path that calls\nget_replay_opts() and later sequencer_remove_state() - in other words\nwe'd always allocate a new string into rebase_options.strategy but\nonly sometimes did we free it. We now make sure that rebase_options\nand replay_opts both own their own copies of strategy, and each copy\nis free'd independently.\n\nThis was first seen when running t0021 with LSAN, but t2012 helped catch\nthe fact that we can't just free(options.strategy) at the end of\ncmd_rebase (as that can cause a double-free). LSAN output from t0021:\n\nLSAN output from t0021:\n\nDirect leak of 4 byte(s) in 1 object(s) allocated from:\n #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n #1 0xa71eb8 in xstrdup wrapper.c:29:14\n #2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22\n #3 0x4ce83e in run_builtin git.c:475:11\n #4 0x4ccafe in handle_builtin git.c:729:3\n #5 0x4cb01c in run_argv git.c:818:4\n #6 0x4cb01c in cmd_main git.c:949:19\n #7 0x6b3fad in main common-main.c:52:11\n #8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt \n---\n builtin/rebase.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d..33e0961900 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \treplay.ignore_date = opts->ignore_date;\n \treplay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);\n \tif (opts->strategy)\n-\t\treplay.strategy = opts->strategy;\n+\t\treplay.strategy = xstrdup_or_null(opts->strategy);\n \telse if (!replay.strategy && replay.default_strategy) {\n \t\treplay.strategy = replay.default_strategy;\n \t\treplay.default_strategy = NULL;\n@@ -2109,6 +2109,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n+\tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \treturn ret;\n-- \n2.26.2\n\n"},{"id":"431165","messageId":"CAP8UFD1h7b0GkZWzuXfatFVVvXHvEWs=V63poUT4ovoM-extUA@mail.gmail.com","threadId":"55969","inReplyTo":"CABPp-BG0a0OM7s7cmO8yCeyA5TOCD_yOSJJepQE8MFEHct4EQA@mail.gmail.com","subject":"Re: [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-07-26T08:01:22Z","receivedAt":"2021-07-26T08:01:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 21, 2021 at 11:54 PM Elijah Newren wrote:\n>\n> On Sun, Jun 20, 2021 at 8:14 AM wrote:\n> >\n> > From: Andrzej Hunt \n> >\n> > This series plugs more of the leaks that were found while running\n> > t0002-t0099 with LSAN.\n> >\n> > See also the first series (already merged) at [1]. I'm currently\n> > expecting at least another 2 series before t0002-t0099 run leak free.\n> > I'm not being particularly systematic about the order of patches -\n> > although I am trying to send out \"real\" (if mostly small) leaks first,\n> > before sending out the more boring patches that add free()/UNLEAK() to\n> > cmd_* and direct helpers thereof.\n>\n> I've read over the series. It provides some good clear fixes. I\n> noted on patches 2, 6, and 12 that a some greps suggested that leaks\n> similar to the ones being fixed likely also affect other places of the\n> codebase. Those other places don't need to be fixed as part of this\n> series, but they might be good items for #leftoverbits or GSoC early\n> tasks (cc: Christian in case he wants to record those somewhere).\n\nYeah, thanks for letting me know!\n"},{"id":"431224","messageId":"xmqqr1fkj30i.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-2-andrzej@ahunt.org","subject":"Re: [PATCH v2 01/12] fmt-merge-msg: free newly allocated temporary strings when done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T19:20:29Z","receivedAt":"2021-07-26T19:20:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\n> index 0f66818e0f..b969dc6ebb 100644\n> --- a/fmt-merge-msg.c\n> +++ b/fmt-merge-msg.c\n> @@ -108,6 +108,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n> \tstruct origin_data *origin_data;\n> \tchar *src;\n> \tconst char *origin, *tag_name;\n> +\tchar *to_free = NULL;\n> \tstruct src_data *src_data;\n> \tstruct string_list_item *item;\n> \tint pulling_head = 0;\n> @@ -183,12 +184,13 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n> \tif (!strcmp(\".\", src) || !strcmp(src, origin)) {\n> \t\tint len = strlen(origin);\n> \t\tif (origin[0] == '\\'' && origin[len - 1] == '\\'')\n> -\t\t\torigin = xmemdupz(origin + 1, len - 2);\n> +\t\t\torigin = to_free = xmemdupz(origin + 1, len - 2);\n> \t} else\n> -\t\torigin = xstrfmt(\"%s of %s\", origin, src);\n> +\t\torigin = to_free = xstrfmt(\"%s of %s\", origin, src);\n> \tif (strcmp(\".\", src))\n> \t\torigin_data->is_local_branch = 0;\n> \tstring_list_append(&origins, origin)->util = origin_data;\n> +\tfree(to_free);\n> \treturn 0;\n> }\n\nThanks; obviously correct.\n\n"},{"id":"431228","messageId":"xmqqmtq8j12y.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-5-andrzej@ahunt.org","subject":"Re: [PATCH v2 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:02:13Z","receivedAt":"2021-07-26T20:02:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> cmd_for_each_repo() copies argv into args (a strvec), which is later\n> passed into run_command_on_repo(), which in turn copies that strvec onto\n> the end of child.args. The initial copy is unnecessary (we never modify\n> args). We therefore choose to just pass argv directly into\n> run_command_on_repo(), which lets us avoid the copy and fixes the leak.\n\nMakes sense. There is no reason to make these copies.\n\n>\n> LSAN output from t0068:\n>\n> Direct leak of 192 byte(s) in 1 object(s) allocated from:\n> #0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)\n> #1 0x98d7e6 in xrealloc wrapper.c:126\n> #2 0x916914 in strvec_push_nodup strvec.c:19\n> #3 0x916a6e in strvec_push strvec.c:26\n> #4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n> #5 0x410dcd in run_builtin git.c:475\n> #6 0x410dcd in handle_builtin git.c:729\n> #7 0x414087 in run_argv git.c:818\n> #8 0x414087 in cmd_main git.c:949\n> #9 0x40e9ec in main common-main.c:52\n> #10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> Indirect leak of 22 byte(s) in 2 object(s) allocated from:\n> #0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)\n> #1 0x98d698 in xstrdup wrapper.c:29\n> #2 0x916a63 in strvec_push strvec.c:26\n> #3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49\n> #4 0x410dcd in run_builtin git.c:475\n> #5 0x410dcd in handle_builtin git.c:729\n> #6 0x414087 in run_argv git.c:818\n> #7 0x414087 in cmd_main git.c:949\n> #8 0x40e9ec in main common-main.c:52\n> #9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> See also discussion about the original implementation below - this code\n> appears to have evolved from a callback explaining the double-strvec-copy\n> pattern, but there's no strong reason to keep that now:\n> https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> builtin/for-each-repo.c | 14 ++++----------\n> 1 file changed, 4 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\n> index 52be64a437..fd86e5a861 100644\n> --- a/builtin/for-each-repo.c\n> +++ b/builtin/for-each-repo.c\n> @@ -10,18 +10,16 @@ static const char * const for_each_repo_usage[] = {\n> \tNULL\n> };\n> \n> -static int run_command_on_repo(const char *path,\n> -\t\t\t void *cbdata)\n> +static int run_command_on_repo(const char *path, int argc, const char ** argv)\n> {\n> \tint i;\n> \tstruct child_process child = CHILD_PROCESS_INIT;\n> -\tstruct strvec *args = (struct strvec *)cbdata;\n> \n> \tchild.git_cmd = 1;\n> \tstrvec_pushl(&child.args, \"-C\", path, NULL);\n> \n> -\tfor (i = 0; i < args->nr; i++)\n> -\t\tstrvec_push(&child.args, args->v[i]);\n> +\tfor (i = 0; i < argc; i++)\n> +\t\tstrvec_push(&child.args, argv[i]);\n> \n> \treturn run_command(&child);\n> }\n> @@ -31,7 +29,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n> \tstatic const char *config_key = NULL;\n> \tint i, result = 0;\n> \tconst struct string_list *values;\n> -\tstruct strvec args = STRVEC_INIT;\n> \n> \tconst struct option options[] = {\n> \t\tOPT_STRING(0, \"config\", &config_key, N_(\"config\"),\n> @@ -45,9 +42,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n> \tif (!config_key)\n> \t\tdie(_(\"missing --config=\"));\n> \n> -\tfor (i = 0; i < argc; i++)\n> -\t\tstrvec_push(&args, argv[i]);\n> -\n> \tvalues = repo_config_get_value_multi(the_repository,\n> \t\t\t\t\t config_key);\n> \n> @@ -59,7 +53,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n> \t\treturn 0;\n> \n> \tfor (i = 0; !result && i < values->nr; i++)\n> -\t\tresult = run_command_on_repo(values->items[i].string, &args);\n> +\t\tresult = run_command_on_repo(values->items[i].string, argc, argv);\n> \n> \treturn result;\n> }\n"},{"id":"431229","messageId":"xmqqim0wj11t.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-6-andrzej@ahunt.org","subject":"Re: [PATCH v2 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:02:54Z","receivedAt":"2021-07-26T20:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> old_dir/new_dir are free()'d at the end of update_dir_rename_counts,\n> however if we return early we'll never free those strings. Therefore\n> we should move all new allocations after the possible early return,\n> avoiding a leak.\n>\n> This seems like a fairly recent leak, that started happening since the\n> early-return was added in:\n> 1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27)\n\nYup. It is not surprising to have issues in younger parts of the\ncode. Thanks.\n"},{"id":"431230","messageId":"xmqqeebkj0z7.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-7-andrzej@ahunt.org","subject":"Re: [PATCH v2 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:04:28Z","receivedAt":"2021-07-26T20:04:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> u.head is populated using resolve_refdup(), which returns a newly\n> allocated string - hence we also need to free() it.\n\nCorrect. The solution makes me wonder if this approach scales as we\nadd more and more members to u.* union that need deallocating, but\nfor now, this is perfectly adequate.\n\nThanks.\n\n>\n> Found while running t0041 with LSAN:\n>\n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n> #1 0xa8be98 in xstrdup wrapper.c:29:14\n> #2 0x9481db in head_atom_parser ref-filter.c:549:17\n> #3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30\n> #4 0x9400e3 in verify_ref_format ref-filter.c:974:8\n> #5 0x4f9e8b in print_ref_list builtin/branch.c:439:6\n> #6 0x4f9e8b in cmd_branch builtin/branch.c:757:3\n> #7 0x4ce83e in run_builtin git.c:475:11\n> #8 0x4ccafe in handle_builtin git.c:729:3\n> #9 0x4cb01c in run_argv git.c:818:4\n> #10 0x4cb01c in cmd_main git.c:949:19\n> #11 0x6bdc2d in main common-main.c:52:11\n> #12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> ref-filter.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f45d3a1b26..0cfef7b719 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2226,8 +2226,12 @@ void ref_array_clear(struct ref_array *array)\n> \tFREE_AND_NULL(array->items);\n> \tarray->nr = array->alloc = 0;\n> \n> -\tfor (i = 0; i < used_atom_cnt; i++)\n> -\t\tfree((char *)used_atom[i].name);\n> +\tfor (i = 0; i < used_atom_cnt; i++) {\n> +\t\tstruct used_atom *atom = &used_atom[i];\n> +\t\tif (atom->atom_type == ATOM_HEAD)\n> +\t\t\tfree(atom->u.head);\n> +\t\tfree((char *)atom->name);\n> +\t}\n> \tFREE_AND_NULL(used_atom);\n> \tused_atom_cnt = 0;\n"},{"id":"431231","messageId":"xmqqa6m8j0p4.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-8-andrzej@ahunt.org","subject":"Re: [PATCH v2 07/12] read-cache: call diff_setup_done to avoid leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:10:31Z","receivedAt":"2021-07-26T20:10:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> repo_diff_setup() calls through to diff.c's static prep_parse_options(),\n> which in turn allocates a new array into diff_opts.parseopts.\n> diff_setup_done() is responsible for freeing that array, and has the\n> benefit of verifying diff_opts too - hence we add a call to\n> diff_setup_done() to avoid leaking parseopts.\n\nRight. Thanks.\n\n>\n> Output from the leak as found while running t0090 with LSAN:\n>\n> Direct leak of 7120 byte(s) in 1 object(s) allocated from:\n> #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3\n> #1 0xa8bf89 in do_xmalloc wrapper.c:41:8\n> #2 0x7a7bae in prep_parse_options diff.c:5636:2\n> #3 0x7a7bae in repo_diff_setup diff.c:4611:2\n> #4 0x93716c in repo_index_has_changes read-cache.c:2518:3\n> #5 0x872233 in unclean merge-ort-wrappers.c:12:14\n> #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6\n> #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12\n> #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9\n> #9 0x4ce83e in run_builtin git.c:475:11\n> #10 0x4ccafe in handle_builtin git.c:729:3\n> #11 0x4cb01c in run_argv git.c:818:4\n> #12 0x4cb01c in cmd_main git.c:949:19\n> #13 0x6bdc2d in main common-main.c:52:11\n> #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> read-cache.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 46ccd66f34..83d1817ad0 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2505,6 +2505,7 @@ int repo_index_has_changes(struct repository *repo,\n> \t\topt.flags.exit_with_status = 1;\n> \t\tif (!sb)\n> \t\t\topt.flags.quick = 1;\n> +\t\tdiff_setup_done(&opt);\n> \t\tdo_diff_cache(&cmp, &opt);\n> \t\tdiffcore_std(&opt);\n> \t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n"},{"id":"431232","messageId":"xmqq5ywwj0h4.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-9-andrzej@ahunt.org","subject":"Re: [PATCH v2 08/12] convert: release strbuf to avoid leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:15:19Z","receivedAt":"2021-07-26T20:15:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> apply_multi_file_filter and async_query_available_blobs both query\n> subprocess output using subprocess_read_status, which writes data into\n> the identically named filter_status strbuf. We add a strbuf_release to\n> avoid leaking their contents.\n>\n> Leak output seen when running t0021 with LSAN:\n\nObviously correct. Thanks.\n\n>\n> Direct leak of 24 byte(s) in 1 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa8c2b5 in xrealloc wrapper.c:126:8\n> #2 0x9ff99d in strbuf_grow strbuf.c:98:2\n> #3 0x9ff99d in strbuf_addbuf strbuf.c:304:2\n> #4 0xa101d6 in subprocess_read_status sub-process.c:45:5\n> #5 0x77793c in apply_multi_file_filter convert.c:886:8\n> #6 0x77793c in apply_filter convert.c:1042:10\n> #7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7\n> #8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2\n> #9 0x8b48cd in index_fd object-file.c:2248:9\n> #10 0x597411 in hash_fd builtin/hash-object.c:43:9\n> #11 0x596be1 in hash_object builtin/hash-object.c:59:2\n> #12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3\n> #13 0x4ce83e in run_builtin git.c:475:11\n> #14 0x4ccafe in handle_builtin git.c:729:3\n> #15 0x4cb01c in run_argv git.c:818:4\n> #16 0x4cb01c in cmd_main git.c:949:19\n> #17 0x6bdc2d in main common-main.c:52:11\n> #18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).\n>\n> Direct leak of 120 byte(s) in 5 object(s) allocated from:\n> #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3\n> #1 0xa8c295 in xrealloc wrapper.c:126:8\n> #2 0x9ff97d in strbuf_grow strbuf.c:98:2\n> #3 0x9ff97d in strbuf_addbuf strbuf.c:304:2\n> #4 0xa101b6 in subprocess_read_status sub-process.c:45:5\n> #5 0x775c73 in async_query_available_blobs convert.c:960:8\n> #6 0x80029d in finish_delayed_checkout entry.c:183:9\n> #7 0xa65d1e in check_updates unpack-trees.c:493:10\n> #8 0xa5f469 in unpack_trees unpack-trees.c:1747:8\n> #9 0x525971 in checkout builtin/clone.c:815:6\n> #10 0x525971 in cmd_clone builtin/clone.c:1409:8\n> #11 0x4ce83e in run_builtin git.c:475:11\n> #12 0x4ccafe in handle_builtin git.c:729:3\n> #13 0x4cb01c in run_argv git.c:818:4\n> #14 0x4cb01c in cmd_main git.c:949:19\n> #15 0x6bdc2d in main common-main.c:52:11\n> #16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n>\n> SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).\n>\n> Signed-off-by: Andrzej Hunt \n> ---\n> convert.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/convert.c b/convert.c\n> index fd9c84b025..0d6fb3410a 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -916,6 +916,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n> \telse\n> \t\tstrbuf_swap(dst, &nbuf);\n> \tstrbuf_release(&nbuf);\n> +\tstrbuf_release(&filter_status);\n> \treturn !err;\n> }\n> \n> @@ -966,6 +967,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p\n> \n> \tif (err)\n> \t\thandle_filter_error(&filter_status, entry, 0);\n> +\tstrbuf_release(&filter_status);\n> \treturn !err;\n> }\n"},{"id":"431233","messageId":"xmqq1r7kj088.fsf@gitster.g","threadId":"55969","inReplyTo":"20210725130830.5145-1-andrzej@ahunt.org","subject":"Re: [PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T20:20:39Z","receivedAt":"2021-07-26T20:20:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"andrzej@ahunt.org writes:\n\n> From: Andrzej Hunt \n>\n> V2 fixes patch 11/12 (rebase_options.strategy lifecycle) as per review\n> discussion. Many thanks to Phillip and Elijah for spotting the issues there!\n\nThanks. Looking good.\n\nWill queue.\n"},{"id":"431334","messageId":"b03736e4-af30-7f91-d920-d917fc619d12@gmail.com","threadId":"55969","inReplyTo":"9f298c97-07d6-7117-baab-6a44359c44d2@ahunt.org","subject":"Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-27T19:34:33Z","receivedAt":"2021-07-27T19:34:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Andrzej\n\nOn 25/07/2021 14:03, Andrzej Hunt wrote:\n> [...]\n>>>> Given that we are\n>>>> allocating a copy above I think maybe your alternative approach of\n>>>> always freeing opts->strategy would be better.\n> \n> I will go down this route for V2. Although on further thought: instead \n> of my original idea of moving the string to replay_opts (and NULL'ing \n> out rebase_options->strategy), I think it's better to create a new copy \n> when populating replay_opts. The move/NULL approach I suggested in V1 \n> happens to work OK, but I think it's non-obvious and could break if we \n> ever wanted to use get_replay_opts() more than once - creating separate \n> copies reduces the number of surprises.\n\nCopying the string sounds like a good approach. I've looked at the V2 \npatch and it looks fine to me.\n\nThanks\n\nPhillip\n"}]}