{"thread":{"id":"45310","subject":"Crash on MSYS2 with GIT_WORK_TREE","startedAt":"2017-03-07T21:36:13Z","lastAt":"2017-03-08T22:20:00Z","messageCount":13,"participants":["valtron","Johannes Schindelin","Brandon Williams","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"313453","messageId":"CAFKRc7y_kpCGNORENUZ2qw_4qBwjjyaaDFxAEQa52fTryj+w7A@mail.gmail.com","threadId":"45310","inReplyTo":null,"subject":"Crash on MSYS2 with GIT_WORK_TREE","fromName":"valtron","fromEmail":"valtron2000@gmail.com","sentAt":"2017-03-07T21:28:57Z","receivedAt":"2017-03-07T21:36:13Z","isPatch":false,"sender":{"key":"valtron2000@gmail.com","avatar":null},"body":"Git 1.12.0. When GIT_WORK_TREE contains a drive-letter and\nforward-slashes, some git commands crash:\n\nC:\\repo>set GIT_WORK_TREE=C:/repo\nC:\\repo>git rev-parse HEAD\n     1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping\nstack trace to git.exe.stackdump\nC:\\repo>set GIT_WORK_TREE=\nC:\\repo>git rev-parse HEAD\na394e40861e1012a96f9578a1f0cf0c5a49ede11\n\nOn the other hand, \"C:\\repo\" and \"/c/repo\" don't have this issue.\n\nStacktrace from GDB (on git-rev-parse) is:\n\n#0  0x000000018019634d in strcmp (s1=0x600062080 \"/c/repo\", s2=0x0)\n   at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83\n#1  0x00000001005239f1 in ?? ()\n#2  0x0000000100523f36 in ?? ()\n#3  0x000000010046c6fa in ?? ()\n#4  0x0000000100401b6d in ?? ()\n#5  0x0000000100401e4b in ?? ()\n#6  0x0000000100563593 in ?? ()\n#7  0x0000000180047c37 in _cygwin_exit_return ()\n   at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031\n#8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,\nfunc=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,\n   buf=buf@entry=0xffffcdf0) at\n/msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40\n#9  0x0000000180045924 in _cygtls::call (func=<optimized out>,\narg=<optimized out>)\n   at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27\n#10 0x0000000000000000 in ?? ()\nBacktrace stopped: previous frame inner to this frame (corrupt stack?)\n\nIt seems \"C:/repo\" was changed to \"/c/repo\", but it crashes because it\ngets compared to a nullptr.\n"},{"id":"313468","messageId":"alpine.DEB.2.20.1703072345530.3767@virtualbox","threadId":"45310","inReplyTo":"CAFKRc7y_kpCGNORENUZ2qw_4qBwjjyaaDFxAEQa52fTryj+w7A@mail.gmail.com","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-07T23:03:30Z","receivedAt":"2017-03-08T00:02:50Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi valtron,\n\nOn Tue, 7 Mar 2017, valtron wrote:\n\n> Git 1.12.0.\n\nI take it you meant 2.12.0. And you probably also meant to imply that you\nare referring to MSYS2's git.exe in /usr/bin/.\n\n> When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git\n> commands crash:\n> \n> C:\\repo>set GIT_WORK_TREE=C:/repo\n> C:\\repo>git rev-parse HEAD\n>      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping\n> stack trace to git.exe.stackdump\n\nThis suggests that my assumption above is correct: it looks as if you are\nexecuting <MSYS2>\\usr\\bin\\git.exe here.\n\nThe proof lies in the pudding, though, and you are the only person who can\ndo that unless you post the contents of that git.exe.stackdump.\n\n> Stacktrace from GDB (on git-rev-parse) is:\n> \n> #0  0x000000018019634d in strcmp (s1=0x600062080 \"/c/repo\", s2=0x0)\n>    at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83\n> #1  0x00000001005239f1 in ?? ()\n> #2  0x0000000100523f36 in ?? ()\n> #3  0x000000010046c6fa in ?? ()\n> #4  0x0000000100401b6d in ?? ()\n> #5  0x0000000100401e4b in ?? ()\n> #6  0x0000000100563593 in ?? ()\n> #7  0x0000000180047c37 in _cygwin_exit_return ()\n>    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031\n> #8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,\n> func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,\n>    buf=buf@entry=0xffffcdf0) at\n> /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40\n> #9  0x0000000180045924 in _cygtls::call (func=<optimized out>,\n> arg=<optimized out>)\n>    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27\n> #10 0x0000000000000000 in ?? ()\n> Backtrace stopped: previous frame inner to this frame (corrupt stack?)\n\nThis may be the wrong thread, though. You can find out what other threads\nthere are with `info threads` and switch by `thread <id>`, the\nbacktrace(s) of the other thread(s) may be informative.\n\nPlease also note that installing the `msys2-runtime-devel` package via\nPacman may be able to get you nicer symbols.\n\nIn any case, this problem is squarely within the MSYS2 runtime. It has\nnothing to do with Git except for the motivation to set an environment\nvariable to an absolute path as you outlined.\n\nHaving said that, the problem also occurs when using *MSYS2* git.exe (i.e.\n/usr/bin/git.exe, not /mingw64/bin/git.exe) in the Git for Windows SDK.\n\nCiao,\nJohannes\n"},{"id":"313473","messageId":"CAFKRc7ysOAOVx-7ww7MLF1qKpuKdJQqAtAhLJcYh3yMD3G2ncA@mail.gmail.com","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703080104580.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"valtron","fromEmail":"valtron2000@gmail.com","sentAt":"2017-03-08T01:08:29Z","receivedAt":"2017-03-08T01:09:29Z","isPatch":false,"sender":{"key":"valtron2000@gmail.com","avatar":null},"body":"Hi Johannes,\n\nThanks for looking at this! Yes, it's 2.12.0, sorry for the typo.\n\nI only ran into this because of git-gui, where I eventually tracked it\ndown to line 1330:\n\n    set env(GIT_WORK_TREE) $_gitworktree\n\nWith that line commented out, it works. I'll look into why git-gui\nsets it to a windows-path-with-forward-slashes, but that's a separate\nissue from the crash. Also, from the stack trace, I think git is still\nable to understand the path, since it appears to correctly convert it\nto /c/repo, but I might be wrong since I haven't look at the code.\n\nOn Tue, Mar 7, 2017 at 5:51 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi valtron,\n>\n> On Wed, 8 Mar 2017, Johannes Schindelin wrote:\n>\n>> On Tue, 7 Mar 2017, valtron wrote:\n>>\n>> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git\n>> > commands crash:\n>> >\n>> > C:\\repo>set GIT_WORK_TREE=C:/repo\n>> > C:\\repo>git rev-parse HEAD\n>> >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping\n>> > stack trace to git.exe.stackdump\n>>\n>> [...]\n>>\n>> In any case, this problem is squarely within the MSYS2 runtime. It has\n>> nothing to do with Git except for the motivation to set an environment\n>> variable to an absolute path as you outlined.\n>\n> Oh boy was I *wrong*! I take that back and apologize for my premature\n> verdict.\n>\n> It is true that you should not set GIT_WORKTREE=c:/repo if you want to\n> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,\n> and it will simply try to guess correctly and convert Windows paths with\n> drive letters and backslashes to that form.\n>\n> But that does not excuse a crash.\n>\n> The problem is actually even worse: On *Linux*, this happens:\n>\n>         $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD\n>         Segmentation fault (core dumped)\n>\n> The reason is this: when set_git_work_tree() was converted from using\n> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that\n> the former passed die_on_error = 1 to strbuf_realpath(), while the latter\n> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and\n> the current code does not expect set_git_work_tree() to return\n> successfully after setting work_tree to NULL.\n>\n> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use\n> real_pathdup and strbuf_realpath, 2016-12-12).\n>\n> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())\n> -> real_pathdup() sites have a problem now. The previous contract was that\n> real_path() would die() if the passed path is invalid. The new contract is\n> that real_pathdup() returns NULL in such a case. I believe that the\n> following call sites are problematic in particular:\n>\n> builtin/init-db.c: init_db():\n>         char *original_git_dir = real_pathdup(git_dir);\n>\n> builtin/init-db.c: cmd_init_db():\n>         real_git_dir = real_pathdup(real_git_dir);\n>         ...\n>         git_work_tree_cfg = real_pathdup(rel);\n>\n> environment.c: set_git_work_tree():\n>         work_tree = real_pathdup(new_work_tree);\n>\n> setup.c: setup_discovered_git_dir():\n>         gitdir = real_pathdup(gitdir);\n>\n> submodule.c: connect_work_tree_and_git_dir():\n>         const char *real_work_tree = real_pathdup(work_tree);\n>\n> transport.c: refs_from_alternate_cb():\n>         other = real_pathdup(e->path);\n>\n> worktree.c: find_worktree():\n>         path = real_pathdup(arg);\n>\n> I verified that all calls are still there, except for the submodule.c one\n> which simply moved to dir.c and the transport.c one which apparently now\n> no longer die()s but simply ignores non-existing paths now.\n>\n> That leaves six places to patch, methinks... This diff may serve as an\n> initial version, but I have not really had a deep look at all call sites\n> (and it is an unwise idea to trust me at this hour anyway, look at the\n> time when I sent this mail):\n>\n> -- snipsnap --\n> diff --git a/abspath.c b/abspath.c\n> index 2f0c26e0e2c..b02e068aa34 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)\n>         return strbuf_realpath(&realpath, path, 0);\n>  }\n>\n> -char *real_pathdup(const char *path)\n> +char *real_pathdup(const char *path, int die_on_error)\n>  {\n>         struct strbuf realpath = STRBUF_INIT;\n>         char *retval = NULL;\n>\n> -       if (strbuf_realpath(&realpath, path, 0))\n> +       if (strbuf_realpath(&realpath, path, die_on_error))\n>                 retval = strbuf_detach(&realpath, NULL);\n>\n>         strbuf_release(&realpath);\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 1d4d6a00789..8a6acb0ec69 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n>  {\n>         int reinit;\n>         int exist_ok = flags & INIT_DB_EXIST_OK;\n> -       char *original_git_dir = real_pathdup(git_dir);\n> +       char *original_git_dir = real_pathdup(git_dir, 1);\n>\n>         if (real_git_dir) {\n>                 struct stat st;\n> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>         argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n>\n>         if (real_git_dir && !is_absolute_path(real_git_dir))\n> -               real_git_dir = real_pathdup(real_git_dir);\n> +               real_git_dir = real_pathdup(real_git_dir, 1);\n>\n>         if (argc == 1) {\n>                 int mkdir_tried = 0;\n> @@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>                 const char *git_dir_parent = strrchr(git_dir, '/');\n>                 if (git_dir_parent) {\n>                         char *rel = xstrndup(git_dir, git_dir_parent - git_dir);\n> -                       git_work_tree_cfg = real_pathdup(rel);\n> +                       git_work_tree_cfg = real_pathdup(rel, 1);\n>                         free(rel);\n>                 }\n>                 if (!git_work_tree_cfg)\n> diff --git a/cache.h b/cache.h\n> index e7b57457e73..7168c1e5ff0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n>                       int die_on_error);\n>  const char *real_path(const char *path);\n>  const char *real_path_if_valid(const char *path);\n> -char *real_pathdup(const char *path);\n> +char *real_pathdup(const char *path, int die_on_error);\n>  const char *absolute_path(const char *path);\n>  char *absolute_pathdup(const char *path);\n>  const char *remove_leading_path(const char *in, const char *prefix);\n> diff --git a/dir.c b/dir.c\n> index 4541f9e1460..aeeb5ce1049 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)\n>  {\n>         struct strbuf file_name = STRBUF_INIT;\n>         struct strbuf rel_path = STRBUF_INIT;\n> -       char *git_dir = real_pathdup(git_dir_);\n> -       char *work_tree = real_pathdup(work_tree_);\n> +       char *git_dir = real_pathdup(git_dir_, 1);\n> +       char *work_tree = real_pathdup(work_tree_, 1);\n>\n>         /* Update gitfile */\n>         strbuf_addf(&file_name, \"%s/.git\", work_tree);\n> diff --git a/environment.c b/environment.c\n> index c07fb17fb70..42dc3106d2f 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)\n>                 return;\n>         }\n>         git_work_tree_initialized = 1;\n> -       work_tree = real_pathdup(new_work_tree);\n> +       work_tree = real_pathdup(new_work_tree, 1);\n>  }\n>\n>  const char *get_git_work_tree(void)\n> diff --git a/setup.c b/setup.c\n> index 9118b48590a..d51549a6de3 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n>         /* --work-tree is set without --git-dir; use discovered one */\n>         if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {\n>                 if (offset != cwd->len && !is_absolute_path(gitdir))\n> -                       gitdir = real_pathdup(gitdir);\n> +                       gitdir = real_pathdup(gitdir, 1);\n>                 if (chdir(cwd->buf))\n>                         die_errno(\"Could not come back to cwd\");\n>                 return setup_explicit_git_dir(gitdir, cwd, nongit_ok);\n> @@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n>                 /* Keep entry but do not canonicalize it */\n>                 return 1;\n>         } else {\n> -               char *real_path = real_pathdup(ceil);\n> +               char *real_path = real_pathdup(ceil, 0);\n>                 if (!real_path) {\n>                         return 0;\n>                 }\n> diff --git a/submodule.c b/submodule.c\n> index 3b98766a6bc..1d4c0ce86ee 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n>                 /* If it is an actual gitfile, it doesn't need migration. */\n>                 return;\n>\n> -       real_old_git_dir = real_pathdup(old_git_dir);\n> +       real_old_git_dir = real_pathdup(old_git_dir, 0);\n>\n>         sub = submodule_from_path(null_sha1, path);\n>         if (!sub)\n> @@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n>         new_git_dir = git_path(\"modules/%s\", sub->name);\n>         if (safe_create_leading_directories_const(new_git_dir) < 0)\n>                 die(_(\"could not create directory '%s'\"), new_git_dir);\n> -       real_new_git_dir = real_pathdup(new_git_dir);\n> +       real_new_git_dir = real_pathdup(new_git_dir, 0);\n>\n>         if (!prefix)\n>                 prefix = get_super_prefix();\n> @@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>                 new_git_dir = git_path(\"modules/%s\", sub->name);\n>                 if (safe_create_leading_directories_const(new_git_dir) < 0)\n>                         die(_(\"could not create directory '%s'\"), new_git_dir);\n> -               real_new_git_dir = real_pathdup(new_git_dir);\n> +               real_new_git_dir = real_pathdup(new_git_dir, 0);\n>                 connect_work_tree_and_git_dir(path, real_new_git_dir);\n>\n>                 free(real_new_git_dir);\n>         } else {\n>                 /* Is it already absorbed into the superprojects git dir? */\n> -               char *real_sub_git_dir = real_pathdup(sub_git_dir);\n> -               char *real_common_git_dir = real_pathdup(get_git_common_dir());\n> +               char *real_sub_git_dir = real_pathdup(sub_git_dir, 0);\n> +               char *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);\n>\n>                 if (!starts_with(real_sub_git_dir, real_common_git_dir))\n>                         relocate_single_git_dir_into_superproject(prefix, path);\n> diff --git a/worktree.c b/worktree.c\n> index d633761575b..0486e31ad4a 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,\n>                 return wt;\n>\n>         arg = prefix_filename(prefix, strlen(prefix), arg);\n> -       path = real_pathdup(arg);\n> +       path = real_pathdup(arg, 1);\n>         for (; *list; list++)\n>                 if (!fspathcmp(path, real_path((*list)->path)))\n>                         break;\n"},{"id":"313475","messageId":"alpine.DEB.2.20.1703080159420.3767@virtualbox","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703072345530.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T01:05:51Z","receivedAt":"2017-03-08T01:12:44Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi valtron,\n\njust to set the record straight on a few of my suggestions that turned out\nto be incorrect:\n\nOn Wed, 8 Mar 2017, Johannes Schindelin wrote:\n\n> On Tue, 7 Mar 2017, valtron wrote:\n> \n> > Stacktrace from GDB (on git-rev-parse) is:\n> > \n> > #0  0x000000018019634d in strcmp (s1=0x600062080 \"/c/repo\", s2=0x0)\n> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83\n> > #1  0x00000001005239f1 in ?? ()\n> > #2  0x0000000100523f36 in ?? ()\n> > #3  0x000000010046c6fa in ?? ()\n> > #4  0x0000000100401b6d in ?? ()\n> > #5  0x0000000100401e4b in ?? ()\n> > #6  0x0000000100563593 in ?? ()\n> > #7  0x0000000180047c37 in _cygwin_exit_return ()\n> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031\n> > #8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,\n> > func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,\n> >    buf=buf@entry=0xffffcdf0) at\n> > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40\n> > #9  0x0000000180045924 in _cygtls::call (func=<optimized out>,\n> > arg=<optimized out>)\n> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27\n> > #10 0x0000000000000000 in ?? ()\n> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)\n> \n> This may be the wrong thread, though. You can find out what other threads\n> there are with `info threads` and switch by `thread <id>`, the\n> backtrace(s) of the other thread(s) may be informative.\n\nIt was actually the correct thread.\n\n> Please also note that installing the `msys2-runtime-devel` package via\n> Pacman may be able to get you nicer symbols.\n\nI was wrong. The problem here is git.exe: you would have to uncomment the\n'#options(\"debug\",\"!strip\")' line in the git/PKGBUILD file in\nhttps://github.com/Alexpux/MSYS2-packages before rebuilding the package\n[*1*], and then reinstalling it (which increases the installed size by\n94.82MB to 119.63MB due to the unstripped symbols). But then gdb would\nfind the symbols alright and you'd see that the crash happens in\n\n#1  0x000000010051aecb in setup_explicit_git_dir\n    (gitdirenv=gitdirenv@entry=0x1005a00cc <sign_off_header+556> \".git\",\n     cwd=cwd@entry=0x10055e970 <cwd>, nongit_ok=nongit_ok@entry=0x0) at\n    setup.c:669\n669             if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */\n\nand worktree indeed was NULL.\n\nCiao,\nJohannes\n"},{"id":"313477","messageId":"alpine.DEB.2.20.1703080104580.3767@virtualbox","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703072345530.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T00:51:22Z","receivedAt":"2017-03-08T01:45:37Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi valtron,\n\nOn Wed, 8 Mar 2017, Johannes Schindelin wrote:\n\n> On Tue, 7 Mar 2017, valtron wrote:\n> \n> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git\n> > commands crash:\n> > \n> > C:\\repo>set GIT_WORK_TREE=C:/repo\n> > C:\\repo>git rev-parse HEAD\n> >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping\n> > stack trace to git.exe.stackdump\n> \n> [...]\n>\n> In any case, this problem is squarely within the MSYS2 runtime. It has\n> nothing to do with Git except for the motivation to set an environment\n> variable to an absolute path as you outlined.\n\nOh boy was I *wrong*! I take that back and apologize for my premature\nverdict.\n\nIt is true that you should not set GIT_WORKTREE=c:/repo if you want to\nwork with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,\nand it will simply try to guess correctly and convert Windows paths with\ndrive letters and backslashes to that form.\n\nBut that does not excuse a crash.\n\nThe problem is actually even worse: On *Linux*, this happens:\n\n\t$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD\n\tSegmentation fault (core dumped)\n\nThe reason is this: when set_git_work_tree() was converted from using\nxstrdup(real_path()) to real_pathdup(), we completely missed the fact that\nthe former passed die_on_error = 1 to strbuf_realpath(), while the latter\npassed die_on_error = 0. As a consequence, work_tree can be NULL now, and\nthe current code does not expect set_git_work_tree() to return\nsuccessfully after setting work_tree to NULL.\n\nI Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use\nreal_pathdup and strbuf_realpath, 2016-12-12).\n\nBrandon, I have a hunch that pretty much all of the xstrdup(real_path())\n-> real_pathdup() sites have a problem now. The previous contract was that\nreal_path() would die() if the passed path is invalid. The new contract is\nthat real_pathdup() returns NULL in such a case. I believe that the\nfollowing call sites are problematic in particular:\n\nbuiltin/init-db.c: init_db():\n\tchar *original_git_dir = real_pathdup(git_dir);\n\nbuiltin/init-db.c: cmd_init_db():\n\treal_git_dir = real_pathdup(real_git_dir);\n\t...\n\tgit_work_tree_cfg = real_pathdup(rel);\n\nenvironment.c: set_git_work_tree():\n\twork_tree = real_pathdup(new_work_tree);\n\nsetup.c: setup_discovered_git_dir():\n\tgitdir = real_pathdup(gitdir);\n\nsubmodule.c: connect_work_tree_and_git_dir():\n\tconst char *real_work_tree = real_pathdup(work_tree);\n\ntransport.c: refs_from_alternate_cb():\n\tother = real_pathdup(e->path);\n\nworktree.c: find_worktree():\n\tpath = real_pathdup(arg);\n\nI verified that all calls are still there, except for the submodule.c one\nwhich simply moved to dir.c and the transport.c one which apparently now\nno longer die()s but simply ignores non-existing paths now.\n\nThat leaves six places to patch, methinks... This diff may serve as an\ninitial version, but I have not really had a deep look at all call sites\n(and it is an unwise idea to trust me at this hour anyway, look at the\ntime when I sent this mail):\n\n-- snipsnap --\ndiff --git a/abspath.c b/abspath.c\nindex 2f0c26e0e2c..b02e068aa34 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)\n \treturn strbuf_realpath(&realpath, path, 0);\n }\n \n-char *real_pathdup(const char *path)\n+char *real_pathdup(const char *path, int die_on_error)\n {\n \tstruct strbuf realpath = STRBUF_INIT;\n \tchar *retval = NULL;\n \n-\tif (strbuf_realpath(&realpath, path, 0))\n+\tif (strbuf_realpath(&realpath, path, die_on_error))\n \t\tretval = strbuf_detach(&realpath, NULL);\n \n \tstrbuf_release(&realpath);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 1d4d6a00789..8a6acb0ec69 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n {\n \tint reinit;\n \tint exist_ok = flags & INIT_DB_EXIST_OK;\n-\tchar *original_git_dir = real_pathdup(git_dir);\n+\tchar *original_git_dir = real_pathdup(git_dir, 1);\n \n \tif (real_git_dir) {\n \t\tstruct stat st;\n@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n \n \tif (real_git_dir && !is_absolute_path(real_git_dir))\n-\t\treal_git_dir = real_pathdup(real_git_dir);\n+\t\treal_git_dir = real_pathdup(real_git_dir, 1);\n \n \tif (argc == 1) {\n \t\tint mkdir_tried = 0;\n@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\tconst char *git_dir_parent = strrchr(git_dir, '/');\n \t\tif (git_dir_parent) {\n \t\t\tchar *rel = xstrndup(git_dir, git_dir_parent - git_dir);\n-\t\t\tgit_work_tree_cfg = real_pathdup(rel);\n+\t\t\tgit_work_tree_cfg = real_pathdup(rel, 1);\n \t\t\tfree(rel);\n \t\t}\n \t\tif (!git_work_tree_cfg)\ndiff --git a/cache.h b/cache.h\nindex e7b57457e73..7168c1e5ff0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t      int die_on_error);\n const char *real_path(const char *path);\n const char *real_path_if_valid(const char *path);\n-char *real_pathdup(const char *path);\n+char *real_pathdup(const char *path, int die_on_error);\n const char *absolute_path(const char *path);\n char *absolute_pathdup(const char *path);\n const char *remove_leading_path(const char *in, const char *prefix);\ndiff --git a/dir.c b/dir.c\nindex 4541f9e1460..aeeb5ce1049 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)\n {\n \tstruct strbuf file_name = STRBUF_INIT;\n \tstruct strbuf rel_path = STRBUF_INIT;\n-\tchar *git_dir = real_pathdup(git_dir_);\n-\tchar *work_tree = real_pathdup(work_tree_);\n+\tchar *git_dir = real_pathdup(git_dir_, 1);\n+\tchar *work_tree = real_pathdup(work_tree_, 1);\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\ndiff --git a/environment.c b/environment.c\nindex c07fb17fb70..42dc3106d2f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)\n \t\treturn;\n \t}\n \tgit_work_tree_initialized = 1;\n-\twork_tree = real_pathdup(new_work_tree);\n+\twork_tree = real_pathdup(new_work_tree, 1);\n }\n \n const char *get_git_work_tree(void)\ndiff --git a/setup.c b/setup.c\nindex 9118b48590a..d51549a6de3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \t/* --work-tree is set without --git-dir; use discovered one */\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {\n \t\tif (offset != cwd->len && !is_absolute_path(gitdir))\n-\t\t\tgitdir = real_pathdup(gitdir);\n+\t\t\tgitdir = real_pathdup(gitdir, 1);\n \t\tif (chdir(cwd->buf))\n \t\t\tdie_errno(\"Could not come back to cwd\");\n \t\treturn setup_explicit_git_dir(gitdir, cwd, nongit_ok);\n@@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n \t\t/* Keep entry but do not canonicalize it */\n \t\treturn 1;\n \t} else {\n-\t\tchar *real_path = real_pathdup(ceil);\n+\t\tchar *real_path = real_pathdup(ceil, 0);\n \t\tif (!real_path) {\n \t\t\treturn 0;\n \t\t}\ndiff --git a/submodule.c b/submodule.c\nindex 3b98766a6bc..1d4c0ce86ee 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \t\t/* If it is an actual gitfile, it doesn't need migration. */\n \t\treturn;\n \n-\treal_old_git_dir = real_pathdup(old_git_dir);\n+\treal_old_git_dir = real_pathdup(old_git_dir, 0);\n \n \tsub = submodule_from_path(null_sha1, path);\n \tif (!sub)\n@@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tnew_git_dir = git_path(\"modules/%s\", sub->name);\n \tif (safe_create_leading_directories_const(new_git_dir) < 0)\n \t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n-\treal_new_git_dir = real_pathdup(new_git_dir);\n+\treal_new_git_dir = real_pathdup(new_git_dir, 0);\n \n \tif (!prefix)\n \t\tprefix = get_super_prefix();\n@@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n \t\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n \t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n-\t\treal_new_git_dir = real_pathdup(new_git_dir);\n+\t\treal_new_git_dir = real_pathdup(new_git_dir, 0);\n \t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n \n \t\tfree(real_new_git_dir);\n \t} else {\n \t\t/* Is it already absorbed into the superprojects git dir? */\n-\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir);\n-\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir());\n+\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir, 0);\n+\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);\n \n \t\tif (!starts_with(real_sub_git_dir, real_common_git_dir))\n \t\t\trelocate_single_git_dir_into_superproject(prefix, path);\ndiff --git a/worktree.c b/worktree.c\nindex d633761575b..0486e31ad4a 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,\n \t\treturn wt;\n \n \targ = prefix_filename(prefix, strlen(prefix), arg);\n-\tpath = real_pathdup(arg);\n+\tpath = real_pathdup(arg, 1);\n \tfor (; *list; list++)\n \t\tif (!fspathcmp(path, real_path((*list)->path)))\n \t\t\tbreak;\n"},{"id":"313478","messageId":"20170308020918.GA1650@google.com","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703080104580.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-08T02:09:18Z","receivedAt":"2017-03-08T02:09:32Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/08, Johannes Schindelin wrote:\n> Hi valtron,\n> \n> On Wed, 8 Mar 2017, Johannes Schindelin wrote:\n> \n> > On Tue, 7 Mar 2017, valtron wrote:\n> > \n> > > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git\n> > > commands crash:\n> > > \n> > > C:\\repo>set GIT_WORK_TREE=C:/repo\n> > > C:\\repo>git rev-parse HEAD\n> > >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping\n> > > stack trace to git.exe.stackdump\n> > \n> > [...]\n> >\n> > In any case, this problem is squarely within the MSYS2 runtime. It has\n> > nothing to do with Git except for the motivation to set an environment\n> > variable to an absolute path as you outlined.\n> \n> Oh boy was I *wrong*! I take that back and apologize for my premature\n> verdict.\n> \n> It is true that you should not set GIT_WORKTREE=c:/repo if you want to\n> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,\n> and it will simply try to guess correctly and convert Windows paths with\n> drive letters and backslashes to that form.\n> \n> But that does not excuse a crash.\n> \n> The problem is actually even worse: On *Linux*, this happens:\n> \n> \t$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD\n> \tSegmentation fault (core dumped)\n> \n> The reason is this: when set_git_work_tree() was converted from using\n> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that\n> the former passed die_on_error = 1 to strbuf_realpath(), while the latter\n> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and\n> the current code does not expect set_git_work_tree() to return\n> successfully after setting work_tree to NULL.\n> \n> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use\n> real_pathdup and strbuf_realpath, 2016-12-12).\n> \n> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())\n> -> real_pathdup() sites have a problem now. The previous contract was that\n> real_path() would die() if the passed path is invalid. The new contract is\n> that real_pathdup() returns NULL in such a case. I believe that the\n> following call sites are problematic in particular:\n\nWelp, looks like I missed that when I made the conversion.  You're\nright, the semantics of getting the real_path were changed which would\ncause a NULL to be returned instead of the program exiting with a call\nto die().  \n\nAfter a cursory look at your patch, I think all of your changes look\nsane.  I would have to take a closer look at the call sites to see if\neach caller would need to die or not.  I'm assuming you took a quick\nglace to make your decision about each call site?\n\n> \n> builtin/init-db.c: init_db():\n> \tchar *original_git_dir = real_pathdup(git_dir);\n> \n> builtin/init-db.c: cmd_init_db():\n> \treal_git_dir = real_pathdup(real_git_dir);\n> \t...\n> \tgit_work_tree_cfg = real_pathdup(rel);\n> \n> environment.c: set_git_work_tree():\n> \twork_tree = real_pathdup(new_work_tree);\n> \n> setup.c: setup_discovered_git_dir():\n> \tgitdir = real_pathdup(gitdir);\n> \n> submodule.c: connect_work_tree_and_git_dir():\n> \tconst char *real_work_tree = real_pathdup(work_tree);\n> \n> transport.c: refs_from_alternate_cb():\n> \tother = real_pathdup(e->path);\n> \n> worktree.c: find_worktree():\n> \tpath = real_pathdup(arg);\n> \n> I verified that all calls are still there, except for the submodule.c one\n> which simply moved to dir.c and the transport.c one which apparently now\n> no longer die()s but simply ignores non-existing paths now.\n> \n> That leaves six places to patch, methinks... This diff may serve as an\n> initial version, but I have not really had a deep look at all call sites\n> (and it is an unwise idea to trust me at this hour anyway, look at the\n> time when I sent this mail):\n> \n> -- snipsnap --\n> diff --git a/abspath.c b/abspath.c\n> index 2f0c26e0e2c..b02e068aa34 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)\n>  \treturn strbuf_realpath(&realpath, path, 0);\n>  }\n>  \n> -char *real_pathdup(const char *path)\n> +char *real_pathdup(const char *path, int die_on_error)\n>  {\n>  \tstruct strbuf realpath = STRBUF_INIT;\n>  \tchar *retval = NULL;\n>  \n> -\tif (strbuf_realpath(&realpath, path, 0))\n> +\tif (strbuf_realpath(&realpath, path, die_on_error))\n>  \t\tretval = strbuf_detach(&realpath, NULL);\n>  \n>  \tstrbuf_release(&realpath);\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 1d4d6a00789..8a6acb0ec69 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n>  {\n>  \tint reinit;\n>  \tint exist_ok = flags & INIT_DB_EXIST_OK;\n> -\tchar *original_git_dir = real_pathdup(git_dir);\n> +\tchar *original_git_dir = real_pathdup(git_dir, 1);\n>  \n>  \tif (real_git_dir) {\n>  \t\tstruct stat st;\n> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n>  \n>  \tif (real_git_dir && !is_absolute_path(real_git_dir))\n> -\t\treal_git_dir = real_pathdup(real_git_dir);\n> +\t\treal_git_dir = real_pathdup(real_git_dir, 1);\n>  \n>  \tif (argc == 1) {\n>  \t\tint mkdir_tried = 0;\n> @@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n>  \t\tconst char *git_dir_parent = strrchr(git_dir, '/');\n>  \t\tif (git_dir_parent) {\n>  \t\t\tchar *rel = xstrndup(git_dir, git_dir_parent - git_dir);\n> -\t\t\tgit_work_tree_cfg = real_pathdup(rel);\n> +\t\t\tgit_work_tree_cfg = real_pathdup(rel, 1);\n>  \t\t\tfree(rel);\n>  \t\t}\n>  \t\tif (!git_work_tree_cfg)\n> diff --git a/cache.h b/cache.h\n> index e7b57457e73..7168c1e5ff0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n>  \t\t      int die_on_error);\n>  const char *real_path(const char *path);\n>  const char *real_path_if_valid(const char *path);\n> -char *real_pathdup(const char *path);\n> +char *real_pathdup(const char *path, int die_on_error);\n>  const char *absolute_path(const char *path);\n>  char *absolute_pathdup(const char *path);\n>  const char *remove_leading_path(const char *in, const char *prefix);\n> diff --git a/dir.c b/dir.c\n> index 4541f9e1460..aeeb5ce1049 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)\n>  {\n>  \tstruct strbuf file_name = STRBUF_INIT;\n>  \tstruct strbuf rel_path = STRBUF_INIT;\n> -\tchar *git_dir = real_pathdup(git_dir_);\n> -\tchar *work_tree = real_pathdup(work_tree_);\n> +\tchar *git_dir = real_pathdup(git_dir_, 1);\n> +\tchar *work_tree = real_pathdup(work_tree_, 1);\n>  \n>  \t/* Update gitfile */\n>  \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n> diff --git a/environment.c b/environment.c\n> index c07fb17fb70..42dc3106d2f 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)\n>  \t\treturn;\n>  \t}\n>  \tgit_work_tree_initialized = 1;\n> -\twork_tree = real_pathdup(new_work_tree);\n> +\twork_tree = real_pathdup(new_work_tree, 1);\n>  }\n>  \n>  const char *get_git_work_tree(void)\n> diff --git a/setup.c b/setup.c\n> index 9118b48590a..d51549a6de3 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n>  \t/* --work-tree is set without --git-dir; use discovered one */\n>  \tif (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {\n>  \t\tif (offset != cwd->len && !is_absolute_path(gitdir))\n> -\t\t\tgitdir = real_pathdup(gitdir);\n> +\t\t\tgitdir = real_pathdup(gitdir, 1);\n>  \t\tif (chdir(cwd->buf))\n>  \t\t\tdie_errno(\"Could not come back to cwd\");\n>  \t\treturn setup_explicit_git_dir(gitdir, cwd, nongit_ok);\n> @@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n>  \t\t/* Keep entry but do not canonicalize it */\n>  \t\treturn 1;\n>  \t} else {\n> -\t\tchar *real_path = real_pathdup(ceil);\n> +\t\tchar *real_path = real_pathdup(ceil, 0);\n>  \t\tif (!real_path) {\n>  \t\t\treturn 0;\n>  \t\t}\n> diff --git a/submodule.c b/submodule.c\n> index 3b98766a6bc..1d4c0ce86ee 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n>  \t\t/* If it is an actual gitfile, it doesn't need migration. */\n>  \t\treturn;\n>  \n> -\treal_old_git_dir = real_pathdup(old_git_dir);\n> +\treal_old_git_dir = real_pathdup(old_git_dir, 0);\n>  \n>  \tsub = submodule_from_path(null_sha1, path);\n>  \tif (!sub)\n> @@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n>  \tnew_git_dir = git_path(\"modules/%s\", sub->name);\n>  \tif (safe_create_leading_directories_const(new_git_dir) < 0)\n>  \t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n> -\treal_new_git_dir = real_pathdup(new_git_dir);\n> +\treal_new_git_dir = real_pathdup(new_git_dir, 0);\n>  \n>  \tif (!prefix)\n>  \t\tprefix = get_super_prefix();\n> @@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,\n>  \t\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n>  \t\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n>  \t\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n> -\t\treal_new_git_dir = real_pathdup(new_git_dir);\n> +\t\treal_new_git_dir = real_pathdup(new_git_dir, 0);\n>  \t\tconnect_work_tree_and_git_dir(path, real_new_git_dir);\n>  \n>  \t\tfree(real_new_git_dir);\n>  \t} else {\n>  \t\t/* Is it already absorbed into the superprojects git dir? */\n> -\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir);\n> -\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir());\n> +\t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir, 0);\n> +\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);\n>  \n>  \t\tif (!starts_with(real_sub_git_dir, real_common_git_dir))\n>  \t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n> diff --git a/worktree.c b/worktree.c\n> index d633761575b..0486e31ad4a 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,\n>  \t\treturn wt;\n>  \n>  \targ = prefix_filename(prefix, strlen(prefix), arg);\n> -\tpath = real_pathdup(arg);\n> +\tpath = real_pathdup(arg, 1);\n>  \tfor (; *list; list++)\n>  \t\tif (!fspathcmp(path, real_path((*list)->path)))\n>  \t\t\tbreak;\n\n-- \nBrandon Williams\n"},{"id":"313481","messageId":"xmqqh9344hq6.fsf@gitster.mtv.corp.google.com","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703080104580.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-08T02:36:49Z","receivedAt":"2017-03-08T03:15:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> The problem is actually even worse: On *Linux*, this happens:\n>\n> \t$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD\n> \tSegmentation fault (core dumped)\n>\n> The reason is this: when set_git_work_tree() was converted from using\n> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that\n> the former passed die_on_error = 1 to strbuf_realpath(), while the latter\n> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and\n> the current code does not expect set_git_work_tree() to return\n> successfully after setting work_tree to NULL.\n\nOuch.\n\n> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())\n> -> real_pathdup() sites have a problem now. The previous contract was that\n> real_path() would die() if the passed path is invalid. The new contract is\n> that real_pathdup() returns NULL in such a case.\n\nOK, so it appears that we'd better audit all the callsites of\nreal_pathdup() and see if anybody _assumes_ that the return values\nare not NULL.  They all need fixing.\n\nThanks for digging it through to the root cause.\n"},{"id":"313488","messageId":"alpine.DEB.2.20.1703081254480.3767@virtualbox","threadId":"45310","inReplyTo":"20170308020918.GA1650@google.com","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T11:59:00Z","receivedAt":"2017-03-08T12:08:22Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Tue, 7 Mar 2017, Brandon Williams wrote:\n\n> On 03/08, Johannes Schindelin wrote:\n> > \n> > [...] On *Linux*, this happens:\n> > \n> > \t$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD\n> > \tSegmentation fault (core dumped)\n> > \n> > The reason is this: when set_git_work_tree() was converted from using\n> > xstrdup(real_path()) to real_pathdup(), we completely missed the fact\n> > that the former passed die_on_error = 1 to strbuf_realpath(), while\n> > the latter passed die_on_error = 0. As a consequence, work_tree can be\n> > NULL now, and the current code does not expect set_git_work_tree() to\n> > return successfully after setting work_tree to NULL.\n> > \n> > I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers\n> > use real_pathdup and strbuf_realpath, 2016-12-12).\n> > \n> > Brandon, I have a hunch that pretty much all of the\n> > xstrdup(real_path()) -> real_pathdup() sites have a problem now. The\n> > previous contract was that real_path() would die() if the passed path\n> > is invalid. The new contract is that real_pathdup() returns NULL in\n> > such a case. I believe that the following call sites are problematic\n> > in particular:\n> \n> Welp, looks like I missed that when I made the conversion.  You're\n> right, the semantics of getting the real_path were changed which would\n> cause a NULL to be returned instead of the program exiting with a call\n> to die().  \n> \n> After a cursory look at your patch, I think all of your changes look\n> sane.  I would have to take a closer look at the call sites to see if\n> each caller would need to die or not.  I'm assuming you took a quick\n> glace to make your decision about each call site?\n\nI did take a quick glance, but did you have a look at the time of day I\nsent this patch? You do not want to trust my judgement after that.\n\nAnother thing: may I ask you to delete the quoted parts of the mail that\nyou are actually not responding to? Junio also often simply keeps the rest\nof the mail quoted, and I always have to scroll all the way to the end\njust to verify that nothing more has been said, which can be slightly\nannoying when you are tired. I do plan to read your mails in the future,\nso culling the quoted-yet-unanswered part would save me trouble.\n\nThanks,\nDscho\n"},{"id":"313489","messageId":"alpine.DEB.2.20.1703081259150.3767@virtualbox","threadId":"45310","inReplyTo":"CAFKRc7ysOAOVx-7ww7MLF1qKpuKdJQqAtAhLJcYh3yMD3G2ncA@mail.gmail.com","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T12:03:24Z","receivedAt":"2017-03-08T12:22:18Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi valtron,\n\n\nOn Tue, 7 Mar 2017, valtron wrote:\n\n> I only ran into this because of git-gui, where I eventually tracked it\n> down to line 1330:\n> \n>     set env(GIT_WORK_TREE) $_gitworktree\n\nAs git-gui is a Tcl script, which in turn runs as a pure Windows\napplication, the path should use backslashes.\n\n> With that line commented out, it works. I'll look into why git-gui\n> sets it to a windows-path-with-forward-slashes, but that's a separate\n> issue from the crash.\n\nIt is... please do contribute your fix when you have one.\n\n> Also, from the stack trace, I think git is still able to understand the\n> path, since it appears to correctly convert it to /c/repo, but I might\n> be wrong since I haven't look at the code.\n\nGit does not convert the path at all. It is the *MSYS2 runtime* that\nconverts Windows paths to POSIX paths, if any. And it does so selectively.\nThe current working directory is always transformed. PATH is always\ntransformed. Environment variables are transformed *when they look like\nWindows paths*. And I am fairly certain that a GIT_WORK_TREE with a colon\nand forward-slashes fails that test: the MSYS2 runtime thinks this is not\na Windows path and leaves it alone. Enter your problem.\n\nCiao,\nJohannes\n"},{"id":"313499","messageId":"alpine.DEB.2.20.1703081304130.3767@virtualbox","threadId":"45310","inReplyTo":"xmqqa88w4bbp.fsf@gitster.mtv.corp.google.com","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T15:34:14Z","receivedAt":"2017-03-08T15:35:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Mar 2017, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > OK, so it appears that we'd better audit all the callsites of\n> > real_pathdup() and see if anybody _assumes_ that the return values are\n> > not NULL.  They all need fixing.\n\nIndeed.\n\n> I just looked at 4ac9006f (\"real_path: have callers use real_pathdup and\n> strbuf_realpath\", 2016-12-12) and it seems all hunks that replaces\n> xstrdup(real_path(...)) with real_pathdup(...) in the commit share the\n> same issue.  \n\nRight, I tried to convey that information in my email to which you\nreplied.\n\n> The one in canonicalize_ceiling_entry() looks OK, though.\n\nYes, it immediately tests whether NULL was returned.\n\n> ec9629b3 (\"submodule absorbing: fix worktree/gitdir pointers\n> recursively for non-moves\", 2017-01-25) introduces a new use of\n> real_pathdup() and the result is immediately used to call\n> connect_work_tree_and_git_dir() without checking its NULL-ness, but\n> the argument to new_git_dir is something that came from git_path()\n> that was successfully passed to safe_create_leading_directories(),\n> so this one should be OK.\n> \n> 1c16df23 (\"Merge branch 'bw/realpath-wo-chdir'\", 2017-01-18) turns a\n> few xstrdup(real_path(...)) in dir.c without thinking.  I think that\n> evil merge probably should be reverted.\n\nRather than a heavy-handed reversal, I would really prefer to perform a\ndiligent audit of all real_pathdup() callers and adjust them\nappropriately.\n\nTurns out that the canonicalize_ceiling_entry() caller is *the only one*\nhandling NULL correctly. All other callers need to be changed.\n\nWill send something out in a moment.\n\nCiao,\nJohannes\n"},{"id":"313511","messageId":"xmqqpohr3cm9.fsf@gitster.mtv.corp.google.com","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703081304130.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-08T17:24:46Z","receivedAt":"2017-03-08T17:25:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Rather than a heavy-handed reversal, I would really prefer to perform a\n> diligent audit of all real_pathdup() callers and adjust them\n> appropriately.\n>\n> Turns out that the canonicalize_ceiling_entry() caller is *the only one*\n> handling NULL correctly. All other callers need to be changed.\n\nThe observation matches what I saw.  \n\nI would have added a patch that introduces real_pathdup_gently()\nthat returns NULL and made real_pathdup() die as before immediately\non top of the Brandon's series (which would ensure that any branch\nthat would want to use real_pathdup() would get the dying version by\ndefault) and flipped selected callers to call the gently() version\nin that patch, but what you posted is a lot more apprporiate for a\nregression fix.  By changing the function signature, the patch\nensures that it covers all the callsites.\n\nAmong the two-and-half regressions known to us post 2.12 so far,\nthis one probably needs to go into a maintenance release without\nwaiting for the other one-and-half, I think.\n\nThanks.\n"},{"id":"313532","messageId":"20170308184606.GB130604@google.com","threadId":"45310","inReplyTo":"alpine.DEB.2.20.1703081254480.3767@virtualbox","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-08T18:46:06Z","receivedAt":"2017-03-08T18:47:28Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/08, Johannes Schindelin wrote:\n> I did take a quick glance, but did you have a look at the time of day I\n> sent this patch? You do not want to trust my judgement after that.\n\nHaha Yeah I did notice, and I trust your newer patch more than the one\nyou sent at 2am :)\n\n> \n> Another thing: may I ask you to delete the quoted parts of the mail that\n> you are actually not responding to? Junio also often simply keeps the rest\n> of the mail quoted, and I always have to scroll all the way to the end\n> just to verify that nothing more has been said, which can be slightly\n> annoying when you are tired. I do plan to read your mails in the future,\n> so culling the quoted-yet-unanswered part would save me trouble.\n\nOf course, I usually try to clear the parts of the mail I'm not\nresponding to...though there are times where I forget or am a bit lazy.\nI'll definitely work on remembering to do that for the future!\n\n-- \nBrandon Williams\n"},{"id":"313563","messageId":"xmqqr3271keq.fsf@gitster.mtv.corp.google.com","threadId":"45310","inReplyTo":"20170308184606.GB130604@google.com","subject":"Re: Crash on MSYS2 with GIT_WORK_TREE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-08T22:19:25Z","receivedAt":"2017-03-08T22:20:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Of course, I usually try to clear the parts of the mail I'm not\n> responding to...though there are times where I forget or am a bit lazy.\n> I'll definitely work on remembering to do that for the future!\n\nThis cuts both ways.  Sometimes it is very useful to be able to see\nother parts that the responder is not _directly_ responding to when\nyou come as a third person to the discussion, which forces you to\nfind messages upthread, so do not overdo it.\n\n"}]}