[PATCH maint 0/3] do not write files outside of work-dir

[PATCH maint 0/3] do not write files outside of work-dir

[Erik Faye-Lund]

Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
where cloning an evil repository can arbitrarily overwrite files outside
the repository. Since many Windows users run as administrators, this can
be used for very nasty purposes.

The first two patches fix "git add" so it reject paths outside of the
repository when specified in the "C:\..."-form on Windows.

Patch 3/3 makes sure we don't try to actually write to these files.

This series applies cleanly to 'maint', and I strongly encourage that
we apply at the very least 3/3 there.

Erik Faye-Lund (1):
  verify_path: consider dos drive prefix

Theo Niessink (2):
  A Windows path starting with a backslash is absolute
  real_path: do not assume '/' is the path seperator

 abspath.c         |    4 ++--
 cache.h           |    2 +-
 compat/mingw.h    |    9 +++++++++
 git-compat-util.h |    4 ++++
 read-cache.c      |    5 ++++-
 5 files changed, 20 insertions(+), 4 deletions(-)

-- 
1.7.5.3.3.g435ff

[PATCH 1/3] A Windows path starting with a backslash is absolute

[Erik Faye-Lund]

From: Theo Niessink <theo@taletn.com>

This fixes prefix_path() not recognizing e.g. \foo\bar as an absolute path
on Windows.

Signed-off-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 cache.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index dd34fed..555bf7f 100644
--- a/cache.h
+++ b/cache.h
@@ -734,7 +734,7 @@ extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
-	return path[0] == '/' || has_dos_drive_prefix(path);
+	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
 }
 int is_directory(const char *);
 const char *real_path(const char *path);
-- 
1.7.5.3.3.g435ff

[PATCH 2/3] real_path: do not assume '/' is the path seperator

[Erik Faye-Lund]

From: Theo Niessink <theo@taletn.com>

real_path currently assumes it's input had '/' as path seperator.
This assumption does not hold true for the code-path from
prefix_path (on Windows), where real_path can be called before
normalize_path_copy.

Fix real_path so it doesn't make this assumption. Create a helper
function to reverse-search for the last path-seperator in a string.

Signed-off-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 abspath.c         |    4 ++--
 compat/mingw.h    |    9 +++++++++
 git-compat-util.h |    4 ++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/abspath.c b/abspath.c
