[PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL

[PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL

[Johannes Schindelin]

This problem was identified as the root cause for what seemed to be a
path conversion problem in the MSYS2 runtime at first glance.

Original bug report:
http://public-inbox.org/git/CAFKRc7y_kpCGNORENUZ2qw_4qBwjjyaaDFxAEQa52fTryj+w7A@mail.gmail.com/

We may want to consider fast-tracking this into v2.12.1, and to that
end, I would appreciate code reviews that focus on the correctness of
this patch and that try to consider undesired side effects.


Johannes Schindelin (2):
  Demonstrate NULL pointer access with invalid GIT_WORK_TREE
  Fix callsites of real_pathdup() that wanted it to die on error

 abspath.c            |  4 ++--
 builtin/init-db.c    |  6 +++---
 cache.h              |  2 +-
 dir.c                |  4 ++--
 environment.c        |  2 +-
 setup.c              |  4 ++--
 submodule.c          | 10 +++++-----
 t/t1501-work-tree.sh |  8 ++++++++
 worktree.c           |  2 +-
 9 files changed, 25 insertions(+), 17 deletions(-)


base-commit: e0688e9b28f2c5ff711460ee8b62077be5df2360
Published-As: https://github.com/dscho/git/releases/tag/real_pathdup-callers-v1
Fetch-It-Via: git fetch https://github.com/dscho/git real_pathdup-callers-v1

-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH 1/2] Demonstrate NULL pointer access with invalid GIT_WORK_TREE

[Johannes Schindelin]

When GIT_WORK_TREE does not specify a valid path, we should error out.
Not crash.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1501-work-tree.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index cc5b870e587..046d9b7909f 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -423,4 +423,12 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
 	)
 '
 
+test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
+	(
+		GIT_WORK_TREE=/.invalid/work/tree &&
+		export GIT_WORK_TREE &&
+		test_expect_code 128 git rev-parse
+	)
+'
+
 test_done
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[Johannes Schindelin]

In 4ac9006f832 (real_path: have callers use real_pathdup and
strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())
pattern to use real_pathdup() directly.

The only problem with this change is that real_path() calls
strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it
with die_on_error = 0. Meaning that in cases where real_path() causes
Git to die() with an error message, real_pathdup() is silent and returns
NULL instead.

The callers, however, are ill-prepared for that change, as they expect
the return value to be non-NULL.

This patch fixes that by extending real_pathdup()'s signature to accept
the die_on_error flag and simply pass it through to strbuf_realpath(),
and then adjust all callers after a careful audit whether they would
handle NULLs well.

Note: this fix not only prevents NULL pointer accesses, but it also
reintroduces the error messages that were lost with the change to
real_pathdup().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 abspath.c            |  4 ++--
 builtin/init-db.c    |  6 +++---
 cache.h              |  2 +-
 dir.c                |  4 ++--
 environment.c        |  2 +-
 setup.c              |  4 ++--
 submodule.c          | 10 +++++-----
 t/t1501-work-tree.sh |  2 +-
 worktree.c           |  2 +-
 9 files changed, 18 insertions(+), 18 deletions(-)

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 80b6372cf76..ae0b4fc70b5 100644
--- a/cache.h
+++ b/cache.h
@@ -1153,7 +1153,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 967f289f1ef..6b48cb91ff2 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);
@@ -806,7 +806,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..0a2831d846d 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, 1);
 
 	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, 1);
 
 	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, 1);
 		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, 1);
+		char *real_common_git_dir = real_pathdup(get_git_common_dir(), 1);
 
 		if (!starts_with(real_sub_git_dir, real_common_git_dir))
 			relocate_single_git_dir_into_superproject(prefix, path);
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 046d9b7909f..b06210ec5e8 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
 	)
 '
 
