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

Commit Message

Linus Arver via GitGitGadget March 6, 2020, 7:03 p.m. UTC

From: Alexandr Miloslavskiy <[e-mail]>

`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 <[e-mail]>
---
 builtin/init-db.c |  4 ++--
 cache.h           |  2 +-
 environment.c     | 11 ++++++++++-
 path.c            |  2 +-
 setup.c           | 18 +++++++++---------
 5 files changed, 23 insertions(+), 14 deletions(-)

Comments

Junio C Hamano March 6, 2020, 9:54 p.m. UTC | #1

"Alexandr Miloslavskiy via GitGitGadget" <[e-mail]>
writes:

> From: Alexandr Miloslavskiy <[e-mail]>
>
> `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.

Alexandr Miloslavskiy March 6, 2020, 10:42 p.m. UTC | #2

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.

Patch

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