{"thread":{"id":"45316","subject":"[PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL","startedAt":"2017-03-08T15:44:12Z","lastAt":"2017-03-09T16:34:18Z","messageCount":10,"participants":["Johannes Schindelin","Jeff King","René Scharfe","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"313502","messageId":"cover.1488987786.git.johannes.schindelin@gmx.de","threadId":"45316","inReplyTo":null,"subject":"[PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T15:43:27Z","receivedAt":"2017-03-08T15:44:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was identified as the root cause for what seemed to be a\npath conversion problem in the MSYS2 runtime at first glance.\n\nOriginal bug report:\nhttp://public-inbox.org/git/CAFKRc7y_kpCGNORENUZ2qw_4qBwjjyaaDFxAEQa52fTryj+w7A@mail.gmail.com/\n\nWe may want to consider fast-tracking this into v2.12.1, and to that\nend, I would appreciate code reviews that focus on the correctness of\nthis patch and that try to consider undesired side effects.\n\n\nJohannes Schindelin (2):\n  Demonstrate NULL pointer access with invalid GIT_WORK_TREE\n  Fix callsites of real_pathdup() that wanted it to die on error\n\n abspath.c            |  4 ++--\n builtin/init-db.c    |  6 +++---\n cache.h              |  2 +-\n dir.c                |  4 ++--\n environment.c        |  2 +-\n setup.c              |  4 ++--\n submodule.c          | 10 +++++-----\n t/t1501-work-tree.sh |  8 ++++++++\n worktree.c           |  2 +-\n 9 files changed, 25 insertions(+), 17 deletions(-)\n\n\nbase-commit: e0688e9b28f2c5ff711460ee8b62077be5df2360\nPublished-As: https://github.com/dscho/git/releases/tag/real_pathdup-callers-v1\nFetch-It-Via: git fetch https://github.com/dscho/git real_pathdup-callers-v1\n\n-- \n2.12.0.windows.1.7.g94dafc3b124\n\n"},{"id":"313503","messageId":"0c0abc667d9b8dff299aa61aeb29a7e9e7316b66.1488987786.git.johannes.schindelin@gmx.de","threadId":"45316","inReplyTo":"cover.1488987786.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T15:43:40Z","receivedAt":"2017-03-08T15:44:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In 4ac9006f832 (real_path: have callers use real_pathdup and\nstrbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())\npattern to use real_pathdup() directly.\n\nThe only problem with this change is that real_path() calls\nstrbuf_realpath() with die_on_error = 1 while real_pathdup() calls it\nwith die_on_error = 0. Meaning that in cases where real_path() causes\nGit to die() with an error message, real_pathdup() is silent and returns\nNULL instead.\n\nThe callers, however, are ill-prepared for that change, as they expect\nthe return value to be non-NULL.\n\nThis patch fixes that by extending real_pathdup()'s signature to accept\nthe die_on_error flag and simply pass it through to strbuf_realpath(),\nand then adjust all callers after a careful audit whether they would\nhandle NULLs well.\n\nNote: this fix not only prevents NULL pointer accesses, but it also\nreintroduces the error messages that were lost with the change to\nreal_pathdup().\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n abspath.c            |  4 ++--\n builtin/init-db.c    |  6 +++---\n cache.h              |  2 +-\n dir.c                |  4 ++--\n environment.c        |  2 +-\n setup.c              |  4 ++--\n submodule.c          | 10 +++++-----\n t/t1501-work-tree.sh |  2 +-\n worktree.c           |  2 +-\n 9 files changed, 18 insertions(+), 18 deletions(-)\n\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 80b6372cf76..ae0b4fc70b5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1153,7 +1153,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 967f289f1ef..6b48cb91ff2 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@@ -806,7 +806,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..0a2831d846d 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, 1);\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, 1);\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, 1);\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, 1);\n+\t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir(), 1);\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/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex 046d9b7909f..b06210ec5e8 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '\n \t)\n '\n \n-test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '\n+test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '\n \t(\n \t\tGIT_WORK_TREE=/.invalid/work/tree &&\n \t\texport GIT_WORK_TREE &&\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-- \n2.12.0.windows.1.7.g94dafc3b124\n"},{"id":"313505","messageId":"c107001ff39e41314e86af3612922f687eb73dd4.1488987786.git.johannes.schindelin@gmx.de","threadId":"45316","inReplyTo":"cover.1488987786.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] Demonstrate NULL pointer access with invalid GIT_WORK_TREE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-08T15:43:34Z","receivedAt":"2017-03-08T15:44:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When GIT_WORK_TREE does not specify a valid path, we should error out.\nNot crash.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t1501-work-tree.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex cc5b870e587..046d9b7909f 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -423,4 +423,12 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '\n \t)\n '\n \n+test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '\n+\t(\n+\t\tGIT_WORK_TREE=/.invalid/work/tree &&\n+\t\texport GIT_WORK_TREE &&\n+\t\ttest_expect_code 128 git rev-parse\n+\t)\n+'\n+\n test_done\n-- \n2.12.0.windows.1.7.g94dafc3b124\n\n\n"},{"id":"313507","messageId":"20170308161742.lpbf6gn6qhoex7et@sigill.intra.peff.net","threadId":"45316","inReplyTo":"cover.1488987786.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-08T16:17:42Z","receivedAt":"2017-03-08T16:18:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 08, 2017 at 04:43:27PM +0100, Johannes Schindelin wrote:\n\n> We may want to consider fast-tracking this into v2.12.1, and to that\n> end, I would appreciate code reviews that focus on the correctness of\n> this patch and that try to consider undesired side effects.\n\nI don't see how it could be not-correct, in the sense that every caller\nnow passes the die_on_error flag (restoring the original behavior)\nexcept for the one which clearly checks for a NULL return immediately\nafterward.\n\nThe only exception would be if there were new calls to real_pathdup()\nthat did not originally use real_path(). But:\n\n  # 7241764076 introduced real_pathdup\n  git log -Sreal_pathdup 7241764076..\n\nshows only one other introduction, and it's just duplicating an existing\ncall.\n\nIt's possible that some of these _could_ handle the error case more\ngracefully (I already fixed one such case in 3a1345af2). But even if\nwe wanted to do so, that should come separately on top of this patch.\nThis can go to 'maint' as a regression fix, and then that gives a stable\nbase for making potential improvements.\n\n-Peff\n"},{"id":"313527","messageId":"81f1e30b-e0e1-d587-4a4b-4848beffd38c@web.de","threadId":"45316","inReplyTo":"0c0abc667d9b8dff299aa61aeb29a7e9e7316b66.1488987786.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-08T18:12:22Z","receivedAt":"2017-03-08T18:13:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:\n> In 4ac9006f832 (real_path: have callers use real_pathdup and\n> strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())\n> pattern to use real_pathdup() directly.\n> \n> The only problem with this change is that real_path() calls\n> strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it\n> with die_on_error = 0. Meaning that in cases where real_path() causes\n> Git to die() with an error message, real_pathdup() is silent and returns\n> NULL instead.\n> \n> The callers, however, are ill-prepared for that change, as they expect\n> the return value to be non-NULL.\n> \n> This patch fixes that by extending real_pathdup()'s signature to accept\n> the die_on_error flag and simply pass it through to strbuf_realpath(),\n> and then adjust all callers after a careful audit whether they would\n> handle NULLs well.\n> \n> Note: this fix not only prevents NULL pointer accesses, but it also\n> reintroduces the error messages that were lost with the change to\n> real_pathdup().\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  abspath.c            |  4 ++--\n>  builtin/init-db.c    |  6 +++---\n>  cache.h              |  2 +-\n>  dir.c                |  4 ++--\n>  environment.c        |  2 +-\n>  setup.c              |  4 ++--\n>  submodule.c          | 10 +++++-----\n>  t/t1501-work-tree.sh |  2 +-\n>  worktree.c           |  2 +-\n>  9 files changed, 18 insertions(+), 18 deletions(-)\n> \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\nAdding a gentle variant (with the current implementation) and making\nreal_pathdup() die on error would be nicer, as it doesn't require\ncallers to pass magic flag values.  Most cases use the dying variant,\nso such a patch would have to touch less places:\n---\n abspath.c            | 7 +++++++\n cache.h              | 1 +\n setup.c              | 2 +-\n t/t1501-work-tree.sh | 2 +-\n 4 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 2f0c26e0e2..f3fcff8b1b 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)\n char *real_pathdup(const char *path)\n {\n \tstruct strbuf realpath = STRBUF_INIT;\n+\tstrbuf_realpath(&realpath, path, 1);\n+\treturn strbuf_detach(&realpath, NULL);\n+}\n+\n+char *real_pathdup_gently(const char *path)\n+{\n+\tstruct strbuf realpath = STRBUF_INIT;\n \tchar *retval = NULL;\n \n \tif (strbuf_realpath(&realpath, path, 0))\ndiff --git a/cache.h b/cache.h\nindex 80b6372cf7..9dfbce702e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\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_gently(const char *path);\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/setup.c b/setup.c\nindex f14cbcd338..398ea8a913 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -806,7 +806,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_gently(ceil);\n \t\tif (!real_path) {\n \t\t\treturn 0;\n \t\t}\ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex 046d9b7909..b06210ec5e 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '\n \t)\n '\n \n-test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '\n+test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '\n \t(\n \t\tGIT_WORK_TREE=/.invalid/work/tree &&\n \t\texport GIT_WORK_TREE &&\n-- \n2.12.0\n\n"},{"id":"313531","messageId":"20170308183840.GA130604@google.com","threadId":"45316","inReplyTo":"81f1e30b-e0e1-d587-4a4b-4848beffd38c@web.de","subject":"Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-08T18:38:40Z","receivedAt":"2017-03-08T18:40:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/08, René Scharfe wrote:\n> Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:\n> > In 4ac9006f832 (real_path: have callers use real_pathdup and\n> > strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())\n> > pattern to use real_pathdup() directly.\n> > \n> > The only problem with this change is that real_path() calls\n> > strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it\n> > with die_on_error = 0. Meaning that in cases where real_path() causes\n> > Git to die() with an error message, real_pathdup() is silent and returns\n> > NULL instead.\n> > \n> > The callers, however, are ill-prepared for that change, as they expect\n> > the return value to be non-NULL.\n> > \n> > This patch fixes that by extending real_pathdup()'s signature to accept\n> > the die_on_error flag and simply pass it through to strbuf_realpath(),\n> > and then adjust all callers after a careful audit whether they would\n> > handle NULLs well.\n> > \n> > Note: this fix not only prevents NULL pointer accesses, but it also\n> > reintroduces the error messages that were lost with the change to\n> > real_pathdup().\n> > \n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  abspath.c            |  4 ++--\n> >  builtin/init-db.c    |  6 +++---\n> >  cache.h              |  2 +-\n> >  dir.c                |  4 ++--\n> >  environment.c        |  2 +-\n> >  setup.c              |  4 ++--\n> >  submodule.c          | 10 +++++-----\n> >  t/t1501-work-tree.sh |  2 +-\n> >  worktree.c           |  2 +-\n> >  9 files changed, 18 insertions(+), 18 deletions(-)\n> > \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> Adding a gentle variant (with the current implementation) and making\n> real_pathdup() die on error would be nicer, as it doesn't require\n> callers to pass magic flag values.  Most cases use the dying variant,\n> so such a patch would have to touch less places:\n\nI agree with Junio and Rene that a gentle version would make the api\nslightly nicer (and more consistant with some of the other api's we have\nin git).\n\nThis is exactly what I should have done back when I originally made the\nchange.  Sorry for missing this!\n> ---\n>  abspath.c            | 7 +++++++\n>  cache.h              | 1 +\n>  setup.c              | 2 +-\n>  t/t1501-work-tree.sh | 2 +-\n>  4 files changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/abspath.c b/abspath.c\n> index 2f0c26e0e2..f3fcff8b1b 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)\n>  char *real_pathdup(const char *path)\n>  {\n>  \tstruct strbuf realpath = STRBUF_INIT;\n> +\tstrbuf_realpath(&realpath, path, 1);\n> +\treturn strbuf_detach(&realpath, NULL);\n> +}\n> +\n> +char *real_pathdup_gently(const char *path)\n> +{\n> +\tstruct strbuf realpath = STRBUF_INIT;\n>  \tchar *retval = NULL;\n>  \n>  \tif (strbuf_realpath(&realpath, path, 0))\n> diff --git a/cache.h b/cache.h\n> index 80b6372cf7..9dfbce702e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\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_gently(const char *path);\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/setup.c b/setup.c\n> index f14cbcd338..398ea8a913 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -806,7 +806,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_gently(ceil);\n>  \t\tif (!real_path) {\n>  \t\t\treturn 0;\n>  \t\t}\n> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\n> index 046d9b7909..b06210ec5e 100755\n> --- a/t/t1501-work-tree.sh\n> +++ b/t/t1501-work-tree.sh\n> @@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '\n>  \t)\n>  '\n>  \n> -test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '\n> +test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '\n>  \t(\n>  \t\tGIT_WORK_TREE=/.invalid/work/tree &&\n>  \t\texport GIT_WORK_TREE &&\n> -- \n> 2.12.0\n> \n\n-- \nBrandon Williams\n"},{"id":"313559","messageId":"xmqq4lz331wb.fsf@gitster.mtv.corp.google.com","threadId":"45316","inReplyTo":"20170308183840.GA130604@google.com","subject":"Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-08T21:16:20Z","receivedAt":"2017-03-08T21:17:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\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>> Adding a gentle variant (with the current implementation) and making\n>> real_pathdup() die on error would be nicer, as it doesn't require\n>> callers to pass magic flag values.  Most cases use the dying variant,\n>> so such a patch would have to touch less places:\n>\n> I agree with Junio and Rene that a gentle version would make the api\n> slightly nicer (and more consistant with some of the other api's we have\n> in git).\n>\n> This is exactly what I should have done back when I originally made the\n> change.  Sorry for missing this!\n\nWhile I agree that the shape of the code Rene gave us here is what\nwe would have liked to have in the original, it is a bit too late\nfor that.\n\nAs I already mentioned, as a regression fix patch, I find what Dscho\nposted more sensible, because it makes it obvious that all existing\ncallsites were looked at while constructing the patch and more\nimportantly, it forces somebody to look at all the new callers of\nthe function that were added by the topics in flight, by changing\nthe func-signature and forcing compilation failure.\n\nIt may be somewhat unfortunate that that somebody needs to be me,\nbut that is what maintainers are for, so ... ;-)\n\nOnce the dust settles, let's do the clean-up along the lines of\nRene's patch.\n\nThanks.\n\n"},{"id":"313632","messageId":"alpine.DEB.2.20.1703091221440.3767@virtualbox","threadId":"45316","inReplyTo":"xmqq4lz331wb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-09T11:24:11Z","receivedAt":"2017-03-09T11:24:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 8 Mar 2017, Junio C Hamano wrote:\n\n> Brandon Williams <bmwill@google.com> writes:\n> \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> >> Adding a gentle variant (with the current implementation) and making\n> >> real_pathdup() die on error would be nicer, as it doesn't require\n> >> callers to pass magic flag values.  Most cases use the dying variant,\n> >> so such a patch would have to touch less places:\n> >\n> > I agree with Junio and Rene that a gentle version would make the api\n> > slightly nicer (and more consistant with some of the other api's we\n> > have in git).\n> >\n> > This is exactly what I should have done back when I originally made\n> > the change.  Sorry for missing this!\n> \n> While I agree that the shape of the code Rene gave us here is what we\n> would have liked to have in the original, it is a bit too late for that.\n> \n> As I already mentioned, as a regression fix patch, I find what Dscho\n> posted more sensible, because it makes it obvious that all existing\n> callsites were looked at while constructing the patch and more\n> importantly, it forces somebody to look at all the new callers of the\n> function that were added by the topics in flight, by changing the\n> func-signature and forcing compilation failure.\n\nWhile I would have agreed earlier that René's patch looks less intrusive,\nI have to point out that there would not have been any possible regression\nif the original patch had introduced the die_on_error parameter. It would\nhave made the contract *obvious*.\n\nThe nicer API made the contract unobvious, and that was the reason that\nthe bug could hide.\n\nCiao,\nJohannes"},{"id":"313633","messageId":"alpine.DEB.2.20.1703091224360.3767@virtualbox","threadId":"45316","inReplyTo":"20170308161742.lpbf6gn6qhoex7et@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-09T11:26:13Z","receivedAt":"2017-03-09T11:44:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 8 Mar 2017, Jeff King wrote:\n\n> On Wed, Mar 08, 2017 at 04:43:27PM +0100, Johannes Schindelin wrote:\n> \n> > We may want to consider fast-tracking this into v2.12.1, and to that\n> > end, I would appreciate code reviews that focus on the correctness of\n> > this patch and that try to consider undesired side effects.\n> \n> I don't see how it could be not-correct, in the sense that every caller\n> now passes the die_on_error flag (restoring the original behavior)\n> except for the one which clearly checks for a NULL return immediately\n> afterward.\n\nIndeed. The principal reason why I extended the function signature was so\nthat any bugs would become obvious.\n\n> The only exception would be if there were new calls to real_pathdup()\n> that did not originally use real_path(). But:\n> \n>   # 7241764076 introduced real_pathdup\n>   git log -Sreal_pathdup 7241764076..\n> \n> shows only one other introduction, and it's just duplicating an existing\n> call.\n\nThanks for digging that up. I really only looked at the existing code in\n`master` to figure out whether the return values were checked against NULL\nor not.\n\n> It's possible that some of these _could_ handle the error case more\n> gracefully (I already fixed one such case in 3a1345af2). But even if\n> we wanted to do so, that should come separately on top of this patch.\n> This can go to 'maint' as a regression fix, and then that gives a stable\n> base for making potential improvements.\n\nFully agree.\n\nThank you for the thorough review,\nJohannes\n"},{"id":"313651","messageId":"40b64644-f01c-d748-1cc3-8b3046be8a83@web.de","threadId":"45316","inReplyTo":"alpine.DEB.2.20.1703091221440.3767@virtualbox","subject":"Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-09T16:33:15Z","receivedAt":"2017-03-09T16:34:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.03.2017 um 12:24 schrieb Johannes Schindelin:\n> While I would have agreed earlier that René's patch looks less intrusive,\n> I have to point out that there would not have been any possible regression\n> if the original patch had introduced the die_on_error parameter. It would\n> have made the contract *obvious*.\n>\n> The nicer API made the contract unobvious, and that was the reason that\n> the bug could hide.\n\nYou mean nicer in the sense of more forgiving, right?\n\nThe meaning of calls of a function with a die_on_error parameter are \nonly obvious if at least the name of that parameter is known.  A \ndeclaration without it would not suffice:\n\n\tchar *real_pathdup(const char *, int);\n\nThere are two possibilities that can't be distinguished by looking at \ncallers alone:\n\n\tchar *real_pathdup(const char *path, int die_on_error);\n\tchar *real_pathdup(const char *path, int gentle);\n\nAn enum can help, so that calls look something like this:\n\n\tcopy_or_death = real_pathdup(path, DIE_ON_ERROR);\n\tcopy = real_pathdup(path, IGNORE_ERRORS); if (copy) ...\n\nFor binary flags we can easily encode that information into the function \nnames and thus simplify the API:\n\n\tcopy_or_death = real_pathdup_or_die(path);\n\tcopy = real_pathdup_gently(path); if (copy) ...\n\nAnd we can drop the suffix of the variant used overwhelmingly often, but \nas you pointed out that relies on readers knowing that convention.\n\nI'm sure Brandon will balance these concerns nicely in follow-up patches. ;)\n\nRené\n"}]}