threads / patch / 22336

patchgit-mv: Fix error with multiple sources.

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

## tl;dr

10 messages between Jan 21, 2010 and Jan 22, 2010. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

David Rydh· Jan 21, 2010, 20:39 UTC · lore

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(-)
Show changes to 4 files +6 −6

builtin-config.c, builtin-mv.c, hash-object.c, setup.c

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
Junio C Hamano· Jan 22, 2010, 05:57 UTC · re: David Rydh · lore

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

"David Rydh" <dary@math.berkeley.edu> writes:
Show 9 quoted lines
> 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.

Show 15 quoted lines
> 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.

David Rydh· Jan 22, 2010, 16:41 UTC · re: Junio C Hamano · lore

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

On Jan 21, 2010, at 9:57 PM, Junio C Hamano wrote:
Show 20 quoted lines
>> 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.
Junio C Hamano· Jan 22, 2010, 06:17 UTC · re: David Rydh · lore

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

"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.
David Rydh· Jan 22, 2010, 16:49 UTC · re: Junio C Hamano · lore

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

On Jan 21, 2010, at 10:17 PM, Junio C Hamano wrote:
Show 14 quoted lines
> "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.
Junio C Hamano· Jan 22, 2010, 06:24 UTC · re: David Rydh · lore

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

"David Rydh" <dary@math.berkeley.edu> writes:
Show 13 quoted lines
> 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;
		}
	}
Matthieu Moy· Jan 22, 2010, 07:03 UTC · re: David Rydh · lore

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

"David Rydh" <dary@math.berkeley.edu> writes:
Show 6 quoted lines
>  		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/
Johannes Sixt· Jan 22, 2010, 07:29 UTC · re: David Rydh · lore

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

David Rydh schrieb:
Show 13 quoted lines
> 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
David Rydh· Jan 22, 2010, 17:30 UTC · lore

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

On Jan 21, 2010, at 10:34 PM, Junio C Hamano wrote:
Show 19 quoted lines
> 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.
Johannes Sixt· Jan 22, 2010, 18:34 UTC · lore

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

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

← back to recent threads