Subject: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe

From: Atneya Nair <[email-masked]>
Date: Mon, 4 Mar 2024 17:21:11 -0800

Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
to remove unsafe shared buffer usage in read_gitfile_gently.

Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
out params to allocate their return values into, rather than returning
a view into a shared buffer.

Leave the shared buffer in case a caller passes null for this param (for
cases we haven't cleaned up yet).

Migrate callers of resolve_gitfile to resolve_gitfile_gently.

Signed-off-by: Atneya Nair <[email-masked]>
---

Notes:
    Open questions:
     - Is checking the return value of read_gitfile necessary if on error,
       we are supposed to die, or set the error field to a non-zero value?
     - Should we clean up the other call-sites of read_gitfile?

 builtin/init-db.c   |  7 ++++---
 builtin/rev-parse.c |  4 +++-
 repository.c        |  9 +++++----
 setup.c             | 36 +++++++++++++++++++++++++-----------
 setup.h             |  7 +++----
 submodule.c         | 32 +++++++++++++++++++++++---------
 worktree.c          | 27 +++++++++++++--------------
 7 files changed, 76 insertions(+), 46 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0170469b84..9135d07a0d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	 */
 	if (real_git_dir) {
 		int err;
-		const char *p;
 		struct strbuf sb = STRBUF_INIT;
+		struct strbuf gitfile = STRBUF_INIT;
 
-		p = read_gitfile_gently(git_dir, &err);
-		if (p && get_common_dir(&sb, p)) {
+		read_gitfile_gently(git_dir, &err, &gitfile);
+		if (!err && get_common_dir(&sb, gitfile.buf)) {
 			struct strbuf mainwt = STRBUF_INIT;
 
 			strbuf_addbuf(&mainwt, &sb);
@@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			git_dir = strbuf_detach(&sb, NULL);
 		}
 		strbuf_release(&sb);
+		strbuf_release(&gitfile);
 	}
 
 	if (is_bare_repository_cfg < 0)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index d08987646a..e1db6b3231 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--resolve-git-dir")) {
 				const char *gitdir = argv[++i];
+				struct strbuf resolved_gitdir = STRBUF_INIT;
 				if (!gitdir)
 					die(_("--resolve-git-dir requires an argument"));
-				gitdir = resolve_gitdir(gitdir);
+				gitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);
 				if (!gitdir)
 					die(_("not a gitdir '%s'"), argv[i]);
 				puts(gitdir);
+				strbuf_release(&resolved_gitdir);
 				continue;
 			}
 		}
diff --git a/repository.c b/repository.c
index 7aacb51b65..3ca6dbcf16 100644
--- a/repository.c
+++ b/repository.c
@@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
 	int ret = 0;
 	int error = 0;
 	char *abspath = NULL;
-	const char *resolved_gitdir;
+	struct strbuf resolved_gitdir = STRBUF_INIT;
 	struct set_gitdir_args args = { NULL };
 
 	abspath = real_pathdup(gitdir, 0);
@@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
 	}
 
 	/* 'gitdir' must reference the gitdir directly */
-	resolved_gitdir = resolve_gitdir_gently(abspath, &error);
-	if (!resolved_gitdir) {
+	resolve_gitdir_gently(abspath, &error, &resolved_gitdir);
+	if (error) {
 		ret = -1;
 		goto out;
 	}
 
-	repo_set_gitdir(repo, resolved_gitdir, &args);
+	repo_set_gitdir(repo, resolved_gitdir.buf, &args);
 
 out:
+	strbuf_release(&resolved_gitdir);
 	free(abspath);
 	return ret;
 }
diff --git a/setup.c b/setup.c
index b69b1cbc2a..2e118cf216 100644
--- a/setup.c
+++ b/setup.c
@@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)
 	int ret = 0;
 	int gitfile_error;
 	size_t orig_path_len = path->len;
+	struct strbuf gitfile = STRBUF_INIT;
+
 	assert(orig_path_len != 0);
 	strbuf_complete(path, '/');
 	strbuf_addstr(path, ".git");
-	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
+	if (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))
 		ret = 1;
 	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
 	    gitfile_error == READ_GITFILE_ERR_READ_FAILED)
 		ret = 1;
+	strbuf_release(&gitfile);
 	strbuf_setlen(path, orig_path_len);
 	return ret;
 }
@@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 
 /*
  * Try to read the location of the git directory from the .git file,
- * return path to git directory if found. The return value comes from
+ * return path to git directory if found. If passed a valid strbuf, the return
+ * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
  * a shared buffer.
  *
  * On failure, if return_error_code is not NULL, return_error_code
@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
  * return_error_code is NULL the function will die instead (for most
  * cases).
  */
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	struct stat st;
 	int fd;
 	ssize_t len;
