@matheusbsbs / matheus-bagliano / commits / f1de981e8b
Fork 0

config: fix leaks from git_config_get_string_const()

There are two functions to get a single config string: - git_config_get_string() - git_config_get_string_const() One might naively think that the first one allocates a new string and the second one just points us to the internal configset storage. But in fact they both allocate a new copy; the second one exists only to avoid having to cast when using it with a const global which we never intend to free. The documentation for the function explains that clearly, but it seems I'm not alone in being surprised by this. Of 17 calls to the function, 13 of them leak the resulting value. We could obviously fix these by adding the appropriate free(). But it would be simpler still if we actually had a non-allocating way to get the string. There's git_config_get_value() but that doesn't quite do what we want. If the config key is present but is a boolean with no value (e.g., "[foo]bar" in the file), then we'll get NULL (whereas the string versions will print an error and die). So let's introduce a new variant, git_config_get_string_tmp(), that behaves as these callers expect. We need a new name because we have new semantics but the same function signature (so even if we converted the four remaining callers, topics in flight might be surprised). The "tmp" is because this value should only be held onto for a short time. In practice it's rare for us to clear and refresh the configset, invalidating the pointer, but hopefully the "tmp" makes callers think about the lifetime. In each of the converted cases here the value only needs to last within the local function or its immediate caller. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 14, 2020 at 12:17 UTC f1de981e8b6dedccf915095792c9afbe3c989591
10 files changed +53 -13
builtin/fetch.c
+1 -1
@@ -645,7 +645,7 @@ static void prepare_format_display(struct ref *ref_map)
645 struct ref *rm;
646 const char *format = "full";
647
648 - git_config_get_string_const("fetch.output", &format);
648 + git_config_get_string_tmp("fetch.output", &format);
649 if (!strcasecmp(format, "full"))
650 compact_format = 0;
651 else if (!strcasecmp(format, "compact"))
builtin/submodule--helper.c
+4 -4
@@ -1511,7 +1511,7 @@ static void determine_submodule_update_strategy(struct repository *r,
1511 if (parse_submodule_update_strategy(update, out) < 0)
1512 die(_("Invalid update mode '%s' for submodule path '%s'"),
1513 update, path);
1514 - } else if (!repo_config_get_string_const(r, key, &val)) {
1514 + } else if (!repo_config_get_string_tmp(r, key, &val)) {
1515 if (parse_submodule_update_strategy(val, out) < 0)
1516 die(_("Invalid update mode '%s' configured for submodule path '%s'"),
1517 val, path);
@@ -1667,7 +1667,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
1667 }
1668
1669 key = xstrfmt("submodule.%s.update", sub->name);
1670 - if (!repo_config_get_string_const(the_repository, key, &update_string)) {
1670 + if (!repo_config_get_string_tmp(the_repository, key, &update_string)) {
1671 update_type = parse_submodule_update_type(update_string);
1672 } else {
1673 update_type = sub->update_strategy.type;
@@ -1690,7 +1690,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
1690
1691 strbuf_reset(&sb);
1692 strbuf_addf(&sb, "submodule.%s.url", sub->name);
1693 - if (repo_config_get_string_const(the_repository, sb.buf, &url)) {
1693 + if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {
1694 if (starts_with_dot_slash(sub->url) ||
1695 starts_with_dot_dot_slash(sub->url)) {
1696 url = compute_submodule_clone_url(sub->url);
@@ -1976,7 +1976,7 @@ static const char *remote_submodule_branch(const char *path)
1976 return NULL;
1977
1978 key = xstrfmt("submodule.%s.branch", sub->name);
1979 - if (repo_config_get_string_const(the_repository, key, &branch))
1979 + if (repo_config_get_string_tmp(the_repository, key, &branch))
1980 branch = sub->branch;
1981 free(key);
1982
config.c
+30
@@ -2020,6 +2020,20 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
2020 return git_configset_get_string_const(cs, key, (const char **)dest);
2021 }
2022
2023 +int git_configset_get_string_tmp(struct config_set *cs, const char *key,
2024 + const char **dest)
2025 +{
2026 + const char *value;
2027 + if (!git_configset_get_value(cs, key, &value)) {
2028 + if (!value)
2029 + return config_error_nonbool(key);
2030 + *dest = value;
2031 + return 0;
2032 + } else {
2033 + return 1;
2034 + }
2035 +}
2036 +
2037 int git_configset_get_int(struct config_set *cs, const char *key, int *dest)
2038 {
2039 const char *value;
@@ -2165,6 +2179,17 @@ int repo_config_get_string(struct repository *repo,
2179 return repo_config_get_string_const(repo, key, (const char **)dest);
2180 }
2181
2182 +int repo_config_get_string_tmp(struct repository *repo,
2183 + const char *key, const char **dest)
2184 +{
2185 + int ret;
2186 + git_config_check_init(repo);
2187 + ret = git_configset_get_string_tmp(repo->config, key, dest);
2188 + if (ret < 0)
2189 + git_die_config(key, NULL);
2190 + return ret;
2191 +}
2192 +
2193 int repo_config_get_int(struct repository *repo,
2194 const char *key, int *dest)
2195 {
@@ -2242,6 +2267,11 @@ int git_config_get_string(const char *key, char **dest)
2267 return repo_config_get_string(the_repository, key, dest);
2268 }
2269
2270 +int git_config_get_string_tmp(const char *key, const char **dest)
2271 +{
2272 + return repo_config_get_string_tmp(the_repository, key, dest);
2273 +}
2274 +
2275 int git_config_get_int(const char *key, int *dest)
2276 {
2277 return repo_config_get_int(the_repository, key, dest);
config.h
+10
@@ -460,6 +460,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
460
461 int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);
462 int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
463 +int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);
464 int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
465 int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);
466 int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);
@@ -478,6 +479,8 @@ int repo_config_get_string_const(struct repository *repo,
479 const char *key, const char **dest);
480 int repo_config_get_string(struct repository *repo,
481 const char *key, char **dest);
482 +int repo_config_get_string_tmp(struct repository *repo,
483 + const char *key, const char **dest);
484 int repo_config_get_int(struct repository *repo,
485 const char *key, int *dest);
486 int repo_config_get_ulong(struct repository *repo,
@@ -537,6 +540,13 @@ int git_config_get_string_const(const char *key, const char **dest);
540 */
541 int git_config_get_string(const char *key, char **dest);
542
543 +/**
544 + * Similar to `git_config_get_string_const`, but does not allocate any new
545 + * memory; on success `dest` will point to memory owned by the config
546 + * machinery, which could be invalidated if it is discarded and reloaded.
547 + */
548 +int git_config_get_string_tmp(const char *key, const char **dest);
549 +
550 /**
551 * Finds and parses the value to an integer for the configuration variable
552 * `key`. Dies on error; otherwise, stores the value of the parsed integer in
connect.c
+2 -2
@@ -1052,7 +1052,7 @@ static const char *get_ssh_command(void)
1052 if ((ssh = getenv("GIT_SSH_COMMAND")))
1053 return ssh;
1054
1055 - if (!git_config_get_string_const("core.sshcommand", &ssh))
1055 + if (!git_config_get_string_tmp("core.sshcommand", &ssh))
1056 return ssh;
1057
1058 return NULL;
@@ -1071,7 +1071,7 @@ static void override_ssh_variant(enum ssh_variant *ssh_variant)
1071 {
1072 const char *variant = getenv("GIT_SSH_VARIANT");
1073
1074 - if (!variant && git_config_get_string_const("ssh.variant", &variant))
1074 + if (!variant && git_config_get_string_tmp("ssh.variant", &variant))
1075 return;
1076
1077 if (!strcmp(variant, "auto"))
editor.c
+1 -1
@@ -40,7 +40,7 @@ const char *git_sequence_editor(void)
40 const char *editor = getenv("GIT_SEQUENCE_EDITOR");
41
42 if (!editor)
43 - git_config_get_string_const("sequence.editor", &editor);
43 + git_config_get_string_tmp("sequence.editor", &editor);
44 if (!editor)
45 editor = git_editor();
46
help.c
+1 -1
@@ -375,7 +375,7 @@ void list_cmds_by_config(struct string_list *list)
375 {
376 const char *cmd_list;
377
378 - if (git_config_get_string_const("completion.commands", &cmd_list))
378 + if (git_config_get_string_tmp("completion.commands", &cmd_list))
379 return;
380
381 string_list_sort(list);
protocol.c
+1 -1
@@ -21,7 +21,7 @@ enum protocol_version get_protocol_version_config(void)
21 const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
22 const char *git_test_v;
23
24 - if (!git_config_get_string_const("protocol.version", &value)) {
24 + if (!git_config_get_string_tmp("protocol.version", &value)) {
25 enum protocol_version version = parse_protocol_version(value);
26
27 if (version == protocol_unknown_version)
submodule.c
+2 -2
@@ -194,7 +194,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
194 char *key;
195
196 key = xstrfmt("submodule.%s.ignore", submodule->name);
197 - if (repo_config_get_string_const(the_repository, key, &ignore))
197 + if (repo_config_get_string_tmp(the_repository, key, &ignore))
198 ignore = submodule->ignore;
199 free(key);
200
@@ -1299,7 +1299,7 @@ static int get_fetch_recurse_config(const struct submodule *submodule,
1299
1300 int fetch_recurse = submodule->fetch_recurse;
1301 key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
1302 - if (!repo_config_get_string_const(spf->r, key, &value)) {
1302 + if (!repo_config_get_string_tmp(spf->r, key, &value)) {
1303 fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
1304 }
1305 free(key);
t/helper/test-config.c
+1 -1
@@ -126,7 +126,7 @@ int cmd__config(int argc, const char **argv)
126 goto exit1;
127 }
128 } else if (argc == 3 && !strcmp(argv[1], "get_string")) {
129 - if (!git_config_get_string_const(argv[2], &v)) {
129 + if (!git_config_get_string_tmp(argv[2], &v)) {
130 printf("%s\n", v);
131 goto exit0;
132 } else {