[PATCH 0/4] Fix bugs related to real_path()

[PATCH 0/4] Fix bugs related to real_path()

[Alexandr Miloslavskiy via GitGitGadget]

The issue with `real_path()` seems to be long-standing, where multiple
people solved parts of it over time. I'm adding another part here
after I have discovered a crash related to it.

Even with this step, there are still problems remaining:
* `read_gitfile_gently()` still uses shared buffer.
* `absolute_path()` was not removed.

These issues remain because there're too many code references and I'd like
to avoid submitting a single topic of a scary size.

Alexandr Miloslavskiy (4):
  set_git_dir: fix crash when used with real_path()
  real_path: remove unsafe API
  real_path_if_valid(): remove unsafe API
  get_superproject_working_tree(): return strbuf

 abspath.c                  | 18 +-----------------
 builtin/clone.c            |  7 ++++++-
 builtin/commit-graph.c     |  6 +++++-
 builtin/init-db.c          |  4 ++--
 builtin/rev-parse.c        | 12 ++++++++----
 builtin/worktree.c         | 10 +++++++---
 cache.h                    |  4 +---
 editor.c                   | 11 +++++++++--
 environment.c              | 18 ++++++++++++++++--
 path.c                     |  4 ++--
 setup.c                    | 37 ++++++++++++++++++++++++-------------
 sha1-file.c                | 13 ++++---------
 submodule.c                | 22 ++++++++++++----------
 submodule.h                |  4 ++--
 t/helper/test-path-utils.c |  6 +++++-
 worktree.c                 | 13 ++++++++++---
 16 files changed, 114 insertions(+), 75 deletions(-)


base-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-575%2FSyntevoAlex%2F%230205(git)_crash_real_path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-575/SyntevoAlex/#0205(git)_crash_real_path-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/575
-- 
gitgitgadget

[PATCH 1/4] set_git_dir: fix crash when used with real_path()

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

`real_path()` returns result from a shared buffer, inviting subtle
reentrance bugs. One of these bugs occur when invoked this way:
    set_git_dir(real_path(git_dir))

In this case, `real_path()` has reentrance:
    real_path
    read_gitfile_gently
    repo_set_gitdir
    setup_git_env
    set_git_dir_1
    set_git_dir

Later, `set_git_dir()` uses its now-dead parameter:
    !is_absolute_path(path)

