[PATCH] git-mv: Fix error with multiple sources.

[PATCH] git-mv: Fix error with multiple sources.

[David Rydh]

Commit b8f26269 (fix directory separator treatment on Windows,
30-06-2009) introduced a bug on Mac OS X. The problem is that
basename() may return a pointer to internal static storage space that
will be overwritten by subsequent calls:

> git mv dir/a.txt dir/b.txt other/

  Checking rename of 'dir/a.txt' to 'other/b.txt'
  Checking rename of 'dir/b.txt' to 'other/b.txt'
  fatal: multiple sources for the same target,
  source=dir/b.txt, destination=other/b.txt

This commit also fixed two potentially dangerous uses of
prefix_filename() -- which returns static storage space -- in
builtin-config.c and hash-object.c.

get_pathspec(): If called with an empty pathspec, allocate a new
pathspec with a copy of prefix. This is consistent with the behavior
when called with a non-empty pathspec and prevents potential errors.

Signed-off-by: David Rydh <dary@math.berkeley.edu>
---
This is my first patch submission to git. Perhaps this patch should
be split into two? The one-line change to builtin-mv.c suffices to
fix the bug.

 builtin-config.c |    4 ++--
 builtin-mv.c     |    2 +-
 hash-object.c    |    2 +-
 setup.c          |    4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 2e3ef91..bf60f93 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -356,9 +356,9 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		config_exclusive_filename = git_etc_gitconfig();
 	else if (given_config_file) {
 		if (!is_absolute_path(given_config_file) && prefix)
-			config_exclusive_filename = prefix_filename(prefix,
+			config_exclusive_filename = xstrdup(prefix_filename(prefix,
 								    strlen(prefix),
-								    argv[2]);
+									argv[2]));
 		else
 			config_exclusive_filename = given_config_file;
 	}
diff --git a/builtin-mv.c b/builtin-mv.c
index 8247186..1c1f8be 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,
 		if (length > 0 && is_dir_sep(result[i][length - 1]))
 			result[i] = xmemdupz(result[i], length - 1);
 		if (base_name)
-			result[i] = basename((char *)result[i]);
+			result[i] = xstrdup(basename((char *)result[i]));
 	}
 	return get_pathspec(prefix, result);
 }
diff --git a/hash-object.c b/hash-object.c
index 9455dd0..3c509aa 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -91,7 +91,7 @@ int main(int argc, const char **argv)
 		prefix = setup_git_directory();
 		prefix_length = prefix ? strlen(prefix) : 0;
 		if (vpath && prefix)
-			vpath = prefix_filename(prefix, prefix_length, vpath);
+			vpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));
 	}
 
 	git_config(git_default_config, NULL);
diff --git a/setup.c b/setup.c
index 710e2f3..80cf535 100644
--- a/setup.c
+++ b/setup.c
@@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 		return NULL;
 
 	if (!entry) {
-		static const char *spec[2];
-		spec[0] = prefix;
+		const char **spec = xmalloc(sizeof(char *) * 2);
+		spec[0] = xstrdup(prefix);
 		spec[1] = NULL;
 		return spec;
 	}
-- 
1.6.6.1

Re: [PATCH] git-mv: Fix error with multiple sources.

[Junio C Hamano]

"David Rydh" <dary@math.berkeley.edu> writes:

> Commit b8f26269 (fix directory separator treatment on Windows,
> 30-06-2009) introduced a bug on Mac OS X. The problem is that
> basename() may return a pointer to internal static storage space that
> will be overwritten by subsequent calls:
>
>> git mv dir/a.txt dir/b.txt other/
>
>   Checking rename of 'dir/a.txt' to 'other/b.txt'
>   Checking rename of 'dir/b.txt' to 'other/b.txt'

Good spotting.  basename(3)/dirname(3) are tricky functions to use
correctly.  In addition to the "static storage" implementation and its
associated problem you encountered, they can also be implemented to modify
the given string in place, and can even return a pointer _into_ the given
string, which means that if you do:

	duped = strdup(string);
        duped = basename(duped);
        ... use duped ...
	free(duped);