index 3005aed..01858eb 100644
--- a/abspath.c
+++ b/abspath.c
@@ -40,7 +40,7 @@ const char *real_path(const char *path)
 
 	while (depth--) {
 		if (!is_directory(buf)) {
-			char *last_slash = strrchr(buf, '/');
+			char *last_slash = find_last_dir_sep(buf);
 			if (last_slash) {
 				*last_slash = '\0';
 				last_elem = xstrdup(last_slash + 1);
@@ -65,7 +65,7 @@ const char *real_path(const char *path)
 			if (len + strlen(last_elem) + 2 > PATH_MAX)
 				die ("Too long path name: '%s/%s'",
 						buf, last_elem);
-			if (len && buf[len-1] != '/')
+			if (len && !is_dir_sep(buf[len-1]))
 				buf[len++] = '/';
 			strcpy(buf + len, last_elem);
 			free(last_elem);
diff --git a/compat/mingw.h b/compat/mingw.h
index 62eccd3..b188776 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -297,6 +297,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format
 
 #define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
+static inline char *mingw_find_last_dir_sep(const char *path)
+{
+	char *ret = NULL;
+	for (; *path; ++path)
+		if (is_dir_sep(*path))
+			ret = (char *)path;
+	return ret;
+}
+#define find_last_dir_sep mingw_find_last_dir_sep
 #define PATH_SEP ';'
 #define PRIuMAX "I64u"
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..08d58f1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -215,6 +215,10 @@ extern char *gitbasename(char *);
 #define is_dir_sep(c) ((c) == '/')
 #endif
 
+#ifndef find_last_dir_sep
+#define find_last_dir_sep(path) strrchr(path, '/')
+#endif
+
 #if __HP_cc >= 61000
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR
-- 
1.7.5.3.3.g435ff

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

[Erik Faye-Lund]

If someone manage to create a repo with a 'C:' entry in the
root-tree, files can be written outside of the working-dir. This
opens up a can-of-worms of exploits.

Fix it by explicitly checking for a dos drive prefix when verifying
a paht. While we're at it, make sure that paths beginning with '\' is
considered absolute as well.

Noticed-by: Theo Niessink <theo@taletn.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 read-cache.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f38471c..68faa51 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -753,11 +753,14 @@ int verify_path(const char *path)
 {
 	char c;
 
+	if (has_dos_drive_prefix(path))
+		return 0;
+
 	goto inside;
 	for (;;) {
 		if (!c)
 			return 1;
-		if (c == '/') {
+		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
 			switch (c) {
-- 
1.7.5.3.3.g435ff

Re: [PATCH maint 0/3] do not write files outside of work-dir

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
> where cloning an evil repository can arbitrarily overwrite files outside
> the repository. Since many Windows users run as administrators, this can
> be used for very nasty purposes.

Which of my integration branches do msysGit/Git for Windows folks base
their releases these days? I could carry this through the regular "next to
master and then sometime later to maint" schedule, but if you are not
using maint and basing primarily on master then I'd rather skip the "and
then sometime later to maint" part.

Re: [PATCH maint 0/3] do not write files outside of work-dir

[Johannes Schindelin]

Hi Junio,

On Fri, 27 May 2011, Junio C Hamano wrote:

> Erik Faye-Lund <kusmabite@gmail.com> writes:
> 
> > Theo Niessink has uncovered a serious sercurity issue in Git for 
> > Windows, where cloning an evil repository can arbitrarily overwrite 
> > files outside the repository. Since many Windows users run as 
> > administrators, this can be used for very nasty purposes.
> 
> Which of my integration branches do msysGit/Git for Windows folks base 
> their releases these days? I could carry this through the regular "next 
> to master and then sometime later to maint" schedule, but if you are not 
> using maint and basing primarily on master then I'd rather skip the "and 
> then sometime later to maint" part.

We follow 'next'.

[Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow 
git@vger]

Thanks,
Johannes

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

[Johannes Sixt]

Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
> If someone manage to create a repo with a 'C:' entry in the
> root-tree, files can be written outside of the working-dir. This
> opens up a can-of-worms of exploits.
> 
> Fix it by explicitly checking for a dos drive prefix when verifying
> a paht. While we're at it, make sure that paths beginning with '\' is
> considered absolute as well.

I think we do agree that the only way to avoid the security breach is to
check a path before it is used to write a file. In practice, it means to
disallow paths in the top-most level of the index that are two
characters long and are letter-colon.

IMHO, it is pointless to avoid that an evil path enters the repository,
because there are so many and a few more ways to create an evil repository.

> diff --git a/read-cache.c b/read-cache.c
> index f38471c..68faa51 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>  {
>  	char c;
>  
> +	if (has_dos_drive_prefix(path))
> +		return 0;
> +

Isn't verify_path used to avoid that a bogus path enters the index? (I
don't know, I'm not familiar with this infrastructure.)

>  	goto inside;
>  	for (;;) {
>  		if (!c)
>  			return 1;
> -		if (c == '/') {
> +		if (is_dir_sep(c)) {
>  inside:

And if so, at this point, all backslashes should have been converted to
forward-slashes already. If not, then this would just paper over the
real bug.

>  			c = *path++;
>  			switch (c) {

-- Hannes

Re: [PATCH maint 0/3] do not write files outside of work-dir

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Which of my integration branches do msysGit/Git for Windows folks base 
>> their releases these days? I could carry this through the regular "next 
>> to master and then sometime later to maint" schedule, but if you are not 
>> using maint and basing primarily on master then I'd rather skip the "and 
>> then sometime later to maint" part.
>
> We follow 'next'.
>
> [Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow 
> git@vger]

Thanks.

Then my preference would be to queue this to "next", wait for msysGit to
cut a release based on that, and then graduate it to "master" on my side.

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

[Erik Faye-Lund]

On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
>> If someone manage to create a repo with a 'C:' entry in the
>> root-tree, files can be written outside of the working-dir. This
>> opens up a can-of-worms of exploits.
>>
>> Fix it by explicitly checking for a dos drive prefix when verifying
>> a paht. While we're at it, make sure that paths beginning with '\' is
>> considered absolute as well.
>
> I think we do agree that the only way to avoid the security breach is to
> check a path before it is used to write a file. In practice, it means to
> disallow paths in the top-most level of the index that are two
> characters long and are letter-colon.
>
> IMHO, it is pointless to avoid that an evil path enters the repository,
> because there are so many and a few more ways to create an evil repository.
>

Yes, but this patch doesn't prevent that; it prevents an evil path
from entering the index and from being checked out if the index is
evil.

>> diff --git a/read-cache.c b/read-cache.c
>> index f38471c..68faa51 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>>  {
>>       char c;
>>
>> +     if (has_dos_drive_prefix(path))
>> +             return 0;
>> +
>
> Isn't verify_path used to avoid that a bogus path enters the index? (I
> don't know, I'm not familiar with this infrastructure.)
>

Yes, it's being used to do that. But it's also being used when reading
the index into memory, which is "the good stuf" for our purposes.

This is the same guard which makes Git on Linux bard on an index
containing paths like "/tmp/foo"

>>       goto inside;
>>       for (;;) {
>>               if (!c)
>>                       return 1;
>> -             if (c == '/') {
>> +             if (is_dir_sep(c)) {
>>  inside:
>
> And if so, at this point, all backslashes should have been converted to
> forward-slashes already. If not, then this would just paper over the
> real bug.

SHOULD, yes. But we could have an evil tree/index which doesn't, and
this if intended to make sure we reject such paths.

So I don't see how this is papering over the bug; this IS the bug (as
far as I can tell).

But I think I might have been a bit too care-less; I didn't fix the
switch-case to check for multiple backslashes on Windows. It's not
immediately obvious if this is needed or not, but I don't think it can
cause harm; we should never have created an index like that anyway.

So something like this on top, perhaps?


diff --git a/read-cache.c b/read-cache.c
index 68faa51..9367349 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -763,15 +763,11 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
+			if (c == '.') {
 				if (verify_dotfile(path))
 					continue;
-			}
+			} else if (!is_dir_sep(c) && c != '\0')
+				continue;
 			return 0;
 		}
 		c = *path++;

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

[Theo Niessink]

Erik Faye-Lund wrote:
> But I think I might have been a bit too care-less; I didn't fix the
> switch-case to check for multiple backslashes on Windows. It's not
> immediately obvious if this is needed or not, but I don't think it can
> cause harm; we should never have created an index like that anyway.
> 
> So something like this on top, perhaps?

Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
continue? It will check for '\0' and is_dir_sep(c) again, but you already
know that both ifs will be false. So you could just as easy jump straight to
c = *path++, which IMHO also makes the code easier to follow:

diff --git a/read-cache.c b/read-cache.c
index 68faa51..089cd3e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -763,17 +763,15 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
+			if (c == '.') {
+				
 				if (verify_dotfile(path))
 					continue;
-			}
+			} else if (!is_dir_sep(c) && c != '\0')
+				goto next;
 			return 0;
 		}
+next:
 		c = *path++;
 	}
 }

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

[Erik Faye-Lund]

On Mon, May 30, 2011 at 12:58 PM, Theo Niessink <theo@taletn.com> wrote:
> Erik Faye-Lund wrote:
>> But I think I might have been a bit too care-less; I didn't fix the
>> switch-case to check for multiple backslashes on Windows. It's not
>> immediately obvious if this is needed or not, but I don't think it can
>> cause harm; we should never have created an index like that anyway.
>>
>> So something like this on top, perhaps?
>
> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
> continue? It will check for '\0' and is_dir_sep(c) again, but you already
> know that both ifs will be false. So you could just as easy jump straight to
> c = *path++, which IMHO also makes the code easier to follow:

Very good point, thanks for noticing. I just rewrote the logic from
switch/case to if/else, but with the rewrite these redundant compares
became more obvious. I think your version is better, indeed.

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

[Johannes Sixt]

Am 30.05.2011 11:32, schrieb Erik Faye-Lund:
> On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:
>>> If someone manage to create a repo with a 'C:' entry in the
>>> root-tree, files can be written outside of the working-dir. This
>>> opens up a can-of-worms of exploits.
>>>
>>> Fix it by explicitly checking for a dos drive prefix when verifying
>>> a paht. While we're at it, make sure that paths beginning with '\' is
>>> considered absolute as well.
>>
>> I think we do agree that the only way to avoid the security breach is to
>> check a path before it is used to write a file. In practice, it means to
>> disallow paths in the top-most level of the index that are two
>> characters long and are letter-colon.
>>
>> IMHO, it is pointless to avoid that an evil path enters the repository,
>> because there are so many and a few more ways to create an evil repository.
>>
> 
> Yes, but this patch doesn't prevent that; it prevents an evil path
> from entering the index and from being checked out if the index is
> evil.
> 
>>> diff --git a/read-cache.c b/read-cache.c
>>> index f38471c..68faa51 100644
>>> --- a/read-cache.c
>>> +++ b/read-cache.c
>>> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>>>  {
>>>       char c;
>>>
>>> +     if (has_dos_drive_prefix(path))
>>> +             return 0;
>>> +
>>
>> Isn't verify_path used to avoid that a bogus path enters the index? (I
>> don't know, I'm not familiar with this infrastructure.)
>>
> 
> Yes, it's being used to do that. But it's also being used when reading
> the index into memory, which is "the good stuf" for our purposes.

OK, I agree with the changes proposed in this patch. git reset and git
checkout go through this function via unpack_trees(). Are there other
ways to write a file, e.g., in merge-recursive?

-- Hannes

Re: [PATCH maint 0/3] do not write files outside of work-dir

[Tait]

> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
> where cloning an evil repository can arbitrarily overwrite files outside
> the repository...

Filenames starting with C: are not necessarily absolute. Consider
"c:foo.txt" where c: is the current directory on drive C, or
"c:stream1" where c is a single-letter filename in the current directory
with an alternate data stream such as would be shown by dir /r. The
has_dos_drive_prefix check is overly broad. Maybe this is intentional and
just needs to be documented. Absolute paths like \\localhost\C$\file.txt
and \\?\C:\file.txt do seem to be caught, because they start with '\'.

Microsoft says[1] a path is relative unless:
  - it begins with "\\"
  - it begins with a disk designator followed by a directory separator
  - it begins with a single "\"

On that basis, has_dos_drive_prefix(path) should be:
  isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])

However, there are also paths within the NT namespace (as opposed to the
Win32 namespace, [1] again) that might be considered absolute, or at least
to which git should not try to write. Examples would be PRN, CONOUT$, AUX,
etc. These will not be caught by the current form of has_dos_drive_prefix,
if that is even the right place to catch them. I think the QueryDosDevice
function (given the part of the path up to the first directory separator,
if one is present [2]) would detect them, and logical drive mappings as
well. However, QueryDosDevice seems to also include many things that are
not worthy of concern, like (on my computer) "DISPLAY5". Does anyone know
the correct approach here?

I gather that other programs can create names like these (with
DefineDosDevice), so a hard-coded exception list from [1] (that being: CON,
PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1,
LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, and LPT9) might not be adequate?

[1] http://msdn.microsoft.com/en-us/library/aa365247(v=vs.85).aspx
[2] http://msdn.microsoft.com/en-us/library/aa365461(v=vs.85).aspx

Re: [PATCH maint 0/3] do not write files outside of work-dir

[Johannes Sixt]

Am 6/1/2011 6:14, schrieb Tait:
>> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,
>> where cloning an evil repository can arbitrarily overwrite files outside
>> the repository...
> 
> Filenames starting with C: are not necessarily absolute. Consider
> "c:foo.txt" where c: is the current directory on drive C, or

We have a different notion of "absolute path". This one *is* absolute per
our definition. See below.

> "c:stream1" where c is a single-letter filename in the current directory
> with an alternate data stream such as would be shown by dir /r. The

On my system, this does not create a file in the current directory with an
alternate data stream, but - while the working directory is somewhere on
drive D - a file is created on drive C.

> has_dos_drive_prefix check is overly broad. Maybe this is intentional and
> just needs to be documented. Absolute paths like \\localhost\C$\file.txt
> and \\?\C:\file.txt do seem to be caught, because they start with '\'.
> 
> Microsoft says[1] a path is relative unless:
>   - it begins with "\\"
>   - it begins with a disk designator followed by a directory separator
>   - it begins with a single "\"
> 
> On that basis, has_dos_drive_prefix(path) should be:
>   isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])

This is not the definition of "relative path" that we are interested in.
Let $PWD be the current directory. For our purposes, a path $P is relative
if $P and $PWD/$P designate the same file system entry. Otherwise, $P is
an absolute path.

With this definition, the current has_dos_drive_prefix() is good enough.

> However, there are also paths within the NT namespace (as opposed to the
> Win32 namespace, [1] again) that might be considered absolute, or at least
> to which git should not try to write. Examples would be PRN, CONOUT$, AUX,

For our purposes, these names are all relative paths. It's a case of
"Doctor, it hurts when I stick my finger in my eye" if you have a
repository with these names.

Note that git never writes to these files: It always first allocates a
temporary file, eg. nul.123456; but this will already fail because these
special file names are forbidden even when a file extension is attached.

-- Hannes

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

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>> know that both ifs will be false. So you could just as easy jump straight to
>> c = *path++, which IMHO also makes the code easier to follow:
>
> Very good point, thanks for noticing. I just rewrote the logic from
> switch/case to if/else, but with the rewrite these redundant compares
> became more obvious. I think your version is better, indeed.

Let's not add an unnecessary goto while at it.  How about this on top
instead?

 read-cache.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 31cf0b5..3593291 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -784,16 +784,9 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
-				if (verify_dotfile(path))
-					continue;
-			}
-			return 0;
+			if ((c == '.' && !verify_dotfile(path)) ||
+			    is_dir_sep(c) || c == '\0')
+				return 0;
 		}
 		c = *path++;
 	}

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

