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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2010, 05:57 UTC
Message-ID: <7v8wbqbs26.fsf@alter.siamese.dyndns.org>
In-Reply-To: <718290.769818367-sendEmail@darysmbp>

"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.