git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Martin Ågren <martin.agren@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>,
	"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: [PATCH v2 3/3] setup: add `clear_repository_format()`
Date: Mon, 14 Jan 2019 19:34:57 +0100
Message-ID: <7b0ca165bce684bde38b753ab98f7858f20130c0.1547488709.git.martin.agren@gmail.com> (raw)
In-Reply-To: <cover.1547488709.git.martin.agren@gmail.com>

After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.

Fixing these memory leaks is complicated by two issues:

  1) As described above, we "steal" the fields of the struct in case of
     success.

  2) We might end up calling `read_repository_format()` more than once
     -- as we enter it a second time, we lose track of our pointers and
     leak memory. As a further complication, we do not initialize (zero)
     the structs before calling `read_...()` so as we first enter it, we
     cannot blindly free the pointers in it.

To free this memory, in light of (1), we must either carefully cover all
error paths but no success paths; or we should stop grabbing the
pointers. To address 2), we must either zero the struct before calling
`read_repository_format()`, or try to keep track of when we should zero
it and when we should first free the memory.

Introduce `REPO_FORMAT_INIT` and `clear_repository_format()` to be used
on each side of `read_repository_format()`. Make the users duplicate the
memory from the structs, rather than copying the pointers.

Call `clear_...()` at the start of `read_...()` instead of just zeroing
the struct. In the error path of the same function, be sure to restore
the error sentinel after we clear it with the rest of the struct.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 I do wonder if introducing and depending on `REPOSITORY_FORMAT_INIT`
 like this is 100% sane. Out-of-tree users could be in for a nasty
 surprise. :-/

 cache.h           | 12 ++++++++++++
 builtin/init-db.c |  3 ++-
 repository.c      |  3 ++-
 setup.c           | 30 ++++++++++++++++++++++--------
 worktree.c        |  4 +++-
 5 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index ca36b44ee0..3ef63d27c4 100644
--- a/cache.h
+++ b/cache.h
@@ -972,6 +972,12 @@ struct repository_format {
 	struct string_list unknown_extensions;
 };
 
+/**
+ * Always use this to initialize a `struct repository_format`
+ * to a well-defined state before calling `read_repository()`.
+ */
+#define REPOSITORY_FORMAT_INIT { 0 }
+
 /*
  * Read the repository format characteristics from the config file "path" into
  * "format" struct. Returns the numeric version. On error, -1 is returned,
@@ -980,6 +986,12 @@ struct repository_format {
  */
 int read_repository_format(struct repository_format *format, const char *path);
 
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
 /*
  * Verify that the repository described by repository_format is something we
  * can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 41faffd28d..04c60eaad5 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;
 	size_t template_len;
-	struct repository_format template_format;
+	struct repository_format template_format = REPOSITORY_FORMAT_INIT;
 	struct strbuf err = STRBUF_INIT;
 	DIR *dir;
 	char *to_free = NULL;
@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)
 	free(to_free);
 	strbuf_release(&path);
 	strbuf_release(&template_path);
+	clear_repository_format(&template_format);
 }
 
 static int git_init_db_config(const char *k, const char *v, void *cb)
diff --git a/repository.c b/repository.c
index 7b02e1dffa..df88705574 100644
--- a/repository.c
+++ b/repository.c
@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,
 	      const char *gitdir,
 	      const char *worktree)
 {
-	struct repository_format format;
+	struct repository_format format = REPOSITORY_FORMAT_INIT;
 	memset(repo, 0, sizeof(*repo));
 
 	repo->objects = raw_object_store_new();
@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,
 	if (worktree)
 		repo_set_worktree(repo, worktree);
 
+	clear_repository_format(&format);
 	return 0;
 
 error:
diff --git a/setup.c b/setup.c
index 4d3d67c50b..70d9007ae5 100644
--- a/setup.c
+++ b/setup.c
@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	}
 
 	repository_format_precious_objects = candidate->precious_objects;
-	repository_format_partial_clone = candidate->partial_clone;
+	repository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);
 	repository_format_worktree_config = candidate->worktree_config;
 	string_list_clear(&candidate->unknown_extensions, 0);
 
@@ -500,11 +500,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 		}
 		if (candidate->work_tree) {
 			free(git_work_tree_cfg);
-			git_work_tree_cfg = candidate->work_tree;
+			git_work_tree_cfg = xstrdup(candidate->work_tree);
 			inside_work_tree = -1;
 		}
-	} else {
-		free(candidate->work_tree);
 	}
 
 	return 0;
@@ -512,15 +510,27 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 
 int read_repository_format(struct repository_format *format, const char *path)
 {
-	memset(format, 0, sizeof(*format));
+	clear_repository_format(format);
 	format->version = -1;
 	format->is_bare = -1;
 	format->hash_algo = GIT_HASH_SHA1;
 	string_list_init(&format->unknown_extensions, 1);
 	git_config_from_file(check_repo_format, path, format);
+	if (format->version == -1) {
+		clear_repository_format(format);
+		format->version = -1;
+	}
 	return format->version;
 }
 
+void clear_repository_format(struct repository_format *format)
+{
+	string_list_clear(&format->unknown_extensions, 0);
+	free(format->work_tree);
+	free(format->partial_clone);
+	memset(format, 0, sizeof(*format));
+}
+
 int verify_repository_format(const struct repository_format *format,
 			     struct strbuf *err)
 {
@@ -1008,7 +1018,7 @@ int discover_git_directory(struct strbuf *commondir,
 	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
 	size_t gitdir_offset = gitdir->len, cwd_len;
 	size_t commondir_offset = commondir->len;
-	struct repository_format candidate;
+	struct repository_format candidate = REPOSITORY_FORMAT_INIT;
 
 	if (strbuf_getcwd(&dir))
 		return -1;
@@ -1045,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,
 		strbuf_release(&err);
 		strbuf_setlen(commondir, commondir_offset);
 		strbuf_setlen(gitdir, gitdir_offset);
+		clear_repository_format(&candidate);
 		return -1;
 	}
 
+	clear_repository_format(&candidate);
 	return 0;
 }
 
@@ -1056,7 +1068,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static struct strbuf cwd = STRBUF_INIT;
 	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
 	const char *prefix;
-	struct repository_format repo_fmt;
+	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 
 	/*
 	 * We may have read an incomplete configuration before
@@ -1146,6 +1158,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	strbuf_release(&dir);
 	strbuf_release(&gitdir);
+	clear_repository_format(&repo_fmt);
 
 	return prefix;
 }
@@ -1203,9 +1216,10 @@ int git_config_perm(const char *var, const char *value)
 
 void check_repository_format(void)
 {
-	struct repository_format repo_fmt;
+	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 	check_repository_format_gently(get_git_dir(), &repo_fmt, NULL);
 	startup_info->have_repository = 1;
+	clear_repository_format(&repo_fmt);
 }
 
 /*
diff --git a/worktree.c b/worktree.c
index d6a0ee7f73..b45bfeb9d3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -444,7 +444,7 @@ int submodule_uses_worktrees(const char *path)
 	DIR *dir;
 	struct dirent *d;
 	int ret = 0;
-	struct repository_format format;
+	struct repository_format format = REPOSITORY_FORMAT_INIT;
 
 	submodule_gitdir = git_pathdup_submodule(path, "%s", "");
 	if (!submodule_gitdir)
@@ -462,8 +462,10 @@ int submodule_uses_worktrees(const char *path)
 	read_repository_format(&format, sb.buf);
 	if (format.version != 0) {
 		strbuf_release(&sb);
+		clear_repository_format(&format);
 		return 1;
 	}
+	clear_repository_format(&format);
 
 	/* Replace config by worktrees. */
 	strbuf_setlen(&sb, sb.len - strlen("config"));
