On 03/08, Johannes Schindelin wrote:
> Hi valtron,
>
> On Wed, 8 Mar 2017, Johannes Schindelin wrote:
>
> > On Tue, 7 Mar 2017, valtron wrote:
> >
> > > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> > > commands crash:
> > >
> > > C:\repo>set GIT_WORK_TREE=C:/repo
> > > C:\repo>git rev-parse HEAD
> > > 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> > > stack trace to git.exe.stackdump
> >
> > [...]
> >
> > In any case, this problem is squarely within the MSYS2 runtime. It has
> > nothing to do with Git except for the motivation to set an environment
> > variable to an absolute path as you outlined.
>
> Oh boy was I *wrong*! I take that back and apologize for my premature
> verdict.
>
> It is true that you should not set GIT_WORKTREE=c:/repo if you want to
> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
> and it will simply try to guess correctly and convert Windows paths with
> drive letters and backslashes to that form.
>
> But that does not excuse a crash.
>
> The problem is actually even worse: On *Linux*, this happens:
>
> $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> Segmentation fault (core dumped)
>
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.
>
> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12).
>
> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case. I believe that the
> following call sites are problematic in particular:
Welp, looks like I missed that when I made the conversion. You're
right, the semantics of getting the real_path were changed which would
cause a NULL to be returned instead of the program exiting with a call
to die().
After a cursory look at your patch, I think all of your changes look
sane. I would have to take a closer look at the call sites to see if
each caller would need to die or not. I'm assuming you took a quick
glace to make your decision about each call site?
>
> builtin/init-db.c: init_db():
> char *original_git_dir = real_pathdup(git_dir);
>
> builtin/init-db.c: cmd_init_db():
> real_git_dir = real_pathdup(real_git_dir);
> ...
> git_work_tree_cfg = real_pathdup(rel);
>
> environment.c: set_git_work_tree():
> work_tree = real_pathdup(new_work_tree);
>
> setup.c: setup_discovered_git_dir():
> gitdir = real_pathdup(gitdir);
>
> submodule.c: connect_work_tree_and_git_dir():
> const char *real_work_tree = real_pathdup(work_tree);
>
> transport.c: refs_from_alternate_cb():
> other = real_pathdup(e->path);
>
> worktree.c: find_worktree():
> path = real_pathdup(arg);
>
> I verified that all calls are still there, except for the submodule.c one
> which simply moved to dir.c and the transport.c one which apparently now
> no longer die()s but simply ignores non-existing paths now.
>
> That leaves six places to patch, methinks... This diff may serve as an
> initial version, but I have not really had a deep look at all call sites
> (and it is an unwise idea to trust me at this hour anyway, look at the
> time when I sent this mail):
>
> -- snipsnap --
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> return strbuf_realpath(&realpath, path, 0);
> }
>
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
> {
> struct strbuf realpath = STRBUF_INIT;
> char *retval = NULL;
>
> - if (strbuf_realpath(&realpath, path, 0))
> + if (strbuf_realpath(&realpath, path, die_on_error))
> retval = strbuf_detach(&realpath, NULL);
>
> strbuf_release(&realpath);
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 1d4d6a00789..8a6acb0ec69 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
> {
> int reinit;
> int exist_ok = flags & INIT_DB_EXIST_OK;
> - char *original_git_dir = real_pathdup(git_dir);
> + char *original_git_dir = real_pathdup(git_dir, 1);
>
> if (real_git_dir) {
> struct stat st;
> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
>
> if (real_git_dir && !is_absolute_path(real_git_dir))
> - real_git_dir = real_pathdup(real_git_dir);
> + real_git_dir = real_pathdup(real_git_dir, 1);
>
> if (argc == 1) {
> int mkdir_tried = 0;
> @@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> const char *git_dir_parent = strrchr(git_dir, '/');
> if (git_dir_parent) {
> char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
> - git_work_tree_cfg = real_pathdup(rel);
> + git_work_tree_cfg = real_pathdup(rel, 1);
> free(rel);
> }
> if (!git_work_tree_cfg)
> diff --git a/cache.h b/cache.h
> index e7b57457e73..7168c1e5ff0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1160,7 +1160,7 @@ 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);
> +char *real_pathdup(const char *path, int die_on_error);
> const char *absolute_path(const char *path);
> char *absolute_pathdup(const char *path);
> const char *remove_leading_path(const char *in, const char *prefix);
> diff --git a/dir.c b/dir.c
> index 4541f9e1460..aeeb5ce1049 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
> {
> struct strbuf file_name = STRBUF_INIT;
> struct strbuf rel_path = STRBUF_INIT;
> - char *git_dir = real_pathdup(git_dir_);
> - char *work_tree = real_pathdup(work_tree_);
> + char *git_dir = real_pathdup(git_dir_, 1);
> + char *work_tree = real_pathdup(work_tree_, 1);
>
> /* Update gitfile */
> strbuf_addf(&file_name, "%s/.git", work_tree);
> diff --git a/environment.c b/environment.c
> index c07fb17fb70..42dc3106d2f 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
> return;
> }
> git_work_tree_initialized = 1;
> - work_tree = real_pathdup(new_work_tree);
> + work_tree = real_pathdup(new_work_tree, 1);
> }
>
> const char *get_git_work_tree(void)
> diff --git a/setup.c b/setup.c
> index 9118b48590a..d51549a6de3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
> /* --work-tree is set without --git-dir; use discovered one */
> if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> if (offset != cwd->len && !is_absolute_path(gitdir))
> - gitdir = real_pathdup(gitdir);
> + gitdir = real_pathdup(gitdir, 1);
> if (chdir(cwd->buf))
> die_errno("Could not come back to cwd");
> return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> @@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
> /* Keep entry but do not canonicalize it */
> return 1;
> } else {
> - char *real_path = real_pathdup(ceil);
> + char *real_path = real_pathdup(ceil, 0);
> if (!real_path) {
> return 0;
> }
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6bc..1d4c0ce86ee 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
> /* If it is an actual gitfile, it doesn't need migration. */
> return;
>
> - real_old_git_dir = real_pathdup(old_git_dir);
> + real_old_git_dir = real_pathdup(old_git_dir, 0);
>
> sub = submodule_from_path(null_sha1, path);
> if (!sub)
> @@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
> new_git_dir = git_path("modules/%s", sub->name);
> if (safe_create_leading_directories_const(new_git_dir) < 0)
> die(_("could not create directory '%s'"), new_git_dir);
> - real_new_git_dir = real_pathdup(new_git_dir);
> + real_new_git_dir = real_pathdup(new_git_dir, 0);
>
> if (!prefix)
> prefix = get_super_prefix();
> @@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
> new_git_dir = git_path("modules/%s", sub->name);
> if (safe_create_leading_directories_const(new_git_dir) < 0)
> die(_("could not create directory '%s'"), new_git_dir);
> - real_new_git_dir = real_pathdup(new_git_dir);
> + real_new_git_dir = real_pathdup(new_git_dir, 0);
> connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> free(real_new_git_dir);
> } else {
> /* Is it already absorbed into the superprojects git dir? */
> - char *real_sub_git_dir = real_pathdup(sub_git_dir);
> - char *real_common_git_dir = real_pathdup(get_git_common_dir());
> + char *real_sub_git_dir = real_pathdup(sub_git_dir, 0);
> + char *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);
>
> if (!starts_with(real_sub_git_dir, real_common_git_dir))
> relocate_single_git_dir_into_superproject(prefix, path);
> diff --git a/worktree.c b/worktree.c
> index d633761575b..0486e31ad4a 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
> return wt;
>
> arg = prefix_filename(prefix, strlen(prefix), arg);
> - path = real_pathdup(arg);
> + path = real_pathdup(arg, 1);
> for (; *list; list++)
> if (!fspathcmp(path, real_path((*list)->path)))
> break;
--
Brandon Williams