Fix bugs related to real_path()

PATCH Superseded Alexandr Miloslavskiy via GitGitGadget 3 participants 17 messages Mar 6, 2020 – Mar 10, 2020

Share & tools

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-06 19:03 UTC #0 lore raw

cover Fix bugs related to real_path()

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

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-06 19:03 UTC #1 lore raw

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

From: Alexandr Miloslavskiy <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c |  4 ++--
 cache.h           |  2 +-
 environment.c     | 11 ++++++++++-
 path.c            |  2 +-
 setup.c           | 18 +++++++++---------
 5 files changed, 23 insertions(+), 14 deletions(-)

builtin/init-db.c +2 -2

index 944ec77fe10..5bf61a7e056 100644
@@ -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;

cache.h +1 -1

index 37c899b53f7..8cee257d3d7 100644
@@ -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);

environment.c +10 -1

index e72a02d0d57..c436de31eef 100644
@@ -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)

path.c +1 -1

index 88cf5930073..c5a8fe4f0c3 100644
@@ -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;
 	}

setup.c +9 -9

index 4ea7a0b081b..fa4317e707a 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-06 19:03 UTC #9 lore raw

PATCH 4/4 get_superproject_working_tree(): return strbuf

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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 <[email protected]>

---
 builtin/rev-parse.c |  7 ++++---
 submodule.c         | 17 ++++++++---------
 submodule.h         |  4 ++--
 3 files changed, 14 insertions(+), 14 deletions(-)

builtin/rev-parse.c +4 -3

index 06ca7175ac7..06056434ed1 100644
@@ -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")) {

submodule.c +8 -9

index 215c62580fc..46f6c2cbfd0 100644
@@ -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);

submodule.h +2 -2

index c81ec1a9b6c..17492e478fc 100644
@@ -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

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-06 19:03 UTC #4 lore raw

PATCH 2/4 real_path: remove unsafe API

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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/[email protected]/

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
 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(-)

abspath.c +1 -7

index 98579853299..d34026bfeb8 100644
@@ -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)
 {

builtin/clone.c +6 -1

index 1ad26f4d8c8..e5c2a229a11 100644
@@ -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)

builtin/commit-graph.c +5 -1

index 4a70b33fb5f..3d7ec640e01 100644
@@ -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);

builtin/rev-parse.c +4 -1

index 7a00da82035..06ca7175ac7 100644
@@ -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;
 					}
 				}

builtin/worktree.c +7 -3

index 24f22800f38..b13b88bec62 100644
@@ -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;
 }

cache.h +0 -1

index 8cee257d3d7..f6937793ec2 100644
@@ -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);

editor.c +9 -2

index f079abbf110..91989ee8a11 100644
@@ -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);

environment.c +6 -1

index c436de31eef..10c9061c432 100644
@@ -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)

path.c +1 -1

index c5a8fe4f0c3..0a42ceb3fb5 100644
@@ -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)
 {

setup.c +14 -3

index fa4317e707a..19dded55788 100644
@@ -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)

submodule.c +3 -1

index 31f391d7d25..bad7a788c06 100644
@@ -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);

t/helper/test-path-utils.c +5 -1

index 409034cf4ee..40548d31dfe 100644
@@ -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;
 	}

worktree.c +4 -1

index eba4fd3a038..e7bbf716f6b 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-06 19:03 UTC #7 lore raw

PATCH 3/4 real_path_if_valid(): remove unsafe API

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

This commit continues the work started with previous commit.

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
 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(-)

abspath.c +0 ~~-10~~

index d34026bfeb8..6f15a418bb6 100644
@@ -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;

cache.h +0 -1

index f6937793ec2..aa3f5ce718a 100644
@@ -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);

setup.c +1 -1

index 19dded55788..d319f499d6b 100644
@@ -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.

sha1-file.c +4 -9

index 616886799e5..f2b24654895 100644
@@ -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)

submodule.c +4 -3

index bad7a788c06..215c62580fc 100644
@@ -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);

worktree.c +6 -2

index e7bbf716f6b..2a340fa939b 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-03-06 21:54 UTC #2 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

Quoted 14 lines · writes:

From: Alexandr Miloslavskiy <[email protected]>
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

Quoted 4 lines

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?

Quoted 6 lines

environment.c +4 -1

-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) {

environment.c static void update_relative_gitdir(const char *name, · line 362

Quoted 10 lines

environment.c +6 -0

+		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.

Junio C Hamano

2020-03-06 22:12 UTC #5 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

Quoted 5 lines · 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.

Quoted 13 lines

builtin/clone.c +1 -0

index 1ad26f4d8c8..e5c2a229a11 100644
@@ -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);

builtin/clone.c static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, · line 459

Quoted 6 lines

builtin/clone.c +2 -1

@@ -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.

Quoted 6 lines

builtin/clone.c +0 -0

 static void clone_local(const char *src_repo, const char *dest_repo)

builtin/commit-graph.c +0 -0

index 4a70b33fb5f..3d7ec640e01 100644

builtin/commit-graph.c static struct object_directory *find_odb(struct repository *r, · line 47

Quoted 10 lines

builtin/commit-graph.c +3 -1

@@ -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.

builtin/commit-graph.c static int abspath_part_inside_repo(char *path) · line 65

Quoted 7 lines

builtin/commit-graph.c +2 -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);

Likewise.

builtin/commit-graph.c static int abspath_part_inside_repo(char *path) · line 77

Quoted 7 lines

builtin/commit-graph.c +2 -1

@@ -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.

Quoted 5 lines

t/helper/test-path-utils.c +0 -0
index 409034cf4ee..40548d31dfe 100644

t/helper/test-path-utils.c int cmd__path_utils(int argc, const char **argv) · line 297

Quoted 9 lines

t/helper/test-path-utils.c +4 -1

@@ -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.

Junio C Hamano

2020-03-06 22:14 UTC #8 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

Quoted 16 lines · writes:

sha1-file.c +3 -3

index 616886799e5..f2b24654895 100644
@@ -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) {

sha1-file.c void add_to_alternates_memory(const char *reference) · line 687

Quoted 10 lines

sha1-file.c +1 -6

 		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 ;-)