-- 
2.20.1


      parent reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-18  7:25 [PATCH 0/3] " Martin Ågren
2018-12-18  7:25 ` [PATCH 1/3] setup: drop return value from `read_repository_format()` Martin Ågren
2018-12-19 15:27   ` Jeff King
2018-12-19 21:42     ` Martin Ågren
2018-12-20  0:17     ` brian m. carlson
2018-12-20  2:52       ` Jeff King
2018-12-20  3:45         ` brian m. carlson
2018-12-20 14:53           ` Jeff King
2018-12-18  7:25 ` [PATCH 2/3] setup: do not use invalid `repository_format` Martin Ågren
2018-12-19  0:18   ` brian m. carlson
2018-12-19 21:43     ` Martin Ågren
2018-12-19 15:38   ` Jeff King
2018-12-19 21:46     ` Martin Ågren
2018-12-19 23:17       ` Jeff King
2018-12-20  0:21     ` brian m. carlson
2018-12-18  7:25 ` [PATCH 3/3] setup: add `clear_repository_format()` Martin Ågren
2018-12-19 15:48   ` Jeff King
2018-12-19 21:49     ` Martin Ågren
2019-01-14 18:34 ` [PATCH v2 0/3] " Martin Ågren
2019-01-14 18:34   ` [PATCH v2 1/3] setup: free old value before setting `work_tree` Martin Ågren
2019-01-14 18:34   ` [PATCH v2 2/3] setup: do not use invalid `repository_format` Martin Ågren
2019-01-15 19:31     ` Jeff King
2019-01-17  6:31       ` Martin Ågren
2019-01-22  7:07         ` Jeff King
2019-01-22 13:34           ` Martin Ågren
2019-01-22 21:45             ` [PATCH v3 0/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-01-22 21:45               ` [PATCH v3 1/2] setup: free old value before setting `work_tree` Martin Ågren
2019-01-22 21:45               ` [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-01-23  5:57                 ` Jeff King
2019-01-24  0:14                   ` brian m. carlson
2019-01-25 19:25                     ` Martin Ågren
2019-01-25 19:24                   ` Martin Ågren
2019-01-25 19:51                     ` Jeff King
2019-02-25 19:21                       ` Martin Ågren
2019-02-26 17:46                         ` Jeff King
2019-02-28 20:36                           ` [PATCH v4 0/2] " Martin Ågren
2019-02-28 20:36                             ` [PATCH v4 1/2] setup: free old value before setting `work_tree` Martin Ågren
2019-02-28 20:36                             ` [PATCH v4 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren
2019-03-06  4:56                             ` [PATCH v4 0/2] " Jeff King
2019-01-14 18:34   ` Martin Ågren [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7b0ca165bce684bde38b753ab98f7858f20130c0.1547488709.git.martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox