git-mv: Fix error with multiple sources.

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:

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.

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

index 2e3ef91..bf60f93 100644
@@ -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;
 	}

index 8247186..1c1f8be 100644
@@ -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);
 }

index 9455dd0..3c509aa 100644
@@ -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);

index 710e2f3..80cf535 100644
@@ -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

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.

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.

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

This looks fine. Currently we have the odd behavior

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'

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.

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.