[Erik Faye-Lund]

On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>>> know that both ifs will be false. So you could just as easy jump straight to
>>> c = *path++, which IMHO also makes the code easier to follow:
>>
>> Very good point, thanks for noticing. I just rewrote the logic from
>> switch/case to if/else, but with the rewrite these redundant compares
>> became more obvious. I think your version is better, indeed.
>
> Let's not add an unnecessary goto while at it.  How about this on top
> instead?
>
>  read-cache.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 31cf0b5..3593291 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>                if (is_dir_sep(c)) {
>  inside:
>                        c = *path++;
> -                       switch (c) {
> -                       default:
> -                               continue;
> -                       case '/': case '\0':
> -                               break;
> -                       case '.':
> -                               if (verify_dotfile(path))
> -                                       continue;
> -                       }
> -                       return 0;
> +                       if ((c == '.' && !verify_dotfile(path)) ||
> +                           is_dir_sep(c) || c == '\0')
> +                               return 0;
>                }
>                c = *path++;
>        }

This change the "c == '.' && verify_dotfile(path)"-case to eat the '.'
character without testing it against is_dir_sep, which is exactly what
we want. The other cases return 0, as they used to. Good.

Indeed, this is a cleaner approach. Thanks!

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

[Theo Niessink]

Junio C Hamano wrote: 
> Let's not add an unnecessary goto while at it.  How about this on top
> instead?

