From f1de981e8b6dedccf915095792c9afbe3c989591 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 14 Aug 2020 12:17:36 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- config.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) (limited to 'config.c') diff --git a/config.c b/config.c index 8db9c77098..facd73d40a 100644 --- a/config.c +++ b/config.c @@ -2020,6 +2020,20 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest return git_configset_get_string_const(cs, key, (const char **)dest); } +int git_configset_get_string_tmp(struct config_set *cs, const char *key, + const char **dest) +{ + const char *value; + if (!git_configset_get_value(cs, key, &value)) { + if (!value) + return config_error_nonbool(key); + *dest = value; + return 0; + } else { + return 1; + } +} + int git_configset_get_int(struct config_set *cs, const char *key, int *dest) { const char *value; @@ -2165,6 +2179,17 @@ int repo_config_get_string(struct repository *repo, return repo_config_get_string_const(repo, key, (const char **)dest); } +int repo_config_get_string_tmp(struct repository *repo, + const char *key, const char **dest) +{ + int ret; + git_config_check_init(repo); + ret = git_configset_get_string_tmp(repo->config, key, dest); + if (ret < 0) + git_die_config(key, NULL); + return ret; +} + int repo_config_get_int(struct repository *repo, const char *key, int *dest) { @@ -2242,6 +2267,11 @@ int git_config_get_string(const char *key, char **dest) return repo_config_get_string(the_repository, key, dest); } +int git_config_get_string_tmp(const char *key, const char **dest) +{ + return repo_config_get_string_tmp(the_repository, key, dest); +} + int git_config_get_int(const char *key, int *dest) { return repo_config_get_int(the_repository, key, dest); -- cgit v1.2.3 From 9a53219f69bd470053cf93c3f4d2a77b6d4df3e5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 17 Aug 2020 17:33:11 -0400 Subject: config: drop git_config_get_string_const() As evidenced by the leak fixes in the previous commit, the "const" in git_config_get_string_const() clearly misleads people into thinking that it does not allocate a copy of the string. We can fix this by renaming it, but it's easier still to just drop it. Of the four remaining callers: - The one in git_config_parse_expiry() still needs to allocate, since that's what its callers expect. We can just use the non-const version and cast our pointer. Slightly ugly, but the damage is contained in one spot. - The two in apply are writing to global "const char *" variables, and need to continue allocating. We often mark these as const because we assign default string literals to them. But in this case we don't do that, so we can just declare them as real "char *" pointers and use the non-const version. - The call in checkout doesn't actually need a copy; it can just use the non-allocating "tmp" version of the function. The function is also mentioned in the MyFirstContribution document. We can swap that call out for the non-allocating "tmp" variant, which fits well in the example given. We'll drop the "configset" and "repo" variants, as well (which are unused). Note that this frees up the "const" name, so we could rename the "tmp" variant back to that. But let's give some time for topics in flight to adapt to the new code before doing so (if we do it too soon, the function semantics will change but the compiler won't alert us). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index facd73d40a..d92321a279 100644 --- a/config.c +++ b/config.c @@ -2006,20 +2006,15 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c return e ? &e->value_list : NULL; } -int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest) +int git_configset_get_string(struct config_set *cs, const char *key, char **dest) { const char *value; if (!git_configset_get_value(cs, key, &value)) - return git_config_string(dest, key, value); + return git_config_string((const char **)dest, key, value); else return 1; } -int git_configset_get_string(struct config_set *cs, const char *key, char **dest) -{ - return git_configset_get_string_const(cs, key, (const char **)dest); -} - int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest) { @@ -2161,24 +2156,17 @@ const struct string_list *repo_config_get_value_multi(struct repository *repo, return git_configset_get_value_multi(repo->config, key); } -int repo_config_get_string_const(struct repository *repo, - const char *key, const char **dest) +int repo_config_get_string(struct repository *repo, + const char *key, char **dest) { int ret; git_config_check_init(repo); - ret = git_configset_get_string_const(repo->config, key, dest); + ret = git_configset_get_string(repo->config, key, dest); if (ret < 0) git_die_config(key, NULL); return ret; } -int repo_config_get_string(struct repository *repo, - const char *key, char **dest) -{ - git_config_check_init(repo); - return repo_config_get_string_const(repo, key, (const char **)dest); -} - int repo_config_get_string_tmp(struct repository *repo, const char *key, const char **dest) { @@ -2257,11 +2245,6 @@ const struct string_list *git_config_get_value_multi(const char *key) return repo_config_get_value_multi(the_repository, key); } -int git_config_get_string_const(const char *key, const char **dest) -{ - return repo_config_get_string_const(the_repository, key, dest); -} - int git_config_get_string(const char *key, char **dest) { return repo_config_get_string(the_repository, key, dest); @@ -2304,7 +2287,7 @@ int git_config_get_pathname(const char *key, const char **dest) int git_config_get_expiry(const char *key, const char **output) { - int ret = git_config_get_string_const(key, output); + int ret = git_config_get_string(key, (char **)output); if (ret) return ret; if (strcmp(*output, "now")) { -- cgit v1.2.3 From 1c890016a147536e4a6faa5664eedd8765a01b32 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 17 Aug 2020 17:33:13 -0400 Subject: config: fix leak in git_config_get_expiry_in_days() We use git_config_get_string() to retrieve the expiry value in a newly allocated string. But after parsing it, we never free it, leaking the memory. We could fix this with a free() obviously, but there's an even better solution: we can use the non-allocating "tmp" variant of the function; we only need it to be valid for the lifetime of our parse function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index d92321a279..82c6552d86 100644 --- a/config.c +++ b/config.c @@ -2300,11 +2300,11 @@ int git_config_get_expiry(const char *key, const char **output) int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now) { - char *expiry_string; + const char *expiry_string; intmax_t days; timestamp_t when; - if (git_config_get_string(key, &expiry_string)) + if (git_config_get_string_tmp(key, &expiry_string)) return 1; /* no such thing */ if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) { -- cgit v1.2.3