From patchwork Fri Aug 14 16:14:14 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714751 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 314D416B1 for ; Fri, 14 Aug 2020 16:14:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 231B12078D for ; Fri, 14 Aug 2020 16:14:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728177AbgHNQOQ (ORCPT ); Fri, 14 Aug 2020 12:14:16 -0400 Received: from cloud.peff.net ([104.130.231.41]:59252 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726377AbgHNQOQ (ORCPT ); Fri, 14 Aug 2020 12:14:16 -0400 Received: (qmail 1025 invoked by uid 109); 14 Aug 2020 16:14:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:14:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26871 invoked by uid 111); 14 Aug 2020 16:14:14 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:14:14 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:14:14 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 1/6] submodule--helper: use strbuf_release() to free strbufs Message-ID: <20200814161414.GA595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org The prepare_to_clone_next_submodule() function has a few local-variable strbufs. We use strbuf_reset() throughout the function to reuse the buffers over and over. But at the end of the function we also use strbuf_reset() as they go out of scope, which means we end up leaking their heap buffers. This should be strbuf_release() instead. These were introduced by 48308681b0 (git submodule update: have a dedicated helper for cloning, 2016-02-29), but it doesn't seem to have the same mistake elsewhere. Likewise, I looked for other instances of the pattern in the submodule--helper file but couldn't find any. Signed-off-by: Jeff King --- builtin/submodule--helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index df135abbf1..1762458345 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1747,8 +1747,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, "--no-single-branch"); cleanup: - strbuf_reset(&displaypath_sb); - strbuf_reset(&sb); + strbuf_release(&displaypath_sb); + strbuf_release(&sb); if (need_free_url) free((void*)url); From patchwork Fri Aug 14 16:14:53 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714753 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 47D38618 for ; Fri, 14 Aug 2020 16:14:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 36EEE2078D for ; Fri, 14 Aug 2020 16:14:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728189AbgHNQOy (ORCPT ); Fri, 14 Aug 2020 12:14:54 -0400 Received: from cloud.peff.net ([104.130.231.41]:59258 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726377AbgHNQOy (ORCPT ); Fri, 14 Aug 2020 12:14:54 -0400 Received: (qmail 1037 invoked by uid 109); 14 Aug 2020 16:14:54 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:14:54 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26895 invoked by uid 111); 14 Aug 2020 16:14:53 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:14:53 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:14:53 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 2/6] checkout: fix leak of non-existent branch names Message-ID: <20200814161453.GB595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org We unconditionally write a branch name into a newly allocated buffer in new_branch_info->path, via setup_branch_path(). We then check to see if the branch exists; if not, we set that field to NULL, leaking the memory. We should take care to free() it when doing so. Signed-off-by: Jeff King --- builtin/checkout.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2837195491..bba64108af 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1120,8 +1120,10 @@ static void setup_new_branch_info_and_source_tree( if (!check_refname_format(new_branch_info->path, 0) && !read_ref(new_branch_info->path, &branch_rev)) oidcpy(rev, &branch_rev); - else + else { + free((char *)new_branch_info->path); new_branch_info->path = NULL; /* not an existing branch */ + } new_branch_info->commit = lookup_commit_reference_gently(the_repository, rev, 1); if (!new_branch_info->commit) { From patchwork Fri Aug 14 16:17:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714759 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E925E13A4 for ; Fri, 14 Aug 2020 16:17:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D040020768 for ; Fri, 14 Aug 2020 16:17:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728218AbgHNQRj (ORCPT ); Fri, 14 Aug 2020 12:17:39 -0400 Received: from cloud.peff.net ([104.130.231.41]:59266 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726377AbgHNQRi (ORCPT ); Fri, 14 Aug 2020 12:17:38 -0400 Received: (qmail 1070 invoked by uid 109); 14 Aug 2020 16:17:37 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:17:37 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26918 invoked by uid 111); 14 Aug 2020 16:17:36 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:17:36 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:17:36 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 3/6] config: fix leaks from git_config_get_string_const() Message-ID: <20200814161736.GC595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- builtin/fetch.c | 2 +- builtin/submodule--helper.c | 8 ++++---- config.c | 30 ++++++++++++++++++++++++++++++ config.h | 10 ++++++++++ connect.c | 4 ++-- editor.c | 2 +- help.c | 2 +- protocol.c | 2 +- submodule.c | 4 ++-- t/helper/test-config.c | 2 +- 10 files changed, 53 insertions(+), 13 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index c49f0e9752..b3b77f8e2b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -645,7 +645,7 @@ static void prepare_format_display(struct ref *ref_map) struct ref *rm; const char *format = "full"; - git_config_get_string_const("fetch.output", &format); + git_config_get_string_tmp("fetch.output", &format); if (!strcasecmp(format, "full")) compact_format = 0; else if (!strcasecmp(format, "compact")) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1762458345..e09605716e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1511,7 +1511,7 @@ static void determine_submodule_update_strategy(struct repository *r, if (parse_submodule_update_strategy(update, out) < 0) die(_("Invalid update mode '%s' for submodule path '%s'"), update, path); - } else if (!repo_config_get_string_const(r, key, &val)) { + } else if (!repo_config_get_string_tmp(r, key, &val)) { if (parse_submodule_update_strategy(val, out) < 0) die(_("Invalid update mode '%s' configured for submodule path '%s'"), val, path); @@ -1667,7 +1667,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, } key = xstrfmt("submodule.%s.update", sub->name); - if (!repo_config_get_string_const(the_repository, key, &update_string)) { + if (!repo_config_get_string_tmp(the_repository, key, &update_string)) { update_type = parse_submodule_update_type(update_string); } else { update_type = sub->update_strategy.type; @@ -1690,7 +1690,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); - if (repo_config_get_string_const(the_repository, sb.buf, &url)) { + if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { url = compute_submodule_clone_url(sub->url); @@ -1976,7 +1976,7 @@ static const char *remote_submodule_branch(const char *path) return NULL; key = xstrfmt("submodule.%s.branch", sub->name); - if (repo_config_get_string_const(the_repository, key, &branch)) + if (repo_config_get_string_tmp(the_repository, key, &branch)) branch = sub->branch; free(key); diff --git a/config.c b/config.c index 2b79fe76ad..2f244c67b6 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); diff --git a/config.h b/config.h index 060874488f..a75b22e0d1 100644 --- a/config.h +++ b/config.h @@ -460,6 +460,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char * 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); +int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest); int git_configset_get_int(struct config_set *cs, const char *key, int *dest); int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest); 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, const char *key, const char **dest); int repo_config_get_string(struct repository *repo, const char *key, char **dest); +int repo_config_get_string_tmp(struct repository *repo, + const char *key, const char **dest); int repo_config_get_int(struct repository *repo, const char *key, int *dest); int repo_config_get_ulong(struct repository *repo, @@ -537,6 +540,13 @@ int git_config_get_string_const(const char *key, const char **dest); */ int git_config_get_string(const char *key, char **dest); +/** + * Similar to `git_config_get_string_const`, but does not allocate any new + * memory; on success `dest` will point to memory owned by the config + * machinery, which could be invalidated if it is discarded and reloaded. + */ +int git_config_get_string_tmp(const char *key, const char **dest); + /** * Finds and parses the value to an integer for the configuration variable * `key`. Dies on error; otherwise, stores the value of the parsed integer in diff --git a/connect.c b/connect.c index 0b6aba177e..8b8f56cf6d 100644 --- a/connect.c +++ b/connect.c @@ -1052,7 +1052,7 @@ static const char *get_ssh_command(void) if ((ssh = getenv("GIT_SSH_COMMAND"))) return ssh; - if (!git_config_get_string_const("core.sshcommand", &ssh)) + if (!git_config_get_string_tmp("core.sshcommand", &ssh)) return ssh; return NULL; @@ -1071,7 +1071,7 @@ static void override_ssh_variant(enum ssh_variant *ssh_variant) { const char *variant = getenv("GIT_SSH_VARIANT"); - if (!variant && git_config_get_string_const("ssh.variant", &variant)) + if (!variant && git_config_get_string_tmp("ssh.variant", &variant)) return; if (!strcmp(variant, "auto")) diff --git a/editor.c b/editor.c index 91989ee8a1..6303ae0ab0 100644 --- a/editor.c +++ b/editor.c @@ -40,7 +40,7 @@ const char *git_sequence_editor(void) const char *editor = getenv("GIT_SEQUENCE_EDITOR"); if (!editor) - git_config_get_string_const("sequence.editor", &editor); + git_config_get_string_tmp("sequence.editor", &editor); if (!editor) editor = git_editor(); diff --git a/help.c b/help.c index d478afb2af..4e2468a44d 100644 --- a/help.c +++ b/help.c @@ -375,7 +375,7 @@ void list_cmds_by_config(struct string_list *list) { const char *cmd_list; - if (git_config_get_string_const("completion.commands", &cmd_list)) + if (git_config_get_string_tmp("completion.commands", &cmd_list)) return; string_list_sort(list); diff --git a/protocol.c b/protocol.c index d1dd3424bb..8d964fc65e 100644 --- a/protocol.c +++ b/protocol.c @@ -21,7 +21,7 @@ enum protocol_version get_protocol_version_config(void) const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; const char *git_test_v; - if (!git_config_get_string_const("protocol.version", &value)) { + if (!git_config_get_string_tmp("protocol.version", &value)) { enum protocol_version version = parse_protocol_version(value); if (version == protocol_unknown_version) diff --git a/submodule.c b/submodule.c index a52b93a87f..d0b70ca536 100644 --- a/submodule.c +++ b/submodule.c @@ -194,7 +194,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt, char *key; key = xstrfmt("submodule.%s.ignore", submodule->name); - if (repo_config_get_string_const(the_repository, key, &ignore)) + if (repo_config_get_string_tmp(the_repository, key, &ignore)) ignore = submodule->ignore; free(key); @@ -1299,7 +1299,7 @@ static int get_fetch_recurse_config(const struct submodule *submodule, int fetch_recurse = submodule->fetch_recurse; key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name); - if (!repo_config_get_string_const(spf->r, key, &value)) { + if (!repo_config_get_string_tmp(spf->r, key, &value)) { fetch_recurse = parse_fetch_recurse_submodules_arg(key, value); } free(key); diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 234c722b48..a6e936721f 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -126,7 +126,7 @@ int cmd__config(int argc, const char **argv) goto exit1; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { - if (!git_config_get_string_const(argv[2], &v)) { + if (!git_config_get_string_tmp(argv[2], &v)) { printf("%s\n", v); goto exit0; } else { From patchwork Fri Aug 14 16:19:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714761 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 13848618 for ; Fri, 14 Aug 2020 16:19:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F400C20791 for ; Fri, 14 Aug 2020 16:19:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728270AbgHNQTX (ORCPT ); Fri, 14 Aug 2020 12:19:23 -0400 Received: from cloud.peff.net ([104.130.231.41]:59274 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727886AbgHNQTX (ORCPT ); Fri, 14 Aug 2020 12:19:23 -0400 Received: (qmail 1089 invoked by uid 109); 14 Aug 2020 16:19:22 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:19:22 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26942 invoked by uid 111); 14 Aug 2020 16:19:22 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:19:22 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:19:21 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 4/6] config: drop git_config_get_string_const() Message-ID: <20200814161921.GD595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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. 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 --- Documentation/MyFirstContribution.txt | 4 ++-- apply.c | 4 ++-- cache.h | 4 ++-- checkout.c | 3 +-- config.c | 7 +------ config.h | 8 +------- environment.c | 4 ++-- 7 files changed, 11 insertions(+), 23 deletions(-) diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index d85c9b5143..4f85a089ef 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -319,14 +319,14 @@ function body: ... git_config(git_default_config, NULL); - if (git_config_get_string_const("user.name", &cfg_name) > 0) + if (git_config_get_string_tmp("user.name", &cfg_name) > 0) printf(_("No name is found in config\n")); else printf(_("Your name: %s\n"), cfg_name); ---- `git_config()` will grab the configuration from config files known to Git and -apply standard precedence rules. `git_config_get_string_const()` will look up +apply standard precedence rules. `git_config_get_string_tmp()` will look up a specific key ("user.name") and give you the value. There are a number of single-key lookup functions like this one; you can see them all (and more info about how to use `git_config()`) in `Documentation/technical/api-config.txt`. diff --git a/apply.c b/apply.c index 402d80602a..2839dae029 100644 --- a/apply.c +++ b/apply.c @@ -30,8 +30,8 @@ struct gitdiff_data { static void git_apply_config(void) { - git_config_get_string_const("apply.whitespace", &apply_default_whitespace); - git_config_get_string_const("apply.ignorewhitespace", &apply_default_ignorewhitespace); + git_config_get_string("apply.whitespace", &apply_default_whitespace); + git_config_get_string("apply.ignorewhitespace", &apply_default_ignorewhitespace); git_config(git_xmerge_config, NULL); } diff --git a/cache.h b/cache.h index 0290849c19..4cad61ffa4 100644 --- a/cache.h +++ b/cache.h @@ -921,8 +921,8 @@ extern int assume_unchanged; extern int prefer_symlink_refs; extern int warn_ambiguous_refs; extern int warn_on_object_refname_ambiguity; -extern const char *apply_default_whitespace; -extern const char *apply_default_ignorewhitespace; +extern char *apply_default_whitespace; +extern char *apply_default_ignorewhitespace; extern const char *git_attributes_file; extern const char *git_hooks_path; extern int zlib_compression_level; diff --git a/checkout.c b/checkout.c index c72e9f9773..6586e30ca5 100644 --- a/checkout.c +++ b/checkout.c @@ -47,15 +47,14 @@ const char *unique_tracking_name(const char *name, struct object_id *oid, { struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT; const char *default_remote = NULL; - if (!git_config_get_string_const("checkout.defaultremote", &default_remote)) + if (!git_config_get_string_tmp("checkout.defaultremote", &default_remote)) cb_data.default_remote = default_remote; cb_data.src_ref = xstrfmt("refs/heads/%s", name); cb_data.dst_oid = oid; for_each_remote(check_tracking_name, &cb_data); if (dwim_remotes_matched) *dwim_remotes_matched = cb_data.num_matches; free(cb_data.src_ref); - free((char *)default_remote); if (cb_data.num_matches == 1) { free(cb_data.default_dst_ref); free(cb_data.default_dst_oid); diff --git a/config.c b/config.c index 2f244c67b6..8bb1945aa9 100644 --- a/config.c +++ b/config.c @@ -2257,11 +2257,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 +2299,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")) { diff --git a/config.h b/config.h index a75b22e0d1..d4d22c34c6 100644 --- a/config.h +++ b/config.h @@ -532,16 +532,10 @@ void git_config_clear(void); * error message and returns -1. When the configuration variable `key` is * not found, returns 1 without touching `dest`. */ -int git_config_get_string_const(const char *key, const char **dest); - -/** - * Similar to `git_config_get_string_const`, except that retrieved value - * copied into the `dest` parameter is a mutable string. - */ int git_config_get_string(const char *key, char **dest); /** - * Similar to `git_config_get_string_const`, but does not allocate any new + * Similar to `git_config_get_string`, but does not allocate any new * memory; on success `dest` will point to memory owned by the config * machinery, which could be invalidated if it is discarded and reloaded. */ diff --git a/environment.c b/environment.c index 52e0c979ba..bb518c61cd 100644 --- a/environment.c +++ b/environment.c @@ -35,8 +35,8 @@ int repository_format_precious_objects; int repository_format_worktree_config; const char *git_commit_encoding; const char *git_log_output_encoding; -const char *apply_default_whitespace; -const char *apply_default_ignorewhitespace; +char *apply_default_whitespace; +char *apply_default_ignorewhitespace; const char *git_attributes_file; const char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; From patchwork Fri Aug 14 16:19:46 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714763 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 57DEF13A4 for ; Fri, 14 Aug 2020 16:19:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4917220829 for ; Fri, 14 Aug 2020 16:19:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728272AbgHNQTs (ORCPT ); Fri, 14 Aug 2020 12:19:48 -0400 Received: from cloud.peff.net ([104.130.231.41]:59284 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727886AbgHNQTs (ORCPT ); Fri, 14 Aug 2020 12:19:48 -0400 Received: (qmail 1104 invoked by uid 109); 14 Aug 2020 16:19:47 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:19:47 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26961 invoked by uid 111); 14 Aug 2020 16:19:46 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:19:46 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:19:46 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 5/6] config: fix leak in git_config_get_expiry_in_days() Message-ID: <20200814161946.GE595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index 8bb1945aa9..968ef28e5b 100644 --- a/config.c +++ b/config.c @@ -2312,11 +2312,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))) { From patchwork Fri Aug 14 16:20:35 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11714765 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 93DC413A4 for ; Fri, 14 Aug 2020 16:20:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7962F20774 for ; Fri, 14 Aug 2020 16:20:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728273AbgHNQUg (ORCPT ); Fri, 14 Aug 2020 12:20:36 -0400 Received: from cloud.peff.net ([104.130.231.41]:59290 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727886AbgHNQUg (ORCPT ); Fri, 14 Aug 2020 12:20:36 -0400 Received: (qmail 1117 invoked by uid 109); 14 Aug 2020 16:20:36 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Aug 2020 16:20:36 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26978 invoked by uid 111); 14 Aug 2020 16:20:35 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Aug 2020 12:20:35 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Aug 2020 12:20:35 -0400 From: Jeff King To: git@vger.kernel.org Cc: Eric Sunshine Subject: [PATCH 6/6] submodule--helper: fix leak of core.worktree value Message-ID: <20200814162035.GF595698@coredump.intra.peff.net> References: <20200814161328.GA153929@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200814161328.GA153929@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org In the ensure_core_worktree() function, we load the core.worktree value of the submodule repository using repo_config_get_string(). This function copies the string, but we never free it, leaking the memory. We can instead use the "tmp" version of that function to avoid the allocation at all. We don't have to worry about lifetime issues, since we never even look at the value (we just want to know if it's set). Signed-off-by: Jeff King --- builtin/submodule--helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index e09605716e..a59d8e4bda 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2101,7 +2101,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix) { const struct submodule *sub; const char *path; - char *cw; + const char *cw; struct repository subrepo; if (argc != 2) @@ -2116,7 +2116,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix) if (repo_submodule_init(&subrepo, the_repository, sub)) die(_("could not get a repository handle for submodule '%s'"), path); - if (!repo_config_get_string(&subrepo, "core.worktree", &cw)) { + if (!repo_config_get_string_tmp(&subrepo, "core.worktree", &cw)) { char *cfg_file, *abs_path; const char *rel_path; struct strbuf sb = STRBUF_INIT;