Yeah, that is much cleaner indeed.

- Theo

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

[Erik Faye-Lund]

On Tue, Jun 7, 2011 at 12:07 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>>> Nitpick: If you already know that c != '\0' and !is_dir_sep(c), then why do
>>>> continue? It will check for '\0' and is_dir_sep(c) again, but you already
>>>> know that both ifs will be false. So you could just as easy jump straight to
>>>> c = *path++, which IMHO also makes the code easier to follow:
>>>
>>> Very good point, thanks for noticing. I just rewrote the logic from
>>> switch/case to if/else, but with the rewrite these redundant compares
>>> became more obvious. I think your version is better, indeed.
>>
>> Let's not add an unnecessary goto while at it.  How about this on top
>> instead?
>>
>>  read-cache.c |   13 +++----------
>>  1 files changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index 31cf0b5..3593291 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>>                if (is_dir_sep(c)) {
>>  inside:
>>                        c = *path++;
>> -                       switch (c) {
>> -                       default:
>> -                               continue;
>> -                       case '/': case '\0':
>> -                               break;
>> -                       case '.':
>> -                               if (verify_dotfile(path))
>> -                                       continue;
>> -                       }
>> -                       return 0;
>> +                       if ((c == '.' && !verify_dotfile(path)) ||
>> +                           is_dir_sep(c) || c == '\0')
>> +                               return 0;
>>                }
>>                c = *path++;
>>        }
>
> This change the "c == '.' && verify_dotfile(path)"-case to eat the '.'
> character without testing it against is_dir_sep, which is exactly what
> we want. The other cases return 0, as they used to. Good.
>
> Indeed, this is a cleaner approach. Thanks!
>

