Re: [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root

Adam Spiers [off-list ref] writes:

Fix a corner case where check-ignore would segfault when run with the
'.' argument from the top level of a repository, due to prefix_path()
converting '.' into the empty string.  It doesn't make much sense to
call check-ignore from the top level with '.' as a parameter, since
the top-level directory would never typically be ignored, but of
course it should not segfault in this case.

Signed-off-by: Adam Spiers <redacted>
---

Please step back a bit and explain why the original had check for
path[0] in the first place?

If the answer is "the code wanted to special case the question 'is
the top-level excluded?', but used a wrong variable to implement the
check, and this patch is a fix to that", then the proposed commit
log message looks incomplete.  The cause of the segv is not that
prefix_path() returns an empty string, but because the function
called inside the "if" block was written without expecting to be fed
the path that refers to the top-level of the working tree, no?

While this change certainly will prevent the "check the top-level"
request to last-exclude-matching-path, I have to wonder if it is a
good idea to force the caller of the l-e-m-p function to even care.

In other words, would it be a cleaner approach to fix the l-e-m-p
function so that the caller can ask "check the top-level" and give a
sensible answer (perhaps the answer may be "nothing matches"), and
remove the "&& path[0]" (or "&& full_path[0]") special case from
this call site?

The last sentence "It doesn't make much sense..." in the proposed
log message would become a good justification for such a special
case at the beginning of l-e-m-p function, I would think.

On Tue, Feb 19, 2013 at 5:54 PM, Junio C Hamano [off-list ref] wrote:

Adam Spiers [off-list ref] writes:

quoted

Fix a corner case where check-ignore would segfault when run with the
'.' argument from the top level of a repository, due to prefix_path()
converting '.' into the empty string.  It doesn't make much sense to
call check-ignore from the top level with '.' as a parameter, since
the top-level directory would never typically be ignored, but of
course it should not segfault in this case.

Signed-off-by: Adam Spiers <redacted>
---

Please step back a bit and explain why the original had check for
path[0] in the first place?

I can't remember to be honest.

If the answer is "the code wanted to special case the question 'is
the top-level excluded?',

Yes, I think that's the most likely explanation.  Maybe it got missed
in a variable renaming refactoring.

but used a wrong variable to implement the
check, and this patch is a fix to that", then the proposed commit
log message looks incomplete.  The cause of the segv is not that
prefix_path() returns an empty string, but because the function
called inside the "if" block was written without expecting to be fed
the path that refers to the top-level of the working tree, no?

While this change certainly will prevent the "check the top-level"
request to last-exclude-matching-path, I have to wonder if it is a
good idea to force the caller of the l-e-m-p function to even care.

In other words, would it be a cleaner approach to fix the l-e-m-p
function so that the caller can ask "check the top-level" and give a
sensible answer (perhaps the answer may be "nothing matches"), and
remove the "&& path[0]" (or "&& full_path[0]") special case from
this call site?

Yes, that did cross my mind.  I also wondered whether hash_name()
should do stricter input validation, but I guess that could have an
impact on performance.

The last sentence "It doesn't make much sense..." in the proposed
log message would become a good justification for such a special
case at the beginning of l-e-m-p function, I would think.

Fair enough.  I'll reply to this with a new version.[0]

[0] I wish there was a clean way to include the new version inline,
    but as I've noted before, there doesn't seem to be:

    http://article.gmane.org/gmane.comp.version-control.git/146110

Fix a corner case where check-ignore would segfault when run with the
'.' argument from the top level of a repository, due to prefix_path()
converting '.' into the empty string.  It doesn't make much sense to
call check-ignore from the top level with '.' as a parameter, since
the top-level directory would never typically be ignored, but of
course it should not segfault in this case.  The existing code
attempted to check for this case but failed due to using the wrong
variable.  Instead we move the check to last_exclude_matching_path(),
in case other callers present or future have a similar issue.

Signed-off-by: Adam Spiers <redacted>
---
 builtin/check-ignore.c | 2 +-
 dir.c                  | 8 ++++++++
 t/t0008-ignores.sh     | 5 +++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 709535c..0240f99 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c

@@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)
 					? strlen(prefix) : 0, path);
 		full_path = check_path_for_gitlink(full_path);
 		die_if_path_beyond_symlink(full_path, prefix);
-		if (!seen[i] && path[0]) {
+		if (!seen[i]) {
 			exclude = last_exclude_matching_path(&check, full_path,
 							     -1, &dtype);
 			if (exclude) {

diff --git a/dir.c b/dir.c
index 57394e4..1ae0b90 100644
--- a/dir.c
+++ b/dir.c

@@ -828,6 +828,14 @@ struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
 	struct exclude *exclude;
 
 	/*
+	 * name could be the empty string, e.g. if check-ignore was
+	 * invoked from the top level with '.', prefix_path() will
+	 * convert it into "".
+	 */
+	if (!*name)
+		return NULL;
+
+	/*
 	 * we allow the caller to pass namelen as an optimization; it
 	 * must match the length of the name, as we eventually call
 	 * is_excluded() on the whole name string.

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index ebe7c70..9c1bde1 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh

@@ -138,6 +138,7 @@ test_expect_success 'setup' '
 	cat <<-\EOF >.gitignore &&
 		one
 		ignored-*
+		top-level-dir/
 	EOF
 	for dir in . a
 	do

@@ -177,6 +178,10 @@ test_expect_success 'setup' '
 #
 # test invalid inputs
 
+test_expect_success_multi '. corner-case' '' '
+	test_check_ignore . 1
+'
+
 test_expect_success_multi 'empty command line' '' '
 	test_check_ignore "" 128 &&
 	stderr_contains "fatal: no path specified"

-- 
1.8.2.rc0.18.g543d1e4