you may be burned quite badly.  The other call site of basename(3) is in
diff.c and it looks safe.

> diff --git a/setup.c b/setup.c
> index 710e2f3..80cf535 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>  		return NULL;
>  
>  	if (!entry) {
> -		static const char *spec[2];
> -		spec[0] = prefix;
> +		const char **spec = xmalloc(sizeof(char *) * 2);
> +		spec[0] = xstrdup(prefix);
>  		spec[1] = NULL;
>  		return spec;
>  	}

I don't understand this change.  Because elements of returned pathspec
from this function are often simply the pathspec argument itself (which in
turn is often argv[] of the calling program), and other times allocated by
this function, the callers are never going to free() them.

Re: [PATCH] git-mv: Fix error with multiple sources.

[Junio C Hamano]

"David Rydh" <dary@math.berkeley.edu> writes:

> This commit also fixed two potentially dangerous uses of
> prefix_filename() -- which returns static storage space -- in
> builtin-config.c and hash-object.c.

This should probably be a separate patch.  builtin-hash-object.c also uses
prefix_filename() first to obtain vpath without strdup() and then uses the
function to create arg, which seems to be unsafe to me.  I've looked at
all the hits from 

    $ git grep -n -e prefix_filename\( -- '*.c'

and other places seem to be Ok.

Re: [PATCH] git-mv: Fix error with multiple sources.

[Junio C Hamano]

"David Rydh" <dary@math.berkeley.edu> writes:

> diff --git a/builtin-mv.c b/builtin-mv.c
> index 8247186..1c1f8be 100644
> --- a/builtin-mv.c
> +++ b/builtin-mv.c
> @@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,
>  		if (length > 0 && is_dir_sep(result[i][length - 1]))
>  			result[i] = xmemdupz(result[i], length - 1);
>  		if (base_name)
> -			result[i] = basename((char *)result[i]);
> +			result[i] = xstrdup(basename((char *)result[i]));
>  	}
>  	return get_pathspec(prefix, result);
>  }

Given that basename(3) is allowed to modify its parameter, I think the
above code is still not portable.  casting constness away and feeding
result[i], especially when we didn't obtain our own copy by calling
xmemdupz(), is especially problematic.

Perhaps something ugly like this?

	for (i = 0; i < count; i++) {
		int length = strlen(result[i]);
		int to_copy = length;
                while (to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
			to_copy--;
		if (to_copy != length || basename) {
                	char *it = xmemdupz(result[i], to_copy);
                        result[i] = base_name ? strdup(basename(it)) : it;
		}
	}

Re: [PATCH] git-mv: Fix error with multiple sources.

[Matthieu Moy]

"David Rydh" <dary@math.berkeley.edu> writes:

>  		if (!is_absolute_path(given_config_file) && prefix)
> -			config_exclusive_filename = prefix_filename(prefix,
> +			config_exclusive_filename = xstrdup(prefix_filename(prefix,
>  								    strlen(prefix),
> -								    argv[2]);
> +									argv[2]));

Broken indentation on the last two lines.

Other than that, you use strdup without free-ing the result. Probably
not so serious, but it may be cleaner to actually call free, or to
explain why you don't (in commit message).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] git-mv: Fix error with multiple sources.

[Johannes Sixt]

David Rydh schrieb:
> diff --git a/builtin-mv.c b/builtin-mv.c
> index 8247186..1c1f8be 100644
> --- a/builtin-mv.c
> +++ b/builtin-mv.c
> @@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,
>  		if (length > 0 && is_dir_sep(result[i][length - 1]))
>  			result[i] = xmemdupz(result[i], length - 1);
>  		if (base_name)
> -			result[i] = basename((char *)result[i]);
> +			result[i] = xstrdup(basename((char *)result[i]));
>  	}
>  	return get_pathspec(prefix, result);
>  }

We are already leaking memory of magnitude O(argc*length of file names),
and IMO, this new leak of the same magnitude doesn't hurt.

If you want to avoid it, you can set NO_LIBGEN_H in Makefile.

The other changes in this patch should really be a separate patch. They do
not fix an immediate problem, right?

