Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

Andrew Wong [off-list ref] writes:

On 3/7/13, Junio C Hamano [off-list ref] wrote:

quoted

This did not error out for me, though.

    $ cd t && git ls-files ":(top"

No error message at all? Hm, maybe in your case, the byte after the
end of string happens to be '\0' and the loop ended by chance?

git doesn't crash for me, but it generates this error:
    $ git ls-files ":(top"
    fatal: Invalid pathspec magic 'LS_COLORS=' in ':(top'

What I meant was that I do not get any error _after_ applying your
patch.

It is broken to behave as if "LS_COLORS=..." (which is totally
unrelated string that happens to be laid out next in the memory) is
a part of the pathspec magic specification your ":(top" started.
Your patch makes the code stop doing that.

But it is equally broken to behave as if there is nothing wrong in
the incomplete magic ":(top" that is not closed, isn't it?

On 03/07/13 20:51, Junio C Hamano wrote:

But it is equally broken to behave as if there is nothing wrong in
the incomplete magic ":(top" that is not closed, isn't it?

Ah, yea, I did notice that, but then I saw a few lines below:
        if (*copyfrom == ')')
            copyfrom++;
which is explicitly making the ")" optional. So I thought maybe that was
the original intention, and left it at that. Though the doc says to end
with ")", so I guess it should error out after all? If that's the case,
I can try to come up with a patch to error it out (through die() ?).

The previous code was assuming length ends at either ")" or ",", and was
not handling the case where strcspn returns length due to end of string.
So specifying ":(top" as pathspec will cause the loop to go pass the end
of string.

Signed-off-by: Andrew Wong <redacted>
---
 setup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 1dee47e..f4c4e73 100644
--- a/setup.c
+++ b/setup.c

@@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
 		     *copyfrom && *copyfrom != ')';
 		     copyfrom = nextat) {
 			size_t len = strcspn(copyfrom, ",)");
-			if (copyfrom[len] == ')')
+			if (copyfrom[len] == '\0')
 				nextat = copyfrom + len;
-			else
+			else if (copyfrom[len] == ')')
+				nextat = copyfrom + len;
+			else if (copyfrom[len] == ',')
 				nextat = copyfrom + len + 1;
 			if (!len)
 				continue;

-- 
1.7.12.4

The previous code allowed the ")" to be optional.

Signed-off-by: Andrew Wong <redacted>
---
 setup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index f4c4e73..5ed2b93 100644
--- a/setup.c
+++ b/setup.c

@@ -225,8 +225,9 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
 				die("Invalid pathspec magic '%.*s' in '%s'",
 				    (int) len, copyfrom, elt);
 		}
-		if (*copyfrom == ')')
-			copyfrom++;
+		if (*copyfrom != ')')
+			die("Missing ')' at the end of pathspec magic in '%s'", elt);
+		copyfrom++;
 	} else {
 		/* shorthand */
 		for (copyfrom = elt + 1;

-- 
1.7.12.4