Fix this by using a dedicated `strbuf` to hold `strbuf_realpath()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/init-db.c |  4 ++--
 cache.h           |  2 +-
 environment.c     | 11 ++++++++++-
 path.c            |  2 +-
 setup.c           | 18 +++++++++---------
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 944ec77fe10..5bf61a7e056 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -356,12 +356,12 @@ int init_db(const char *git_dir, const char *real_git_dir,
 		if (!exist_ok && !stat(real_git_dir, &st))
 			die(_("%s already exists"), real_git_dir);
 
-		set_git_dir(real_path(real_git_dir));
+		set_git_dir(real_git_dir, 1);
 		git_dir = get_git_dir();
 		separate_git_dir(git_dir, original_git_dir);
 	}
 	else {
-		set_git_dir(real_path(git_dir));
+		set_git_dir(git_dir, 1);
 		git_dir = get_git_dir();
 	}
 	startup_info->have_repository = 1;
diff --git a/cache.h b/cache.h
index 37c899b53f7..8cee257d3d7 100644
--- a/cache.h
+++ b/cache.h
@@ -543,7 +543,7 @@ const char *get_git_common_dir(void);
 char *get_object_directory(void);
 char *get_index_file(void);
 char *get_graft_file(struct repository *r);
-void set_git_dir(const char *path);
+void set_git_dir(const char *path, int make_realpath);
 int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 int get_common_dir(struct strbuf *sb, const char *gitdir);
 const char *get_git_namespace(void);
diff --git a/environment.c b/environment.c
index e72a02d0d57..c436de31eef 100644
--- a/environment.c
+++ b/environment.c
@@ -345,11 +345,20 @@ static void update_relative_gitdir(const char *name,
 	free(path);
 }
 
-void set_git_dir(const char *path)
+void set_git_dir(const char *path, int make_realpath)
 {
+	struct strbuf realpath = STRBUF_INIT;
+
+	if (make_realpath) {
+		strbuf_realpath(&realpath, path, 1);
+		path = realpath.buf;
+	}
+
 	set_git_dir_1(path);
 	if (!is_absolute_path(path))
 		chdir_notify_register(NULL, update_relative_gitdir, NULL);
+
+	strbuf_release(&realpath);
 }
 
 const char *get_log_output_encoding(void)
diff --git a/path.c b/path.c
index 88cf5930073..c5a8fe4f0c3 100644
--- a/path.c
+++ b/path.c
@@ -850,7 +850,7 @@ const char *enter_repo(const char *path, int strict)
 	}
 
 	if (is_git_directory(".")) {
-		set_git_dir(".");
+		set_git_dir(".", 0);
 		check_repository_format();
 		return path;
 	}
diff --git a/setup.c b/setup.c
index 4ea7a0b081b..fa4317e707a 100644
--- a/setup.c
+++ b/setup.c
@@ -725,7 +725,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		}
 
 		/* #18, #26 */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
@@ -747,7 +747,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	}
 	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
 		/* #16d */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
@@ -759,14 +759,14 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 
 	/* both get_git_work_tree() and cwd are already normalized */
 	if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
 
 	offset = dir_inside_of(cwd->buf, worktree);
 	if (offset >= 0) {	/* cwd inside worktree? */
-		set_git_dir(real_path(gitdirenv));
+		set_git_dir(gitdirenv, 1);
 		if (chdir(worktree))
 			die_errno(_("cannot chdir to '%s'"), worktree);
 		strbuf_addch(cwd, '/');
@@ -775,7 +775,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	}
 
 	/* cwd outside worktree */
-	set_git_dir(gitdirenv);
+	set_git_dir(gitdirenv, 0);
 	free(gitfile);
 	return NULL;
 }
@@ -804,7 +804,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 
 	/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
 	if (is_bare_repository_cfg > 0) {
-		set_git_dir(offset == cwd->len ? gitdir : real_path(gitdir));
+		set_git_dir(gitdir, (offset != cwd->len));
 		if (chdir(cwd->buf))
 			die_errno(_("cannot come back to cwd"));
 		return NULL;
@@ -813,7 +813,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	/* #0, #1, #5, #8, #9, #12, #13 */
 	set_git_work_tree(".");
 	if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
-		set_git_dir(gitdir);
+		set_git_dir(gitdir, 0);
 	inside_git_dir = 0;
 	inside_work_tree = 1;
 	if (offset >= cwd->len)
@@ -856,10 +856,10 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
 			die_errno(_("cannot come back to cwd"));
 		root_len = offset_1st_component(cwd->buf);
 		strbuf_setlen(cwd, offset > root_len ? offset : root_len);
-		set_git_dir(cwd->buf);
+		set_git_dir(cwd->buf, 0);
 	}
 	else
-		set_git_dir(".");
+		set_git_dir(".", 0);
 	return NULL;
 }
 
-- 
gitgitgadget

[PATCH 2/4] real_path: remove unsafe API

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Returning a shared buffer invites very subtle bugs due to reentrancy or
multi-threading, as demonstrated by the previous patch.

There was an unfinished effort to abolish this [1].

Let's finally rid of `real_path()`, using `strbuf_realpath()` instead.

This patch uses a local `strbuf` for most places where `real_path()` was
previously called.

However, two places return the value of `real_path()` to the caller. For
them, a `static` local `strbuf` was added, effectively pushing the
problem one level higher:
    read_gitfile_gently()
    get_superproject_working_tree()

[1] https://lore.kernel.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 abspath.c                  |  8 +-------
 builtin/clone.c            |  7 ++++++-
 builtin/commit-graph.c     |  6 +++++-
 builtin/rev-parse.c        |  5 ++++-
 builtin/worktree.c         | 10 +++++++---
 cache.h                    |  1 -
 editor.c                   | 11 +++++++++--
 environment.c              |  7 ++++++-
 path.c                     |  2 +-
 setup.c                    | 17 ++++++++++++++---
 submodule.c                |  4 +++-
 t/helper/test-path-utils.c |  6 +++++-
 worktree.c                 |  5 ++++-
 13 files changed, 65 insertions(+), 24 deletions(-)

diff --git a/abspath.c b/abspath.c
index 98579853299..d34026bfeb8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -206,12 +206,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
  * Resolve `path` into an absolute, cleaned-up path. The return value
  * comes from a shared buffer.
  */
-const char *real_path(const char *path)
-{
-	static struct strbuf realpath = STRBUF_INIT;
-	return strbuf_realpath(&realpath, path, 1);
-}
-
 const char *real_path_if_valid(const char *path)
 {
 	static struct strbuf realpath = STRBUF_INIT;
@@ -233,7 +227,7 @@ char *real_pathdup(const char *path, int die_on_error)
 
 /*
  * Use this to get an absolute path from a relative one. If you want
- * to resolve links, you should use real_path.
+ * to resolve links, you should use strbuf_realpath.
  */
 const char *absolute_path(const char *path)
 {
diff --git a/builtin/clone.c b/builtin/clone.c
index 1ad26f4d8c8..e5c2a229a11 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 	struct dir_iterator *iter;
 	int iter_status;
 	unsigned int flags;
+	struct strbuf realpath = STRBUF_INIT;
 
 	mkdir_if_missing(dest->buf, 0777);
 
@@ -454,7 +455,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		if (unlink(dest->buf) && errno != ENOENT)
 			die_errno(_("failed to unlink '%s'"), dest->buf);
 		if (!option_no_hardlinks) {
-			if (!link(real_path(src->buf), dest->buf))
+			strbuf_reset(&realpath);
+			strbuf_realpath(&realpath, src->buf, 1);
+			if (!link(realpath.buf, dest->buf))
 				continue;
 			if (option_local > 0)
 				die_errno(_("failed to create link '%s'"), dest->buf);
@@ -468,6 +471,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		strbuf_setlen(src, src_len);
 		die(_("failed to iterate over '%s'"), src->buf);
 	}
+
+	strbuf_release(&realpath);
 }
 
 static void clone_local(const char *src_repo, const char *dest_repo)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4a70b33fb5f..3d7ec640e01 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -39,14 +39,18 @@ static struct object_directory *find_odb(struct repository *r,
 {
 	struct object_directory *odb;
 	char *obj_dir_real = real_pathdup(obj_dir, 1);
+	struct strbuf odb_path_real = STRBUF_INIT;
 
 	prepare_alt_odb(r);
 	for (odb = r->objects->odb; odb; odb = odb->next) {
-		if (!strcmp(obj_dir_real, real_path(odb->path)))
+		strbuf_reset(&odb_path_real);
+		strbuf_realpath(&odb_path_real, odb->path, 1);
+		if (!strcmp(obj_dir_real, odb_path_real.buf))
 			break;
 	}
 
 	free(obj_dir_real);
+	strbuf_release(&odb_path_real);
 
 	if (!odb)
 		die(_("could not find object directory matching %s"), obj_dir);
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 7a00da82035..06ca7175ac7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -857,7 +857,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					if (!gitdir && !prefix)
 						gitdir = ".git";
 					if (gitdir) {
-						puts(real_path(gitdir));
+						struct strbuf realpath = STRBUF_INIT;
+						strbuf_realpath(&realpath, gitdir, 1);
+						puts(realpath.buf);
+						strbuf_release(&realpath);
 						continue;
 					}
 				}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 24f22800f38..b13b88bec62 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -258,7 +258,7 @@ static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
-	struct strbuf sb = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
 	const char *name;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct argv_array child_env = ARGV_ARRAY_INIT;
@@ -330,9 +330,12 @@ static int add_worktree(const char *path, const char *refname,
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	write_file(sb.buf, "%s", real_path(sb_git.buf));
+	strbuf_realpath(&realpath, sb_git.buf, 1);
+	write_file(sb.buf, "%s", realpath.buf);
+	strbuf_reset(&realpath);
+	strbuf_realpath(&realpath, get_git_common_dir(), 1);
 	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
-		   real_path(get_git_common_dir()), name);
+		   realpath.buf, name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
 	 * or is_git_directory() will reject the directory. Any value which
@@ -418,6 +421,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_release(&sb_repo);
 	strbuf_release(&sb_git);
 	strbuf_release(&sb_name);
+	strbuf_release(&realpath);
 	return ret;
 }
 
diff --git a/cache.h b/cache.h
index 8cee257d3d7..f6937793ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)
 int is_directory(const char *);
 char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
-const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path, int die_on_error);
 const char *absolute_path(const char *path);
diff --git a/editor.c b/editor.c
index f079abbf110..91989ee8a11 100644
--- a/editor.c
+++ b/editor.c
@@ -54,7 +54,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		return error("Terminal is dumb, but EDITOR unset");
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, real_path(path), NULL };
+		struct strbuf realpath = STRBUF_INIT;
+		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
 		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
@@ -75,16 +76,22 @@ static int launch_specified_editor(const char *editor, const char *path,
 			fflush(stderr);
 		}
 
+		strbuf_realpath(&realpath, path, 1);
+		args[1] = realpath.buf;
+
 		p.argv = args;
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		if (start_command(&p) < 0)
+		if (start_command(&p) < 0) {
+			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
+		}
 
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
diff --git a/environment.c b/environment.c
index c436de31eef..10c9061c432 100644
--- a/environment.c
+++ b/environment.c
@@ -254,8 +254,11 @@ static int git_work_tree_initialized;
  */
 void set_git_work_tree(const char *new_work_tree)
 {
+	struct strbuf realpath = STRBUF_INIT;
+
 	if (git_work_tree_initialized) {
-		new_work_tree = real_path(new_work_tree);
+		strbuf_realpath(&realpath, new_work_tree, 1);
+		new_work_tree = realpath.buf;
 		if (strcmp(new_work_tree, the_repository->worktree))
 			die("internal error: work tree has already been set\n"
 			    "Current worktree: %s\nNew worktree: %s",
@@ -264,6 +267,8 @@ void set_git_work_tree(const char *new_work_tree)
 	}
 	git_work_tree_initialized = 1;
 	repo_set_worktree(the_repository, new_work_tree);
+
+	strbuf_release(&realpath);
 }
 
 const char *get_git_work_tree(void)
diff --git a/path.c b/path.c
index c5a8fe4f0c3..0a42ceb3fb5 100644
--- a/path.c
+++ b/path.c
@@ -723,7 +723,7 @@ static struct passwd *getpw_str(const char *username, size_t len)
  * then it is a newly allocated string. Returns NULL on getpw failure or
  * if path is NULL.
  *
- * If real_home is true, real_path($HOME) is used in the expansion.
+ * If real_home is true, strbuf_realpath($HOME) is used in the expansion.
  */
 char *expand_user_path(const char *path, int real_home)
 {
diff --git a/setup.c b/setup.c
index fa4317e707a..19dded55788 100644
--- a/setup.c
+++ b/setup.c
@@ -32,6 +32,7 @@ static int abspath_part_inside_repo(char *path)
 	char *path0;
 	int off;
 	const char *work_tree = get_git_work_tree();
+	struct strbuf realpath = STRBUF_INIT;
 
 	if (!work_tree)
 		return -1;
@@ -60,8 +61,11 @@ static int abspath_part_inside_repo(char *path)
 		path++;
 		if (*path == '/') {
 			*path = '\0';
-			if (fspathcmp(real_path(path0), work_tree) == 0) {
+			strbuf_reset(&realpath);
+			strbuf_realpath(&realpath, path0, 1);
+			if (fspathcmp(realpath.buf, work_tree) == 0) {
 				memmove(path0, path + 1, len - (path - path0));
+				strbuf_release(&realpath);
 				return 0;
 			}
 			*path = '/';
@@ -69,11 +73,15 @@ static int abspath_part_inside_repo(char *path)
 	}
 
 	/* check whole path */
-	if (fspathcmp(real_path(path0), work_tree) == 0) {
+	strbuf_reset(&realpath);
+	strbuf_realpath(&realpath, path0, 1);
+	if (fspathcmp(realpath.buf, work_tree) == 0) {
 		*path0 = '\0';
+		strbuf_release(&realpath);
 		return 0;
 	}
 
+	strbuf_release(&realpath);
 	return -1;
 }
 
@@ -619,6 +627,7 @@ 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;
 
 	if (stat(path, &st)) {
 		/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -669,7 +678,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}
-	path = real_path(dir);
+
+	strbuf_realpath(&realpath, dir, 1);
+	path = realpath.buf;
 
 cleanup_return:
 	if (return_error_code)
diff --git a/submodule.c b/submodule.c
index 31f391d7d25..bad7a788c06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2170,6 +2170,7 @@ void absorb_git_dir_into_superproject(const char *path,
 
 const char *get_superproject_working_tree(void)
 {
+	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *one_up = real_path_if_valid("../");
@@ -2231,7 +2232,8 @@ const char *get_superproject_working_tree(void)
 		super_wt = xstrdup(cwd);
 		super_wt[cwd_len - super_sub_len] = '\0';
 
-		ret = real_path(super_wt);
+		strbuf_realpath(&realpath, super_wt, 1);
+		ret = realpath.buf;
 		free(super_wt);
 	}
 	strbuf_release(&sb);
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 409034cf4ee..40548d31dfe 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -290,11 +290,15 @@ int cmd__path_utils(int argc, const char **argv)
 	}
 
 	if (argc >= 2 && !strcmp(argv[1], "real_path")) {
+		struct strbuf realpath = STRBUF_INIT;
 		while (argc > 2) {
-			puts(real_path(argv[2]));
+			strbuf_reset(&realpath);
+			strbuf_realpath(&realpath, argv[2], 1);
+			puts(realpath.buf);
 			argc--;
 			argv++;
 		}
+		strbuf_release(&realpath);
 		return 0;
 	}
 
diff --git a/worktree.c b/worktree.c
index eba4fd3a038..e7bbf716f6b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -285,6 +285,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		      unsigned flags)
 {
 	struct strbuf wt_path = STRBUF_INIT;
+	struct strbuf realpath = STRBUF_INIT;
 	char *path = NULL;
 	int err, ret = -1;
 
@@ -336,7 +337,8 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		goto done;
 	}
 
-	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+	strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
+	ret = fspathcmp(path, realpath.buf);
 
 	if (ret)
 		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
@@ -344,6 +346,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 done:
 	free(path);
 	strbuf_release(&wt_path);
+	strbuf_release(&realpath);
 	return ret;
 }
 
-- 
gitgitgadget

[PATCH 3/4] real_path_if_valid(): remove unsafe API

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

This commit continues the work started with previous commit.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 abspath.c   | 10 ----------
 cache.h     |  1 -
 setup.c     |  2 +-
 sha1-file.c | 13 ++++---------
 submodule.c |  7 ++++---
 worktree.c  |  8 ++++++--
 6 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/abspath.c b/abspath.c
index d34026bfeb8..6f15a418bb6 100644
--- a/abspath.c
+++ b/abspath.c
@@ -202,16 +202,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
 	return retval;
 }
 
-/*
- * Resolve `path` into an absolute, cleaned-up path. The return value
- * comes from a shared buffer.
- */
-const char *real_path_if_valid(const char *path)
-{
-	static struct strbuf realpath = STRBUF_INIT;
-	return strbuf_realpath(&realpath, path, 0);
-}
-
 char *real_pathdup(const char *path, int die_on_error)
 {
 	struct strbuf realpath = STRBUF_INIT;
diff --git a/cache.h b/cache.h
index f6937793ec2..aa3f5ce718a 100644
--- a/cache.h
+++ b/cache.h
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)
 int is_directory(const char *);
 char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
-const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path, int die_on_error);
 const char *absolute_path(const char *path);
 char *absolute_pathdup(const char *path);
diff --git a/setup.c b/setup.c
index 19dded55788..d319f499d6b 100644
--- a/setup.c
+++ b/setup.c
@@ -888,7 +888,7 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 
 /*
  * A "string_list_each_func_t" function that canonicalizes an entry
- * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
+ * from GIT_CEILING_DIRECTORIES using real_pathdup(), or
  * discards it if unusable.  The presence of an empty entry in
  * GIT_CEILING_DIRECTORIES turns off canonicalization for all
  * subsequent entries.
diff --git a/sha1-file.c b/sha1-file.c
index 616886799e5..f2b24654895 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)
 char *compute_alternate_path(const char *path, struct strbuf *err)
 {
 	char *ref_git = NULL;
-	const char *repo, *ref_git_s;
+	const char *repo;
 	int seen_error = 0;
 
-	ref_git_s = real_path_if_valid(path);
-	if (!ref_git_s) {
+	ref_git = real_pathdup(path, 0);
+	if (!ref_git) {
 		seen_error = 1;
 		strbuf_addf(err, _("path '%s' does not exist"), path);
 		goto out;
-	} else
-		/*
-		 * Beware: read_gitfile(), real_path() and mkpath()
-		 * return static buffer
-		 */
-		ref_git = xstrdup(ref_git_s);
+	}
 
 	repo = read_gitfile(ref_git);
 	if (!repo)
diff --git a/submodule.c b/submodule.c
index bad7a788c06..215c62580fc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2173,7 +2173,7 @@ const char *get_superproject_working_tree(void)
 	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *one_up = real_path_if_valid("../");
+	struct strbuf one_up = STRBUF_INIT;
 	const char *cwd = xgetcwd();
 	const char *ret = NULL;
 	const char *subpath;
@@ -2188,10 +2188,11 @@ const char *get_superproject_working_tree(void)
 		 */
 		return NULL;
 
-	if (!one_up)
+	if (!strbuf_realpath(&one_up, "../", 0))
 		return NULL;
 
-	subpath = relative_path(cwd, one_up, &sb);
+	subpath = relative_path(cwd, one_up.buf, &sb);
+	strbuf_release(&one_up);
 
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_pop(&cp.env_array);
diff --git a/worktree.c b/worktree.c
index e7bbf716f6b..2a340fa939b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -226,17 +226,21 @@ struct worktree *find_worktree(struct worktree **list,
 
 struct worktree *find_worktree_by_path(struct worktree **list, const char *p)
 {
+	struct strbuf wt_path = STRBUF_INIT;
 	char *path = real_pathdup(p, 0);
 
 	if (!path)
 		return NULL;
 	for (; *list; list++) {
-		const char *wt_path = real_path_if_valid((*list)->path);
+		strbuf_reset(&wt_path);
+		if (!strbuf_realpath(&wt_path, (*list)->path, 0))
+			continue;
 
-		if (wt_path && !fspathcmp(path, wt_path))
+		if (!fspathcmp(path, wt_path.buf))
 			break;
 	}
 	free(path);
+	strbuf_release(&wt_path);
 	return *list;
 }
 
-- 
gitgitgadget

[PATCH 4/4] get_superproject_working_tree(): return strbuf

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Together with the previous commits, this commit fully fixes the problem
of using shared buffer for `real_path()` in `get_superproject_working_tree()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/rev-parse.c |  7 ++++---
 submodule.c         | 17 ++++++++---------
 submodule.h         |  4 ++--
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 06ca7175ac7..06056434ed1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -808,9 +808,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--show-superproject-working-tree")) {
-				const char *superproject = get_superproject_working_tree();
-				if (superproject)
-					puts(superproject);
+				struct strbuf superproject = STRBUF_INIT;
+				if (get_superproject_working_tree(&superproject))
+					puts(superproject.buf);
+				strbuf_release(&superproject);
 				continue;
 			}
 			if (!strcmp(arg, "--show-prefix")) {
diff --git a/submodule.c b/submodule.c
index 215c62580fc..46f6c2cbfd0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path,
 	}
 }
 