-test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
+test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '
 	(
 		GIT_WORK_TREE=/.invalid/work/tree &&
 		export GIT_WORK_TREE &&
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;
-- 
2.12.0.windows.1.7.g94dafc3b124

Re: [PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL

[Jeff King]

On Wed, Mar 08, 2017 at 04:43:27PM +0100, Johannes Schindelin wrote:

> We may want to consider fast-tracking this into v2.12.1, and to that
> end, I would appreciate code reviews that focus on the correctness of
> this patch and that try to consider undesired side effects.

I don't see how it could be not-correct, in the sense that every caller
now passes the die_on_error flag (restoring the original behavior)
except for the one which clearly checks for a NULL return immediately
afterward.

The only exception would be if there were new calls to real_pathdup()
that did not originally use real_path(). But:

  # 7241764076 introduced real_pathdup
  git log -Sreal_pathdup 7241764076..

shows only one other introduction, and it's just duplicating an existing
call.

It's possible that some of these _could_ handle the error case more
gracefully (I already fixed one such case in 3a1345af2). But even if
we wanted to do so, that should come separately on top of this patch.
This can go to 'maint' as a regression fix, and then that gives a stable
base for making potential improvements.

-Peff

Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[René Scharfe]

Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:
> In 4ac9006f832 (real_path: have callers use real_pathdup and
> strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())
> pattern to use real_pathdup() directly.
> 
> The only problem with this change is that real_path() calls
> strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it
> with die_on_error = 0. Meaning that in cases where real_path() causes
> Git to die() with an error message, real_pathdup() is silent and returns
> NULL instead.
> 
> The callers, however, are ill-prepared for that change, as they expect
> the return value to be non-NULL.
> 
> This patch fixes that by extending real_pathdup()'s signature to accept
> the die_on_error flag and simply pass it through to strbuf_realpath(),
> and then adjust all callers after a careful audit whether they would
> handle NULLs well.
> 
> Note: this fix not only prevents NULL pointer accesses, but it also
> reintroduces the error messages that were lost with the change to
> real_pathdup().
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  abspath.c            |  4 ++--
>  builtin/init-db.c    |  6 +++---
>  cache.h              |  2 +-
>  dir.c                |  4 ++--
>  environment.c        |  2 +-
>  setup.c              |  4 ++--
>  submodule.c          | 10 +++++-----
>  t/t1501-work-tree.sh |  2 +-
>  worktree.c           |  2 +-
>  9 files changed, 18 insertions(+), 18 deletions(-)
> 
> 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)

Adding a gentle variant (with the current implementation) and making
real_pathdup() die on error would be nicer, as it doesn't require
callers to pass magic flag values.  Most cases use the dying variant,
so such a patch would have to touch less places:
---
 abspath.c            | 7 +++++++
 cache.h              | 1 +
 setup.c              | 2 +-
 t/t1501-work-tree.sh | 2 +-
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2..f3fcff8b1b 100644
--- a/abspath.c
+++ b/abspath.c
@@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)
 char *real_pathdup(const char *path)
 {
 	struct strbuf realpath = STRBUF_INIT;
+	strbuf_realpath(&realpath, path, 1);
+	return strbuf_detach(&realpath, NULL);
+}
+
+char *real_pathdup_gently(const char *path)
+{
+	struct strbuf realpath = STRBUF_INIT;
 	char *retval = NULL;
 
 	if (strbuf_realpath(&realpath, path, 0))
diff --git a/cache.h b/cache.h
index 80b6372cf7..9dfbce702e 100644
--- a/cache.h
+++ b/cache.h
@@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
 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_gently(const char *path);
 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/setup.c b/setup.c
index f14cbcd338..398ea8a913 100644
--- a/setup.c
+++ b/setup.c
@@ -806,7 +806,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_gently(ceil);
 		if (!real_path) {
 			return 0;
 		}
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 046d9b7909..b06210ec5e 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
 	)
 '
 
