Web lists-archives.com

Re: Crash on MSYS2 with GIT_WORK_TREE




Hi Johannes,

Thanks for looking at this! Yes, it's 2.12.0, sorry for the typo.

I only ran into this because of git-gui, where I eventually tracked it
down to line 1330:

    set env(GIT_WORK_TREE) $_gitworktree

With that line commented out, it works. I'll look into why git-gui
sets it to a windows-path-with-forward-slashes, but that's a separate
issue from the crash. Also, from the stack trace, I think git is still
able to understand the path, since it appears to correctly convert it
to /c/repo, but I might be wrong since I haven't look at the code.

On Tue, Mar 7, 2017 at 5:51 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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:
>
> 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;