-const char *get_superproject_working_tree(void)
+int get_superproject_working_tree(struct strbuf* buf)
 {
-	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf one_up = STRBUF_INIT;
 	const char *cwd = xgetcwd();
-	const char *ret = NULL;
+	int ret = 0;
 	const char *subpath;
 	int code;
 	ssize_t len;
@@ -2186,10 +2185,10 @@ const char *get_superproject_working_tree(void)
 		 * We might have a superproject, but it is harder
 		 * to determine.
 		 */
-		return NULL;
+		return 0;
 
 	if (!strbuf_realpath(&one_up, "../", 0))
-		return NULL;
+		return 0;
 
 	subpath = relative_path(cwd, one_up.buf, &sb);
 	strbuf_release(&one_up);
@@ -2233,8 +2232,8 @@ const char *get_superproject_working_tree(void)
 		super_wt = xstrdup(cwd);
 		super_wt[cwd_len - super_sub_len] = '\0';
 
-		strbuf_realpath(&realpath, super_wt, 1);
-		ret = realpath.buf;
+		strbuf_realpath(buf, super_wt, 1);
+		ret = 1;
 		free(super_wt);
 	}
 	strbuf_release(&sb);
@@ -2243,10 +2242,10 @@ const char *get_superproject_working_tree(void)
 
 	if (code == 128)
 		/* '../' is not a git repository */