-test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
+test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '
 	(
 		GIT_WORK_TREE=/.invalid/work/tree &&
 		export GIT_WORK_TREE &&
-- 
2.12.0

Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[Brandon Williams]

On 03/08, René Scharfe wrote:
> Am 08.03.2017 um 16:43 schrieb Johannes Schindelin:
> > In 4ac9006f832 (real_path: have callers use real_pathdup and
> > strbuf_realpath, 2016-12-12), we changed the xstrdup(real_path())
> > pattern to use real_pathdup() directly.
> > 
> > The only problem with this change is that real_path() calls
> > strbuf_realpath() with die_on_error = 1 while real_pathdup() calls it
> > with die_on_error = 0. Meaning that in cases where real_path() causes
> > Git to die() with an error message, real_pathdup() is silent and returns
> > NULL instead.
> > 
> > The callers, however, are ill-prepared for that change, as they expect
> > the return value to be non-NULL.
> > 
> > This patch fixes that by extending real_pathdup()'s signature to accept
> > the die_on_error flag and simply pass it through to strbuf_realpath(),
> > and then adjust all callers after a careful audit whether they would
> > handle NULLs well.
> > 
> > Note: this fix not only prevents NULL pointer accesses, but it also
> > reintroduces the error messages that were lost with the change to
> > real_pathdup().
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  abspath.c            |  4 ++--
> >  builtin/init-db.c    |  6 +++---
> >  cache.h              |  2 +-
> >  dir.c                |  4 ++--
> >  environment.c        |  2 +-
> >  setup.c              |  4 ++--
> >  submodule.c          | 10 +++++-----
> >  t/t1501-work-tree.sh |  2 +-
> >  worktree.c           |  2 +-
> >  9 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > 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)
> 
> Adding a gentle variant (with the current implementation) and making
> real_pathdup() die on error would be nicer, as it doesn't require
> callers to pass magic flag values.  Most cases use the dying variant,
> so such a patch would have to touch less places:

I agree with Junio and Rene that a gentle version would make the api
slightly nicer (and more consistant with some of the other api's we have
in git).

This is exactly what I should have done back when I originally made the
change.  Sorry for missing this!
> ---
>  abspath.c            | 7 +++++++
>  cache.h              | 1 +
>  setup.c              | 2 +-
>  t/t1501-work-tree.sh | 2 +-
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2..f3fcff8b1b 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -217,6 +217,13 @@ const char *real_path_if_valid(const char *path)
>  char *real_pathdup(const char *path)
>  {
>  	struct strbuf realpath = STRBUF_INIT;
> +	strbuf_realpath(&realpath, path, 1);
> +	return strbuf_detach(&realpath, NULL);
> +}
> +
> +char *real_pathdup_gently(const char *path)
> +{
> +	struct strbuf realpath = STRBUF_INIT;
>  	char *retval = NULL;
>  
>  	if (strbuf_realpath(&realpath, path, 0))
> diff --git a/cache.h b/cache.h
> index 80b6372cf7..9dfbce702e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1154,6 +1154,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  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_gently(const char *path);
>  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/setup.c b/setup.c
> index f14cbcd338..398ea8a913 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -806,7 +806,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_gently(ceil);
>  		if (!real_path) {
>  			return 0;
>  		}
> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> index 046d9b7909..b06210ec5e 100755
> --- a/t/t1501-work-tree.sh
> +++ b/t/t1501-work-tree.sh
> @@ -423,7 +423,7 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
>  	)
>  '
>  
> -test_expect_failure 'error out gracefully on invalid $GIT_WORK_TREE' '
> +test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '
>  	(
>  		GIT_WORK_TREE=/.invalid/work/tree &&
>  		export GIT_WORK_TREE &&
> -- 
> 2.12.0
> 

-- 
Brandon Williams

Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> > 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)
>> 
>> Adding a gentle variant (with the current implementation) and making
>> real_pathdup() die on error would be nicer, as it doesn't require
>> callers to pass magic flag values.  Most cases use the dying variant,
>> so such a patch would have to touch less places:
>
> I agree with Junio and Rene that a gentle version would make the api
> slightly nicer (and more consistant with some of the other api's we have
> in git).
>
> This is exactly what I should have done back when I originally made the
> change.  Sorry for missing this!

While I agree that the shape of the code Rene gave us here is what
we would have liked to have in the original, it is a bit too late
for that.

As I already mentioned, as a regression fix patch, I find what Dscho
posted more sensible, because it makes it obvious that all existing
callsites were looked at while constructing the patch and more
importantly, it forces somebody to look at all the new callers of
the function that were added by the topics in flight, by changing
the func-signature and forcing compilation failure.

It may be somewhat unfortunate that that somebody needs to be me,
but that is what maintainers are for, so ... ;-)

Once the dust settles, let's do the clean-up along the lines of
Rene's patch.

Thanks.

Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