Alexandr Miloslavskiy

2020-03-06 22:42 UTC #3 lore raw

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

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

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.

Junio C Hamano

2020-03-06 22:44 UTC #10 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 814

Quoted 8 lines · writes:

builtin/rev-parse.c +4 -3

 			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.

submodule.c void absorb_git_dir_into_superproject(const char *path, · line 2171

Quoted 13 lines

builtin/rev-parse.c +0 -0

 				continue;
 			}
 			if (!strcmp(arg, "--show-prefix")) {

submodule.c +1 -1

index 215c62580fc..46f6c2cbfd0 100644
@@ -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".

submodule.h void absorb_git_dir_into_superproject(const char *path, · line 157

Quoted 13 lines

submodule.h +2 -2

index c81ec1a9b6c..17492e478fc 100644
@@ -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.

Alexandr Miloslavskiy

2020-03-06 22:54 UTC #6 lore raw

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

In reply to Junio C Hamano

Quoted 8 lines · Junio C Hamano

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

Quoted 5 lines

	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.

Alexandr Miloslavskiy

2020-03-06 23:06 UTC #11 lore raw

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

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

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 :(

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-10 13:17 UTC #16 lore raw

PATCH v2 4/4 get_superproject_working_tree(): return strbuf

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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 <[email protected]>

---
 builtin/rev-parse.c |  7 ++++---
 submodule.c         | 17 ++++++++---------
 submodule.h         |  4 ++--
 3 files changed, 14 insertions(+), 14 deletions(-)

builtin/rev-parse.c +4 -3

index 06ca7175ac7..06056434ed1 100644
@@ -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")) {

submodule.c +8 -9

index 215c62580fc..c3aadf3fff8 100644
@@ -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);

submodule.h +2 -2

index c81ec1a9b6c..4dad649f942 100644
@@ -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

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-10 13:17 UTC #15 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

This commit continues the work started with previous commit.

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
 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(-)

abspath.c +0 ~~-10~~

index d34026bfeb8..6f15a418bb6 100644
@@ -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;

cache.h +0 -1

index f6937793ec2..aa3f5ce718a 100644
@@ -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);

setup.c +1 -1

index 1ae3f203016..9e8fa46bc78 100644
@@ -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.

sha1-file.c +4 -9

index 616886799e5..f2b24654895 100644
@@ -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)

submodule.c +4 -3

index bad7a788c06..215c62580fc 100644
@@ -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);

worktree.c +5 -2

index e7bbf716f6b..543472f0c7b 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-10 13:17 UTC #14 lore raw

PATCH v2 2/4 real_path: remove unsafe API

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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.

[1] https://lore.kernel.org/git/[email protected]/

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
 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(-)

abspath.c +1 -7

index 98579853299..d34026bfeb8 100644
@@ -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)
 {

builtin/clone.c +5 -1

index 1ad26f4d8c8..488bdb07417 100644
@@ -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)

builtin/commit-graph.c +4 -1

index 4a70b33fb5f..d1ab6625f63 100644
@@ -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);

builtin/rev-parse.c +4 -1

index 7a00da82035..06ca7175ac7 100644
@@ -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;
 					}
 				}

builtin/worktree.c +6 -3

index 24f22800f38..d99db356684 100644
@@ -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;
 }

cache.h +0 -1

index 8cee257d3d7..f6937793ec2 100644
@@ -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);

editor.c +9 -2

index f079abbf110..91989ee8a11 100644
@@ -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);

environment.c +6 -1

index c436de31eef..10c9061c432 100644
@@ -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)

path.c +1 -1

index c5a8fe4f0c3..0a42ceb3fb5 100644
@@ -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)
 {

setup.c +12 -3

index fa4317e707a..1ae3f203016 100644
@@ -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)

submodule.c +3 -1

index 31f391d7d25..bad7a788c06 100644
@@ -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);

t/helper/test-path-utils.c +4 -1

index 409034cf4ee..313a153209c 100644
@@ -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;
 	}

worktree.c +4 -1

index eba4fd3a038..e7bbf716f6b 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-10 13:17 UTC #13 lore raw

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

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c |  4 ++--
 cache.h           |  2 +-
 environment.c     | 11 ++++++++++-
 path.c            |  2 +-
 setup.c           | 18 +++++++++---------
 5 files changed, 23 insertions(+), 14 deletions(-)

builtin/init-db.c +2 -2

index 944ec77fe10..5bf61a7e056 100644
@@ -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;

cache.h +1 -1

index 37c899b53f7..8cee257d3d7 100644
@@ -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);

environment.c +10 -1

index e72a02d0d57..c436de31eef 100644
@@ -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)

path.c +1 -1

index 88cf5930073..c5a8fe4f0c3 100644
@@ -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;
 	}

setup.c +9 -9

index 4ea7a0b081b..fa4317e707a 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy via GitGitGadget

author

2020-03-10 13:18 UTC #12 lore raw

v2 cover Fix bugs related to real_path()

In reply to 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.

 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

Signature · 1 line

gitgitgadget