threads / patch / 27576

patch, 3 partsRE: [PATCH 3/3] verify_path: consider dos drive prefix

Subject: RE: [PATCH 3/3] verify_path: consider dos drive prefix

## tl;dr

4 messages between Jun 8, 2011 and Jun 8, 2011. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Theo Niessink· Jun 8, 2011, 09:55 UTC · lore

Junio C Hamano wrote:

> Here is what I queued last night. If it looks Ok then I'll merge it down
> to 'next'.

I have run a couple of quick tests, and everything seems OK, except the following backslashed paths, which are verified OK while they should be rejected:

foo\.\bar foo\..\bar

This is caused by verify_dotfile(), which doesn't use is_dir_sep(). So I propose this patch on verify_dotfile():

Show changes to read-cache.c +4 −3

diff --git a/read-cache.c b/read-cache.c
index 282c0c1..72be7cd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -726,11 +726,12 @@ static int verify_dotfile(const char *rest)
 	 * has already been discarded, we now test
 	 * the rest.
 	 */
-	switch (*rest) {
+
 	/* "." is not allowed */
-	case '\0': case '/':
+	if (*rest == '\0' || is_dir_sep(*rest))
 		return 0;
 
+	switch (*rest) {
 	/*
 	 * ".git" followed by  NUL or slash is bad. This
 	 * shares the path end test with the ".." case.
@@ -743,7 +744,7 @@ static int verify_dotfile(const char *rest)
 		rest += 2;
 	/* fallthrough */
 	case '.':
-		if (rest[1] == '\0' || rest[1] == '/')
+		if (rest[1] == '\0' || is_dir_sep(rest[1]))
 			return 0;
 	}
 	return 1;

Erik Faye-Lund· Jun 8, 2011, 10:45 UTC · re: Theo Niessink · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Wed, Jun 8, 2011 at 11:55 AM, Theo Niessink <niessink@martinic.com> wrote:

Show 45 quoted lines

> Junio C Hamano wrote:
>> Here is what I queued last night. If it looks Ok then I'll merge it down
>> to 'next'.
>
> I have run a couple of quick tests, and everything seems OK, except the
> following backslashed paths, which are verified OK while they should be
> rejected:
>
> foo\.\bar
> foo\..\bar
>
> This is caused by verify_dotfile(), which doesn't use is_dir_sep(). So I
> propose this patch on verify_dotfile():
>
> diff --git a/read-cache.c b/read-cache.c
> index 282c0c1..72be7cd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -726,11 +726,12 @@ static int verify_dotfile(const char *rest)
>         * has already been discarded, we now test
>         * the rest.
>         */
> -       switch (*rest) {
> +
>        /* "." is not allowed */
> -       case '\0': case '/':
> +       if (*rest == '\0' || is_dir_sep(*rest))
>                return 0;
>
> +       switch (*rest) {
>        /*
>         * ".git" followed by  NUL or slash is bad. This
>         * shares the path end test with the ".." case.
> @@ -743,7 +744,7 @@ static int verify_dotfile(const char *rest)
>                rest += 2;
>        /* fallthrough */
>        case '.':
> -               if (rest[1] == '\0' || rest[1] == '/')
> +               if (rest[1] == '\0' || is_dir_sep(rest[1]))
>                        return 0;
>        }
>        return 1;
>
>
>

This looks obviously correct to me. Thanks for spotting the problem.

Would you mind writing up a commit-message and supply a sign-off?

Theo Niessink· Jun 8, 2011, 12:04 UTC · re: Erik Faye-Lund · lore

Erik Faye-Lund wrote:

> This looks obviously correct to me. Thanks for spotting the problem.
> 
> Would you mind writing up a commit-message and supply a sign-off?

Like this you mean?

-- >8 --
Subject: [PATCH] verify_dotfile(): do not assume '/' is the path seperator

verify_dotfile() currently assumes that the path seperator is '/', but on
Windows it can also be '\\', so use is_dir_sep() instead.
    
Signed-off-by: Theo Niessink <theo@taletn.com>
---
 read-cache.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

Show changes to read-cache.c +4 −3

diff --git a/read-cache.c b/read-cache.c
index 282c0c1..72be7cd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -726,11 +726,12 @@ static int verify_dotfile(const char *rest)
 	 * has already been discarded, we now test
 	 * the rest.
 	 */
-	switch (*rest) {
+
 	/* "." is not allowed */
-	case '\0': case '/':
+	if (*rest == '\0' || is_dir_sep(*rest))
 		return 0;
 
+	switch (*rest) {
 	/*
 	 * ".git" followed by  NUL or slash is bad. This
 	 * shares the path end test with the ".." case.
@@ -743,7 +744,7 @@ static int verify_dotfile(const char *rest)
 		rest += 2;
 	/* fallthrough */
 	case '.':
-		if (rest[1] == '\0' || rest[1] == '/')
+		if (rest[1] == '\0' || is_dir_sep(rest[1]))
 			return 0;
 	}
 	return 1;

-- 
1.7.5.3776.g5dcaf.dirty

Erik Faye-Lund· Jun 8, 2011, 12:15 UTC · re: Theo Niessink · lore

Re: [PATCH 3/3] verify_path: consider dos drive prefix

On Wed, Jun 8, 2011 at 2:04 PM, Theo Niessink <theo@taletn.com> wrote:

Show 6 quoted lines

> Erik Faye-Lund wrote:
>> This looks obviously correct to me. Thanks for spotting the problem.
>>
>> Would you mind writing up a commit-message and supply a sign-off?
>
> Like this you mean?

Just like that, indeed!

Junio, what do you think, is this OK? I'd be great if we could have this applied upstream, and then rebase-merge our branch on top for the next Git for Windows release.