-		return NULL;
+		return 0;
 	if (code == 0 && len == 0)
 		/* There is an unrelated git repository at '../' */
-		return NULL;
+		return 0;
 	if (code)
 		die(_("ls-tree returned unexpected return code %d"), code);
 
diff --git a/submodule.h b/submodule.h
index c81ec1a9b6c..17492e478fc 100644
--- a/submodule.h
+++ b/submodule.h
@@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,
 /*
  * Return the absolute path of the working tree of the superproject, which this
  * project is a submodule of. If this repository is not a submodule of
- * another repository, return NULL.
+ * another repository, return 0.
  */
-const char *get_superproject_working_tree(void);
+int get_superproject_working_tree(struct strbuf* buf);
 
 #endif
-- 
gitgitgadget

Re: [PATCH 1/4] set_git_dir: fix crash when used with real_path()

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> `real_path()` returns result from a shared buffer, inviting subtle
> reentrance bugs. One of these bugs occur when invoked this way:
>     set_git_dir(real_path(git_dir))
>
> In this case, `real_path()` has reentrance:
>     real_path
>     read_gitfile_gently
>     repo_set_gitdir
>     setup_git_env
>     set_git_dir_1
>     set_git_dir
>
> Later, `set_git_dir()` uses its now-dead parameter:
>     !is_absolute_path(path)
>
> Fix this by using a dedicated `strbuf` to hold `strbuf_realpath()`.

With this detailed explanation, I expected to see a test or two that
demonstrates a breakage, but reading a stale value may not
reproducibly give the same wrong result or crash the program,
perhaps?

> -void set_git_dir(const char *path)
> +void set_git_dir(const char *path, int make_realpath)
>  {
> +	struct strbuf realpath = STRBUF_INIT;
> +
> +	if (make_realpath) {
> +		strbuf_realpath(&realpath, path, 1);
> +		path = realpath.buf;
> +	}
> +
>  	set_git_dir_1(path);
>  	if (!is_absolute_path(path))
>  		chdir_notify_register(NULL, update_relative_gitdir, NULL);
> +
> +	strbuf_release(&realpath);
>  }

Makes sense.  I looked at changes to the callers in this patch and
it all made sense.

Re: [PATCH 2/4] real_path: remove unsafe API

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> However, two places return the value of `real_path()` to the caller. For
> them, a `static` local `strbuf` was added, effectively pushing the
> problem one level higher:
>     read_gitfile_gently()
>     get_superproject_working_tree()

Yeah, I noticed that while reading the patch.  It is not making it
any worse, and other parts of the patch made tons of sense (except
one small thing).

It was especially pleasing to see that care has been taken to avoid
introducing strbuf leaks.