-	static struct strbuf realpath = STRBUF_INIT;
+	static struct strbuf shared = STRBUF_INIT;
+	if (!result_buf) {
+		result_buf = &shared;
+	}
 
 	if (stat(path, &st)) {
 		/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		goto cleanup_return;
 	}
 
-	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	strbuf_realpath(result_buf, dir, 1);
+	path = result_buf->buf;
+	// TODO is this valid?
+	if (!path) die(_("Unexpected null from realpath '%s'"), dir);
 
 cleanup_return:
 	if (return_error_code)
@@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		int offset = dir->len, error_code = 0;
 		char *gitdir_path = NULL;
 		char *gitfile = NULL;
+		struct strbuf gitdirenvbuf = STRBUF_INIT;
 
 		if (offset > min_offset)
 			strbuf_addch(dir, '/');
 		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
 		gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
-						NULL : &error_code);
+						NULL : &error_code, &gitdirenvbuf);
 		if (!gitdirenv) {
 			if (die_on_error ||
 			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
@@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
 					gitdir_path = xstrdup(dir->buf);
 				}
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
+				strbuf_release(&gitdirenvbuf);
 				return GIT_DIR_INVALID_GITFILE;
+			}
 		} else
 			gitfile = xstrdup(dir->buf);
 		/*
@@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			 */
 			free(gitdir_path);
 			free(gitfile);
-
+			strbuf_release(&gitdirenvbuf);
 			return ret;
 		}
+		strbuf_release(&gitdirenvbuf);
 
 		if (is_git_directory(dir->buf)) {
 			trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
@@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)
 	return setup_git_directory_gently(NULL);
 }
 
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,
+		struct strbuf* return_buf)
 {
 	if (is_git_directory(suspect))
 		return suspect;
-	return read_gitfile_gently(suspect, return_error_code);
+	return read_gitfile_gently(suspect, return_error_code, return_buf);
 }
 
 /* if any standard file descriptor is missing open it to /dev/null */
diff --git a/setup.h b/setup.h
index 3599aec93c..cf5a63762b 100644
--- a/setup.h
+++ b/setup.h
@@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
-const char *read_gitfile_gently(const char *path, int *return_error_code);
-#define read_gitfile(path) read_gitfile_gently((path), NULL)
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
-#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);
+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);
 
 void setup_work_tree(void);
 
diff --git a/submodule.c b/submodule.c
index 213da79f66..455444321b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
 	int ret = 0;
 	char *gitdir = xstrfmt("%s/.git", path);
 
-	if (resolve_gitdir_gently(gitdir, return_error_code))
+	struct strbuf resolved_gitdir_buf = STRBUF_INIT;
+	if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
 		ret = 1;
-
+	strbuf_release(&resolved_gitdir_buf);
 	free(gitdir);
 	return ret;
 }
@@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf gitdirbuf = STRBUF_INIT;
 	FILE *fp;
 	unsigned dirty_submodule = 0;
 	const char *git_dir;
 	int ignore_cp_exit_code = 0;
 
 	strbuf_addf(&buf, "%s/.git", path);
