From patchwork Mon Aug 17 21:33:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719227 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 DBF2B618 for ; Mon, 17 Aug 2020 21:33:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CD23120760 for ; Mon, 17 Aug 2020 21:33:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728420AbgHQVdE (ORCPT ); Mon, 17 Aug 2020 17:33:04 -0400 Received: from cloud.peff.net ([104.130.231.41]:33554 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728355AbgHQVdD (ORCPT ); Mon, 17 Aug 2020 17:33:03 -0400 Received: (qmail 6680 invoked by uid 109); 17 Aug 2020 21:33:03 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:03 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22252 invoked by uid 111); 17 Aug 2020 21:33:02 -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; Mon, 17 Aug 2020 17:33:02 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:02 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 1/7] clear_pattern_list(): clear embedded hashmaps Message-ID: <20200817213302.GA1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns, 2019-11-21) added some auxiliary hashmaps to the pattern_list struct, but they're leaked when clear_pattern_list() is called. Signed-off-by: Jeff King --- dir.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dir.c b/dir.c index fe64be30ed..9411b94e9b 100644 --- a/dir.c +++ b/dir.c @@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl) free(pl->patterns[i]); free(pl->patterns); free(pl->filebuf); + hashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent); + hashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent); memset(pl, 0, sizeof(*pl)); } From patchwork Mon Aug 17 21:33:04 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719229 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 603EE15E6 for ; Mon, 17 Aug 2020 21:33:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4617220738 for ; Mon, 17 Aug 2020 21:33:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728439AbgHQVdJ (ORCPT ); Mon, 17 Aug 2020 17:33:09 -0400 Received: from cloud.peff.net ([104.130.231.41]:33556 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726634AbgHQVdG (ORCPT ); Mon, 17 Aug 2020 17:33:06 -0400 Received: (qmail 6686 invoked by uid 109); 17 Aug 2020 21:33:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22263 invoked by uid 111); 17 Aug 2020 21:33:04 -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; Mon, 17 Aug 2020 17:33:04 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:04 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 2/7] submodule--helper: use strbuf_release() to free strbufs Message-ID: <20200817213304.GB1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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 Mon Aug 17 21:33:06 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719231 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 67764618 for ; Mon, 17 Aug 2020 21:33:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5830920758 for ; Mon, 17 Aug 2020 21:33:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728449AbgHQVdP (ORCPT ); Mon, 17 Aug 2020 17:33:15 -0400 Received: from cloud.peff.net ([104.130.231.41]:33564 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728432AbgHQVdI (ORCPT ); Mon, 17 Aug 2020 17:33:08 -0400 Received: (qmail 6697 invoked by uid 109); 17 Aug 2020 21:33:07 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:07 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22268 invoked by uid 111); 17 Aug 2020 21:33:06 -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; Mon, 17 Aug 2020 17:33:06 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:06 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 3/7] checkout: fix leak of non-existent branch names Message-ID: <20200817213306.GC1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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 Mon Aug 17 21:33:08 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719233 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 267C7618 for ; Mon, 17 Aug 2020 21:33:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0F17B20738 for ; Mon, 17 Aug 2020 21:33:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728452AbgHQVdQ (ORCPT ); Mon, 17 Aug 2020 17:33:16 -0400 Received: from cloud.peff.net ([104.130.231.41]:33572 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728440AbgHQVdL (ORCPT ); Mon, 17 Aug 2020 17:33:11 -0400 Received: (qmail 6706 invoked by uid 109); 17 Aug 2020 21:33:09 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:09 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22273 invoked by uid 111); 17 Aug 2020 21:33:09 -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; Mon, 17 Aug 2020 17:33:09 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:08 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 4/7] config: fix leaks from git_config_get_string_const() Message-ID: <20200817213308.GD1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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 Mon Aug 17 21:33:11 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719239 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 919A016B1 for ; Mon, 17 Aug 2020 21:33:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7E81B20758 for ; Mon, 17 Aug 2020 21:33:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728462AbgHQVdZ (ORCPT ); Mon, 17 Aug 2020 17:33:25 -0400 Received: from cloud.peff.net ([104.130.231.41]:33574 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726634AbgHQVdQ (ORCPT ); Mon, 17 Aug 2020 17:33:16 -0400 Received: (qmail 6714 invoked by uid 109); 17 Aug 2020 21:33:12 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:12 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22278 invoked by uid 111); 17 Aug 2020 21:33:11 -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; Mon, 17 Aug 2020 17:33:11 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:11 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 5/7] config: drop git_config_get_string_const() Message-ID: <20200817213311.GE1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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. 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 --- Documentation/MyFirstContribution.txt | 4 ++-- apply.c | 4 ++-- cache.h | 4 ++-- checkout.c | 3 +-- config.c | 29 ++++++--------------------- config.h | 11 +--------- environment.c | 4 ++-- 7 files changed, 16 insertions(+), 43 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..f0367c76ad 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")) { diff --git a/config.h b/config.h index a75b22e0d1..91cdfbfb41 100644 --- a/config.h +++ b/config.h @@ -458,7 +458,6 @@ void git_configset_clear(struct config_set *cs); */ int git_configset_get_value(struct config_set *cs, const char *key, const char **dest); -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); @@ -475,8 +474,6 @@ int repo_config_get_value(struct repository *repo, const char *key, const char **value); const struct string_list *repo_config_get_value_multi(struct repository *repo, const char *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 repo_config_get_string_tmp(struct repository *repo, @@ -532,16 +529,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 Mon Aug 17 21:33:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719235 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 F0CE6618 for ; Mon, 17 Aug 2020 21:33:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E2AAC2065D for ; Mon, 17 Aug 2020 21:33:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728458AbgHQVdW (ORCPT ); Mon, 17 Aug 2020 17:33:22 -0400 Received: from cloud.peff.net ([104.130.231.41]:33590 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728445AbgHQVdP (ORCPT ); Mon, 17 Aug 2020 17:33:15 -0400 Received: (qmail 6730 invoked by uid 109); 17 Aug 2020 21:33:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22310 invoked by uid 111); 17 Aug 2020 21:33: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; Mon, 17 Aug 2020 17:33:14 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:13 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 6/7] config: fix leak in git_config_get_expiry_in_days() Message-ID: <20200817213313.GF1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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 f0367c76ad..2bdff4457b 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))) { From patchwork Mon Aug 17 21:33:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11719237 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 0038A618 for ; Mon, 17 Aug 2020 21:33:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DB4852065D for ; Mon, 17 Aug 2020 21:33:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728461AbgHQVdX (ORCPT ); Mon, 17 Aug 2020 17:33:23 -0400 Received: from cloud.peff.net ([104.130.231.41]:33596 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728440AbgHQVdR (ORCPT ); Mon, 17 Aug 2020 17:33:17 -0400 Received: (qmail 6741 invoked by uid 109); 17 Aug 2020 21:33:17 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 17 Aug 2020 21:33:17 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22315 invoked by uid 111); 17 Aug 2020 21:33:16 -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; Mon, 17 Aug 2020 17:33:16 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 17 Aug 2020 17:33:16 -0400 From: Jeff King To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine Subject: [PATCH v2 7/7] submodule--helper: fix leak of core.worktree value Message-ID: <20200817213316.GG1854722@coredump.intra.peff.net> References: <20200817213228.GA1854603@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200817213228.GA1854603@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;