Hi,

On Wed, 8 Mar 2017, Junio C Hamano wrote:

> Brandon Williams <bmwill@google.com> writes:
> 
> >> > 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)
> >> 
> >> Adding a gentle variant (with the current implementation) and making
> >> real_pathdup() die on error would be nicer, as it doesn't require
> >> callers to pass magic flag values.  Most cases use the dying variant,
> >> so such a patch would have to touch less places:
> >
> > I agree with Junio and Rene that a gentle version would make the api
> > slightly nicer (and more consistant with some of the other api's we
> > have in git).
> >
> > This is exactly what I should have done back when I originally made
> > the change.  Sorry for missing this!
> 
> While I agree that the shape of the code Rene gave us here is what we
> would have liked to have in the original, it is a bit too late for that.
> 
> As I already mentioned, as a regression fix patch, I find what Dscho
> posted more sensible, because it makes it obvious that all existing
> callsites were looked at while constructing the patch and more
> importantly, it forces somebody to look at all the new callers of the
> function that were added by the topics in flight, by changing the
> func-signature and forcing compilation failure.

While I would have agreed earlier that René's patch looks less intrusive,
I have to point out that there would not have been any possible regression
if the original patch had introduced the die_on_error parameter. It would
have made the contract *obvious*.

The nicer API made the contract unobvious, and that was the reason that
the bug could hide.

Ciao,
Johannes

Re: [PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL

[Johannes Schindelin]

Hi Peff,

On Wed, 8 Mar 2017, Jeff King wrote:

> On Wed, Mar 08, 2017 at 04:43:27PM +0100, Johannes Schindelin wrote:
> 
> > We may want to consider fast-tracking this into v2.12.1, and to that
> > end, I would appreciate code reviews that focus on the correctness of
> > this patch and that try to consider undesired side effects.
> 
> I don't see how it could be not-correct, in the sense that every caller
> now passes the die_on_error flag (restoring the original behavior)
> except for the one which clearly checks for a NULL return immediately
> afterward.

Indeed. The principal reason why I extended the function signature was so
that any bugs would become obvious.

> The only exception would be if there were new calls to real_pathdup()
> that did not originally use real_path(). But:
> 
>   # 7241764076 introduced real_pathdup
>   git log -Sreal_pathdup 7241764076..
> 
> shows only one other introduction, and it's just duplicating an existing
> call.

Thanks for digging that up. I really only looked at the existing code in
`master` to figure out whether the return values were checked against NULL
or not.

> It's possible that some of these _could_ handle the error case more
> gracefully (I already fixed one such case in 3a1345af2). But even if
> we wanted to do so, that should come separately on top of this patch.
> This can go to 'maint' as a regression fix, and then that gives a stable
> base for making potential improvements.

Fully agree.

Thank you for the thorough review,
Johannes

Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[René Scharfe]

Am 09.03.2017 um 12:24 schrieb Johannes Schindelin:
> While I would have agreed earlier that René's patch looks less intrusive,
> I have to point out that there would not have been any possible regression
> if the original patch had introduced the die_on_error parameter. It would
> have made the contract *obvious*.
>
> The nicer API made the contract unobvious, and that was the reason that
> the bug could hide.

You mean nicer in the sense of more forgiving, right?

The meaning of calls of a function with a die_on_error parameter are 
only obvious if at least the name of that parameter is known.  A 
declaration without it would not suffice:

	char *real_pathdup(const char *, int);

There are two possibilities that can't be distinguished by looking at 
callers alone:

	char *real_pathdup(const char *path, int die_on_error);
	char *real_pathdup(const char *path, int gentle);

An enum can help, so that calls look something like this:

	copy_or_death = real_pathdup(path, DIE_ON_ERROR);
	copy = real_pathdup(path, IGNORE_ERRORS); if (copy) ...

For binary flags we can easily encode that information into the function 
names and thus simplify the API:

	copy_or_death = real_pathdup_or_die(path);
	copy = real_pathdup_gently(path); if (copy) ...

And we can drop the suffix of the variant used overwhelmingly often, but 
as you pointed out that relies on readers knowing that convention.

I'm sure Brandon will balance these concerns nicely in follow-up patches. ;)

René