-- Hannes

Re: [PATCH] git-mv: Fix error with multiple sources.

[David Rydh]

On Jan 21, 2010, at 9:57 PM, Junio C Hamano wrote:

>> diff --git a/setup.c b/setup.c
>> index 710e2f3..80cf535 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>> 		return NULL;
>> 
>> 	if (!entry) {
>> -		static const char *spec[2];
>> -		spec[0] = prefix;
>> +		const char **spec = xmalloc(sizeof(char *) * 2);
>> +		spec[0] = xstrdup(prefix);
>> 		spec[1] = NULL;
>> 		return spec;
>> 	}
> 
> I don't understand this change.  Because elements of returned pathspec
> from this function are often simply the pathspec argument itself (which in
> turn is often argv[] of the calling program), and other times allocated by
> this function, the callers are never going to free() them.

The xstrdup of prefix is for good measure. The important change is the removal of the static spec-array. Two invocations of get_pathspec with different prefixes could invalidate the contents of a pathspec in use.

When called with a non-empty pathspec, all the entries are allocated so I think it is reasonable to allocate them in the degenerate case as well. My impression of the git-code is that it is leaking quite a bit when it comes to strings anyway.

Re: [PATCH] git-mv: Fix error with multiple sources.

[David Rydh]

On Jan 21, 2010, at 10:17 PM, Junio C Hamano wrote:

> "David Rydh" <dary@math.berkeley.edu> writes:
> 
>> This commit also fixed two potentially dangerous uses of
>> prefix_filename() -- which returns static storage space -- in
>> builtin-config.c and hash-object.c.
> 
> This should probably be a separate patch.  builtin-hash-object.c also uses
> prefix_filename() first to obtain vpath without strdup() and then uses the
> function to create arg, which seems to be unsafe to me.  I've looked at
> all the hits from 
> 
>    $ git grep -n -e prefix_filename\( -- '*.c'
> 
> and other places seem to be Ok.


Yes, this was how I found these two places. Do you agree that strdup'ing vpath as I did in the patch fixes this flaw? (I assume that builtin-hash-object.c = hash-object.c) I'll split it up into two patches.

Re: [PATCH] git-mv: Fix error with multiple sources.

[David Rydh]

On Jan 21, 2010, at 10:34 PM, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Given that basename(3) is allowed to modify its parameter, I think the
>> above code is still not portable.  casting constness away and feeding
>> result[i], especially when we didn't obtain our own copy by calling
>> xmemdupz(), is especially problematic.
>> 
>> Perhaps something ugly like this?
>> 
>> 	for (i = 0; i < count; i++) {
>> 		int length = strlen(result[i]);
>> 		int to_copy = length;
>>                while (to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
>> 			to_copy--;
>> 		if (to_copy != length || basename) {
>>                	char *it = xmemdupz(result[i], to_copy);
>>                        result[i] = base_name ? strdup(basename(it)) : it;
>> 		}
>> 	}

This looks fine. Currently we have the odd behavior

> git mv -n dir/ new-dir
Checking rename of 'dir' to 'new-dir'
Checking rename of 'dir/a.txt' to 'new-dir/a.txt'
Checking rename of 'dir/b.txt' to 'new-dir/b.txt'

> git mv -n dir// new-dir
Checking rename of 'dir/' to 'new-dir'
fatal: source directory is empty, source=dir/, destination=new-dir

Note that at the end of copy_pathspec we call get_pathspec which squashes multiple slashes (even if prefix=NULL) but does not remove a trailing slash so it is necessary to squash them all as above.

Re: [PATCH] git-mv: Fix error with multiple sources.

[Johannes Sixt]

On Freitag, 22. Januar 2010, Junio C Hamano wrote:
> Why does '/' matter in builtin-mv.c that needed b8f2626, but we can
> compare with '/' [in make_relative_path()]?

We should be using is_dir_sep() in make_relative_path() as well (see the fixup 
that I posted in response to your patch). The thing is that the incorrect 
result does not matter from a correctness-POV for the only caller of this 
routine.

-- Hannes