-	git_dir = read_gitfile(buf.buf);
+	git_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);
 	if (!git_dir)
 		git_dir = buf.buf;
 	if (!is_git_directory(git_dir)) {
 		if (is_directory(git_dir))
 			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
+		strbuf_release(&gitdirbuf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
 	}
+		strbuf_release(&gitdirbuf);
 	strbuf_reset(&buf);
 
 	strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
@@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	const char *git_dir;
+	struct strbuf gitfilebuf = STRBUF_INIT;
 
 	strbuf_addf(&buf, "%s/.git", path);
-	git_dir = read_gitfile(buf.buf);
-	if (!git_dir) {
+	read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
+	if (!gitfilebuf.buf) {
 		strbuf_release(&buf);
 		return 0;
 	}
 	strbuf_release(&buf);
+	strbuf_release(&gitfilebuf);
 
 	/* Now test that all nested submodules use a gitfile too */
 	strvec_pushl(&cp.args,
@@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,
 {
 	char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
 	struct strbuf new_gitdir = STRBUF_INIT;
+	struct strbuf gitfilebuf = STRBUF_INIT;
 	const struct submodule *sub;
 
 	if (submodule_uses_worktrees(path))
@@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,
 		      "more than one worktree not supported"), path);
 
 	old_git_dir = xstrfmt("%s/.git", path);
-	if (read_gitfile(old_git_dir))
+	if (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {
 		/* If it is an actual gitfile, it doesn't need migration. */
+		strbuf_release(&gitfilebuf);
 		return;
+	}
+	strbuf_release(&gitfilebuf);
 
 	real_old_git_dir = real_pathdup(old_git_dir, 1);
 
@@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,
 	int err_code;
 	const char *sub_git_dir;
 	struct strbuf gitdir = STRBUF_INIT;
+	struct strbuf resolved_gitdir_buf = STRBUF_INIT;
 	strbuf_addf(&gitdir, "%s/.git", path);
-	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
+	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);
 
 	/* Not populated? */
 	if (!sub_git_dir) {
@@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,
 		free(real_sub_git_dir);
 		free(real_common_git_dir);
 	}
+
+	strbuf_release(&resolved_gitdir_buf);
 	strbuf_release(&gitdir);
 
 	absorb_git_dir_into_superproject_recurse(path, super_prefix);
@@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
 	const struct submodule *sub;
 	const char *git_dir;
 	int ret = 0;
+	struct strbuf gitfilebuf = STRBUF_INIT;
 
 	strbuf_reset(buf);
 	strbuf_addstr(buf, submodule);
 	strbuf_complete(buf, '/');
 	strbuf_addstr(buf, ".git");
 
-	git_dir = read_gitfile(buf->buf);
+	git_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);
 	if (git_dir) {
 		strbuf_reset(buf);
 		strbuf_addstr(buf, git_dir);
 	}
+	strbuf_release(&gitfilebuf);
 	if (!is_git_directory(buf->buf)) {
 		sub = submodule_from_path(the_repository, null_oid(),
 					  submodule);
diff --git a/worktree.c b/worktree.c
index b02a05a74a..a6f125c8da 100644
--- a/worktree.c
+++ b/worktree.c
@@ -309,7 +309,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 {
 	struct strbuf wt_path = STRBUF_INIT;
 	struct strbuf realpath = STRBUF_INIT;
-	char *path = NULL;
+	struct strbuf gitfile = STRBUF_INIT;
 	int err, ret = -1;
 
 	strbuf_addf(&wt_path, "%s/.git", wt->path);
@@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		goto done;
 	}
 
-	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
-	if (!path) {
+	if (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {
 		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
 				   wt_path.buf, err);
 		goto done;
 	}
 
 	strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
-	ret = fspathcmp(path, realpath.buf);
+	ret = fspathcmp(gitfile.buf, realpath.buf);
 
 	if (ret)
 		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
 				   wt->path, git_common_path("worktrees/%s", wt->id));
 done:
-	free(path);
+	strbuf_release(&gitfile);
 	strbuf_release(&wt_path);
 	strbuf_release(&realpath);
 	return ret;
@@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,
 {
 	struct strbuf dotgit = STRBUF_INIT;
 	struct strbuf repo = STRBUF_INIT;
-	char *backlink;
+	struct strbuf backlink = STRBUF_INIT;
 	const char *repair = NULL;
 	int err;
 
@@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,
 
 	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
 	strbuf_addf(&dotgit, "%s/.git", wt->path);
-	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+	read_gitfile_gently(dotgit.buf, &err, &backlink);
 
 	if (err == READ_GITFILE_ERR_NOT_A_FILE)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
-	else if (fspathcmp(backlink, repo.buf))
+	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");
 
 	if (repair) {
@@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,
 		write_file(dotgit.buf, "gitdir: %s", repo.buf);
 	}
 
-	free(backlink);
+	strbuf_release(&backlink);
 	strbuf_release(&repo);
 	strbuf_release(&dotgit);
 }
@@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
 	struct strbuf realdotgit = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
 	struct strbuf olddotgit = STRBUF_INIT;
-	char *backlink = NULL;
+	struct strbuf backlink = STRBUF_INIT;
 	const char *repair = NULL;
 	int err;
 
@@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
 		goto done;
 	}
 
-	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+	read_gitfile_gently(realdotgit.buf, &err, &backlink);
 	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
 		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-		if (!(backlink = infer_backlink(realdotgit.buf))) {
+		if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
 			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
 			goto done;
 		}
@@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,
 		goto done;
 	}
 
-	strbuf_addf(&gitdir, "%s/gitdir", backlink);
+	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
 	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
 		repair = _("gitdir unreadable");
 	else {
@@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,
 		write_file(gitdir.buf, "%s", realdotgit.buf);
 	}
 done:
-	free(backlink);
+	strbuf_release(&backlink);
 	strbuf_release(&olddotgit);
 	strbuf_release(&gitdir);
 	strbuf_release(&realdotgit);
-- 
2.44.0.rc1.240.g4c46232300-goog