{"thread":{"id":"54044","subject":"[PATCH 0/6] more small leak fixes","startedAt":"2020-08-14T16:13:30Z","lastAt":"2020-08-17T21:33:27Z","messageCount":20,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"403673","messageId":"20200814161328.GA153929@coredump.intra.peff.net","threadId":"54044","inReplyTo":null,"subject":"[PATCH 0/6] more small leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:13:28Z","receivedAt":"2020-08-14T16:13:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Based on the discussion over in [1], I wondered how close we were to\nbeing able to run some test scripts with a leak-checker like LSan not\ncomplaining. The answer is...not close.\n\nI picked t5526 more or less at random (it was the first failure when I\ndid a parallel \"make test\") to see what it would take to get it passing.\nAfter much effort...I have t5526.1, the setup test, running clean. :)\n\nThere were quite a few false positives, but it did actually uncover some\nlegitimate leaks. This series fixes those. I did it independently of the\nleak-fix in [2], but it would be fine to just lump it all together as\none topic.\n\n[1] https://lore.kernel.org/git/20200813155426.GA896769@coredump.intra.peff.net/\n[2] https://lore.kernel.org/git/20200814111049.GA4101811@coredump.intra.peff.net/\n\nThe patches are:\n\n  [1/6]: submodule--helper: use strbuf_release() to free strbufs\n  [2/6]: checkout: fix leak of non-existent branch names\n  [3/6]: config: fix leaks from git_config_get_string_const()\n  [4/6]: config: drop git_config_get_string_const()\n  [5/6]: config: fix leak in git_config_get_expiry_in_days()\n  [6/6]: submodule--helper: fix leak of core.worktree value\n\n Documentation/MyFirstContribution.txt |  4 +--\n apply.c                               |  4 +--\n builtin/checkout.c                    |  4 ++-\n builtin/fetch.c                       |  2 +-\n builtin/submodule--helper.c           | 16 +++++------\n cache.h                               |  4 +--\n checkout.c                            |  3 +--\n config.c                              | 39 ++++++++++++++++++++++-----\n config.h                              | 12 ++++++---\n connect.c                             |  4 +--\n editor.c                              |  2 +-\n environment.c                         |  4 +--\n help.c                                |  2 +-\n protocol.c                            |  2 +-\n submodule.c                           |  4 +--\n t/helper/test-config.c                |  2 +-\n 16 files changed, 69 insertions(+), 39 deletions(-)\n\n-Peff\n"},{"id":"403674","messageId":"20200814161414.GA595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 1/6] submodule--helper: use strbuf_release() to free strbufs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:14:14Z","receivedAt":"2020-08-14T16:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prepare_to_clone_next_submodule() function has a few local-variable\nstrbufs. We use strbuf_reset() throughout the function to reuse the\nbuffers over and over. But at the end of the function we also use\nstrbuf_reset() as they go out of scope, which means we end up leaking\ntheir heap buffers. This should be strbuf_release() instead.\n\nThese were introduced by 48308681b0 (git submodule update: have a\ndedicated helper for cloning, 2016-02-29), but it doesn't seem to have\nthe same mistake elsewhere. Likewise, I looked for other instances of\nthe pattern in the submodule--helper file but couldn't find any.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex df135abbf1..1762458345 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1747,8 +1747,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t      \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_reset(&displaypath_sb);\n-\tstrbuf_reset(&sb);\n+\tstrbuf_release(&displaypath_sb);\n+\tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\n \n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403675","messageId":"20200814161453.GB595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 2/6] checkout: fix leak of non-existent branch names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:14:53Z","receivedAt":"2020-08-14T16:14:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We unconditionally write a branch name into a newly allocated buffer in\nnew_branch_info->path, via setup_branch_path(). We then check to see if\nthe branch exists; if not, we set that field to NULL, leaking the\nmemory. We should take care to free() it when doing so.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2837195491..bba64108af 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1120,8 +1120,10 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!check_refname_format(new_branch_info->path, 0) &&\n \t    !read_ref(new_branch_info->path, &branch_rev))\n \t\toidcpy(rev, &branch_rev);\n-\telse\n+\telse {\n+\t\tfree((char *)new_branch_info->path);\n \t\tnew_branch_info->path = NULL; /* not an existing branch */\n+\t}\n \n \tnew_branch_info->commit = lookup_commit_reference_gently(the_repository, rev, 1);\n \tif (!new_branch_info->commit) {\n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403676","messageId":"20200814161736.GC595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 3/6] config: fix leaks from git_config_get_string_const()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:17:36Z","receivedAt":"2020-08-14T16:17:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two functions to get a single config string:\n\n  - git_config_get_string()\n\n  - git_config_get_string_const()\n\nOne might naively think that the first one allocates a new string and\nthe second one just points us to the internal configset storage. But\nin fact they both allocate a new copy; the second one exists only to\navoid having to cast when using it with a const global which we never\nintend to free.\n\nThe documentation for the function explains that clearly, but it seems\nI'm not alone in being surprised by this. Of 17 calls to the function,\n13 of them leak the resulting value.\n\nWe could obviously fix these by adding the appropriate free(). But it\nwould be simpler still if we actually had a non-allocating way to get\nthe string. There's git_config_get_value() but that doesn't quite do\nwhat we want. If the config key is present but is a boolean with no\nvalue (e.g., \"[foo]bar\" in the file), then we'll get NULL (whereas the\nstring versions will print an error and die).\n\nSo let's introduce a new variant, git_config_get_string_tmp(), that\nbehaves as these callers expect. We need a new name because we have new\nsemantics but the same function signature (so even if we converted the\nfour remaining callers, topics in flight might be surprised). The \"tmp\"\nis because this value should only be held onto for a short time. In\npractice it's rare for us to clear and refresh the configset,\ninvalidating the pointer, but hopefully the \"tmp\" makes callers think\nabout the lifetime. In each of the converted cases here the value only\nneeds to last within the local function or its immediate caller.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c             |  2 +-\n builtin/submodule--helper.c |  8 ++++----\n config.c                    | 30 ++++++++++++++++++++++++++++++\n config.h                    | 10 ++++++++++\n connect.c                   |  4 ++--\n editor.c                    |  2 +-\n help.c                      |  2 +-\n protocol.c                  |  2 +-\n submodule.c                 |  4 ++--\n t/helper/test-config.c      |  2 +-\n 10 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c49f0e9752..b3b77f8e2b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -645,7 +645,7 @@ static void prepare_format_display(struct ref *ref_map)\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n-\tgit_config_get_string_const(\"fetch.output\", &format);\n+\tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n \t\tcompact_format = 0;\n \telse if (!strcasecmp(format, \"compact\"))\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1762458345..e09605716e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1511,7 +1511,7 @@ static void determine_submodule_update_strategy(struct repository *r,\n \t\tif (parse_submodule_update_strategy(update, out) < 0)\n \t\t\tdie(_(\"Invalid update mode '%s' for submodule path '%s'\"),\n \t\t\t\tupdate, path);\n-\t} else if (!repo_config_get_string_const(r, key, &val)) {\n+\t} else if (!repo_config_get_string_tmp(r, key, &val)) {\n \t\tif (parse_submodule_update_strategy(val, out) < 0)\n \t\t\tdie(_(\"Invalid update mode '%s' configured for submodule path '%s'\"),\n \t\t\t\tval, path);\n@@ -1667,7 +1667,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t}\n \n \tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n-\tif (!repo_config_get_string_const(the_repository, key, &update_string)) {\n+\tif (!repo_config_get_string_tmp(the_repository, key, &update_string)) {\n \t\tupdate_type = parse_submodule_update_type(update_string);\n \t} else {\n \t\tupdate_type = sub->update_strategy.type;\n@@ -1690,7 +1690,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n+\tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n \t\tif (starts_with_dot_slash(sub->url) ||\n \t\t    starts_with_dot_dot_slash(sub->url)) {\n \t\t\turl = compute_submodule_clone_url(sub->url);\n@@ -1976,7 +1976,7 @@ static const char *remote_submodule_branch(const char *path)\n \t\treturn NULL;\n \n \tkey = xstrfmt(\"submodule.%s.branch\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, key, &branch))\n+\tif (repo_config_get_string_tmp(the_repository, key, &branch))\n \t\tbranch = sub->branch;\n \tfree(key);\n \ndiff --git a/config.c b/config.c\nindex 2b79fe76ad..2f244c67b6 100644\n--- a/config.c\n+++ b/config.c\n@@ -2020,6 +2020,20 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest\n \treturn git_configset_get_string_const(cs, key, (const char **)dest);\n }\n \n+int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n+\t\t\t\t const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\t*dest = value;\n+\t\treturn 0;\n+\t} else {\n+\t\treturn 1;\n+\t}\n+}\n+\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n {\n \tconst char *value;\n@@ -2165,6 +2179,17 @@ int repo_config_get_string(struct repository *repo,\n \treturn repo_config_get_string_const(repo, key, (const char **)dest);\n }\n \n+int repo_config_get_string_tmp(struct repository *repo,\n+\t\t\t       const char *key, const char **dest)\n+{\n+\tint ret;\n+\tgit_config_check_init(repo);\n+\tret = git_configset_get_string_tmp(repo->config, key, dest);\n+\tif (ret < 0)\n+\t\tgit_die_config(key, NULL);\n+\treturn ret;\n+}\n+\n int repo_config_get_int(struct repository *repo,\n \t\t\tconst char *key, int *dest)\n {\n@@ -2242,6 +2267,11 @@ int git_config_get_string(const char *key, char **dest)\n \treturn repo_config_get_string(the_repository, key, dest);\n }\n \n+int git_config_get_string_tmp(const char *key, const char **dest)\n+{\n+\treturn repo_config_get_string_tmp(the_repository, key, dest);\n+}\n+\n int git_config_get_int(const char *key, int *dest)\n {\n \treturn repo_config_get_int(the_repository, key, dest);\ndiff --git a/config.h b/config.h\nindex 060874488f..a75b22e0d1 100644\n--- a/config.h\n+++ b/config.h\n@@ -460,6 +460,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \n int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n+int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n@@ -478,6 +479,8 @@ int repo_config_get_string_const(struct repository *repo,\n \t\t\t\t const char *key, const char **dest);\n int repo_config_get_string(struct repository *repo,\n \t\t\t   const char *key, char **dest);\n+int repo_config_get_string_tmp(struct repository *repo,\n+\t\t\t       const char *key, const char **dest);\n int repo_config_get_int(struct repository *repo,\n \t\t\tconst char *key, int *dest);\n int repo_config_get_ulong(struct repository *repo,\n@@ -537,6 +540,13 @@ int git_config_get_string_const(const char *key, const char **dest);\n  */\n int git_config_get_string(const char *key, char **dest);\n \n+/**\n+ * Similar to `git_config_get_string_const`, but does not allocate any new\n+ * memory; on success `dest` will point to memory owned by the config\n+ * machinery, which could be invalidated if it is discarded and reloaded.\n+ */\n+int git_config_get_string_tmp(const char *key, const char **dest);\n+\n /**\n  * Finds and parses the value to an integer for the configuration variable\n  * `key`. Dies on error; otherwise, stores the value of the parsed integer in\ndiff --git a/connect.c b/connect.c\nindex 0b6aba177e..8b8f56cf6d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1052,7 +1052,7 @@ static const char *get_ssh_command(void)\n \tif ((ssh = getenv(\"GIT_SSH_COMMAND\")))\n \t\treturn ssh;\n \n-\tif (!git_config_get_string_const(\"core.sshcommand\", &ssh))\n+\tif (!git_config_get_string_tmp(\"core.sshcommand\", &ssh))\n \t\treturn ssh;\n \n \treturn NULL;\n@@ -1071,7 +1071,7 @@ static void override_ssh_variant(enum ssh_variant *ssh_variant)\n {\n \tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n \n-\tif (!variant && git_config_get_string_const(\"ssh.variant\", &variant))\n+\tif (!variant && git_config_get_string_tmp(\"ssh.variant\", &variant))\n \t\treturn;\n \n \tif (!strcmp(variant, \"auto\"))\ndiff --git a/editor.c b/editor.c\nindex 91989ee8a1..6303ae0ab0 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -40,7 +40,7 @@ const char *git_sequence_editor(void)\n \tconst char *editor = getenv(\"GIT_SEQUENCE_EDITOR\");\n \n \tif (!editor)\n-\t\tgit_config_get_string_const(\"sequence.editor\", &editor);\n+\t\tgit_config_get_string_tmp(\"sequence.editor\", &editor);\n \tif (!editor)\n \t\teditor = git_editor();\n \ndiff --git a/help.c b/help.c\nindex d478afb2af..4e2468a44d 100644\n--- a/help.c\n+++ b/help.c\n@@ -375,7 +375,7 @@ void list_cmds_by_config(struct string_list *list)\n {\n \tconst char *cmd_list;\n \n-\tif (git_config_get_string_const(\"completion.commands\", &cmd_list))\n+\tif (git_config_get_string_tmp(\"completion.commands\", &cmd_list))\n \t\treturn;\n \n \tstring_list_sort(list);\ndiff --git a/protocol.c b/protocol.c\nindex d1dd3424bb..8d964fc65e 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -21,7 +21,7 @@ enum protocol_version get_protocol_version_config(void)\n \tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n \tconst char *git_test_v;\n \n-\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n+\tif (!git_config_get_string_tmp(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n \n \t\tif (version == protocol_unknown_version)\ndiff --git a/submodule.c b/submodule.c\nindex a52b93a87f..d0b70ca536 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -194,7 +194,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\tchar *key;\n \n \t\tkey = xstrfmt(\"submodule.%s.ignore\", submodule->name);\n-\t\tif (repo_config_get_string_const(the_repository, key, &ignore))\n+\t\tif (repo_config_get_string_tmp(the_repository, key, &ignore))\n \t\t\tignore = submodule->ignore;\n \t\tfree(key);\n \n@@ -1299,7 +1299,7 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \n \t\tint fetch_recurse = submodule->fetch_recurse;\n \t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n-\t\tif (!repo_config_get_string_const(spf->r, key, &value)) {\n+\t\tif (!repo_config_get_string_tmp(spf->r, key, &value)) {\n \t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n \t\t}\n \t\tfree(key);\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 234c722b48..a6e936721f 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -126,7 +126,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tgoto exit1;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n-\t\tif (!git_config_get_string_const(argv[2], &v)) {\n+\t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n \t\t\tgoto exit0;\n \t\t} else {\n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403677","messageId":"20200814161921.GD595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 4/6] config: drop git_config_get_string_const()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:19:21Z","receivedAt":"2020-08-14T16:19:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As evidenced by the leak fixes in the previous commit, the \"const\" in\ngit_config_get_string_const() clearly misleads people into thinking that\nit does not allocate a copy of the string. We can fix this by renaming\nit, but it's easier still to just drop it. Of the four remaining\ncallers:\n\n  - The one in git_config_parse_expiry() still needs to allocate, since\n    that's what its callers expect. We can just use the non-const\n    version and cast our pointer. Slightly ugly, but the damage is\n    contained in one spot.\n\n  - The two in apply are writing to global \"const char *\" variables, and\n    need to continue allocating. We often mark these as const because we\n    assign default string literals to them. But in this case we don't do\n    that, so we can just declare them as real \"char *\" pointers and use\n    the non-const version.\n\n  - The call in checkout doesn't actually need a copy; it can just use\n    the non-allocating \"tmp\" version of the function.\n\nThe function is also mentioned in the MyFirstContribution document. We\ncan swap that call out for the non-allocating \"tmp\" variant, which fits\nwell in the example given.\n\nNote that this frees up the \"const\" name, so we could rename the \"tmp\"\nvariant back to that. But let's give some time for topics in flight to\nadapt to the new code before doing so (if we do it too soon, the\nfunction semantics will change but the compiler won't alert us).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/MyFirstContribution.txt | 4 ++--\n apply.c                               | 4 ++--\n cache.h                               | 4 ++--\n checkout.c                            | 3 +--\n config.c                              | 7 +------\n config.h                              | 8 +-------\n environment.c                         | 4 ++--\n 7 files changed, 11 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt\nindex d85c9b5143..4f85a089ef 100644\n--- a/Documentation/MyFirstContribution.txt\n+++ b/Documentation/MyFirstContribution.txt\n@@ -319,14 +319,14 @@ function body:\n ...\n \n \tgit_config(git_default_config, NULL);\n-\tif (git_config_get_string_const(\"user.name\", &cfg_name) > 0)\n+\tif (git_config_get_string_tmp(\"user.name\", &cfg_name) > 0)\n \t\tprintf(_(\"No name is found in config\\n\"));\n \telse\n \t\tprintf(_(\"Your name: %s\\n\"), cfg_name);\n ----\n \n `git_config()` will grab the configuration from config files known to Git and\n-apply standard precedence rules. `git_config_get_string_const()` will look up\n+apply standard precedence rules. `git_config_get_string_tmp()` will look up\n a specific key (\"user.name\") and give you the value. There are a number of\n single-key lookup functions like this one; you can see them all (and more info\n about how to use `git_config()`) in `Documentation/technical/api-config.txt`.\ndiff --git a/apply.c b/apply.c\nindex 402d80602a..2839dae029 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -30,8 +30,8 @@ struct gitdiff_data {\n \n static void git_apply_config(void)\n {\n-\tgit_config_get_string_const(\"apply.whitespace\", &apply_default_whitespace);\n-\tgit_config_get_string_const(\"apply.ignorewhitespace\", &apply_default_ignorewhitespace);\n+\tgit_config_get_string(\"apply.whitespace\", &apply_default_whitespace);\n+\tgit_config_get_string(\"apply.ignorewhitespace\", &apply_default_ignorewhitespace);\n \tgit_config(git_xmerge_config, NULL);\n }\n \ndiff --git a/cache.h b/cache.h\nindex 0290849c19..4cad61ffa4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -921,8 +921,8 @@ extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n-extern const char *apply_default_whitespace;\n-extern const char *apply_default_ignorewhitespace;\n+extern char *apply_default_whitespace;\n+extern char *apply_default_ignorewhitespace;\n extern const char *git_attributes_file;\n extern const char *git_hooks_path;\n extern int zlib_compression_level;\ndiff --git a/checkout.c b/checkout.c\nindex c72e9f9773..6586e30ca5 100644\n--- a/checkout.c\n+++ b/checkout.c\n@@ -47,15 +47,14 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,\n {\n \tstruct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;\n \tconst char *default_remote = NULL;\n-\tif (!git_config_get_string_const(\"checkout.defaultremote\", &default_remote))\n+\tif (!git_config_get_string_tmp(\"checkout.defaultremote\", &default_remote))\n \t\tcb_data.default_remote = default_remote;\n \tcb_data.src_ref = xstrfmt(\"refs/heads/%s\", name);\n \tcb_data.dst_oid = oid;\n \tfor_each_remote(check_tracking_name, &cb_data);\n \tif (dwim_remotes_matched)\n \t\t*dwim_remotes_matched = cb_data.num_matches;\n \tfree(cb_data.src_ref);\n-\tfree((char *)default_remote);\n \tif (cb_data.num_matches == 1) {\n \t\tfree(cb_data.default_dst_ref);\n \t\tfree(cb_data.default_dst_oid);\ndiff --git a/config.c b/config.c\nindex 2f244c67b6..8bb1945aa9 100644\n--- a/config.c\n+++ b/config.c\n@@ -2257,11 +2257,6 @@ const struct string_list *git_config_get_value_multi(const char *key)\n \treturn repo_config_get_value_multi(the_repository, key);\n }\n \n-int git_config_get_string_const(const char *key, const char **dest)\n-{\n-\treturn repo_config_get_string_const(the_repository, key, dest);\n-}\n-\n int git_config_get_string(const char *key, char **dest)\n {\n \treturn repo_config_get_string(the_repository, key, dest);\n@@ -2304,7 +2299,7 @@ int git_config_get_pathname(const char *key, const char **dest)\n \n int git_config_get_expiry(const char *key, const char **output)\n {\n-\tint ret = git_config_get_string_const(key, output);\n+\tint ret = git_config_get_string(key, (char **)output);\n \tif (ret)\n \t\treturn ret;\n \tif (strcmp(*output, \"now\")) {\ndiff --git a/config.h b/config.h\nindex a75b22e0d1..d4d22c34c6 100644\n--- a/config.h\n+++ b/config.h\n@@ -532,16 +532,10 @@ void git_config_clear(void);\n  * error message and returns -1. When the configuration variable `key` is\n  * not found, returns 1 without touching `dest`.\n  */\n-int git_config_get_string_const(const char *key, const char **dest);\n-\n-/**\n- * Similar to `git_config_get_string_const`, except that retrieved value\n- * copied into the `dest` parameter is a mutable string.\n- */\n int git_config_get_string(const char *key, char **dest);\n \n /**\n- * Similar to `git_config_get_string_const`, but does not allocate any new\n+ * Similar to `git_config_get_string`, but does not allocate any new\n  * memory; on success `dest` will point to memory owned by the config\n  * machinery, which could be invalidated if it is discarded and reloaded.\n  */\ndiff --git a/environment.c b/environment.c\nindex 52e0c979ba..bb518c61cd 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,8 +35,8 @@ int repository_format_precious_objects;\n int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n-const char *apply_default_whitespace;\n-const char *apply_default_ignorewhitespace;\n+char *apply_default_whitespace;\n+char *apply_default_ignorewhitespace;\n const char *git_attributes_file;\n const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403678","messageId":"20200814161946.GE595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 5/6] config: fix leak in git_config_get_expiry_in_days()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:19:46Z","receivedAt":"2020-08-14T16:19:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use git_config_get_string() to retrieve the expiry value in a newly\nallocated string. But after parsing it, we never free it, leaking the\nmemory.\n\nWe could fix this with a free() obviously, but there's an even better\nsolution: we can use the non-allocating \"tmp\" variant of the function;\nwe only need it to be valid for the lifetime of our parse function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 8bb1945aa9..968ef28e5b 100644\n--- a/config.c\n+++ b/config.c\n@@ -2312,11 +2312,11 @@ int git_config_get_expiry(const char *key, const char **output)\n \n int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now)\n {\n-\tchar *expiry_string;\n+\tconst char *expiry_string;\n \tintmax_t days;\n \ttimestamp_t when;\n \n-\tif (git_config_get_string(key, &expiry_string))\n+\tif (git_config_get_string_tmp(key, &expiry_string))\n \t\treturn 1; /* no such thing */\n \n \tif (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {\n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403679","messageId":"20200814162035.GF595698@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH 6/6] submodule--helper: fix leak of core.worktree value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:20:35Z","receivedAt":"2020-08-14T16:20:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the ensure_core_worktree() function, we load the core.worktree value\nof the submodule repository using repo_config_get_string(). This\nfunction copies the string, but we never free it, leaking the memory.\n\nWe can instead use the \"tmp\" version of that function to avoid the\nallocation at all. We don't have to worry about lifetime issues, since\nwe never even look at the value (we just want to know if it's set).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e09605716e..a59d8e4bda 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2101,7 +2101,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n \tconst char *path;\n-\tchar *cw;\n+\tconst char *cw;\n \tstruct repository subrepo;\n \n \tif (argc != 2)\n@@ -2116,7 +2116,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tif (repo_submodule_init(&subrepo, the_repository, sub))\n \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n \n-\tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\n+\tif (!repo_config_get_string_tmp(&subrepo, \"core.worktree\", &cw)) {\n \t\tchar *cfg_file, *abs_path;\n \t\tconst char *rel_path;\n \t\tstruct strbuf sb = STRBUF_INIT;\n-- \n2.28.0.596.g9c08d63829\n"},{"id":"403681","messageId":"20200814162525.GA595840@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] more small leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:25:25Z","receivedAt":"2020-08-14T16:25:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 14, 2020 at 12:13:28PM -0400, Jeff King wrote:\n\n> Based on the discussion over in [1], I wondered how close we were to\n> being able to run some test scripts with a leak-checker like LSan not\n> complaining. The answer is...not close.\n> \n> I picked t5526 more or less at random (it was the first failure when I\n> did a parallel \"make test\") to see what it would take to get it passing.\n> After much effort...I have t5526.1, the setup test, running clean. :)\n\nFor reference, here are the UNLEAK() calls I had to add to silence the\nfalse positives. Some of these are kind of sketchy. For example,\ndeclaring that wt_status_collect_changes_index() is allowed to leak is a\nbit low-level for my tastes (in general it's probably only called once\nper program, but it's getting quite far from the bottom of the stack).\nBut there's actually no convenient way to free the various bits of a\nrev_info, so marking it with UNLEAK() is an expedient hack.\n\n---\n builtin/clone.c             | 9 +++++++++\n builtin/init-db.c           | 1 +\n builtin/rev-list.c          | 1 +\n builtin/submodule--helper.c | 4 ++++\n parse-options.c             | 1 +\n wt-status.c                 | 3 +++\n 6 files changed, 19 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..b2578c3c50 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -998,6 +998,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n+\tUNLEAK(path);\n \tif (path)\n \t\trepo = absolute_pathdup(repo_name);\n \telse if (strchr(repo_name, ':')) {\n@@ -1047,6 +1048,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\twork_tree = dir;\n \t\tgit_dir = mkpathdup(\"%s/.git\", dir);\n \t}\n+\tUNLEAK(git_dir);\n \n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n@@ -1166,6 +1168,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport->family = family;\n \n \tpath = get_repo_path(remote->url[0], &is_bundle);\n+\tUNLEAK(path);\n \tis_local = option_local != 0 && path && !is_bundle;\n \tif (is_local) {\n \t\tif (option_depth)\n@@ -1336,5 +1339,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n \tstrvec_clear(&ref_prefixes);\n+\n+\tUNLEAK(remote_head_points_at);\n+\tUNLEAK(refs);\n+\tUNLEAK(mapped_refs);\n+\tUNLEAK(repo);\n+\n \treturn err;\n }\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex f70076d38e..04087ed7e6 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -665,6 +665,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \tUNLEAK(real_git_dir);\n \tUNLEAK(git_dir);\n \tUNLEAK(work_tree);\n+\tUNLEAK(template_dir);\n \n \tflags |= INIT_DB_EXIST_OK;\n \treturn init_db(git_dir, real_git_dir, template_dir, hash_algo,\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f520111eda..71c92f6747 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -506,6 +506,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\trevs.do_not_die_on_missing_tree = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n+\tUNLEAK(revs);\n \n \tmemset(&info, 0, sizeof(info));\n \tinfo.revs = &revs;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a59d8e4bda..850c8ff966 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -734,6 +734,7 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \t\tinfo.flags |= OPT_QUIET;\n \n \tfor_each_listed_submodule(&list, init_submodule_cb, &info);\n+\tUNLEAK(list);\n \n \treturn 0;\n }\n@@ -1874,6 +1875,8 @@ static int update_submodules(struct submodule_update_clone *suc)\n \t\t\t\t   update_clone_task_finished, suc, \"submodule\",\n \t\t\t\t   \"parallel/update\");\n \n+\tUNLEAK(*suc);\n+\n \t/*\n \t * We saved the output and put it out all at once now.\n \t * That means:\n@@ -2133,6 +2136,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&sb);\n \t}\n \n+\trepo_clear(&subrepo);\n \treturn 0;\n }\n \ndiff --git a/parse-options.c b/parse-options.c\nindex c57618d537..b87cb3d70a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -675,6 +675,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n \t\t\tnewopt[i].short_name = short_name;\n \t\t\tnewopt[i].long_name = long_name;\n \t\t\tnewopt[i].help = strbuf_detach(&help, NULL);\n+\t\t\tUNLEAK(newopt[i].help);\n \t\t\tbreak;\n \t\t}\n \ndiff --git a/wt-status.c b/wt-status.c\nindex d75399085d..7f605958f2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -616,6 +616,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n+\tUNLEAK(rev);\n }\n \n static void wt_status_collect_changes_index(struct wt_status *s)\n@@ -627,6 +628,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = s->is_initial ? empty_tree_oid_hex() : s->reference;\n \tsetup_revisions(0, NULL, &rev, &opt);\n+\tUNLEAK(rev);\n \n \trev.diffopt.flags.override_submodule_config = 1;\n \trev.diffopt.ita_invisible_in_index = 1;\n@@ -652,6 +654,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n+\tUNLEAK(rev);\n }\n \n static void wt_status_collect_changes_initial(struct wt_status *s)\n-- \n2.28.0.596.g9c08d63829\n\n"},{"id":"403682","messageId":"20200814162749.GB595840@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814162525.GA595840@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] more small leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T16:27:49Z","receivedAt":"2020-08-14T16:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 14, 2020 at 12:25:25PM -0400, Jeff King wrote:\n\n> On Fri, Aug 14, 2020 at 12:13:28PM -0400, Jeff King wrote:\n> \n> > Based on the discussion over in [1], I wondered how close we were to\n> > being able to run some test scripts with a leak-checker like LSan not\n> > complaining. The answer is...not close.\n> > \n> > I picked t5526 more or less at random (it was the first failure when I\n> > did a parallel \"make test\") to see what it would take to get it passing.\n> > After much effort...I have t5526.1, the setup test, running clean. :)\n> \n> For reference, here are the UNLEAK() calls I had to add to silence the\n> false positives. Some of these are kind of sketchy. For example,\n> declaring that wt_status_collect_changes_index() is allowed to leak is a\n> bit low-level for my tastes (in general it's probably only called once\n> per program, but it's getting quite far from the bottom of the stack).\n> But there's actually no convenient way to free the various bits of a\n> rev_info, so marking it with UNLEAK() is an expedient hack.\n\nAnd by the way, having gone through this exercise, I'm re-convinced that\nUNLEAK() is reasonable to keep. A lot of these cases would have been\nvery awkward with free(). Not insurmountable, but cases where a variable\nis sometimes-allocated and sometimes-not get rather tricky.\n\n-Peff\n"},{"id":"403711","messageId":"xmqq364pvv03.fsf@gitster.c.googlers.com","threadId":"54044","inReplyTo":"20200814161921.GD595698@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] config: drop git_config_get_string_const()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-14T20:21:32Z","receivedAt":"2020-08-14T20:21:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> As evidenced by the leak fixes in the previous commit, the \"const\" in\n> git_config_get_string_const() clearly misleads people into thinking that\n> it does not allocate a copy of the string. We can fix this by renaming\n> it, but it's easier still to just drop it.\n\nThis turns out to be a bit more painful than I imagined.\n\nDo we want to do the same with repo_config_get_string_const(), by\nthe way?  Which would open the wound even wider, but may be worth\ndoing for consistency.\n\n"},{"id":"403780","messageId":"20200815063437.GA619381@coredump.intra.peff.net","threadId":"54044","inReplyTo":"xmqq364pvv03.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/6] config: drop git_config_get_string_const()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-15T06:34:37Z","receivedAt":"2020-08-15T23:23:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 14, 2020 at 01:21:32PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > As evidenced by the leak fixes in the previous commit, the \"const\" in\n> > git_config_get_string_const() clearly misleads people into thinking that\n> > it does not allocate a copy of the string. We can fix this by renaming\n> > it, but it's easier still to just drop it.\n> \n> This turns out to be a bit more painful than I imagined.\n> \n> Do we want to do the same with repo_config_get_string_const(), by\n> the way?  Which would open the wound even wider, but may be worth\n> doing for consistency.\n\nWhoops, I actually meant to do so (and made sure we didn't have any\ncallers) but forgot to actually include it in patch 4. It doesn't make\nany sense to keep those other variants.\n\nThe diff is pretty straightforward (see below). I'll include it in a\nre-roll (squashed into patch 4), but will wait for any other comments.\n\nIt doesn't look like there's any extra fallout from merging this with\n\"seen\".\n\ndiff --git a/config.c b/config.c\nindex 8bb1945aa9..f0367c76ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -2006,20 +2006,15 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n \treturn e ? &e->value_list : NULL;\n }\n \n-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n {\n \tconst char *value;\n \tif (!git_configset_get_value(cs, key, &value))\n-\t\treturn git_config_string(dest, key, value);\n+\t\treturn git_config_string((const char **)dest, key, value);\n \telse\n \t\treturn 1;\n }\n \n-int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n-{\n-\treturn git_configset_get_string_const(cs, key, (const char **)dest);\n-}\n-\n int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n \t\t\t\t const char **dest)\n {\n@@ -2161,24 +2156,17 @@ const struct string_list *repo_config_get_value_multi(struct repository *repo,\n \treturn git_configset_get_value_multi(repo->config, key);\n }\n \n-int repo_config_get_string_const(struct repository *repo,\n-\t\t\t\t const char *key, const char **dest)\n+int repo_config_get_string(struct repository *repo,\n+\t\t\t   const char *key, char **dest)\n {\n \tint ret;\n \tgit_config_check_init(repo);\n-\tret = git_configset_get_string_const(repo->config, key, dest);\n+\tret = git_configset_get_string(repo->config, key, dest);\n \tif (ret < 0)\n \t\tgit_die_config(key, NULL);\n \treturn ret;\n }\n \n-int repo_config_get_string(struct repository *repo,\n-\t\t\t   const char *key, char **dest)\n-{\n-\tgit_config_check_init(repo);\n-\treturn repo_config_get_string_const(repo, key, (const char **)dest);\n-}\n-\n int repo_config_get_string_tmp(struct repository *repo,\n \t\t\t       const char *key, const char **dest)\n {\ndiff --git a/config.h b/config.h\nindex d4d22c34c6..91cdfbfb41 100644\n--- a/config.h\n+++ b/config.h\n@@ -458,7 +458,6 @@ void git_configset_clear(struct config_set *cs);\n  */\n int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n \n-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n@@ -475,8 +474,6 @@ int repo_config_get_value(struct repository *repo,\n \t\t\t  const char *key, const char **value);\n const struct string_list *repo_config_get_value_multi(struct repository *repo,\n \t\t\t\t\t\t      const char *key);\n-int repo_config_get_string_const(struct repository *repo,\n-\t\t\t\t const char *key, const char **dest);\n int repo_config_get_string(struct repository *repo,\n \t\t\t   const char *key, char **dest);\n int repo_config_get_string_tmp(struct repository *repo,\n"},{"id":"403845","messageId":"xmqqft8l2mzn.fsf@gitster.c.googlers.com","threadId":"54044","inReplyTo":"20200815063437.GA619381@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] config: drop git_config_get_string_const()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-17T17:36:28Z","receivedAt":"2020-08-17T17:37:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Whoops, I actually meant to do so (and made sure we didn't have any\n> callers) but forgot to actually include it in patch 4. It doesn't make\n> any sense to keep those other variants.\n>\n> The diff is pretty straightforward (see below). I'll include it in a\n> re-roll (squashed into patch 4), but will wait for any other comments.\n>\n> It doesn't look like there's any extra fallout from merging this with\n> \"seen\".\n\nOK, thanks.\n\n>\n> diff --git a/config.c b/config.c\n> index 8bb1945aa9..f0367c76ad 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2006,20 +2006,15 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n>  \treturn e ? &e->value_list : NULL;\n>  }\n>  \n> -int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n> +int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n>  {\n>  \tconst char *value;\n>  \tif (!git_configset_get_value(cs, key, &value))\n> -\t\treturn git_config_string(dest, key, value);\n> +\t\treturn git_config_string((const char **)dest, key, value);\n>  \telse\n>  \t\treturn 1;\n>  }\n>  \n> -int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n> -{\n> -\treturn git_configset_get_string_const(cs, key, (const char **)dest);\n> -}\n> -\n>  int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n>  \t\t\t\t const char **dest)\n>  {\n> @@ -2161,24 +2156,17 @@ const struct string_list *repo_config_get_value_multi(struct repository *repo,\n>  \treturn git_configset_get_value_multi(repo->config, key);\n>  }\n>  \n> -int repo_config_get_string_const(struct repository *repo,\n> -\t\t\t\t const char *key, const char **dest)\n> +int repo_config_get_string(struct repository *repo,\n> +\t\t\t   const char *key, char **dest)\n>  {\n>  \tint ret;\n>  \tgit_config_check_init(repo);\n> -\tret = git_configset_get_string_const(repo->config, key, dest);\n> +\tret = git_configset_get_string(repo->config, key, dest);\n>  \tif (ret < 0)\n>  \t\tgit_die_config(key, NULL);\n>  \treturn ret;\n>  }\n>  \n> -int repo_config_get_string(struct repository *repo,\n> -\t\t\t   const char *key, char **dest)\n> -{\n> -\tgit_config_check_init(repo);\n> -\treturn repo_config_get_string_const(repo, key, (const char **)dest);\n> -}\n> -\n>  int repo_config_get_string_tmp(struct repository *repo,\n>  \t\t\t       const char *key, const char **dest)\n>  {\n> diff --git a/config.h b/config.h\n> index d4d22c34c6..91cdfbfb41 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -458,7 +458,6 @@ void git_configset_clear(struct config_set *cs);\n>   */\n>  int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n>  \n> -int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n>  int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n>  int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n> @@ -475,8 +474,6 @@ int repo_config_get_value(struct repository *repo,\n>  \t\t\t  const char *key, const char **value);\n>  const struct string_list *repo_config_get_value_multi(struct repository *repo,\n>  \t\t\t\t\t\t      const char *key);\n> -int repo_config_get_string_const(struct repository *repo,\n> -\t\t\t\t const char *key, const char **dest);\n>  int repo_config_get_string(struct repository *repo,\n>  \t\t\t   const char *key, char **dest);\n>  int repo_config_get_string_tmp(struct repository *repo,\n"},{"id":"403888","messageId":"20200817213228.GA1854603@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200814161328.GA153929@coredump.intra.peff.net","subject":"[PATCH v2 0/7] leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:32:28Z","receivedAt":"2020-08-17T21:32:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 14, 2020 at 12:13:28PM -0400, Jeff King wrote:\n\n> There were quite a few false positives, but it did actually uncover some\n> legitimate leaks. This series fixes those. I did it independently of the\n> leak-fix in [2], but it would be fine to just lump it all together as\n> one topic.\n\nHere's a re-roll that drops the repo_config_get_string_const() and\ngit_configset_get_string_const() helpers in patch 4, as noticed by\nJunio. That's the only patch with changes (range-diff below).\n\n(I also rolled in the earlier leak-fix as \"patch 1\", which matches what\ngot queued in jk/leakfix).\n\n  [1/7]: clear_pattern_list(): clear embedded hashmaps\n  [2/7]: submodule--helper: use strbuf_release() to free strbufs\n  [3/7]: checkout: fix leak of non-existent branch names\n  [4/7]: config: fix leaks from git_config_get_string_const()\n  [5/7]: config: drop git_config_get_string_const()\n  [6/7]: config: fix leak in git_config_get_expiry_in_days()\n  [7/7]: submodule--helper: fix leak of core.worktree value\n\n Documentation/MyFirstContribution.txt |  4 +--\n apply.c                               |  4 +--\n builtin/checkout.c                    |  4 ++-\n builtin/fetch.c                       |  2 +-\n builtin/submodule--helper.c           | 16 ++++-----\n cache.h                               |  4 +--\n checkout.c                            |  3 +-\n config.c                              | 47 +++++++++++++++++----------\n config.h                              | 15 +++++----\n connect.c                             |  4 +--\n dir.c                                 |  2 ++\n editor.c                              |  2 +-\n environment.c                         |  4 +--\n help.c                                |  2 +-\n protocol.c                            |  2 +-\n submodule.c                           |  4 +--\n t/helper/test-config.c                |  2 +-\n 17 files changed, 69 insertions(+), 52 deletions(-)\n\n1:  7a878fa4d3 = 1:  24248d0203 clear_pattern_list(): clear embedded hashmaps\n2:  84d8edd867 = 2:  5bdbb11601 submodule--helper: use strbuf_release() to free strbufs\n3:  76ae8ffb83 = 3:  44f3539461 checkout: fix leak of non-existent branch names\n4:  e8f85a47ff = 4:  b1b5444ca1 config: fix leaks from git_config_get_string_const()\n5:  9875ddeab3 ! 5:  a82f9724b6 config: drop git_config_get_string_const()\n    @@ Commit message\n         can swap that call out for the non-allocating \"tmp\" variant, which fits\n         well in the example given.\n     \n    +    We'll drop the \"configset\" and \"repo\" variants, as well (which are\n    +    unused).\n    +\n         Note that this frees up the \"const\" name, so we could rename the \"tmp\"\n         variant back to that. But let's give some time for topics in flight to\n         adapt to the new code before doing so (if we do it too soon, the\n    @@ checkout.c: const char *unique_tracking_name(const char *name, struct object_id\n      \t\tfree(cb_data.default_dst_oid);\n     \n      ## config.c ##\n    +@@ config.c: const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n    + \treturn e ? &e->value_list : NULL;\n    + }\n    + \n    +-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n    ++int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n    + {\n    + \tconst char *value;\n    + \tif (!git_configset_get_value(cs, key, &value))\n    +-\t\treturn git_config_string(dest, key, value);\n    ++\t\treturn git_config_string((const char **)dest, key, value);\n    + \telse\n    + \t\treturn 1;\n    + }\n    + \n    +-int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n    +-{\n    +-\treturn git_configset_get_string_const(cs, key, (const char **)dest);\n    +-}\n    +-\n    + int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n    + \t\t\t\t const char **dest)\n    + {\n    +@@ config.c: const struct string_list *repo_config_get_value_multi(struct repository *repo,\n    + \treturn git_configset_get_value_multi(repo->config, key);\n    + }\n    + \n    +-int repo_config_get_string_const(struct repository *repo,\n    +-\t\t\t\t const char *key, const char **dest)\n    ++int repo_config_get_string(struct repository *repo,\n    ++\t\t\t   const char *key, char **dest)\n    + {\n    + \tint ret;\n    + \tgit_config_check_init(repo);\n    +-\tret = git_configset_get_string_const(repo->config, key, dest);\n    ++\tret = git_configset_get_string(repo->config, key, dest);\n    + \tif (ret < 0)\n    + \t\tgit_die_config(key, NULL);\n    + \treturn ret;\n    + }\n    + \n    +-int repo_config_get_string(struct repository *repo,\n    +-\t\t\t   const char *key, char **dest)\n    +-{\n    +-\tgit_config_check_init(repo);\n    +-\treturn repo_config_get_string_const(repo, key, (const char **)dest);\n    +-}\n    +-\n    + int repo_config_get_string_tmp(struct repository *repo,\n    + \t\t\t       const char *key, const char **dest)\n    + {\n     @@ config.c: const struct string_list *git_config_get_value_multi(const char *key)\n      \treturn repo_config_get_value_multi(the_repository, key);\n      }\n    @@ config.c: int git_config_get_pathname(const char *key, const char **dest)\n      \tif (strcmp(*output, \"now\")) {\n     \n      ## config.h ##\n    +@@ config.h: void git_configset_clear(struct config_set *cs);\n    +  */\n    + int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n    + \n    +-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n    + int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n    + int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n    + int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n    +@@ config.h: int repo_config_get_value(struct repository *repo,\n    + \t\t\t  const char *key, const char **value);\n    + const struct string_list *repo_config_get_value_multi(struct repository *repo,\n    + \t\t\t\t\t\t      const char *key);\n    +-int repo_config_get_string_const(struct repository *repo,\n    +-\t\t\t\t const char *key, const char **dest);\n    + int repo_config_get_string(struct repository *repo,\n    + \t\t\t   const char *key, char **dest);\n    + int repo_config_get_string_tmp(struct repository *repo,\n     @@ config.h: void git_config_clear(void);\n       * error message and returns -1. When the configuration variable `key` is\n       * not found, returns 1 without touching `dest`.\n6:  ee69df55de = 6:  7eefd80257 config: fix leak in git_config_get_expiry_in_days()\n7:  c4fd15a2ac = 7:  feb66301fc submodule--helper: fix leak of core.worktree value\n"},{"id":"403889","messageId":"20200817213302.GA1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 1/7] clear_pattern_list(): clear embedded hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:02Z","receivedAt":"2020-08-17T21:33:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,\n2019-11-21) added some auxiliary hashmaps to the pattern_list struct,\nbut they're leaked when clear_pattern_list() is called.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex fe64be30ed..9411b94e9b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n \tfree(pl->filebuf);\n+\thashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n+\thashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n \n \tmemset(pl, 0, sizeof(*pl));\n }\n-- \n2.28.0.605.g35fde94f44\n\n"},{"id":"403890","messageId":"20200817213304.GB1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 2/7] submodule--helper: use strbuf_release() to free strbufs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:04Z","receivedAt":"2020-08-17T21:33:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prepare_to_clone_next_submodule() function has a few local-variable\nstrbufs. We use strbuf_reset() throughout the function to reuse the\nbuffers over and over. But at the end of the function we also use\nstrbuf_reset() as they go out of scope, which means we end up leaking\ntheir heap buffers. This should be strbuf_release() instead.\n\nThese were introduced by 48308681b0 (git submodule update: have a\ndedicated helper for cloning, 2016-02-29), but it doesn't seem to have\nthe same mistake elsewhere. Likewise, I looked for other instances of\nthe pattern in the submodule--helper file but couldn't find any.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex df135abbf1..1762458345 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1747,8 +1747,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t      \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_reset(&displaypath_sb);\n-\tstrbuf_reset(&sb);\n+\tstrbuf_release(&displaypath_sb);\n+\tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\n \n-- \n2.28.0.605.g35fde94f44\n\n"},{"id":"403891","messageId":"20200817213306.GC1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 3/7] checkout: fix leak of non-existent branch names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:06Z","receivedAt":"2020-08-17T21:33:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We unconditionally write a branch name into a newly allocated buffer in\nnew_branch_info->path, via setup_branch_path(). We then check to see if\nthe branch exists; if not, we set that field to NULL, leaking the\nmemory. We should take care to free() it when doing so.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2837195491..bba64108af 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1120,8 +1120,10 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!check_refname_format(new_branch_info->path, 0) &&\n \t    !read_ref(new_branch_info->path, &branch_rev))\n \t\toidcpy(rev, &branch_rev);\n-\telse\n+\telse {\n+\t\tfree((char *)new_branch_info->path);\n \t\tnew_branch_info->path = NULL; /* not an existing branch */\n+\t}\n \n \tnew_branch_info->commit = lookup_commit_reference_gently(the_repository, rev, 1);\n \tif (!new_branch_info->commit) {\n-- \n2.28.0.605.g35fde94f44\n\n"},{"id":"403892","messageId":"20200817213308.GD1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 4/7] config: fix leaks from git_config_get_string_const()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:08Z","receivedAt":"2020-08-17T21:33:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two functions to get a single config string:\n\n  - git_config_get_string()\n\n  - git_config_get_string_const()\n\nOne might naively think that the first one allocates a new string and\nthe second one just points us to the internal configset storage. But\nin fact they both allocate a new copy; the second one exists only to\navoid having to cast when using it with a const global which we never\nintend to free.\n\nThe documentation for the function explains that clearly, but it seems\nI'm not alone in being surprised by this. Of 17 calls to the function,\n13 of them leak the resulting value.\n\nWe could obviously fix these by adding the appropriate free(). But it\nwould be simpler still if we actually had a non-allocating way to get\nthe string. There's git_config_get_value() but that doesn't quite do\nwhat we want. If the config key is present but is a boolean with no\nvalue (e.g., \"[foo]bar\" in the file), then we'll get NULL (whereas the\nstring versions will print an error and die).\n\nSo let's introduce a new variant, git_config_get_string_tmp(), that\nbehaves as these callers expect. We need a new name because we have new\nsemantics but the same function signature (so even if we converted the\nfour remaining callers, topics in flight might be surprised). The \"tmp\"\nis because this value should only be held onto for a short time. In\npractice it's rare for us to clear and refresh the configset,\ninvalidating the pointer, but hopefully the \"tmp\" makes callers think\nabout the lifetime. In each of the converted cases here the value only\nneeds to last within the local function or its immediate caller.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c             |  2 +-\n builtin/submodule--helper.c |  8 ++++----\n config.c                    | 30 ++++++++++++++++++++++++++++++\n config.h                    | 10 ++++++++++\n connect.c                   |  4 ++--\n editor.c                    |  2 +-\n help.c                      |  2 +-\n protocol.c                  |  2 +-\n submodule.c                 |  4 ++--\n t/helper/test-config.c      |  2 +-\n 10 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c49f0e9752..b3b77f8e2b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -645,7 +645,7 @@ static void prepare_format_display(struct ref *ref_map)\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n-\tgit_config_get_string_const(\"fetch.output\", &format);\n+\tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n \t\tcompact_format = 0;\n \telse if (!strcasecmp(format, \"compact\"))\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1762458345..e09605716e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1511,7 +1511,7 @@ static void determine_submodule_update_strategy(struct repository *r,\n \t\tif (parse_submodule_update_strategy(update, out) < 0)\n \t\t\tdie(_(\"Invalid update mode '%s' for submodule path '%s'\"),\n \t\t\t\tupdate, path);\n-\t} else if (!repo_config_get_string_const(r, key, &val)) {\n+\t} else if (!repo_config_get_string_tmp(r, key, &val)) {\n \t\tif (parse_submodule_update_strategy(val, out) < 0)\n \t\t\tdie(_(\"Invalid update mode '%s' configured for submodule path '%s'\"),\n \t\t\t\tval, path);\n@@ -1667,7 +1667,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t}\n \n \tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n-\tif (!repo_config_get_string_const(the_repository, key, &update_string)) {\n+\tif (!repo_config_get_string_tmp(the_repository, key, &update_string)) {\n \t\tupdate_type = parse_submodule_update_type(update_string);\n \t} else {\n \t\tupdate_type = sub->update_strategy.type;\n@@ -1690,7 +1690,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n+\tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n \t\tif (starts_with_dot_slash(sub->url) ||\n \t\t    starts_with_dot_dot_slash(sub->url)) {\n \t\t\turl = compute_submodule_clone_url(sub->url);\n@@ -1976,7 +1976,7 @@ static const char *remote_submodule_branch(const char *path)\n \t\treturn NULL;\n \n \tkey = xstrfmt(\"submodule.%s.branch\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, key, &branch))\n+\tif (repo_config_get_string_tmp(the_repository, key, &branch))\n \t\tbranch = sub->branch;\n \tfree(key);\n \ndiff --git a/config.c b/config.c\nindex 2b79fe76ad..2f244c67b6 100644\n--- a/config.c\n+++ b/config.c\n@@ -2020,6 +2020,20 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest\n \treturn git_configset_get_string_const(cs, key, (const char **)dest);\n }\n \n+int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n+\t\t\t\t const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\t*dest = value;\n+\t\treturn 0;\n+\t} else {\n+\t\treturn 1;\n+\t}\n+}\n+\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n {\n \tconst char *value;\n@@ -2165,6 +2179,17 @@ int repo_config_get_string(struct repository *repo,\n \treturn repo_config_get_string_const(repo, key, (const char **)dest);\n }\n \n+int repo_config_get_string_tmp(struct repository *repo,\n+\t\t\t       const char *key, const char **dest)\n+{\n+\tint ret;\n+\tgit_config_check_init(repo);\n+\tret = git_configset_get_string_tmp(repo->config, key, dest);\n+\tif (ret < 0)\n+\t\tgit_die_config(key, NULL);\n+\treturn ret;\n+}\n+\n int repo_config_get_int(struct repository *repo,\n \t\t\tconst char *key, int *dest)\n {\n@@ -2242,6 +2267,11 @@ int git_config_get_string(const char *key, char **dest)\n \treturn repo_config_get_string(the_repository, key, dest);\n }\n \n+int git_config_get_string_tmp(const char *key, const char **dest)\n+{\n+\treturn repo_config_get_string_tmp(the_repository, key, dest);\n+}\n+\n int git_config_get_int(const char *key, int *dest)\n {\n \treturn repo_config_get_int(the_repository, key, dest);\ndiff --git a/config.h b/config.h\nindex 060874488f..a75b22e0d1 100644\n--- a/config.h\n+++ b/config.h\n@@ -460,6 +460,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \n int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n+int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n@@ -478,6 +479,8 @@ int repo_config_get_string_const(struct repository *repo,\n \t\t\t\t const char *key, const char **dest);\n int repo_config_get_string(struct repository *repo,\n \t\t\t   const char *key, char **dest);\n+int repo_config_get_string_tmp(struct repository *repo,\n+\t\t\t       const char *key, const char **dest);\n int repo_config_get_int(struct repository *repo,\n \t\t\tconst char *key, int *dest);\n int repo_config_get_ulong(struct repository *repo,\n@@ -537,6 +540,13 @@ int git_config_get_string_const(const char *key, const char **dest);\n  */\n int git_config_get_string(const char *key, char **dest);\n \n+/**\n+ * Similar to `git_config_get_string_const`, but does not allocate any new\n+ * memory; on success `dest` will point to memory owned by the config\n+ * machinery, which could be invalidated if it is discarded and reloaded.\n+ */\n+int git_config_get_string_tmp(const char *key, const char **dest);\n+\n /**\n  * Finds and parses the value to an integer for the configuration variable\n  * `key`. Dies on error; otherwise, stores the value of the parsed integer in\ndiff --git a/connect.c b/connect.c\nindex 0b6aba177e..8b8f56cf6d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1052,7 +1052,7 @@ static const char *get_ssh_command(void)\n \tif ((ssh = getenv(\"GIT_SSH_COMMAND\")))\n \t\treturn ssh;\n \n-\tif (!git_config_get_string_const(\"core.sshcommand\", &ssh))\n+\tif (!git_config_get_string_tmp(\"core.sshcommand\", &ssh))\n \t\treturn ssh;\n \n \treturn NULL;\n@@ -1071,7 +1071,7 @@ static void override_ssh_variant(enum ssh_variant *ssh_variant)\n {\n \tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n \n-\tif (!variant && git_config_get_string_const(\"ssh.variant\", &variant))\n+\tif (!variant && git_config_get_string_tmp(\"ssh.variant\", &variant))\n \t\treturn;\n \n \tif (!strcmp(variant, \"auto\"))\ndiff --git a/editor.c b/editor.c\nindex 91989ee8a1..6303ae0ab0 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -40,7 +40,7 @@ const char *git_sequence_editor(void)\n \tconst char *editor = getenv(\"GIT_SEQUENCE_EDITOR\");\n \n \tif (!editor)\n-\t\tgit_config_get_string_const(\"sequence.editor\", &editor);\n+\t\tgit_config_get_string_tmp(\"sequence.editor\", &editor);\n \tif (!editor)\n \t\teditor = git_editor();\n \ndiff --git a/help.c b/help.c\nindex d478afb2af..4e2468a44d 100644\n--- a/help.c\n+++ b/help.c\n@@ -375,7 +375,7 @@ void list_cmds_by_config(struct string_list *list)\n {\n \tconst char *cmd_list;\n \n-\tif (git_config_get_string_const(\"completion.commands\", &cmd_list))\n+\tif (git_config_get_string_tmp(\"completion.commands\", &cmd_list))\n \t\treturn;\n \n \tstring_list_sort(list);\ndiff --git a/protocol.c b/protocol.c\nindex d1dd3424bb..8d964fc65e 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -21,7 +21,7 @@ enum protocol_version get_protocol_version_config(void)\n \tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n \tconst char *git_test_v;\n \n-\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n+\tif (!git_config_get_string_tmp(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n \n \t\tif (version == protocol_unknown_version)\ndiff --git a/submodule.c b/submodule.c\nindex a52b93a87f..d0b70ca536 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -194,7 +194,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\tchar *key;\n \n \t\tkey = xstrfmt(\"submodule.%s.ignore\", submodule->name);\n-\t\tif (repo_config_get_string_const(the_repository, key, &ignore))\n+\t\tif (repo_config_get_string_tmp(the_repository, key, &ignore))\n \t\t\tignore = submodule->ignore;\n \t\tfree(key);\n \n@@ -1299,7 +1299,7 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \n \t\tint fetch_recurse = submodule->fetch_recurse;\n \t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n-\t\tif (!repo_config_get_string_const(spf->r, key, &value)) {\n+\t\tif (!repo_config_get_string_tmp(spf->r, key, &value)) {\n \t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n \t\t}\n \t\tfree(key);\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 234c722b48..a6e936721f 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -126,7 +126,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\tgoto exit1;\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n-\t\tif (!git_config_get_string_const(argv[2], &v)) {\n+\t\tif (!git_config_get_string_tmp(argv[2], &v)) {\n \t\t\tprintf(\"%s\\n\", v);\n \t\t\tgoto exit0;\n \t\t} else {\n-- \n2.28.0.605.g35fde94f44\n\n"},{"id":"403893","messageId":"20200817213313.GF1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 6/7] config: fix leak in git_config_get_expiry_in_days()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:13Z","receivedAt":"2020-08-17T21:33:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use git_config_get_string() to retrieve the expiry value in a newly\nallocated string. But after parsing it, we never free it, leaking the\nmemory.\n\nWe could fix this with a free() obviously, but there's an even better\nsolution: we can use the non-allocating \"tmp\" variant of the function;\nwe only need it to be valid for the lifetime of our parse function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f0367c76ad..2bdff4457b 100644\n--- a/config.c\n+++ b/config.c\n@@ -2300,11 +2300,11 @@ int git_config_get_expiry(const char *key, const char **output)\n \n int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now)\n {\n-\tchar *expiry_string;\n+\tconst char *expiry_string;\n \tintmax_t days;\n \ttimestamp_t when;\n \n-\tif (git_config_get_string(key, &expiry_string))\n+\tif (git_config_get_string_tmp(key, &expiry_string))\n \t\treturn 1; /* no such thing */\n \n \tif (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {\n-- \n2.28.0.605.g35fde94f44\n\n"},{"id":"403894","messageId":"20200817213316.GG1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 7/7] submodule--helper: fix leak of core.worktree value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:16Z","receivedAt":"2020-08-17T21:33:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the ensure_core_worktree() function, we load the core.worktree value\nof the submodule repository using repo_config_get_string(). This\nfunction copies the string, but we never free it, leaking the memory.\n\nWe can instead use the \"tmp\" version of that function to avoid the\nallocation at all. We don't have to worry about lifetime issues, since\nwe never even look at the value (we just want to know if it's set).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e09605716e..a59d8e4bda 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2101,7 +2101,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n \tconst char *path;\n-\tchar *cw;\n+\tconst char *cw;\n \tstruct repository subrepo;\n \n \tif (argc != 2)\n@@ -2116,7 +2116,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tif (repo_submodule_init(&subrepo, the_repository, sub))\n \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n \n-\tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\n+\tif (!repo_config_get_string_tmp(&subrepo, \"core.worktree\", &cw)) {\n \t\tchar *cfg_file, *abs_path;\n \t\tconst char *rel_path;\n \t\tstruct strbuf sb = STRBUF_INIT;\n-- \n2.28.0.605.g35fde94f44\n"},{"id":"403895","messageId":"20200817213311.GE1854722@coredump.intra.peff.net","threadId":"54044","inReplyTo":"20200817213228.GA1854603@coredump.intra.peff.net","subject":"[PATCH v2 5/7] config: drop git_config_get_string_const()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T21:33:11Z","receivedAt":"2020-08-17T21:33:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As evidenced by the leak fixes in the previous commit, the \"const\" in\ngit_config_get_string_const() clearly misleads people into thinking that\nit does not allocate a copy of the string. We can fix this by renaming\nit, but it's easier still to just drop it. Of the four remaining\ncallers:\n\n  - The one in git_config_parse_expiry() still needs to allocate, since\n    that's what its callers expect. We can just use the non-const\n    version and cast our pointer. Slightly ugly, but the damage is\n    contained in one spot.\n\n  - The two in apply are writing to global \"const char *\" variables, and\n    need to continue allocating. We often mark these as const because we\n    assign default string literals to them. But in this case we don't do\n    that, so we can just declare them as real \"char *\" pointers and use\n    the non-const version.\n\n  - The call in checkout doesn't actually need a copy; it can just use\n    the non-allocating \"tmp\" version of the function.\n\nThe function is also mentioned in the MyFirstContribution document. We\ncan swap that call out for the non-allocating \"tmp\" variant, which fits\nwell in the example given.\n\nWe'll drop the \"configset\" and \"repo\" variants, as well (which are\nunused).\n\nNote that this frees up the \"const\" name, so we could rename the \"tmp\"\nvariant back to that. But let's give some time for topics in flight to\nadapt to the new code before doing so (if we do it too soon, the\nfunction semantics will change but the compiler won't alert us).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/MyFirstContribution.txt |  4 ++--\n apply.c                               |  4 ++--\n cache.h                               |  4 ++--\n checkout.c                            |  3 +--\n config.c                              | 29 ++++++---------------------\n config.h                              | 11 +---------\n environment.c                         |  4 ++--\n 7 files changed, 16 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt\nindex d85c9b5143..4f85a089ef 100644\n--- a/Documentation/MyFirstContribution.txt\n+++ b/Documentation/MyFirstContribution.txt\n@@ -319,14 +319,14 @@ function body:\n ...\n \n \tgit_config(git_default_config, NULL);\n-\tif (git_config_get_string_const(\"user.name\", &cfg_name) > 0)\n+\tif (git_config_get_string_tmp(\"user.name\", &cfg_name) > 0)\n \t\tprintf(_(\"No name is found in config\\n\"));\n \telse\n \t\tprintf(_(\"Your name: %s\\n\"), cfg_name);\n ----\n \n `git_config()` will grab the configuration from config files known to Git and\n-apply standard precedence rules. `git_config_get_string_const()` will look up\n+apply standard precedence rules. `git_config_get_string_tmp()` will look up\n a specific key (\"user.name\") and give you the value. There are a number of\n single-key lookup functions like this one; you can see them all (and more info\n about how to use `git_config()`) in `Documentation/technical/api-config.txt`.\ndiff --git a/apply.c b/apply.c\nindex 402d80602a..2839dae029 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -30,8 +30,8 @@ struct gitdiff_data {\n \n static void git_apply_config(void)\n {\n-\tgit_config_get_string_const(\"apply.whitespace\", &apply_default_whitespace);\n-\tgit_config_get_string_const(\"apply.ignorewhitespace\", &apply_default_ignorewhitespace);\n+\tgit_config_get_string(\"apply.whitespace\", &apply_default_whitespace);\n+\tgit_config_get_string(\"apply.ignorewhitespace\", &apply_default_ignorewhitespace);\n \tgit_config(git_xmerge_config, NULL);\n }\n \ndiff --git a/cache.h b/cache.h\nindex 0290849c19..4cad61ffa4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -921,8 +921,8 @@ extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n-extern const char *apply_default_whitespace;\n-extern const char *apply_default_ignorewhitespace;\n+extern char *apply_default_whitespace;\n+extern char *apply_default_ignorewhitespace;\n extern const char *git_attributes_file;\n extern const char *git_hooks_path;\n extern int zlib_compression_level;\ndiff --git a/checkout.c b/checkout.c\nindex c72e9f9773..6586e30ca5 100644\n--- a/checkout.c\n+++ b/checkout.c\n@@ -47,15 +47,14 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,\n {\n \tstruct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;\n \tconst char *default_remote = NULL;\n-\tif (!git_config_get_string_const(\"checkout.defaultremote\", &default_remote))\n+\tif (!git_config_get_string_tmp(\"checkout.defaultremote\", &default_remote))\n \t\tcb_data.default_remote = default_remote;\n \tcb_data.src_ref = xstrfmt(\"refs/heads/%s\", name);\n \tcb_data.dst_oid = oid;\n \tfor_each_remote(check_tracking_name, &cb_data);\n \tif (dwim_remotes_matched)\n \t\t*dwim_remotes_matched = cb_data.num_matches;\n \tfree(cb_data.src_ref);\n-\tfree((char *)default_remote);\n \tif (cb_data.num_matches == 1) {\n \t\tfree(cb_data.default_dst_ref);\n \t\tfree(cb_data.default_dst_oid);\ndiff --git a/config.c b/config.c\nindex 2f244c67b6..f0367c76ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -2006,20 +2006,15 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n \treturn e ? &e->value_list : NULL;\n }\n \n-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n {\n \tconst char *value;\n \tif (!git_configset_get_value(cs, key, &value))\n-\t\treturn git_config_string(dest, key, value);\n+\t\treturn git_config_string((const char **)dest, key, value);\n \telse\n \t\treturn 1;\n }\n \n-int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n-{\n-\treturn git_configset_get_string_const(cs, key, (const char **)dest);\n-}\n-\n int git_configset_get_string_tmp(struct config_set *cs, const char *key,\n \t\t\t\t const char **dest)\n {\n@@ -2161,24 +2156,17 @@ const struct string_list *repo_config_get_value_multi(struct repository *repo,\n \treturn git_configset_get_value_multi(repo->config, key);\n }\n \n-int repo_config_get_string_const(struct repository *repo,\n-\t\t\t\t const char *key, const char **dest)\n+int repo_config_get_string(struct repository *repo,\n+\t\t\t   const char *key, char **dest)\n {\n \tint ret;\n \tgit_config_check_init(repo);\n-\tret = git_configset_get_string_const(repo->config, key, dest);\n+\tret = git_configset_get_string(repo->config, key, dest);\n \tif (ret < 0)\n \t\tgit_die_config(key, NULL);\n \treturn ret;\n }\n \n-int repo_config_get_string(struct repository *repo,\n-\t\t\t   const char *key, char **dest)\n-{\n-\tgit_config_check_init(repo);\n-\treturn repo_config_get_string_const(repo, key, (const char **)dest);\n-}\n-\n int repo_config_get_string_tmp(struct repository *repo,\n \t\t\t       const char *key, const char **dest)\n {\n@@ -2257,11 +2245,6 @@ const struct string_list *git_config_get_value_multi(const char *key)\n \treturn repo_config_get_value_multi(the_repository, key);\n }\n \n-int git_config_get_string_const(const char *key, const char **dest)\n-{\n-\treturn repo_config_get_string_const(the_repository, key, dest);\n-}\n-\n int git_config_get_string(const char *key, char **dest)\n {\n \treturn repo_config_get_string(the_repository, key, dest);\n@@ -2304,7 +2287,7 @@ int git_config_get_pathname(const char *key, const char **dest)\n \n int git_config_get_expiry(const char *key, const char **output)\n {\n-\tint ret = git_config_get_string_const(key, output);\n+\tint ret = git_config_get_string(key, (char **)output);\n \tif (ret)\n \t\treturn ret;\n \tif (strcmp(*output, \"now\")) {\ndiff --git a/config.h b/config.h\nindex a75b22e0d1..91cdfbfb41 100644\n--- a/config.h\n+++ b/config.h\n@@ -458,7 +458,6 @@ void git_configset_clear(struct config_set *cs);\n  */\n int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);\n \n-int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n@@ -475,8 +474,6 @@ int repo_config_get_value(struct repository *repo,\n \t\t\t  const char *key, const char **value);\n const struct string_list *repo_config_get_value_multi(struct repository *repo,\n \t\t\t\t\t\t      const char *key);\n-int repo_config_get_string_const(struct repository *repo,\n-\t\t\t\t const char *key, const char **dest);\n int repo_config_get_string(struct repository *repo,\n \t\t\t   const char *key, char **dest);\n int repo_config_get_string_tmp(struct repository *repo,\n@@ -532,16 +529,10 @@ void git_config_clear(void);\n  * error message and returns -1. When the configuration variable `key` is\n  * not found, returns 1 without touching `dest`.\n  */\n-int git_config_get_string_const(const char *key, const char **dest);\n-\n-/**\n- * Similar to `git_config_get_string_const`, except that retrieved value\n- * copied into the `dest` parameter is a mutable string.\n- */\n int git_config_get_string(const char *key, char **dest);\n \n /**\n- * Similar to `git_config_get_string_const`, but does not allocate any new\n+ * Similar to `git_config_get_string`, but does not allocate any new\n  * memory; on success `dest` will point to memory owned by the config\n  * machinery, which could be invalidated if it is discarded and reloaded.\n  */\ndiff --git a/environment.c b/environment.c\nindex 52e0c979ba..bb518c61cd 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,8 +35,8 @@ int repository_format_precious_objects;\n int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n-const char *apply_default_whitespace;\n-const char *apply_default_ignorewhitespace;\n+char *apply_default_whitespace;\n+char *apply_default_ignorewhitespace;\n const char *git_attributes_file;\n const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n-- \n2.28.0.605.g35fde94f44\n\n"}]}