> diff --git a/builtin/clone.c b/builtin/clone.c
> index 1ad26f4d8c8..e5c2a229a11 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  	struct dir_iterator *iter;
>  	int iter_status;
>  	unsigned int flags;
> +	struct strbuf realpath = STRBUF_INIT;
>  
>  	mkdir_if_missing(dest->buf, 0777);
>  
> @@ -454,7 +455,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  		if (unlink(dest->buf) && errno != ENOENT)
>  			die_errno(_("failed to unlink '%s'"), dest->buf);
>  		if (!option_no_hardlinks) {
> -			if (!link(real_path(src->buf), dest->buf))
> +			strbuf_reset(&realpath);
> +			strbuf_realpath(&realpath, src->buf, 1);

This is inside a loop, so "struct strbuf realpath" here in the
second or subsequent iteration may not be empty; it is true that
strbuf_reset() is necessary _somewhere_ in the loop to discard
the path that was created for the previous iteration.

If my reading of the code is correct, however, the first thing that
is done by strbuf_realpath() is to empty the output buffer by using
strbuf_reset() indirectly via get_root_part().  Calling strbuf_reset()
here should not hurt, but it is unnecessary, I would think.  An even
worse effect such a redundant strbuf_reset() has is that by repeatedly
seeing the "reset then call realpath" pattern, readers who do not read
the implementation of strbuf_realpath() might mistakenly think that

	strbuf_addf(&message, "the path '%s' is really ", path);
	strbuf_realpath(&message, path);

is how realpath() is expected to be used, i.e. keep the current
contents in the buffer and append the resolved path to it.


>  static void clone_local(const char *src_repo, const char *dest_repo)
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4a70b33fb5f..3d7ec640e01 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -39,14 +39,18 @@ static struct object_directory *find_odb(struct repository *r,
>  {
>  	struct object_directory *odb;
>  	char *obj_dir_real = real_pathdup(obj_dir, 1);
> +	struct strbuf odb_path_real = STRBUF_INIT;
>  
>  	prepare_alt_odb(r);
>  	for (odb = r->objects->odb; odb; odb = odb->next) {
> -		if (!strcmp(obj_dir_real, real_path(odb->path)))
> +		strbuf_reset(&odb_path_real);
> +		strbuf_realpath(&odb_path_real, odb->path, 1);

Likewise.

> @@ -60,8 +61,11 @@ static int abspath_part_inside_repo(char *path)
>  		path++;
>  		if (*path == '/') {
>  			*path = '\0';
> -			if (fspathcmp(real_path(path0), work_tree) == 0) {
> +			strbuf_reset(&realpath);
> +			strbuf_realpath(&realpath, path0, 1);

Likewise.

> @@ -69,11 +73,15 @@ static int abspath_part_inside_repo(char *path)
>  	}
>  
>  	/* check whole path */
> -	if (fspathcmp(real_path(path0), work_tree) == 0) {
> +	strbuf_reset(&realpath);
> +	strbuf_realpath(&realpath, path0, 1);

Likewise.

> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index 409034cf4ee..40548d31dfe 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -290,11 +290,15 @@ int cmd__path_utils(int argc, const char **argv)
>  	}
>  
>  	if (argc >= 2 && !strcmp(argv[1], "real_path")) {
> +		struct strbuf realpath = STRBUF_INIT;
>  		while (argc > 2) {
> -			puts(real_path(argv[2]));
> +			strbuf_reset(&realpath);
> +			strbuf_realpath(&realpath, argv[2], 1);
> +			puts(realpath.buf);

Likewise.


Thanks.

Re: [PATCH 3/4] real_path_if_valid(): remove unsafe API

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/sha1-file.c b/sha1-file.c
> index 616886799e5..f2b24654895 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)
>  char *compute_alternate_path(const char *path, struct strbuf *err)
>  {
>  	char *ref_git = NULL;
> -	const char *repo, *ref_git_s;
> +	const char *repo;
>  	int seen_error = 0;
>  
> -	ref_git_s = real_path_if_valid(path);
> -	if (!ref_git_s) {
> +	ref_git = real_pathdup(path, 0);
> +	if (!ref_git) {
>  		seen_error = 1;
>  		strbuf_addf(err, _("path '%s' does not exist"), path);
>  		goto out;
> -	} else
> -		/*
> -		 * Beware: read_gitfile(), real_path() and mkpath()
> -		 * return static buffer
> -		 */
> -		ref_git = xstrdup(ref_git_s);
> +	}

It is amusing to see that rewriting not to use the unsafe function
makes the code a lot easier to follow ;-)

Re: [PATCH 1/4] set_git_dir: fix crash when used with real_path()

[Alexandr Miloslavskiy]

On 06.03.2020 22:54, Junio C Hamano wrote:
> With this detailed explanation, I expected to see a test or two that
> demonstrates a breakage, but reading a stale value may not
> reproducibly give the same wrong result or crash the program,
> perhaps?

Let's put it this way: one of the tests hits the bug every single time,
yet still the bug has gone unnoticed for years. So yes, it's not super
reliable. I think I could make a test that crashes often enough, but
the effort will probably not be justified. The problem here is rather
apparent when a finger is pointed to it.

Re: [PATCH 4/4] get_superproject_working_tree(): return strbuf

[Junio C Hamano]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  			if (!strcmp(arg, "--show-superproject-working-tree")) {
> -				const char *superproject = get_superproject_working_tree();
> -				if (superproject)
> -					puts(superproject);
> +				struct strbuf superproject = STRBUF_INIT;
> +				if (get_superproject_working_tree(&superproject))
> +					puts(superproject.buf);
> +				strbuf_release(&superproject);

The new calling convention makes sense here.

>  				continue;
>  			}
>  			if (!strcmp(arg, "--show-prefix")) {
> diff --git a/submodule.c b/submodule.c
> index 215c62580fc..46f6c2cbfd0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path,
>  	}
>  }
>  
> -const char *get_superproject_working_tree(void)
> +int get_superproject_working_tree(struct strbuf* buf)

Micronit.  

The asterisk sticks to the identifier, not type, in our codebase.
I.e. "struct strbuf *buf".

> diff --git a/submodule.h b/submodule.h
> index c81ec1a9b6c..17492e478fc 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,
>  /*
>   * Return the absolute path of the working tree of the superproject, which this
>   * project is a submodule of. If this repository is not a submodule of
> - * another repository, return NULL.
> + * another repository, return 0.
>   */
> -const char *get_superproject_working_tree(void);
> +int get_superproject_working_tree(struct strbuf* buf);

Likewise.

The conversion of the function body looked quite sensible.

Thanks.

Re: [PATCH 2/4] real_path: remove unsafe API

[Alexandr Miloslavskiy]

On 06.03.2020 23:12, Junio C Hamano wrote:
> If my reading of the code is correct, however, the first thing that
> is done by strbuf_realpath() is to empty the output buffer by using
> strbuf_reset() indirectly via get_root_part().  Calling strbuf_reset()
> here should not hurt, but it is unnecessary, I would think.  An even
> worse effect such a redundant strbuf_reset() has is that by repeatedly
> seeing the "reset then call realpath" pattern, readers who do not read
> the implementation of strbuf_realpath() might mistakenly think that
> 
> 	strbuf_addf(&message, "the path '%s' is really ", path);
> 	strbuf_realpath(&message, path);
> 
> is how realpath() is expected to be used, i.e. keep the current
> contents in the buffer and append the resolved path to it.

Thanks, will change in V2 next week.

Re: [PATCH 4/4] get_superproject_working_tree(): return strbuf

[Alexandr Miloslavskiy]

On 06.03.2020 23:44, Junio C Hamano wrote:
> Micronit.
> 
> The asterisk sticks to the identifier, not type, in our codebase.
> I.e. "struct strbuf *buf".

Sorry, having difficulties switching between many different styles.
Will fix in V2 next week.


Thank you very much for giving such a quick response! It is a great 
pleasure when my patches get movement instead of just gathering dust 
like in some other opensource projects :(

[PATCH v2 0/4] Fix bugs related to real_path()

[Alexandr Miloslavskiy via GitGitGadget]

Changes since V1
-------------------
1) Removed `strbuf_realpath()` that weren't needed
2) Code style in declaration of `get_superproject_working_tree()`

Original description
-------------------
The issue with `real_path()` seems to be long-standing, where multiple
people solved parts of it over time. I'm adding another part here
after I have discovered a crash related to it.

Even with this step, there are still problems remaining:
* `read_gitfile_gently()` still uses shared buffer.
* `absolute_path()` was not removed.

These issues remain because there're too many code references and I'd like
to avoid submitting a single topic of a scary size.

Alexandr Miloslavskiy (4):
  set_git_dir: fix crash when used with real_path()
  real_path: remove unsafe API
  real_path_if_valid(): remove unsafe API
  get_superproject_working_tree(): return strbuf

 abspath.c                  | 18 +-----------------
 builtin/clone.c            |  6 +++++-
 builtin/commit-graph.c     |  5 ++++-
 builtin/init-db.c          |  4 ++--
 builtin/rev-parse.c        | 12 ++++++++----
 builtin/worktree.c         |  9 ++++++---
 cache.h                    |  4 +---
 editor.c                   | 11 +++++++++--
 environment.c              | 18 ++++++++++++++++--
 path.c                     |  4 ++--
 setup.c                    | 35 ++++++++++++++++++++++-------------
 sha1-file.c                | 13 ++++---------
 submodule.c                | 22 ++++++++++++----------
 submodule.h                |  4 ++--
 t/helper/test-path-utils.c |  5 ++++-
 worktree.c                 | 12 +++++++++---
 16 files changed, 107 insertions(+), 75 deletions(-)


base-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-575%2FSyntevoAlex%2F%230205(git)_crash_real_path-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-575/SyntevoAlex/#0205(git)_crash_real_path-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/575

Range-diff vs v1:

 1:  f7afcb4cc83 = 1:  f7afcb4cc83 set_git_dir: fix crash when used with real_path()
 2:  039d3d36866 ! 2:  29e7133dcd9 real_path: remove unsafe API
     @@ -64,7 +64,6 @@
       			die_errno(_("failed to unlink '%s'"), dest->buf);
       		if (!option_no_hardlinks) {
      -			if (!link(real_path(src->buf), dest->buf))
     -+			strbuf_reset(&realpath);
      +			strbuf_realpath(&realpath, src->buf, 1);
      +			if (!link(realpath.buf, dest->buf))
       				continue;
     @@ -92,7 +91,6 @@
       	prepare_alt_odb(r);
       	for (odb = r->objects->odb; odb; odb = odb->next) {
      -		if (!strcmp(obj_dir_real, real_path(odb->path)))
     -+		strbuf_reset(&odb_path_real);
      +		strbuf_realpath(&odb_path_real, odb->path, 1);
      +		if (!strcmp(obj_dir_real, odb_path_real.buf))
       			break;
     @@ -139,7 +137,6 @@
      -	write_file(sb.buf, "%s", real_path(sb_git.buf));
      +	strbuf_realpath(&realpath, sb_git.buf, 1);
      +	write_file(sb.buf, "%s", realpath.buf);
     -+	strbuf_reset(&realpath);
      +	strbuf_realpath(&realpath, get_git_common_dir(), 1);
       	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
      -		   real_path(get_git_common_dir()), name);
     @@ -261,7 +258,6 @@
       		if (*path == '/') {
       			*path = '\0';
      -			if (fspathcmp(real_path(path0), work_tree) == 0) {
     -+			strbuf_reset(&realpath);
      +			strbuf_realpath(&realpath, path0, 1);
      +			if (fspathcmp(realpath.buf, work_tree) == 0) {
       				memmove(path0, path + 1, len - (path - path0));
     @@ -274,7 +270,6 @@
       
       	/* check whole path */
      -	if (fspathcmp(real_path(path0), work_tree) == 0) {
     -+	strbuf_reset(&realpath);
      +	strbuf_realpath(&realpath, path0, 1);
      +	if (fspathcmp(realpath.buf, work_tree) == 0) {
       		*path0 = '\0';
     @@ -338,7 +333,6 @@
      +		struct strbuf realpath = STRBUF_INIT;
       		while (argc > 2) {
      -			puts(real_path(argv[2]));
     -+			strbuf_reset(&realpath);
      +			strbuf_realpath(&realpath, argv[2], 1);
      +			puts(realpath.buf);
       			argc--;
 3:  59af49ad9f6 ! 3:  a4917638671 real_path_if_valid(): remove unsafe API
     @@ -122,7 +122,6 @@
       		return NULL;
       	for (; *list; list++) {
      -		const char *wt_path = real_path_if_valid((*list)->path);
     -+		strbuf_reset(&wt_path);
      +		if (!strbuf_realpath(&wt_path, (*list)->path, 0))
      +			continue;
       
 4:  2eeefda3d41 ! 4:  41950069a16 get_superproject_working_tree(): return strbuf
     @@ -33,7 +33,7 @@
       }
       
      -const char *get_superproject_working_tree(void)
     -+int get_superproject_working_tree(struct strbuf* buf)
     ++int get_superproject_working_tree(struct strbuf *buf)
       {
      -	static struct strbuf realpath = STRBUF_INIT;
       	struct child_process cp = CHILD_PROCESS_INIT;
     @@ -94,6 +94,6 @@
      + * another repository, return 0.
        */
      -const char *get_superproject_working_tree(void);
     -+int get_superproject_working_tree(struct strbuf* buf);
     ++int get_superproject_working_tree(struct strbuf *buf);
       
       #endif

-- 
gitgitgadget

[PATCH v2 1/4] set_git_dir: fix crash when used with real_path()

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

`real_path()` returns result from a shared buffer, inviting subtle
reentrance bugs. One of these bugs occur when invoked this way:
    set_git_dir(real_path(git_dir))

In this case, `real_path()` has reentrance:
    real_path
    read_gitfile_gently
    repo_set_gitdir
    setup_git_env
    set_git_dir_1
    set_git_dir

Later, `set_git_dir()` uses its now-dead parameter:
    !is_absolute_path(path)

Fix this by using a dedicated `strbuf` to hold `strbuf_realpath()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/init-db.c |  4 ++--
 cache.h           |  2 +-
 environment.c     | 11 ++++++++++-
 path.c            |  2 +-
 setup.c           | 18 +++++++++---------
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 944ec77fe10..5bf61a7e056 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -356,12 +356,12 @@ int init_db(const char *git_dir, const char *real_git_dir,
 		if (!exist_ok && !stat(real_git_dir, &st))
 			die(_("%s already exists"), real_git_dir);
 
-		set_git_dir(real_path(real_git_dir));
+		set_git_dir(real_git_dir, 1);
 		git_dir = get_git_dir();
 		separate_git_dir(git_dir, original_git_dir);
 	}
 	else {
-		set_git_dir(real_path(git_dir));
+		set_git_dir(git_dir, 1);
 		git_dir = get_git_dir();
 	}
 	startup_info->have_repository = 1;
diff --git a/cache.h b/cache.h
index 37c899b53f7..8cee257d3d7 100644
--- a/cache.h
+++ b/cache.h
@@ -543,7 +543,7 @@ const char *get_git_common_dir(void);
 char *get_object_directory(void);
 char *get_index_file(void);
 char *get_graft_file(struct repository *r);
-void set_git_dir(const char *path);
+void set_git_dir(const char *path, int make_realpath);
 int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 int get_common_dir(struct strbuf *sb, const char *gitdir);
 const char *get_git_namespace(void);
diff --git a/environment.c b/environment.c
index e72a02d0d57..c436de31eef 100644
--- a/environment.c
+++ b/environment.c
@@ -345,11 +345,20 @@ static void update_relative_gitdir(const char *name,
 	free(path);
 }
 
-void set_git_dir(const char *path)
+void set_git_dir(const char *path, int make_realpath)
 {
+	struct strbuf realpath = STRBUF_INIT;
+
+	if (make_realpath) {
+		strbuf_realpath(&realpath, path, 1);
+		path = realpath.buf;
+	}
+
 	set_git_dir_1(path);
 	if (!is_absolute_path(path))
 		chdir_notify_register(NULL, update_relative_gitdir, NULL);
+
+	strbuf_release(&realpath);
 }
 
 const char *get_log_output_encoding(void)
diff --git a/path.c b/path.c
index 88cf5930073..c5a8fe4f0c3 100644
--- a/path.c
+++ b/path.c
@@ -850,7 +850,7 @@ const char *enter_repo(const char *path, int strict)
 	}
 
 	if (is_git_directory(".")) {
-		set_git_dir(".");
+		set_git_dir(".", 0);
 		check_repository_format();
 		return path;
 	}
diff --git a/setup.c b/setup.c
index 4ea7a0b081b..fa4317e707a 100644
--- a/setup.c
+++ b/setup.c
@@ -725,7 +725,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		}
 
 		/* #18, #26 */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
@@ -747,7 +747,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	}
 	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
 		/* #16d */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
@@ -759,14 +759,14 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 
 	/* both get_git_work_tree() and cwd are already normalized */
 	if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
-		set_git_dir(gitdirenv);
+		set_git_dir(gitdirenv, 0);
 		free(gitfile);
 		return NULL;
 	}
 
 	offset = dir_inside_of(cwd->buf, worktree);
 	if (offset >= 0) {	/* cwd inside worktree? */
-		set_git_dir(real_path(gitdirenv));
+		set_git_dir(gitdirenv, 1);
 		if (chdir(worktree))
 			die_errno(_("cannot chdir to '%s'"), worktree);
 		strbuf_addch(cwd, '/');
@@ -775,7 +775,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	}
 
 	/* cwd outside worktree */
-	set_git_dir(gitdirenv);
+	set_git_dir(gitdirenv, 0);
 	free(gitfile);
 	return NULL;
 }
@@ -804,7 +804,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 
 	/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
 	if (is_bare_repository_cfg > 0) {
-		set_git_dir(offset == cwd->len ? gitdir : real_path(gitdir));
+		set_git_dir(gitdir, (offset != cwd->len));
 		if (chdir(cwd->buf))
 			die_errno(_("cannot come back to cwd"));
 		return NULL;
@@ -813,7 +813,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	/* #0, #1, #5, #8, #9, #12, #13 */
 	set_git_work_tree(".");
 	if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
-		set_git_dir(gitdir);
+		set_git_dir(gitdir, 0);
 	inside_git_dir = 0;
 	inside_work_tree = 1;
 	if (offset >= cwd->len)
@@ -856,10 +856,10 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
 			die_errno(_("cannot come back to cwd"));
 		root_len = offset_1st_component(cwd->buf);
 		strbuf_setlen(cwd, offset > root_len ? offset : root_len);
-		set_git_dir(cwd->buf);
+		set_git_dir(cwd->buf, 0);
 	}
 	else
-		set_git_dir(".");
+		set_git_dir(".", 0);
 	return NULL;
 }
 
-- 
gitgitgadget

[PATCH v2 2/4] real_path: remove unsafe API

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Returning a shared buffer invites very subtle bugs due to reentrancy or
multi-threading, as demonstrated by the previous patch.

There was an unfinished effort to abolish this [1].

Let's finally rid of `real_path()`, using `strbuf_realpath()` instead.

This patch uses a local `strbuf` for most places where `real_path()` was
previously called.

However, two places return the value of `real_path()` to the caller. For
them, a `static` local `strbuf` was added, effectively pushing the
problem one level higher:
    read_gitfile_gently()
    get_superproject_working_tree()

[1] https://lore.kernel.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 abspath.c                  |  8 +-------
 builtin/clone.c            |  6 +++++-
 builtin/commit-graph.c     |  5 ++++-
 builtin/rev-parse.c        |  5 ++++-
 builtin/worktree.c         |  9 ++++++---
 cache.h                    |  1 -
 editor.c                   | 11 +++++++++--
 environment.c              |  7 ++++++-
 path.c                     |  2 +-
 setup.c                    | 15 ++++++++++++---
 submodule.c                |  4 +++-
 t/helper/test-path-utils.c |  5 ++++-
 worktree.c                 |  5 ++++-
 13 files changed, 59 insertions(+), 24 deletions(-)

diff --git a/abspath.c b/abspath.c
index 98579853299..d34026bfeb8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -206,12 +206,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
  * Resolve `path` into an absolute, cleaned-up path. The return value
  * comes from a shared buffer.
  */
-const char *real_path(const char *path)
-{
-	static struct strbuf realpath = STRBUF_INIT;
-	return strbuf_realpath(&realpath, path, 1);
-}
-
 const char *real_path_if_valid(const char *path)
 {
 	static struct strbuf realpath = STRBUF_INIT;
@@ -233,7 +227,7 @@ char *real_pathdup(const char *path, int die_on_error)
 
 /*
  * Use this to get an absolute path from a relative one. If you want
- * to resolve links, you should use real_path.
+ * to resolve links, you should use strbuf_realpath.
  */
 const char *absolute_path(const char *path)
 {
diff --git a/builtin/clone.c b/builtin/clone.c
index 1ad26f4d8c8..488bdb07417 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 	struct dir_iterator *iter;
 	int iter_status;
 	unsigned int flags;
+	struct strbuf realpath = STRBUF_INIT;
 
 	mkdir_if_missing(dest->buf, 0777);
 
@@ -454,7 +455,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		if (unlink(dest->buf) && errno != ENOENT)
 			die_errno(_("failed to unlink '%s'"), dest->buf);
 		if (!option_no_hardlinks) {
-			if (!link(real_path(src->buf), dest->buf))
+			strbuf_realpath(&realpath, src->buf, 1);
+			if (!link(realpath.buf, dest->buf))
 				continue;
 			if (option_local > 0)
 				die_errno(_("failed to create link '%s'"), dest->buf);
@@ -468,6 +470,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		strbuf_setlen(src, src_len);
 		die(_("failed to iterate over '%s'"), src->buf);
 	}
+
+	strbuf_release(&realpath);
 }
 
 static void clone_local(const char *src_repo, const char *dest_repo)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4a70b33fb5f..d1ab6625f63 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -39,14 +39,17 @@ static struct object_directory *find_odb(struct repository *r,
 {
 	struct object_directory *odb;
 	char *obj_dir_real = real_pathdup(obj_dir, 1);
+	struct strbuf odb_path_real = STRBUF_INIT;
 
 	prepare_alt_odb(r);
 	for (odb = r->objects->odb; odb; odb = odb->next) {
-		if (!strcmp(obj_dir_real, real_path(odb->path)))
+		strbuf_realpath(&odb_path_real, odb->path, 1);
+		if (!strcmp(obj_dir_real, odb_path_real.buf))
 			break;
 	}
 
 	free(obj_dir_real);
+	strbuf_release(&odb_path_real);
 
 	if (!odb)
 		die(_("could not find object directory matching %s"), obj_dir);
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 7a00da82035..06ca7175ac7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -857,7 +857,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					if (!gitdir && !prefix)
 						gitdir = ".git";
 					if (gitdir) {
-						puts(real_path(gitdir));
+						struct strbuf realpath = STRBUF_INIT;
+						strbuf_realpath(&realpath, gitdir, 1);
+						puts(realpath.buf);
+						strbuf_release(&realpath);
 						continue;
 					}
 				}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 24f22800f38..d99db356684 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -258,7 +258,7 @@ static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
-	struct strbuf sb = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
 	const char *name;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct argv_array child_env = ARGV_ARRAY_INIT;
@@ -330,9 +330,11 @@ static int add_worktree(const char *path, const char *refname,
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
-	write_file(sb.buf, "%s", real_path(sb_git.buf));
+	strbuf_realpath(&realpath, sb_git.buf, 1);
+	write_file(sb.buf, "%s", realpath.buf);
+	strbuf_realpath(&realpath, get_git_common_dir(), 1);
 	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
-		   real_path(get_git_common_dir()), name);
+		   realpath.buf, name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
 	 * or is_git_directory() will reject the directory. Any value which
@@ -418,6 +420,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_release(&sb_repo);
 	strbuf_release(&sb_git);
 	strbuf_release(&sb_name);
+	strbuf_release(&realpath);
 	return ret;
 }
 
diff --git a/cache.h b/cache.h
index 8cee257d3d7..f6937793ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)
 int is_directory(const char *);
 char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
-const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path, int die_on_error);
 const char *absolute_path(const char *path);
diff --git a/editor.c b/editor.c
index f079abbf110..91989ee8a11 100644
--- a/editor.c
+++ b/editor.c
@@ -54,7 +54,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		return error("Terminal is dumb, but EDITOR unset");
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, real_path(path), NULL };
+		struct strbuf realpath = STRBUF_INIT;
+		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
 		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
@@ -75,16 +76,22 @@ static int launch_specified_editor(const char *editor, const char *path,
 			fflush(stderr);
 		}
 
+		strbuf_realpath(&realpath, path, 1);
+		args[1] = realpath.buf;
+
 		p.argv = args;
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		if (start_command(&p) < 0)
+		if (start_command(&p) < 0) {
+			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
+		}
 
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
diff --git a/environment.c b/environment.c
index c436de31eef..10c9061c432 100644
--- a/environment.c
+++ b/environment.c
@@ -254,8 +254,11 @@ static int git_work_tree_initialized;
  */
 void set_git_work_tree(const char *new_work_tree)
 {
+	struct strbuf realpath = STRBUF_INIT;
+
 	if (git_work_tree_initialized) {
-		new_work_tree = real_path(new_work_tree);
+		strbuf_realpath(&realpath, new_work_tree, 1);
+		new_work_tree = realpath.buf;
 		if (strcmp(new_work_tree, the_repository->worktree))
 			die("internal error: work tree has already been set\n"
 			    "Current worktree: %s\nNew worktree: %s",
@@ -264,6 +267,8 @@ void set_git_work_tree(const char *new_work_tree)
 	}
 	git_work_tree_initialized = 1;
 	repo_set_worktree(the_repository, new_work_tree);
+
+	strbuf_release(&realpath);
 }
 
 const char *get_git_work_tree(void)
diff --git a/path.c b/path.c
index c5a8fe4f0c3..0a42ceb3fb5 100644
--- a/path.c
+++ b/path.c
@@ -723,7 +723,7 @@ static struct passwd *getpw_str(const char *username, size_t len)
  * then it is a newly allocated string. Returns NULL on getpw failure or
  * if path is NULL.
  *
- * If real_home is true, real_path($HOME) is used in the expansion.
+ * If real_home is true, strbuf_realpath($HOME) is used in the expansion.
  */
 char *expand_user_path(const char *path, int real_home)
 {
diff --git a/setup.c b/setup.c
index fa4317e707a..1ae3f203016 100644
--- a/setup.c
+++ b/setup.c
@@ -32,6 +32,7 @@ static int abspath_part_inside_repo(char *path)
 	char *path0;
 	int off;
 	const char *work_tree = get_git_work_tree();
+	struct strbuf realpath = STRBUF_INIT;
 
 	if (!work_tree)
 		return -1;
@@ -60,8 +61,10 @@ static int abspath_part_inside_repo(char *path)
 		path++;
 		if (*path == '/') {
 			*path = '\0';
-			if (fspathcmp(real_path(path0), work_tree) == 0) {
+			strbuf_realpath(&realpath, path0, 1);
+			if (fspathcmp(realpath.buf, work_tree) == 0) {
 				memmove(path0, path + 1, len - (path - path0));
+				strbuf_release(&realpath);
 				return 0;
 			}
 			*path = '/';
@@ -69,11 +72,14 @@ static int abspath_part_inside_repo(char *path)
 	}
 
 	/* check whole path */
-	if (fspathcmp(real_path(path0), work_tree) == 0) {
+	strbuf_realpath(&realpath, path0, 1);
+	if (fspathcmp(realpath.buf, work_tree) == 0) {
 		*path0 = '\0';
+		strbuf_release(&realpath);
 		return 0;
 	}
 
+	strbuf_release(&realpath);
 	return -1;
 }
 
@@ -619,6 +625,7 @@ 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;
 
 	if (stat(path, &st)) {
 		/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -669,7 +676,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}
-	path = real_path(dir);
+
+	strbuf_realpath(&realpath, dir, 1);
+	path = realpath.buf;
 
 cleanup_return:
 	if (return_error_code)
diff --git a/submodule.c b/submodule.c
index 31f391d7d25..bad7a788c06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2170,6 +2170,7 @@ void absorb_git_dir_into_superproject(const char *path,
 
 const char *get_superproject_working_tree(void)
 {
+	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *one_up = real_path_if_valid("../");
@@ -2231,7 +2232,8 @@ const char *get_superproject_working_tree(void)
 		super_wt = xstrdup(cwd);
 		super_wt[cwd_len - super_sub_len] = '\0';
 
-		ret = real_path(super_wt);
+		strbuf_realpath(&realpath, super_wt, 1);
+		ret = realpath.buf;
 		free(super_wt);
 	}
 	strbuf_release(&sb);
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 409034cf4ee..313a153209c 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -290,11 +290,14 @@ int cmd__path_utils(int argc, const char **argv)
 	}
 
 	if (argc >= 2 && !strcmp(argv[1], "real_path")) {
+		struct strbuf realpath = STRBUF_INIT;
 		while (argc > 2) {
-			puts(real_path(argv[2]));
+			strbuf_realpath(&realpath, argv[2], 1);
+			puts(realpath.buf);
 			argc--;
 			argv++;
 		}
+		strbuf_release(&realpath);
 		return 0;
 	}
 
diff --git a/worktree.c b/worktree.c
index eba4fd3a038..e7bbf716f6b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -285,6 +285,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		      unsigned flags)
 {
 	struct strbuf wt_path = STRBUF_INIT;
+	struct strbuf realpath = STRBUF_INIT;
 	char *path = NULL;
 	int err, ret = -1;
 
@@ -336,7 +337,8 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 		goto done;
 	}
 
-	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+	strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
+	ret = fspathcmp(path, realpath.buf);
 
 	if (ret)
 		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
@@ -344,6 +346,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
 done:
 	free(path);
 	strbuf_release(&wt_path);
+	strbuf_release(&realpath);
 	return ret;
 }
 
-- 
gitgitgadget

[PATCH v2 3/4] real_path_if_valid(): remove unsafe API

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

This commit continues the work started with previous commit.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 abspath.c   | 10 ----------
 cache.h     |  1 -
 setup.c     |  2 +-
 sha1-file.c | 13 ++++---------
 submodule.c |  7 ++++---
 worktree.c  |  7 +++++--
 6 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/abspath.c b/abspath.c
index d34026bfeb8..6f15a418bb6 100644
--- a/abspath.c
+++ b/abspath.c
@@ -202,16 +202,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
 	return retval;
 }
 
-/*
- * Resolve `path` into an absolute, cleaned-up path. The return value
- * comes from a shared buffer.
- */
-const char *real_path_if_valid(const char *path)
-{
-	static struct strbuf realpath = STRBUF_INIT;
-	return strbuf_realpath(&realpath, path, 0);
-}
-
 char *real_pathdup(const char *path, int die_on_error)
 {
 	struct strbuf realpath = STRBUF_INIT;
diff --git a/cache.h b/cache.h
index f6937793ec2..aa3f5ce718a 100644
--- a/cache.h
+++ b/cache.h
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)
 int is_directory(const char *);
 char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
-const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path, int die_on_error);
 const char *absolute_path(const char *path);
 char *absolute_pathdup(const char *path);
diff --git a/setup.c b/setup.c
index 1ae3f203016..9e8fa46bc78 100644
--- a/setup.c
+++ b/setup.c
@@ -886,7 +886,7 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 
 /*
  * A "string_list_each_func_t" function that canonicalizes an entry
- * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
+ * from GIT_CEILING_DIRECTORIES using real_pathdup(), or
  * discards it if unusable.  The presence of an empty entry in
  * GIT_CEILING_DIRECTORIES turns off canonicalization for all
  * subsequent entries.
diff --git a/sha1-file.c b/sha1-file.c
index 616886799e5..f2b24654895 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)
 char *compute_alternate_path(const char *path, struct strbuf *err)
 {
 	char *ref_git = NULL;
-	const char *repo, *ref_git_s;
+	const char *repo;
 	int seen_error = 0;
 
-	ref_git_s = real_path_if_valid(path);
-	if (!ref_git_s) {
+	ref_git = real_pathdup(path, 0);
+	if (!ref_git) {
 		seen_error = 1;
 		strbuf_addf(err, _("path '%s' does not exist"), path);
 		goto out;
-	} else
-		/*
-		 * Beware: read_gitfile(), real_path() and mkpath()
-		 * return static buffer
-		 */
-		ref_git = xstrdup(ref_git_s);
+	}
 
 	repo = read_gitfile(ref_git);
 	if (!repo)
diff --git a/submodule.c b/submodule.c
index bad7a788c06..215c62580fc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2173,7 +2173,7 @@ const char *get_superproject_working_tree(void)
 	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
-	const char *one_up = real_path_if_valid("../");
+	struct strbuf one_up = STRBUF_INIT;
 	const char *cwd = xgetcwd();
 	const char *ret = NULL;
 	const char *subpath;
@@ -2188,10 +2188,11 @@ const char *get_superproject_working_tree(void)
 		 */
 		return NULL;
 
-	if (!one_up)
+	if (!strbuf_realpath(&one_up, "../", 0))
 		return NULL;
 
-	subpath = relative_path(cwd, one_up, &sb);
+	subpath = relative_path(cwd, one_up.buf, &sb);
+	strbuf_release(&one_up);
 
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_pop(&cp.env_array);
diff --git a/worktree.c b/worktree.c
index e7bbf716f6b..543472f0c7b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -226,17 +226,20 @@ struct worktree *find_worktree(struct worktree **list,
 
 struct worktree *find_worktree_by_path(struct worktree **list, const char *p)
 {
+	struct strbuf wt_path = STRBUF_INIT;
 	char *path = real_pathdup(p, 0);
 
 	if (!path)
 		return NULL;
 	for (; *list; list++) {
-		const char *wt_path = real_path_if_valid((*list)->path);
+		if (!strbuf_realpath(&wt_path, (*list)->path, 0))
+			continue;
 
-		if (wt_path && !fspathcmp(path, wt_path))
+		if (!fspathcmp(path, wt_path.buf))
 			break;
 	}
 	free(path);
+	strbuf_release(&wt_path);
 	return *list;
 }
 
-- 
gitgitgadget

[PATCH v2 4/4] get_superproject_working_tree(): return strbuf

[Alexandr Miloslavskiy via GitGitGadget]

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Together with the previous commits, this commit fully fixes the problem
of using shared buffer for `real_path()` in `get_superproject_working_tree()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/rev-parse.c |  7 ++++---
 submodule.c         | 17 ++++++++---------
 submodule.h         |  4 ++--
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 06ca7175ac7..06056434ed1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -808,9 +808,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--show-superproject-working-tree")) {
-				const char *superproject = get_superproject_working_tree();
-				if (superproject)
-					puts(superproject);
+				struct strbuf superproject = STRBUF_INIT;
+				if (get_superproject_working_tree(&superproject))
+					puts(superproject.buf);
+				strbuf_release(&superproject);
 				continue;
 			}
 			if (!strcmp(arg, "--show-prefix")) {
diff --git a/submodule.c b/submodule.c
index 215c62580fc..c3aadf3fff8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path,
 	}
 }
 
-const char *get_superproject_working_tree(void)
+int get_superproject_working_tree(struct strbuf *buf)
 {
-	static struct strbuf realpath = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf one_up = STRBUF_INIT;
 	const char *cwd = xgetcwd();
-	const char *ret = NULL;
+	int ret = 0;
 	const char *subpath;
 	int code;
 	ssize_t len;
@@ -2186,10 +2185,10 @@ const char *get_superproject_working_tree(void)
 		 * We might have a superproject, but it is harder
 		 * to determine.
 		 */
-		return NULL;
+		return 0;
 
 	if (!strbuf_realpath(&one_up, "../", 0))
-		return NULL;
+		return 0;
 
 	subpath = relative_path(cwd, one_up.buf, &sb);
 	strbuf_release(&one_up);
@@ -2233,8 +2232,8 @@ const char *get_superproject_working_tree(void)
 		super_wt = xstrdup(cwd);
 		super_wt[cwd_len - super_sub_len] = '\0';
 
-		strbuf_realpath(&realpath, super_wt, 1);
-		ret = realpath.buf;
+		strbuf_realpath(buf, super_wt, 1);
+		ret = 1;
 		free(super_wt);
 	}
 	strbuf_release(&sb);
@@ -2243,10 +2242,10 @@ const char *get_superproject_working_tree(void)
 
 	if (code == 128)
 		/* '../' is not a git repository */
-		return NULL;
+		return 0;
 	if (code == 0 && len == 0)
 		/* There is an unrelated git repository at '../' */
-		return NULL;
+		return 0;
 	if (code)
 		die(_("ls-tree returned unexpected return code %d"), code);
 
diff --git a/submodule.h b/submodule.h
index c81ec1a9b6c..4dad649f942 100644
--- a/submodule.h
+++ b/submodule.h
@@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,
 /*
  * Return the absolute path of the working tree of the superproject, which this
  * project is a submodule of. If this repository is not a submodule of
- * another repository, return NULL.
+ * another repository, return 0.
  */
-const char *get_superproject_working_tree(void);
+int get_superproject_working_tree(struct strbuf *buf);
 
 #endif
-- 
gitgitgadget