{"thread":{"id":"59850","subject":"tests: mark as passing with SANITIZE=leak","startedAt":"2023-06-11T18:29:17Z","lastAt":"2023-06-17T06:43:31Z","messageCount":47,"participants":["Rubén Justo","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"478228","messageId":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","threadId":"59850","inReplyTo":null,"subject":"tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:29:09Z","receivedAt":"2023-06-11T18:29:17Z","isPatch":false,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The goal in this series is to pass t3200 with SANITIZE=leak.\n\nAs a result of the fixes, other tests also pass.\n\nThis is the list of tests that no longer trigger any leak after this\nseries:\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nEach of the commits (except 11/11) fixes a leak. They have no\ndependencies on each other. As a result, they can be reordered.\n\nTo review one leak, the commit can be moved to the tip or reverted.\nE.g. to review: \"branch: fix a leak in check_tracking_branch\", this can\nbe used:\n\n $ git revert --no-edit HEAD~3\n $ make SANITIZE=leak test T=t3200-branch.sh \n\nAlso, each commit have a minimal script in the message that can be used\nto reproduce the leak.\n\nRubén Justo (11):\n rev-parse: fix a leak with --abbrev-ref\n config: fix a leak in git_config_copy_or_rename_section_in_file\n remote: fix a leak in query_matches_negative_refspec\n branch: fix a leak in dwim_and_setup_tracking\n branch: fix a leak in setup_tracking\n branch: fix a leak in cmd_branch\n branch: fix a leak in inherit_tracking\n branch: fix a leak in check_tracking_branch\n branch: fix a leak in setup_tracking\n config: fix a leak in git_config_copy_or_rename_section_in_file\n tests: mark as passing with SANITIZE=leak\n\n branch.c | 14 +++++++++-----\n builtin/branch.c | 2 +-\n builtin/rev-parse.c | 7 +++++--\n config.c | 2 ++\n remote.c | 2 +-\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 16 files changed, 29 insertions(+), 9 deletions(-)\n\n-- \n2.40.1\n"},{"id":"478229","messageId":"dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 01/11] rev-parse: fix a leak with --abbrev-ref","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:49:17Z","receivedAt":"2023-06-11T18:49:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"To handle \"--abbrev-ref\" we use shorten_unambiguous_ref(). This\nfunction takes a refname and returns a shortened refname, which is a\nnewly allocated string that needs to be freed.\n\nUnfortunately, the refname variable is reused to receive the shortened\none. Therefore, we lose the original refname, which needs to be freed\nas well, producing a leak.\n\nThis leak can be reviewed with:\n\n $ for a in {1..10}; do git branch foo_${a}; done\n $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}\n\n Direct leak of 171 byte(s) in 10 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in expand_ref refs.c\n ... in repo_dwim_ref refs.c\n ... in show_rev builtin/rev-parse.c\n ... in cmd_rev_parse builtin/rev-parse.c\n ... in run_builtin git.c\n\nWe have this leak since a45d34691e (rev-parse: --abbrev-ref option to\nshorten ref name, 2009-04-13) when \"--abbrev-ref\" was introduced.\n\nLet's fix it.\n\nSigned-off-by: Rubén Justo \n---\n builtin/rev-parse.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e340..9fd7095431 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -141,7 +141,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \tif ((symbolic || abbrev_ref) && name) {\n \t\tif (symbolic == SHOW_SYMBOLIC_FULL || abbrev_ref) {\n \t\t\tstruct object_id discard;\n-\t\t\tchar *full;\n+\t\t\tchar *full, *to_free = NULL;\n \n \t\t\tswitch (repo_dwim_ref(the_repository, name,\n \t\t\t\t\t strlen(name), &discard, &full,\n@@ -156,9 +156,11 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\tcase 1: /* happy */\n-\t\t\t\tif (abbrev_ref)\n+\t\t\t\tif (abbrev_ref) {\n+\t\t\t\t\tto_free = full;\n \t\t\t\t\tfull = shorten_unambiguous_ref(full,\n \t\t\t\t\t\tabbrev_ref_strict);\n+\t\t\t\t}\n \t\t\t\tshow_with_type(type, full);\n \t\t\t\tbreak;\n \t\t\tdefault: /* ambiguous */\n@@ -166,6 +168,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tfree(full);\n+\t\t\tfree(to_free);\n \t\t} else {\n \t\t\tshow_with_type(type, name);\n \t\t}\n-- \n2.40.1\n"},{"id":"478230","messageId":"2ce6f067-9d02-2a10-5eb9-6820e66f0b84@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 02/11] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:49:28Z","receivedAt":"2023-06-11T18:49:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m),\n2017-06-18) a new strbuf variable was introduced, but not released.\n\nThus, when copying a branch that has any configuration, we have a\nleak.\n\n $ git branch foo\n $ git config branch.foo.some-key some_value\n $ git branch -c foo bar\n\n Direct leak of 65 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_vaddf strbuf.c\n ... in strbuf_addf strbuf.c\n ... in store_create_section config.c\n ... in git_config_copy_or_rename_section_in_file config.c\n ... in git_config_copy_section_in_file config.c\n ... in git_config_copy_section config.c\n ... in copy_or_rename_branch builtin/branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nLet's fix that leak.\n\nSigned-off-by: Rubén Justo \n---\n config.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.c b/config.c\nindex b79baf83e3..39a7d7422c 100644\n--- a/config.c\n+++ b/config.c\n@@ -3879,6 +3879,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \tfree(filename_buf);\n \tconfig_store_data_clear(&store);\n \tstrbuf_release(&buf);\n+\tstrbuf_release(©str);\n \treturn ret;\n }\n \n-- \n2.40.1\n"},{"id":"478231","messageId":"a9b27053-ff2b-7b67-f2ba-5691f4bda961@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 03/11] remote: fix a leak in query_matches_negative_refspec","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:49:35Z","receivedAt":"2023-06-11T18:49:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In c0192df630 (refspec: add support for negative refspecs, 2020-09-30)\nquery_matches_negative_refspec() was introduced.\n\nThe function was implemented as a two-loop process, where the former\nloop accumulates and the latter evaluates. To accumulate, a string_list\nis used.\n\nWithin the first loop, there are three cases where a string is added to\nthe string_list. Two of them add strings that do not need to be\nfreed. But in the third case, the string added is returned by\nmatch_name_with_pattern(), which needs to be freed.\n\nThe string_list is initialized with STRING_LIST_INIT_NODUP, i.e. when\ncleared, the strings added are not freed. Therefore, the string\nreturned by match_name_with_pattern() is not freed, so we have a leak.\n\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch --track bar local/foo\n\n Direct leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_matches_negative_refspec remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\n Direct leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_matches_negative_refspec remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in check_tracking_branch branch.c\n ... in for_each_remote remote.c\n ... in validate_remote_tracking_branch branch.c\n ... in dwim_branch_start branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nAn interesting point to note is that while string_list_append() is used\nin the first two cases described, string_list_append_nodup() is used in\nthe third. This seems to indicate an intention to delegate the\nresponsibility for freeing the string, to the string_list. As if the\nstring_list had been initialized with STRING_LIST_INIT_DUP, i.e. the\nstrings are strdup()'d when added (except if the \"_nodup\" API is used)\nand freed when cleared.\n\nSwitching to STRING_LIST_INIT_DUP fixes the leak and probably is what we\nwanted to do originally. Let's do it.\n\nSigned-off-by: Rubén Justo \n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0764fca0db..1bcd36e358 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -890,7 +890,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n {\n \tint i, matched_negative = 0;\n \tint find_src = !query->src;\n-\tstruct string_list reversed = STRING_LIST_INIT_NODUP;\n+\tstruct string_list reversed = STRING_LIST_INIT_DUP;\n \tconst char *needle = find_src ? query->dst : query->src;\n \n \t/*\n-- \n2.40.1\n"},{"id":"478232","messageId":"a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:49:43Z","receivedAt":"2023-06-11T18:49:54Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In e89f151db1 (branch: move --set-upstream-to behavior to\ndwim_and_setup_tracking(), 2022-01-28) the string returned by\ndwim_branch_start() was not freed, resulting in a memory leak.\n\nIt can be reviewed with:\n\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch --set-upstream-to local/foo foo\n\n Direct leak of 23 byte(s) in 1 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in expand_ref refs.c\n ... in repo_dwim_ref refs.c\n ... in dwim_branch_start branch.c\n ... in dwim_and_setup_tracking branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nLet's free it now.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex ba3914adf5..a7333a4c32 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,\n \t\t\t const char *orig_ref, enum branch_track track,\n \t\t\t int quiet)\n {\n-\tchar *real_orig_ref;\n+\tchar *real_orig_ref = NULL;\n \tdwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);\n \tsetup_tracking(new_ref, real_orig_ref, track, quiet);\n+\tfree(real_orig_ref);\n }\n \n /**\n-- \n2.40.1\n"},{"id":"478233","messageId":"49a4339b-d736-2c30-0c57-194ab33f377c@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 05/11] branch: fix a leak in setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:49:51Z","receivedAt":"2023-06-11T18:49:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bdaf1dfae7 (branch: new autosetupmerge option \"simple\" for matching\nbranches, 2022-04-29) a new exit for setup_tracking() missed the\nclean-up, producing a leak.\n\n $ git config branch.autoSetupMerge simple\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch bar local/foo\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\n Indirect leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\nThe return introduced in bdaf1dfae7 was to avoid setting up the\ntracking, but even in that case it is still necessary to do the\nclean-up. Let's do it.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex a7333a4c32..ff81c2266a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\tif (!skip_prefix(tracking.srcs->items[0].string,\n \t\t\t\t \"refs/heads/\", &tracked_branch) ||\n \t\t strcmp(tracked_branch, new_ref))\n-\t\t\treturn;\n+\t\t\tgoto cleanup;\n \t}\n \n \tif (tracking.srcs->nr < 1)\n-- \n2.40.1\n"},{"id":"478234","messageId":"6cbdf9b9-227a-4665-5725-6a863676e95d@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 06/11] branch: fix a leak in cmd_branch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:05Z","receivedAt":"2023-06-11T18:50:11Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 98e7ab6d42 (for-each-ref: delay parsing of --sort= options,\n2021-10-20) a new string_list was introduced to accumulate any\n\"branch.sort\" setting.\n\nThat string_list is cleared in ref_sorting_options(), which is only\ncalled when processing the \"--list\" sub-command. Therefore, with other\nsub-command, while having any sort option set, a leak is produced, e.g.:\n\n $ git config branch.sort invalid_sort_option\n $ git branch --edit-description\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\n Indirect leak of 20 byte(s) in 1 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nWe don't have a common clean-up section in cmd_branch(). To avoid\nrefactoring and keep the fix simple, and while we find a better\nsolution, let's silence the leak-hunter making the list static.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..759480fe8d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -709,7 +709,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tenum branch_track track;\n \tstruct ref_filter filter;\n \tstatic struct ref_sorting *sorting;\n-\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n+\tstatic struct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option options[] = {\n-- \n2.40.1\n"},{"id":"478235","messageId":"25e68755-7073-9523-dacc-d79e4e10eb39@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 07/11] branch: fix a leak in inherit_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:16Z","receivedAt":"2023-06-11T18:50:22Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In d3115660b4 (branch: add flags and config to inherit tracking,\n2021-12-20) a new option was introduced to allow creating a new branch,\ninheriting the tracking of another branch.\n\nThe new code, strdup()'d the remote_name of the existing branch, but\nunfortunately it was not freed, producing a leak.\n\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch --track bar local/foo\n branch 'bar' set up to track 'local/foo'.\n $ git branch --track=inherit baz bar\n branch 'baz' set up to track 'local/foo'.\n\n Direct leak of 6 byte(s) in 1 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in inherit_tracking branch.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nActually, the string we're strdup()'ing is from the struct branch\nreturned by get_branch(). Which, in turn, retrieves the string from the\nglobal \"struct repository\". This makes perfectly valid to use the\nstring throughout the entire execution of create_branch(). There is no\nneed to duplicate it.\n\nLet's fix the leak, removing the strdup().\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex ff81c2266a..19d606d360 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -233,7 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \t\treturn -1;\n \t}\n \n-\ttracking->remote = xstrdup(branch->remote_name);\n+\ttracking->remote = branch->remote_name;\n \tfor (i = 0; i < branch->merge_nr; i++)\n \t\tstring_list_append(tracking->srcs, branch->merge_name[i]);\n \treturn 0;\n-- \n2.40.1\n"},{"id":"478236","messageId":"98c68cb4-abbe-0b86-0052-517f47711271@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 08/11] branch: fix a leak in check_tracking_branch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:27Z","receivedAt":"2023-06-11T18:50:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Let's fix a leak we have in check_tracking_branch() since the function\nwas introduced in 41c21f22d0 (branch.c: Validate tracking branches with\nrefspecs instead of refs/remotes/*, 2013-04-21).\n\nThe leak can be reviewed with:\n\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch --track bar local/foo\n\n Direct leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in check_tracking_branch branch.c\n ... in for_each_remote remote.c\n ... in validate_remote_tracking_branch branch.c\n ... in dwim_branch_start branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 19d606d360..09b9563ae7 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -480,9 +480,12 @@ static int check_tracking_branch(struct remote *remote, void *cb_data)\n {\n \tchar *tracking_branch = cb_data;\n \tstruct refspec_item query;\n+\tint res;\n \tmemset(&query, 0, sizeof(struct refspec_item));\n \tquery.dst = tracking_branch;\n-\treturn !remote_find_tracking(remote, &query);\n+\tres = !remote_find_tracking(remote, &query);\n+\tfree(query.src);\n+\treturn res;\n }\n \n static int validate_remote_tracking_branch(char *ref)\n-- \n2.40.1\n"},{"id":"478237","messageId":"471dcc04-d60c-b745-15f0-05fe4e93ff11@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 09/11] branch: fix a leak in setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:36Z","receivedAt":"2023-06-11T18:50:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The commit d3115660b4 (branch: add flags and config to inherit tracking,\n2021-12-20) replaced in \"struct tracking\", the member \"char *src\" by a\nnew \"struct string_list *srcs\".\n\nThis caused a modification in find_tracked_branch(). The string\nreturned by remote_find_tracking(), previously assigned to \"src\", is now\nadded to the string_list \"srcs\".\n\nThat string_list is initialized with STRING_LIST_INIT_DUP, which means\nthat what is added is not the given string, but a duplicate. Therefore,\nthe string returned by remote_find_tracking() is leaked.\n\nThe leak can be reviewed with:\n\n $ git branch foo\n $ git remote add local .\n $ git fetch local\n $ git branch --track bar local/foo\n\n Direct leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nLet's fix the leak, using the \"_nodup\" API to add to the string_list.\nThis way, the string itself will be added instead of being strdup()'d.\nAnd when the string_list is cleared, the string will be freed.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 09b9563ae7..d88f50a48a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -37,7 +37,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n \t\tswitch (++tracking->matches) {\n \t\tcase 1:\n-\t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n+\t\t\tstring_list_append_nodup(tracking->srcs, tracking->spec.src);\n \t\t\ttracking->remote = remote->name;\n \t\t\tbreak;\n \t\tcase 2:\n-- \n2.40.1\n"},{"id":"478238","messageId":"3ee93720-dce0-8f90-68bf-0242e5731a36@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:45Z","receivedAt":"2023-06-11T18:50:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"A branch can have its configuration spread over several configuration\nsections. This situation was already foreseen in 52d59cc645 (branch:\nadd a --copy (-c) option to go with --move (-m), 2017-06-18) when\n\"branch -c\" was introduced.\n\nUnfortunately, a leak was also introduced:\n\n $ git branch foo\n $ cat >> .git/config <\n---\n config.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.c b/config.c\nindex 39a7d7422c..207e4394a3 100644\n--- a/config.c\n+++ b/config.c\n@@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t\t\t\t\t\toutput[0] = '\\t';\n \t\t\t\t\t}\n \t\t\t\t} else {\n+\t\t\t\t\tstrbuf_release(©str);\n \t\t\t\t\tcopystr = store_create_section(new_name, &store);\n \t\t\t\t}\n \t\t\t}\n-- \n2.40.1\n"},{"id":"478239","messageId":"32527a42-ed33-cfd0-60dc-03ccc679ba20@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH 11/11] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T18:50:58Z","receivedAt":"2023-06-11T18:51:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The tests listed below, since previous commits, no longer trigger any\nleak.\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nLet's mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromptly any new leak that may be introduced and triggered by them, in\nthe future.\n\nAs a reference, the list has been formed using:\n\n $ GIT_TEST_PASSING_SANITIZE_LEAK=check make SANITIZE=leak test\n\nSigned-off-by: Rubén Justo \n---\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 11 files changed, 11 insertions(+)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex cb9ef7e329..b9af6b3ac0 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -5,6 +5,7 @@ test_description='test @{upstream} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 87a4286414..e841309d0e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check() {\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex d868a08110..a835a196aa 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -4,6 +4,7 @@ test_description='test @{push} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n resolve () {\ndiff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh\nindex dca35aa3e3..a8bbc60954 100755\n--- a/t/t2027-checkout-track.sh\n+++ b/t/t2027-checkout-track.sh\n@@ -5,6 +5,7 @@ test_description='tests for git branch --track'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 98b6c8ac34..daf1666df7 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -8,6 +8,7 @@ test_description='git branch assorted tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 3399344f25..594e3e43e1 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n expect_branch() {\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex cc07889667..51737eeafe 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh\nindex a448e169bd..6d4944a728 100755\n--- a/t/t5517-push-mirror.sh\n+++ b/t/t5517-push-mirror.sh\n@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n D=$(pwd)\ndiff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh\nindex 45815f7378..3a28f1ded5 100755\n--- a/t/t5525-fetch-tagopt.sh\n+++ b/t/t5525-fetch-tagopt.sh\n@@ -2,6 +2,7 @@\n \n test_description='tagopt variable affects \"git fetch\" and is overridden by commandline.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n setup_clone () {\ndiff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh\nindex a313849406..7ddbd96e58 100755\n--- a/t/t6040-tracking-info.sh\n+++ b/t/t6040-tracking-info.sh\n@@ -5,6 +5,7 @@ test_description='remote tracking stats'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n advance () {\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex aed07c5b62..4c1f03e609 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -5,6 +5,7 @@\n \n test_description='git status'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-- \n2.40.1\n"},{"id":"478240","messageId":"72cc21d8-1684-2bae-09a1-de647f49c293@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"Re: tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-11T21:23:47Z","receivedAt":"2023-06-11T21:23:54Z","isPatch":false,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 20:29:09, Rubén Justo wrote:\n\nSorry. This should have been:\n\n[PATCH 00/11] tests: mark as passing with SANITIZE=leak\n\nI'll resend with the correct subject, but will wait some time for\nreviews.\n \n> The goal in this series is to pass t3200 with SANITIZE=leak.\n> \n> As a result of the fixes, other tests also pass.\n> \n> This is the list of tests that no longer trigger any leak after this\n> series:\n> \n> + t1507-rev-parse-upstream.sh\n> + t1508-at-combinations.sh\n> + t1514-rev-parse-push.sh\n> + t2027-checkout-track.sh\n> + t3200-branch.sh\n> + t3204-branch-name-interpretation.sh\n> + t5404-tracking-branches.sh\n> + t5517-push-mirror.sh\n> + t5525-fetch-tagopt.sh\n> + t6040-tracking-info.sh\n> + t7508-status.sh\n> \n> Each of the commits (except 11/11) fixes a leak. They have no\n> dependencies on each other. As a result, they can be reordered.\n> \n> To review one leak, the commit can be moved to the tip or reverted.\n> E.g. to review: \"branch: fix a leak in check_tracking_branch\", this can\n> be used:\n> \n> $ git revert --no-edit HEAD~3\n> $ make SANITIZE=leak test T=t3200-branch.sh \n> \n> Also, each commit have a minimal script in the message that can be used\n> to reproduce the leak.\n> \n> Rubén Justo (11):\n> rev-parse: fix a leak with --abbrev-ref\n> config: fix a leak in git_config_copy_or_rename_section_in_file\n> remote: fix a leak in query_matches_negative_refspec\n> branch: fix a leak in dwim_and_setup_tracking\n> branch: fix a leak in setup_tracking\n> branch: fix a leak in cmd_branch\n> branch: fix a leak in inherit_tracking\n> branch: fix a leak in check_tracking_branch\n> branch: fix a leak in setup_tracking\n> config: fix a leak in git_config_copy_or_rename_section_in_file\n> tests: mark as passing with SANITIZE=leak\n> \n> branch.c | 14 +++++++++-----\n> builtin/branch.c | 2 +-\n> builtin/rev-parse.c | 7 +++++--\n> config.c | 2 ++\n> remote.c | 2 +-\n> t/t1507-rev-parse-upstream.sh | 1 +\n> t/t1508-at-combinations.sh | 1 +\n> t/t1514-rev-parse-push.sh | 1 +\n> t/t2027-checkout-track.sh | 1 +\n> t/t3200-branch.sh | 1 +\n> t/t3204-branch-name-interpretation.sh | 1 +\n> t/t5404-tracking-branches.sh | 1 +\n> t/t5517-push-mirror.sh | 1 +\n> t/t5525-fetch-tagopt.sh | 1 +\n> t/t6040-tracking-info.sh | 1 +\n> t/t7508-status.sh | 1 +\n> 16 files changed, 29 insertions(+), 9 deletions(-)\n> \n> -- \n> 2.40.1\n"},{"id":"478242","messageId":"20230612031239.GA306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com","subject":"Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:12:39Z","receivedAt":"2023-06-12T03:13:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:49:17PM +0200, Rubén Justo wrote:\n\n> To handle \"--abbrev-ref\" we use shorten_unambiguous_ref(). This\n> function takes a refname and returns a shortened refname, which is a\n> newly allocated string that needs to be freed.\n> \n> Unfortunately, the refname variable is reused to receive the shortened\n> one. Therefore, we lose the original refname, which needs to be freed\n> as well, producing a leak.\n\nMakes sense, but...\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 852e49e340..9fd7095431 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -141,7 +141,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n> \tif ((symbolic || abbrev_ref) && name) {\n> \t\tif (symbolic == SHOW_SYMBOLIC_FULL || abbrev_ref) {\n> \t\t\tstruct object_id discard;\n> -\t\t\tchar *full;\n> +\t\t\tchar *full, *to_free = NULL;\n> \n> \t\t\tswitch (repo_dwim_ref(the_repository, name,\n> \t\t\t\t\t strlen(name), &discard, &full,\n> @@ -156,9 +156,11 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n> \t\t\t\t */\n> \t\t\t\tbreak;\n> \t\t\tcase 1: /* happy */\n> -\t\t\t\tif (abbrev_ref)\n> +\t\t\t\tif (abbrev_ref) {\n> +\t\t\t\t\tto_free = full;\n> \t\t\t\t\tfull = shorten_unambiguous_ref(full,\n> \t\t\t\t\t\tabbrev_ref_strict);\n> +\t\t\t\t}\n> \t\t\t\tshow_with_type(type, full);\n> \t\t\t\tbreak;\n> \t\t\tdefault: /* ambiguous */\n> @@ -166,6 +168,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \t\t\tfree(full);\n> +\t\t\tfree(to_free);\n> \t\t} else {\n> \t\t\tshow_with_type(type, name);\n> \t\t}\n\nThe lifetime of \"to_free\" is much longer than it needs to be here. We'd\nusually use a \"to_free\" pattern when the memory is aliased to another\n(usually const) pointer, and the allocation needs to live as long as\nthat other pointer. But here, nobody cares about the old value as soon\nas we replace it. We could almost just free() it ahead of time, but of\ncourse we also need to pass it to the shortening function. So maybe:\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e340..6dc8548e1f 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\tcase 1: /* happy */\n-\t\t\t\tif (abbrev_ref)\n-\t\t\t\t\tfull = shorten_unambiguous_ref(full,\n+\t\t\t\tif (abbrev_ref) {\n+\t\t\t\t\tchar *old = full;\n+\t\t\t\t\tfull = shorten_unambiguous_ref(old,\n \t\t\t\t\t\tabbrev_ref_strict);\n+\t\t\t\t\tfree(old);\n+\t\t\t\t}\n \t\t\t\tshow_with_type(type, full);\n \t\t\t\tbreak;\n \t\t\tdefault: /* ambiguous */\n\n-Peff\n"},{"id":"478243","messageId":"20230612031425.GB306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"2ce6f067-9d02-2a10-5eb9-6820e66f0b84@gmail.com","subject":"Re: [PATCH 02/11] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:14:25Z","receivedAt":"2023-06-12T03:14:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:49:28PM +0200, Rubén Justo wrote:\n\n> In 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m),\n> 2017-06-18) a new strbuf variable was introduced, but not released.\n> \n> Thus, when copying a branch that has any configuration, we have a\n> leak.\n> \n> $ git branch foo\n> $ git config branch.foo.some-key some_value\n> $ git branch -c foo bar\n\nLooks good. Thanks for digging up the commit which introduced the\nproblem. I always find that gives me more confidence that the leak is\njust a simple \"oops, we forgot to free\", and there isn't something more\nsubtle going on.\n\n-Peff\n"},{"id":"478244","messageId":"20230612031732.GC306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"a9b27053-ff2b-7b67-f2ba-5691f4bda961@gmail.com","subject":"Re: [PATCH 03/11] remote: fix a leak in query_matches_negative_refspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:17:32Z","receivedAt":"2023-06-12T03:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:49:35PM +0200, Rubén Justo wrote:\n\n> An interesting point to note is that while string_list_append() is used\n> in the first two cases described, string_list_append_nodup() is used in\n> the third. This seems to indicate an intention to delegate the\n> responsibility for freeing the string, to the string_list. As if the\n> string_list had been initialized with STRING_LIST_INIT_DUP, i.e. the\n> strings are strdup()'d when added (except if the \"_nodup\" API is used)\n> and freed when cleared.\n> \n> Switching to STRING_LIST_INIT_DUP fixes the leak and probably is what we\n> wanted to do originally. Let's do it.\n\nAh, clearing a NODUP string-list strikes again. :) I concur with your\nreasoning here; switching it to DUP is the best fix.\n\n-Peff\n"},{"id":"478245","messageId":"20230612032106.GD306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com","subject":"Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:21:06Z","receivedAt":"2023-06-12T03:22:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote:\n\n> In e89f151db1 (branch: move --set-upstream-to behavior to\n> dwim_and_setup_tracking(), 2022-01-28) the string returned by\n> dwim_branch_start() was not freed, resulting in a memory leak.\n\nYep, looks good. One small comment:\n\n> diff --git a/branch.c b/branch.c\n> index ba3914adf5..a7333a4c32 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,\n> \t\t\t const char *orig_ref, enum branch_track track,\n> \t\t\t int quiet)\n> {\n> -\tchar *real_orig_ref;\n> +\tchar *real_orig_ref = NULL;\n> \tdwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);\n> \tsetup_tracking(new_ref, real_orig_ref, track, quiet);\n> +\tfree(real_orig_ref);\n> }\n\nAdding the NULL-initialization signals that we expect that real_orig_ref\nmight sometimes _not_ be filled in by dwim_branch_start(). But I think\nit always will be; and if it weren't, that would make passing it to\nsetup_tracking() rather suspicious.\n\nSo I don't think the NULL initialization is wrong, and certainly it is\nmore defensive. But I find it a little misleading.\n\n-Peff\n"},{"id":"478246","messageId":"20230612032615.GE306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"49a4339b-d736-2c30-0c57-194ab33f377c@gmail.com","subject":"Re: [PATCH 05/11] branch: fix a leak in setup_tracking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:26:15Z","receivedAt":"2023-06-12T03:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:\n\n> The return introduced in bdaf1dfae7 was to avoid setting up the\n> tracking, but even in that case it is still necessary to do the\n> clean-up. Let's do it.\n\nThat may be a sign that the return introduced by that commit is in the\nwrong spot (i.e., could we be checking it earlier and returning before\ndoing the work that led to the allocations?).\n\nBut I didn't look too carefully, and I think for the purposes of your\nseries it is OK to simply fix the leak without digging too far.\n\nI'll cc the author (and quote the patch below) though, as sometimes in\ncases like these they may be interested in looking deeper themselves.\n\n-Peff\n\n> Signed-off-by: Rubén Justo \n> ---\n> branch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/branch.c b/branch.c\n> index a7333a4c32..ff81c2266a 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n> \t\tif (!skip_prefix(tracking.srcs->items[0].string,\n> \t\t\t\t \"refs/heads/\", &tracked_branch) ||\n> \t\t strcmp(tracked_branch, new_ref))\n> -\t\t\treturn;\n> +\t\t\tgoto cleanup;\n> \t}\n> \n> \tif (tracking.srcs->nr < 1)\n> -- \n> 2.40.1\n"},{"id":"478247","messageId":"20230612034639.GF306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"6cbdf9b9-227a-4665-5725-6a863676e95d@gmail.com","subject":"Re: [PATCH 06/11] branch: fix a leak in cmd_branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:46:39Z","receivedAt":"2023-06-12T03:46:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:50:05PM +0200, Rubén Justo wrote:\n\n> We don't have a common clean-up section in cmd_branch(). To avoid\n> refactoring and keep the fix simple, and while we find a better\n> solution, let's silence the leak-hunter making the list static.\n\nGross. :)\n\nIf we are just going to annotate here, I'd prefer to use UNLEAK(). It\nmakes it more obvious this is about leak-checking, and doesn't imply\nthat we otherwise care about the lifetime of this field. Like:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..075e580d22 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (list)\n \t\tsetup_auto_pager(\"branch\", 1);\n \n+\tUNLEAK(sorting_options);\n+\n \tif (delete) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index e6c2655af6..759480fe8d 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -709,7 +709,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \tenum branch_track track;\n> \tstruct ref_filter filter;\n> \tstatic struct ref_sorting *sorting;\n> -\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n> +\tstatic struct string_list sorting_options = STRING_LIST_INIT_DUP;\n\nIt's interesting that the ref_sorting pointer is also static. This goes\nback to aedcb7dc75 (branch.c: use 'ref-filter' APIs, 2015-09-23). I\nwonder if it was trying to accomplish the same thing (back then we added\ndirectly to the list), though back then we did not have any real\nleak-detection tools set up.\n\nIt should probably move into the \"if (list)\" block, but that is\northogonal to your patch.\n\nAs for the more complete fix, I think we'd perhaps want something like\nthis, which would also detect when --sort is used in a non-list mode (as\nyour example shows, it's a little complicated because we stuff\nconfigured entries into the same list):\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..bcd2d9e1d9 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -708,8 +708,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t recurse_submodules_explicit = 0;\n \tenum branch_track track;\n \tstruct ref_filter filter;\n-\tstatic struct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n+\tsize_t nr_configured_sorting_options;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option options[] = {\n@@ -773,6 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_branch_usage, options);\n \n \tgit_config(git_branch_config, &sorting_options);\n+\tnr_configured_sorting_options = sorting_options.nr;\n \n \ttrack = git_branch_track;\n \n@@ -832,6 +833,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (list)\n \t\tsetup_auto_pager(\"branch\", 1);\n \n+\tif (!list) {\n+\t\tif (sorting_options.nr != nr_configured_sorting_options)\n+\t\t\tdie(_(\"--sort does not make sense when not listing\"));\n+\t\tstring_list_clear(&sorting_options, 0);\n+\t}\n+\n \tif (delete) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n@@ -840,6 +847,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_current_branch_name();\n \t\treturn 0;\n \t} else if (list) {\n+\t\tstruct ref_sorting *sorting;\n \t\t/* git branch --list also shows HEAD when it is detached */\n \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n\n-Peff\n"},{"id":"478248","messageId":"20230612034831.GG306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"25e68755-7073-9523-dacc-d79e4e10eb39@gmail.com","subject":"Re: [PATCH 07/11] branch: fix a leak in inherit_tracking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:48:31Z","receivedAt":"2023-06-12T03:48:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:50:16PM +0200, Rubén Justo wrote:\n\n> Actually, the string we're strdup()'ing is from the struct branch\n> returned by get_branch(). Which, in turn, retrieves the string from the\n> global \"struct repository\". This makes perfectly valid to use the\n> string throughout the entire execution of create_branch(). There is no\n> need to duplicate it.\n> \n> Let's fix the leak, removing the strdup().\n\nYep, good reasoning. I agree this is a good way to fix the leak.\n\n-Peff\n"},{"id":"478249","messageId":"20230612035557.GH306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"98c68cb4-abbe-0b86-0052-517f47711271@gmail.com","subject":"Re: [PATCH 08/11] branch: fix a leak in check_tracking_branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:55:57Z","receivedAt":"2023-06-12T03:58:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:50:27PM +0200, Rubén Justo wrote:\n\n> diff --git a/branch.c b/branch.c\n> index 19d606d360..09b9563ae7 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -480,9 +480,12 @@ static int check_tracking_branch(struct remote *remote, void *cb_data)\n> {\n> \tchar *tracking_branch = cb_data;\n> \tstruct refspec_item query;\n> +\tint res;\n> \tmemset(&query, 0, sizeof(struct refspec_item));\n> \tquery.dst = tracking_branch;\n> -\treturn !remote_find_tracking(remote, &query);\n> +\tres = !remote_find_tracking(remote, &query);\n> +\tfree(query.src);\n> +\treturn res;\n> }\n\nOK, so we expect remote_find_tracking() to fill in the \"src\" field, but\nwe don't actually care about the value here (we are just validating). It\nprobably doesn't fill in the value for some error cases, but then we'd\nbe left with a NULL, which is OK to feed to free(). Makes sense. I\ndouble-checked that it always allocates \"src\" when it is assigned to, so\nI think this fix is good.\n\n-Peff\n"},{"id":"478250","messageId":"20230612035948.GI306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"471dcc04-d60c-b745-15f0-05fe4e93ff11@gmail.com","subject":"Re: [PATCH 09/11] branch: fix a leak in setup_tracking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T03:59:48Z","receivedAt":"2023-06-12T03:59:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:50:36PM +0200, Rubén Justo wrote:\n\n> The commit d3115660b4 (branch: add flags and config to inherit tracking,\n> 2021-12-20) replaced in \"struct tracking\", the member \"char *src\" by a\n> new \"struct string_list *srcs\".\n> \n> This caused a modification in find_tracked_branch(). The string\n> returned by remote_find_tracking(), previously assigned to \"src\", is now\n> added to the string_list \"srcs\".\n> \n> That string_list is initialized with STRING_LIST_INIT_DUP, which means\n> that what is added is not the given string, but a duplicate. Therefore,\n> the string returned by remote_find_tracking() is leaked.\n\nYour fix makes sense. I had to stare at the existing code for a long\ntime to make sure the _other_ side of the switch wasn't leaking, but it\nworks by falling through \"case 2\" into \"default\", which frees\ntracking->spec.src. So this should plug the leak completely.\n\n-Peff\n"},{"id":"478251","messageId":"20230612040518.GJ306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"3ee93720-dce0-8f90-68bf-0242e5731a36@gmail.com","subject":"Re: [PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T04:05:18Z","receivedAt":"2023-06-12T04:05:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:\n\n> diff --git a/config.c b/config.c\n> index 39a7d7422c..207e4394a3 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n> \t\t\t\t\t\toutput[0] = '\\t';\n> \t\t\t\t\t}\n> \t\t\t\t} else {\n> +\t\t\t\t\tstrbuf_release(©str);\n> \t\t\t\t\tcopystr = store_create_section(new_name, &store);\n> \t\t\t\t}\n> \t\t\t}\n\nWow, I did a double-take on this code. It is uncommon in our codebase to\nassign a struct by value in a function return like this, and doubly\nweird to assign a strbuf (since the whole point of strbuf is to use the\nopaque functions to make sure we aren't overwriting existing allocations\nor aliasing pointers).\n\nI think your fix here is the correct thing if we aren't going to clean\nup the code further.\n\nThe more usual thing for our codebase would be refactoring like:\n\ndiff --git a/config.c b/config.c\nindex b79baf83e3..f5a7cced7c 100644\n--- a/config.c\n+++ b/config.c\n@@ -3140,37 +3140,36 @@ static int write_error(const char *filename)\n \treturn 4;\n }\n \n-static struct strbuf store_create_section(const char *key,\n-\t\t\t\t\t const struct config_store_data *store)\n+static void store_create_section(const char *key,\n+\t\t\t\t const struct config_store_data *store,\n+\t\t\t\t struct strbuf *sb)\n {\n \tconst char *dot;\n \tsize_t i;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tdot = memchr(key, '.', store->baselen);\n \tif (dot) {\n-\t\tstrbuf_addf(&sb, \"[%.*s \\\"\", (int)(dot - key), key);\n+\t\tstrbuf_addf(sb, \"[%.*s \\\"\", (int)(dot - key), key);\n \t\tfor (i = dot - key + 1; i < store->baselen; i++) {\n \t\t\tif (key[i] == '\"' || key[i] == '\\\\')\n-\t\t\t\tstrbuf_addch(&sb, '\\\\');\n-\t\t\tstrbuf_addch(&sb, key[i]);\n+\t\t\t\tstrbuf_addch(sb, '\\\\');\n+\t\t\tstrbuf_addch(sb, key[i]);\n \t\t}\n-\t\tstrbuf_addstr(&sb, \"\\\"]\\n\");\n+\t\tstrbuf_addstr(sb, \"\\\"]\\n\");\n \t} else {\n-\t\tstrbuf_addch(&sb, '[');\n-\t\tstrbuf_add(&sb, key, store->baselen);\n-\t\tstrbuf_addstr(&sb, \"]\\n\");\n+\t\tstrbuf_addch(sb, '[');\n+\t\tstrbuf_add(sb, key, store->baselen);\n+\t\tstrbuf_addstr(sb, \"]\\n\");\n \t}\n-\n-\treturn sb;\n }\n \n static ssize_t write_section(int fd, const char *key,\n \t\t\t const struct config_store_data *store)\n {\n-\tstruct strbuf sb = store_create_section(key, store);\n+\tstruct strbuf sb = STRBUF_INIT;\n \tssize_t ret;\n \n+\tstore_create_section(key, store, &sb);\n \tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \n@@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t\t\t\t\t\toutput[0] = '\\t';\n \t\t\t\t\t}\n \t\t\t\t} else {\n-\t\t\t\t\tcopystr = store_create_section(new_name, &store);\n+\t\t\t\t\tstrbuf_reset(©str);\n+\t\t\t\t\tstore_create_section(new_name, &store,\n+\t\t\t\t\t\t\t ©str);\n \t\t\t\t}\n \t\t\t}\n \t\t\tremove = 0;\n"},{"id":"478252","messageId":"20230612040651.GK306352@coredump.intra.peff.net","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"Re: tests: mark as passing with SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-12T04:06:51Z","receivedAt":"2023-06-12T04:06:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 11, 2023 at 08:29:09PM +0200, Rubén Justo wrote:\n\n> Each of the commits (except 11/11) fixes a leak. They have no\n> dependencies on each other. As a result, they can be reordered.\n\nThanks for a pleasant read. I think each case is correct, though I left\na few minor comments. Some of them are things I think are worth\nincluding in a re-roll. Others are things that we _could_ go further to\nrefactor for clarity, but I am OK if you want to stay focused on the\nminimal leak-fixes.\n\n-Peff\n"},{"id":"478337","messageId":"xmqqv8fr40nf.fsf@gitster.g","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"Re: tests: mark as passing with SANITIZE=leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-13T19:34:12Z","receivedAt":"2023-06-13T19:34:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> The goal in this series is to pass t3200 with SANITIZE=leak.\n>\n> As a result of the fixes, other tests also pass.\n\nThanks for working on these. I've seen Peff's reviews and picked up\nonly the ones that are unambiguously good, dropping the rest that\nyou may want to update. And as a consequence, 11/11 is also not\nincluded in the set that has been queued. Please do include that\nfinal step when you resend updated bits that I dropped here.\n\nThanks.\n"},{"id":"478480","messageId":"3e124763-944d-e147-88fb-cb989fbd06b7@gmail.com","threadId":"59850","inReplyTo":"20230612031239.GA306352@coredump.intra.peff.net","subject":"Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T22:34:49Z","receivedAt":"2023-06-16T22:34:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 23:12:39, Jeff King wrote:\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 852e49e340..6dc8548e1f 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n> \t\t\t\t */\n> \t\t\t\tbreak;\n> \t\t\tcase 1: /* happy */\n> -\t\t\t\tif (abbrev_ref)\n> -\t\t\t\t\tfull = shorten_unambiguous_ref(full,\n> +\t\t\t\tif (abbrev_ref) {\n> +\t\t\t\t\tchar *old = full;\n> +\t\t\t\t\tfull = shorten_unambiguous_ref(old,\n> \t\t\t\t\t\tabbrev_ref_strict);\n> +\t\t\t\t\tfree(old);\n> +\t\t\t\t}\n> \t\t\t\tshow_with_type(type, full);\n> \t\t\t\tbreak;\n> \t\t\tdefault: /* ambiguous */\n\nSimpler. Nice.\n\nThanks!\n"},{"id":"478481","messageId":"3a675cd7-6847-2a23-561e-5baf513d65e0@gmail.com","threadId":"59850","inReplyTo":"20230612031732.GC306352@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] remote: fix a leak in query_matches_negative_refspec","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T22:37:37Z","receivedAt":"2023-06-16T22:37:43Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 23:17:32, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:49:35PM +0200, Rubén Justo wrote:\n> \n> > An interesting point to note is that while string_list_append() is used\n> > in the first two cases described, string_list_append_nodup() is used in\n> > the third. This seems to indicate an intention to delegate the\n> > responsibility for freeing the string, to the string_list. As if the\n> > string_list had been initialized with STRING_LIST_INIT_DUP, i.e. the\n> > strings are strdup()'d when added (except if the \"_nodup\" API is used)\n> > and freed when cleared.\n> > \n> > Switching to STRING_LIST_INIT_DUP fixes the leak and probably is what we\n> > wanted to do originally. Let's do it.\n> \n> Ah, clearing a NODUP string-list strikes again. :) I concur with your\n\nYes. \"_nodup\" with NODUP string_list, shows red flags, imo.\n\n> reasoning here; switching it to DUP is the best fix.\n\nThank you for reviewing the change.\n"},{"id":"478482","messageId":"e75092a4-2629-85a9-1bd2-3193d6719a65@gmail.com","threadId":"59850","inReplyTo":"20230612032106.GD306352@coredump.intra.peff.net","subject":"Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T22:45:28Z","receivedAt":"2023-06-16T22:45:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 23:21:06, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote:\n> \n> > In e89f151db1 (branch: move --set-upstream-to behavior to\n> > dwim_and_setup_tracking(), 2022-01-28) the string returned by\n> > dwim_branch_start() was not freed, resulting in a memory leak.\n> \n> Yep, looks good. One small comment:\n> \n> > diff --git a/branch.c b/branch.c\n> > index ba3914adf5..a7333a4c32 100644\n> > --- a/branch.c\n> > +++ b/branch.c\n> > @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,\n> > \t\t\t const char *orig_ref, enum branch_track track,\n> > \t\t\t int quiet)\n> > {\n> > -\tchar *real_orig_ref;\n> > +\tchar *real_orig_ref = NULL;\n> > \tdwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);\n> > \tsetup_tracking(new_ref, real_orig_ref, track, quiet);\n> > +\tfree(real_orig_ref);\n> > }\n> \n> Adding the NULL-initialization signals that we expect that real_orig_ref\n> might sometimes _not_ be filled in by dwim_branch_start(). But I think\n> it always will be; and if it weren't, that would make passing it to\n> setup_tracking() rather suspicious.\n> \n> So I don't think the NULL initialization is wrong, and certainly it is\n> more defensive. But I find it a little misleading.\n\nI don't have any objection to this. But I've seen Junio has already\nmerged this to 'next'. I'm not going to re-roll this commit. Sorry.\n\nThank you for your review.\n"},{"id":"478483","messageId":"bb03d449-adb5-5c4d-0a2f-12a26bd48d8c@gmail.com","threadId":"59850","inReplyTo":"20230612032615.GE306352@coredump.intra.peff.net","subject":"Re: [PATCH 05/11] branch: fix a leak in setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T22:46:19Z","receivedAt":"2023-06-16T22:46:25Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 23:26:15, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:\n> \n> > The return introduced in bdaf1dfae7 was to avoid setting up the\n> > tracking, but even in that case it is still necessary to do the\n> > clean-up. Let's do it.\n> \n> That may be a sign that the return introduced by that commit is in the\n> wrong spot (i.e., could we be checking it earlier and returning before\n> doing the work that led to the allocations?).\n> \n> But I didn't look too carefully, and I think for the purposes of your\n> series it is OK to simply fix the leak without digging too far.\n\nThanks.\n\n> \n> I'll cc the author (and quote the patch below) though, as sometimes in\n> cases like these they may be interested in looking deeper themselves.\n\nPerfect. Thank you.\n"},{"id":"478484","messageId":"2ff000c5-5b90-6271-a676-78c5bc93a054@gmail.com","threadId":"59850","inReplyTo":"20230612034639.GF306352@coredump.intra.peff.net","subject":"Re: [PATCH 06/11] branch: fix a leak in cmd_branch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T22:50:46Z","receivedAt":"2023-06-16T22:50:54Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-jun-2023 23:46:39, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:50:05PM +0200, Rubén Justo wrote:\n> \n> > We don't have a common clean-up section in cmd_branch(). To avoid\n> > refactoring and keep the fix simple, and while we find a better\n> > solution, let's silence the leak-hunter making the list static.\n> \n> Gross. :)\n\nXD\n\n> \n> If we are just going to annotate here, I'd prefer to use UNLEAK(). It\n> makes it more obvious this is about leak-checking, and doesn't imply\n> that we otherwise care about the lifetime of this field. Like:\n> \n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index e6c2655af6..075e580d22 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \tif (list)\n> \t\tsetup_auto_pager(\"branch\", 1);\n> \n> +\tUNLEAK(sorting_options);\n> +\n> \tif (delete) {\n> \t\tif (!argc)\n> \t\t\tdie(_(\"branch name required\"));\n\nOK. Will do this.\n\nThank you for reviewing the change with a wide view.\n"},{"id":"478485","messageId":"21325f3e-a502-fdb6-1c67-500bce47e920@gmail.com","threadId":"59850","inReplyTo":"20230612040518.GJ306352@coredump.intra.peff.net","subject":"Re: [PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:04:38Z","receivedAt":"2023-06-16T23:04:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12-jun-2023 00:05:18, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:\n> \n> > diff --git a/config.c b/config.c\n> > index 39a7d7422c..207e4394a3 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n> > \t\t\t\t\t\toutput[0] = '\\t';\n> > \t\t\t\t\t}\n> > \t\t\t\t} else {\n> > +\t\t\t\t\tstrbuf_release(©str);\n> > \t\t\t\t\tcopystr = store_create_section(new_name, &store);\n> > \t\t\t\t}\n> > \t\t\t}\n> \n> Wow, I did a double-take on this code. It is uncommon in our codebase to\n> assign a struct by value in a function return like this, and doubly\n> weird to assign a strbuf (since the whole point of strbuf is to use the\n> opaque functions to make sure we aren't overwriting existing allocations\n> or aliasing pointers).\n> \n> I think your fix here is the correct thing if we aren't going to clean\n> up the code further.\n> \n> The more usual thing for our codebase would be refactoring like:\n> \n> diff --git a/config.c b/config.c\n> index b79baf83e3..f5a7cced7c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -3140,37 +3140,36 @@ static int write_error(const char *filename)\n> \treturn 4;\n> }\n> \n> -static struct strbuf store_create_section(const char *key,\n> -\t\t\t\t\t const struct config_store_data *store)\n> +static void store_create_section(const char *key,\n> +\t\t\t\t const struct config_store_data *store,\n> +\t\t\t\t struct strbuf *sb)\n> {\n> \tconst char *dot;\n> \tsize_t i;\n> -\tstruct strbuf sb = STRBUF_INIT;\n> \n> \tdot = memchr(key, '.', store->baselen);\n> \tif (dot) {\n> -\t\tstrbuf_addf(&sb, \"[%.*s \\\"\", (int)(dot - key), key);\n> +\t\tstrbuf_addf(sb, \"[%.*s \\\"\", (int)(dot - key), key);\n> \t\tfor (i = dot - key + 1; i < store->baselen; i++) {\n> \t\t\tif (key[i] == '\"' || key[i] == '\\\\')\n> -\t\t\t\tstrbuf_addch(&sb, '\\\\');\n> -\t\t\tstrbuf_addch(&sb, key[i]);\n> +\t\t\t\tstrbuf_addch(sb, '\\\\');\n> +\t\t\tstrbuf_addch(sb, key[i]);\n> \t\t}\n> -\t\tstrbuf_addstr(&sb, \"\\\"]\\n\");\n> +\t\tstrbuf_addstr(sb, \"\\\"]\\n\");\n> \t} else {\n> -\t\tstrbuf_addch(&sb, '[');\n> -\t\tstrbuf_add(&sb, key, store->baselen);\n> -\t\tstrbuf_addstr(&sb, \"]\\n\");\n> +\t\tstrbuf_addch(sb, '[');\n> +\t\tstrbuf_add(sb, key, store->baselen);\n> +\t\tstrbuf_addstr(sb, \"]\\n\");\n> \t}\n> -\n> -\treturn sb;\n> }\n> \n> static ssize_t write_section(int fd, const char *key,\n> \t\t\t const struct config_store_data *store)\n> {\n> -\tstruct strbuf sb = store_create_section(key, store);\n> +\tstruct strbuf sb = STRBUF_INIT;\n> \tssize_t ret;\n> \n> +\tstore_create_section(key, store, &sb);\n> \tret = write_in_full(fd, sb.buf, sb.len);\n> \tstrbuf_release(&sb);\n> \n> @@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n> \t\t\t\t\t\toutput[0] = '\\t';\n> \t\t\t\t\t}\n> \t\t\t\t} else {\n> -\t\t\t\t\tcopystr = store_create_section(new_name, &store);\n> +\t\t\t\t\tstrbuf_reset(©str);\n> +\t\t\t\t\tstore_create_section(new_name, &store,\n> +\t\t\t\t\t\t\t ©str);\n> \t\t\t\t}\n> \t\t\t}\n> \t\t\tremove = 0;\n\nI have a draft with the exact same change. I chose the simple fix,\nthough. I wasn't expecting your review :)\n\nI'll definitely re-roll with this.\n\nThanks!\n"},{"id":"478486","messageId":"4e25776e-a490-091f-6f43-7f8f8413bf7b@gmail.com","threadId":"59850","inReplyTo":"20230612040651.GK306352@coredump.intra.peff.net","subject":"Re: tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:14:45Z","receivedAt":"2023-06-16T23:14:52Z","isPatch":false,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12/6/23 6:06, Jeff King wrote:\n> On Sun, Jun 11, 2023 at 08:29:09PM +0200, Rubén Justo wrote:\n> \n>> Each of the commits (except 11/11) fixes a leak. They have no\n>> dependencies on each other. As a result, they can be reordered.\n> \n> Thanks for a pleasant read. I think each case is correct, though I left\n> a few minor comments. Some of them are things I think are worth\n> including in a re-roll. Others are things that we _could_ go further to\n> refactor for clarity, but I am OK if you want to stay focused on the\n> minimal leak-fixes.\n> \n> -Peff\n> \n\nThank you for your prompt reviews, and sorry for my late responses.\n\n"},{"id":"478487","messageId":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","threadId":"59850","inReplyTo":"9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com","subject":"[PATCH v2 0/5] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:27:47Z","receivedAt":"2023-06-16T23:27:56Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The goal in this series is to pass t3200 with SANITIZE=leak.\n\nAs a result of the fixes, other tests also pass.\n\nThis is the list of tests that no longer trigger any leak after this\nseries:\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nEach of the commits (except 5/5) fixes a leak. They have no\ndependencies on each other. As a result, they can be reordered.\n\nTo review one leak, the commit can be moved to the tip or reverted.\nE.g. to review: \"branch: fix a leak in setup_tracking\", this can\nbe used:\n\n $ git revert --no-edit HEAD~3\n $ make SANITIZE=leak test T=t3200-branch.sh\n\nAlso, each commit have a minimal script in the message that can be used\nto reproduce the leak.\n\nThis is the second version of this series. However, a subset of the\npatches from the first version have already been merged to 'next'.\nTherefore, those are not included here.\n\nThese are the rest of them, which address Peff's reviews.\n\nThanks.\n\nRubén Justo (5):\n rev-parse: fix a leak with --abbrev-ref\n branch: fix a leak in setup_tracking\n branch: fix a leak in cmd_branch\n config: fix a leak in git_config_copy_or_rename_section_in_file\n tests: mark as passing with SANITIZE=leak\n\n branch.c | 2 +-\n builtin/branch.c | 2 ++\n builtin/rev-parse.c | 5 ++++-\n config.c | 29 ++++++++++++++-------------\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 15 files changed, 33 insertions(+), 16 deletions(-)\n\n-- \n2.40.1\n"},{"id":"478488","messageId":"e097e997-9de9-85cc-4e20-22eb32e98765@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v2 1/5] rev-parse: fix a leak with --abbrev-ref","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:34:03Z","receivedAt":"2023-06-16T23:34:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"To handle \"--abbrev-ref\" we use shorten_unambiguous_ref(). This\nfunction takes a refname and returns a shortened refname, which is a\nnewly allocated string that needs to be freed.\n\nUnfortunately, the refname variable is reused to receive the shortened\none. Therefore, we lose the original refname, which needs to be freed\nas well, producing a leak.\n\nThis leak can be reviewed with:\n\n $ for a in {1..10}; do git branch foo_${a}; done\n $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}\n\n Direct leak of 171 byte(s) in 10 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in expand_ref refs.c\n ... in repo_dwim_ref refs.c\n ... in show_rev builtin/rev-parse.c\n ... in cmd_rev_parse builtin/rev-parse.c\n ... in run_builtin git.c\n\nWe have this leak since a45d34691e (rev-parse: --abbrev-ref option to\nshorten ref name, 2009-04-13) when \"--abbrev-ref\" was introduced.\n\nLet's fix it.\n\nSigned-off-by: Rubén Justo \n---\n builtin/rev-parse.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e340..d2eb239a08 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\tcase 1: /* happy */\n-\t\t\t\tif (abbrev_ref)\n+\t\t\t\tif (abbrev_ref) {\n+\t\t\t\t\tchar *old = full;\n \t\t\t\t\tfull = shorten_unambiguous_ref(full,\n \t\t\t\t\t\tabbrev_ref_strict);\n+\t\t\t\t\tfree(old);\n+\t\t\t\t}\n \t\t\t\tshow_with_type(type, full);\n \t\t\t\tbreak;\n \t\t\tdefault: /* ambiguous */\n-- \n2.40.1\n"},{"id":"478489","messageId":"7943ec83-8ef3-4397-0d2e-47f47069de16@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v2 2/5] branch: fix a leak in setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:34:22Z","receivedAt":"2023-06-16T23:34:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bdaf1dfae7 (branch: new autosetupmerge option \"simple\" for matching\nbranches, 2022-04-29) a new exit for setup_tracking() missed the\nclean-up, producing a leak.\n\n $ git config branch.autoSetupMerge simple\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch bar local/foo\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\n Indirect leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\nThe return introduced in bdaf1dfae7 was to avoid setting up the\ntracking, but even in that case it is still necessary to do the\nclean-up. Let's do it.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 427bde896f..d88f50a48a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\tif (!skip_prefix(tracking.srcs->items[0].string,\n \t\t\t\t \"refs/heads/\", &tracked_branch) ||\n \t\t strcmp(tracked_branch, new_ref))\n-\t\t\treturn;\n+\t\t\tgoto cleanup;\n \t}\n \n \tif (tracking.srcs->nr < 1)\n-- \n2.40.1\n"},{"id":"478490","messageId":"cfb18a2d-381f-b9ae-865a-26c1669da3b5@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v2 3/5] branch: fix a leak in cmd_branch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:34:30Z","receivedAt":"2023-06-16T23:34:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 98e7ab6d42 (for-each-ref: delay parsing of --sort= options,\n2021-10-20) a new string_list was introduced to accumulate any\n\"branch.sort\" setting.\n\nThat string_list is cleared in ref_sorting_options(), which is only\ncalled when processing the \"--list\" sub-command. Therefore, with other\nsub-command, while having any sort option set, a leak is produced, e.g.:\n\n $ git config branch.sort invalid_sort_option\n $ git branch --edit-description\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\n Indirect leak of 20 byte(s) in 1 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nWe don't have a common clean-up section in cmd_branch(). To avoid\nrefactoring, keep the fix simple, and while we find a better solution\nwhich hopefuly will avoid entirely that string_list, when no sort\noptions are needed; let's squelch the leak sanitizer using UNLEAK().\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..075e580d22 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (list)\n \t\tsetup_auto_pager(\"branch\", 1);\n \n+\tUNLEAK(sorting_options);\n+\n \tif (delete) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \n2.40.1\n"},{"id":"478491","messageId":"adaa7fa7-befe-ce59-7391-5a4ddd3bb2b8@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v2 4/5] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:34:51Z","receivedAt":"2023-06-16T23:35:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"A branch can have its configuration spread over several configuration\nsections. This situation was already foreseen in 52d59cc645 (branch:\nadd a --copy (-c) option to go with --move (-m), 2017-06-18) when\n\"branch -c\" was introduced.\n\nUnfortunately, a leak was also introduced:\n\n $ git branch foo\n $ cat >> .git/config <\n---\n config.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 39a7d7422c..c5f4b59ef3 100644\n--- a/config.c\n+++ b/config.c\n@@ -3140,37 +3140,36 @@ static int write_error(const char *filename)\n \treturn 4;\n }\n \n-static struct strbuf store_create_section(const char *key,\n-\t\t\t\t\t const struct config_store_data *store)\n+static void store_create_section(const char *key,\n+\t\t\t\tconst struct config_store_data *store,\n+\t\t\t\tstruct strbuf *sb)\n {\n \tconst char *dot;\n \tsize_t i;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tdot = memchr(key, '.', store->baselen);\n \tif (dot) {\n-\t\tstrbuf_addf(&sb, \"[%.*s \\\"\", (int)(dot - key), key);\n+\t\tstrbuf_addf(sb, \"[%.*s \\\"\", (int)(dot - key), key);\n \t\tfor (i = dot - key + 1; i < store->baselen; i++) {\n \t\t\tif (key[i] == '\"' || key[i] == '\\\\')\n-\t\t\t\tstrbuf_addch(&sb, '\\\\');\n-\t\t\tstrbuf_addch(&sb, key[i]);\n+\t\t\t\tstrbuf_addch(sb, '\\\\');\n+\t\t\tstrbuf_addch(sb, key[i]);\n \t\t}\n-\t\tstrbuf_addstr(&sb, \"\\\"]\\n\");\n+\t\tstrbuf_addstr(sb, \"\\\"]\\n\");\n \t} else {\n-\t\tstrbuf_addch(&sb, '[');\n-\t\tstrbuf_add(&sb, key, store->baselen);\n-\t\tstrbuf_addstr(&sb, \"]\\n\");\n+\t\tstrbuf_addch(sb, '[');\n+\t\tstrbuf_add(sb, key, store->baselen);\n+\t\tstrbuf_addstr(sb, \"]\\n\");\n \t}\n-\n-\treturn sb;\n }\n \n static ssize_t write_section(int fd, const char *key,\n \t\t\t const struct config_store_data *store)\n {\n-\tstruct strbuf sb = store_create_section(key, store);\n+\tstruct strbuf sb = STRBUF_INIT;\n \tssize_t ret;\n \n+\tstore_create_section(key, store, &sb);\n \tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \n@@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t\t\t\t\t\toutput[0] = '\\t';\n \t\t\t\t\t}\n \t\t\t\t} else {\n-\t\t\t\t\tcopystr = store_create_section(new_name, &store);\n+\t\t\t\t\tstrbuf_reset(©str);\n+\t\t\t\t\tstore_create_section(new_name, &store,\n+\t\t\t\t\t\t\t ©str);\n \t\t\t\t}\n \t\t\t}\n \t\t\tremove = 0;\n-- \n2.40.1\n\n"},{"id":"478492","messageId":"66aae5a7-a2a2-2297-4249-d22fd070e91a@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v2 5/5] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-16T23:35:00Z","receivedAt":"2023-06-16T23:35:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The tests listed below, since previous commits, no longer trigger any\nleak.\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nLet's mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromptly any new leak that may be introduced and triggered by them in\nthe future.\n\nSigned-off-by: Rubén Justo \n---\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 11 files changed, 11 insertions(+)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex cb9ef7e329..b9af6b3ac0 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -5,6 +5,7 @@ test_description='test @{upstream} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 87a4286414..e841309d0e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check() {\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex d868a08110..a835a196aa 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -4,6 +4,7 @@ test_description='test @{push} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n resolve () {\ndiff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh\nindex dca35aa3e3..a8bbc60954 100755\n--- a/t/t2027-checkout-track.sh\n+++ b/t/t2027-checkout-track.sh\n@@ -5,6 +5,7 @@ test_description='tests for git branch --track'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 98b6c8ac34..daf1666df7 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -8,6 +8,7 @@ test_description='git branch assorted tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 3399344f25..594e3e43e1 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n expect_branch() {\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex cc07889667..51737eeafe 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh\nindex a448e169bd..6d4944a728 100755\n--- a/t/t5517-push-mirror.sh\n+++ b/t/t5517-push-mirror.sh\n@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n D=$(pwd)\ndiff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh\nindex 45815f7378..3a28f1ded5 100755\n--- a/t/t5525-fetch-tagopt.sh\n+++ b/t/t5525-fetch-tagopt.sh\n@@ -2,6 +2,7 @@\n \n test_description='tagopt variable affects \"git fetch\" and is overridden by commandline.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n setup_clone () {\ndiff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh\nindex a313849406..7ddbd96e58 100755\n--- a/t/t6040-tracking-info.sh\n+++ b/t/t6040-tracking-info.sh\n@@ -5,6 +5,7 @@ test_description='remote tracking stats'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n advance () {\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex aed07c5b62..4c1f03e609 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -5,6 +5,7 @@\n \n test_description='git status'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-- \n2.40.1\n"},{"id":"478494","messageId":"xmqqfs6rrmxk.fsf@gitster.g","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-16T23:45:43Z","receivedAt":"2023-06-16T23:45:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> This is the second version of this series. However, a subset of the\n> patches from the first version have already been merged to 'next'.\n> Therefore, those are not included here.\n>\n> These are the rest of them, which address Peff's reviews.\n\nThanks!\n"},{"id":"478502","messageId":"20230617054823.GG562686@coredump.intra.peff.net","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-17T05:48:23Z","receivedAt":"2023-06-17T05:48:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2023 at 01:27:47AM +0200, Rubén Justo wrote:\n\n> This is the second version of this series. However, a subset of the\n> patches from the first version have already been merged to 'next'.\n> Therefore, those are not included here.\n> \n> These are the rest of them, which address Peff's reviews.\n\nThese all look good to me. I'd probably have split the config.c refactor\ninto its own commit, but I don't think it's worth going back for.\n\nThanks Rubén for fixing these!\n\n-Peff\n"},{"id":"478503","messageId":"5b7cd886-5747-b6df-dcfb-1cc8624d982d@gmail.com","threadId":"59850","inReplyTo":"20230617054823.GG562686@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:35:10Z","receivedAt":"2023-06-17T06:35:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 17/6/23 7:48, Jeff King wrote:\n> These all look good to me. I'd probably have split the config.c refactor\n> into its own commit\n\nThat's a good point. I'll send the refactor when the dust settles,\nalong with a revision for the UNLEAK().\n\nThank you!\n"},{"id":"478504","messageId":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","threadId":"59850","inReplyTo":"5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com","subject":"[PATCH v3 0/5] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:37:04Z","receivedAt":"2023-06-17T06:37:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The goal in this series is to pass t3200 with SANITIZE=leak.\n\nAs a result of the fixes, other tests also pass.\n\nThis is the list of tests that no longer trigger any leak after this\nseries:\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nEach of the commits (except 5/5) fixes a leak. They have no\ndependencies on each other. As a result, they can be reordered.\n\nTo review one leak, the commit can be moved to the tip or reverted.\nE.g. to review: \"branch: fix a leak in setup_tracking\", this can\nbe used:\n\n $ git revert --no-edit HEAD~3\n $ make SANITIZE=leak test T=t3200-branch.sh\n\nAlso, each commit have a minimal script in the message that can be used\nto reproduce the leak.\n\nThis is the third version of this series. The refactoring in config.c\nhas been reverted, and will be sent outside of this series.\n\nSorry for the noise of a quick re-roll.\n\nThanks.\n\nRubén Justo (5):\n rev-parse: fix a leak with --abbrev-ref\n branch: fix a leak in setup_tracking\n branch: fix a leak in cmd_branch\n config: fix a leak in git_config_copy_or_rename_section_in_file\n tests: mark as passing with SANITIZE=leak\n\n branch.c | 2 +-\n builtin/branch.c | 2 ++\n builtin/rev-parse.c | 5 ++++-\n config.c | 1 +\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 15 files changed, 19 insertions(+), 2 deletions(-)\n\n-- \n2.40.1\n"},{"id":"478505","messageId":"38534e6c-f245-38ad-1468-742b5756ab42@gmail.com","threadId":"59850","inReplyTo":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","subject":"[PATCH v3 1/5] rev-parse: fix a leak with --abbrev-ref","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:40:43Z","receivedAt":"2023-06-17T06:42:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"To handle \"--abbrev-ref\" we use shorten_unambiguous_ref(). This\nfunction takes a refname and returns a shortened refname, which is a\nnewly allocated string that needs to be freed.\n\nUnfortunately, the refname variable is reused to receive the shortened\none. Therefore, we lose the original refname, which needs to be freed\nas well, producing a leak.\n\nThis leak can be reviewed with:\n\n $ for a in {1..10}; do git branch foo_${a}; done\n $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}\n\n Direct leak of 171 byte(s) in 10 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in expand_ref refs.c\n ... in repo_dwim_ref refs.c\n ... in show_rev builtin/rev-parse.c\n ... in cmd_rev_parse builtin/rev-parse.c\n ... in run_builtin git.c\n\nWe have this leak since a45d34691e (rev-parse: --abbrev-ref option to\nshorten ref name, 2009-04-13) when \"--abbrev-ref\" was introduced.\n\nLet's fix it.\n\nSigned-off-by: Rubén Justo \n---\n builtin/rev-parse.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e340..d2eb239a08 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\tcase 1: /* happy */\n-\t\t\t\tif (abbrev_ref)\n+\t\t\t\tif (abbrev_ref) {\n+\t\t\t\t\tchar *old = full;\n \t\t\t\t\tfull = shorten_unambiguous_ref(full,\n \t\t\t\t\t\tabbrev_ref_strict);\n+\t\t\t\t\tfree(old);\n+\t\t\t\t}\n \t\t\t\tshow_with_type(type, full);\n \t\t\t\tbreak;\n \t\t\tdefault: /* ambiguous */\n-- \n2.40.1\n"},{"id":"478506","messageId":"3d33cd59-d4f3-9112-457f-7d1bfdd08714@gmail.com","threadId":"59850","inReplyTo":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","subject":"[PATCH v3 2/5] branch: fix a leak in setup_tracking","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:41:08Z","receivedAt":"2023-06-17T06:42:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bdaf1dfae7 (branch: new autosetupmerge option \"simple\" for matching\nbranches, 2022-04-29) a new exit for setup_tracking() missed the\nclean-up, producing a leak.\n\n $ git config branch.autoSetupMerge simple\n $ git remote add local .\n $ git update-ref refs/remotes/local/foo HEAD\n $ git branch bar local/foo\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\n Indirect leak of 24 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in strbuf_grow strbuf.c\n ... in strbuf_add strbuf.c\n ... in match_name_with_pattern remote.c\n ... in query_refspecs remote.c\n ... in remote_find_tracking remote.c\n ... in find_tracked_branch branch.c\n ... in for_each_remote remote.c\n ... in setup_tracking branch.c\n ... in create_branch branch.c\n ... in cmd_branch builtinbranch.c\n ... in run_builtin git.c\n\nThe return introduced in bdaf1dfae7 was to avoid setting up the\ntracking, but even in that case it is still necessary to do the\nclean-up. Let's do it.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 427bde896f..d88f50a48a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\tif (!skip_prefix(tracking.srcs->items[0].string,\n \t\t\t\t \"refs/heads/\", &tracked_branch) ||\n \t\t strcmp(tracked_branch, new_ref))\n-\t\t\treturn;\n+\t\t\tgoto cleanup;\n \t}\n \n \tif (tracking.srcs->nr < 1)\n-- \n2.40.1\n"},{"id":"478507","messageId":"510e9029-c424-2861-ab3c-30046333f715@gmail.com","threadId":"59850","inReplyTo":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","subject":"[PATCH v3 3/5] branch: fix a leak in cmd_branch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:41:22Z","receivedAt":"2023-06-17T06:42:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 98e7ab6d42 (for-each-ref: delay parsing of --sort= options,\n2021-10-20) a new string_list was introduced to accumulate any\n\"branch.sort\" setting.\n\nThat string_list is cleared in ref_sorting_options(), which is only\ncalled when processing the \"--list\" sub-command. Therefore, with other\nsub-command, while having any sort option set, a leak is produced, e.g.:\n\n $ git config branch.sort invalid_sort_option\n $ git branch --edit-description\n\n Direct leak of 384 byte(s) in 1 object(s) allocated from:\n ... in xrealloc wrapper.c\n ... in string_list_append_nodup string-list.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\n Indirect leak of 20 byte(s) in 1 object(s) allocated from:\n ... in xstrdup wrapper.c\n ... in string_list_append string-list.c\n ... in git_branch_config builtin/branch.c\n ... in configset_iter config.c\n ... in repo_config config.c\n ... in git_config config.c\n ... in cmd_branch builtin/branch.c\n ... in run_builtin git.c\n\nWe don't have a common clean-up section in cmd_branch(). To avoid\nrefactoring, keep the fix simple, and while we find a better solution\nwhich hopefuly will avoid entirely that string_list, when no sort\noptions are needed; let's squelch the leak sanitizer using UNLEAK().\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..075e580d22 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (list)\n \t\tsetup_auto_pager(\"branch\", 1);\n \n+\tUNLEAK(sorting_options);\n+\n \tif (delete) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \n2.40.1\n"},{"id":"478508","messageId":"33fb45d2-5c6a-b5ad-d405-e7acb8fdbabf@gmail.com","threadId":"59850","inReplyTo":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","subject":"[PATCH v3 4/5] config: fix a leak in git_config_copy_or_rename_section_in_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:41:40Z","receivedAt":"2023-06-17T06:43:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"A branch can have its configuration spread over several configuration\nsections. This situation was already foreseen in 52d59cc645 (branch:\nadd a --copy (-c) option to go with --move (-m), 2017-06-18) when\n\"branch -c\" was introduced.\n\nUnfortunately, a leak was also introduced:\n\n $ git branch foo\n $ cat >> .git/config <\n---\n config.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.c b/config.c\nindex 39a7d7422c..207e4394a3 100644\n--- a/config.c\n+++ b/config.c\n@@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t\t\t\t\t\toutput[0] = '\\t';\n \t\t\t\t\t}\n \t\t\t\t} else {\n+\t\t\t\t\tstrbuf_release(©str);\n \t\t\t\t\tcopystr = store_create_section(new_name, &store);\n \t\t\t\t}\n \t\t\t}\n-- \n2.40.1\n"},{"id":"478509","messageId":"94d3610f-4637-5f56-9ab3-7cbd951bc773@gmail.com","threadId":"59850","inReplyTo":"71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com","subject":"[PATCH v3 5/5] tests: mark as passing with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-06-17T06:41:54Z","receivedAt":"2023-06-17T06:43:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The tests listed below, since previous commits, no longer trigger any\nleak.\n\n + t1507-rev-parse-upstream.sh\n + t1508-at-combinations.sh\n + t1514-rev-parse-push.sh\n + t2027-checkout-track.sh\n + t3200-branch.sh\n + t3204-branch-name-interpretation.sh\n + t5404-tracking-branches.sh\n + t5517-push-mirror.sh\n + t5525-fetch-tagopt.sh\n + t6040-tracking-info.sh\n + t7508-status.sh\n\nLet's mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromptly any new leak that may be introduced and triggered by them in\nthe future.\n\nSigned-off-by: Rubén Justo \n---\n t/t1507-rev-parse-upstream.sh | 1 +\n t/t1508-at-combinations.sh | 1 +\n t/t1514-rev-parse-push.sh | 1 +\n t/t2027-checkout-track.sh | 1 +\n t/t3200-branch.sh | 1 +\n t/t3204-branch-name-interpretation.sh | 1 +\n t/t5404-tracking-branches.sh | 1 +\n t/t5517-push-mirror.sh | 1 +\n t/t5525-fetch-tagopt.sh | 1 +\n t/t6040-tracking-info.sh | 1 +\n t/t7508-status.sh | 1 +\n 11 files changed, 11 insertions(+)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex cb9ef7e329..b9af6b3ac0 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -5,6 +5,7 @@ test_description='test @{upstream} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 87a4286414..e841309d0e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check() {\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex d868a08110..a835a196aa 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -4,6 +4,7 @@ test_description='test @{push} syntax'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n resolve () {\ndiff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh\nindex dca35aa3e3..a8bbc60954 100755\n--- a/t/t2027-checkout-track.sh\n+++ b/t/t2027-checkout-track.sh\n@@ -5,6 +5,7 @@ test_description='tests for git branch --track'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 98b6c8ac34..daf1666df7 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -8,6 +8,7 @@ test_description='git branch assorted tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 3399344f25..594e3e43e1 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n expect_branch() {\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex cc07889667..51737eeafe 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh\nindex a448e169bd..6d4944a728 100755\n--- a/t/t5517-push-mirror.sh\n+++ b/t/t5517-push-mirror.sh\n@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n D=$(pwd)\ndiff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh\nindex 45815f7378..3a28f1ded5 100755\n--- a/t/t5525-fetch-tagopt.sh\n+++ b/t/t5525-fetch-tagopt.sh\n@@ -2,6 +2,7 @@\n \n test_description='tagopt variable affects \"git fetch\" and is overridden by commandline.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n setup_clone () {\ndiff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh\nindex a313849406..7ddbd96e58 100755\n--- a/t/t6040-tracking-info.sh\n+++ b/t/t6040-tracking-info.sh\n@@ -5,6 +5,7 @@ test_description='remote tracking stats'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n advance () {\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex aed07c5b62..4c1f03e609 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -5,6 +5,7 @@\n \n test_description='git status'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-- \n2.40.1\n"}]}