I forgot to ask; do you want me to resend? I would imagine the commit
message should be updated to reflect this change as well...

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

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> I forgot to ask; do you want me to resend? I would imagine the commit
> message should be updated to reflect this change as well...

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

-- >8 --
Subject: [PATCH] verify_path(): simplify check at the directory boundary

We simply want to say "At a directory boundary, be careful with a name
that begins with a dot, forbid a name that ends with the boundary
character or has duplicated bounadry characters".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 31cf0b5..3593291 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -784,16 +784,9 @@ int verify_path(const char *path)
 		if (is_dir_sep(c)) {
 inside:
 			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
-				if (verify_dotfile(path))
-					continue;
-			}
-			return 0;
+			if ((c == '.' && !verify_dotfile(path)) ||
+			    is_dir_sep(c) || c == '\0')
+				return 0;
 		}
 		c = *path++;
 	}
-- 
1.7.6.rc0.129.gbe6ef

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

[Erik Faye-Lund]

On Tue, Jun 7, 2011 at 9:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I forgot to ask; do you want me to resend? I would imagine the commit
>> message should be updated to reflect this change as well...
>
> Here is what I queued last night. If it looks Ok then I'll merge it down
> to 'next'.
>
> -- >8 --
> Subject: [PATCH] verify_path(): simplify check at the directory boundary
>
> We simply want to say "At a directory boundary, be careful with a name
> that begins with a dot, forbid a name that ends with the boundary
> character or has duplicated bounadry characters".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  read-cache.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 31cf0b5..3593291 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -784,16 +784,9 @@ int verify_path(const char *path)
>                if (is_dir_sep(c)) {
>  inside:
>                        c = *path++;
> -                       switch (c) {
> -                       default:
> -                               continue;
> -                       case '/': case '\0':
> -                               break;
> -                       case '.':
> -                               if (verify_dotfile(path))
> -                                       continue;
> -                       }
> -                       return 0;
> +                       if ((c == '.' && !verify_dotfile(path)) ||
> +                           is_dir_sep(c) || c == '\0')
> +                               return 0;
>                }
>                c = *path++;
>        }

Looks good to me, thanks for following up on it :)

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

[Erik Faye-Lund]

On Wed, Jun 8, 2011 at 2:04 PM, Theo Niessink <theo@taletn.com> wrote:
> 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.

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

[Theo Niessink]

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(-)

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

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

[Erik Faye-Lund]

On Wed, Jun 8, 2011 at 11:55 AM, Theo Niessink <niessink@martinic.com> wrote:
> 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?

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

[Theo Niessink]

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;