Re: [PATCH/POC 1/7] Make git_path() beware of file relocation in $GIT_DIR

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2013, 16:30 UTC
Message-ID: <xmqqsitwr9wc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1386771333-32574-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 30 quoted lines

> +static void adjust_git_path(char *buf, int git_dir_len)
> +{
> +	/* XXX buffer overflow */
> +	char *base = buf + git_dir_len;
> +	if (git_graft_env && !strcmp(base, "info/grafts"))
> +		strcpy(buf, get_graft_file());
> +	else if (git_index_env && !strcmp(base, "index"))
> +		strcpy(buf, get_index_file());
> +	else if (git_db_env && dir_prefix(base, "objects"))
> +		replace_dir(buf, git_dir_len + 7, get_object_directory());
> +}
> +
>  static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
>  {
>  	const char *git_dir = get_git_dir();
> -	size_t len;
> +	size_t len, total_len;
>  
>  	len = strlen(git_dir);
>  	if (n < len + 1)
> @@ -60,9 +88,10 @@ static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
>  	memcpy(buf, git_dir, len);
>  	if (len && !is_dir_sep(git_dir[len-1]))
>  		buf[len++] = '/';
> -	len += vsnprintf(buf + len, n - len, fmt, args);
> -	if (len >= n)
> +	total_len = len + vsnprintf(buf + len, n - len, fmt, args);
> +	if (total_len >= n)
>  		goto bad;
> +	adjust_git_path(buf, len);

This is a minor tangent, but this part of the patch made me raise my eyebrow, wondering what Git-specific path mangler is doing in a function vsnpath that is named as if it is a lot more generic, until I read the change in context.

The vnspath() is already Git-specific---it is a helper that is used to create a path inside the $GIT_DIR directory.

We probably should do two things to clear things up:

 - Right now, path.c has definitions of functions in this order:

    char *mksnpath(char *buf, size_t n, const char *fmt, ...)
    static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
    char *git_snpath(char *buf, size_t n, const char *fmt, ...)
    char *git_pathdup(const char *fmt, ...)
    char *mkpathdup(const char *fmt, ...)
    char *mkpath(const char *fmt, ...)
    char *git_path(const char *fmt, ...)

   The two functions mkpathdup() and mkpath() are not Git specific
   at all.  They should be moved up to be grouped together with
   mksnpath() that is also not Git-specific.

 - Rename the static vsnpath() to further clarify that it is Git
   specific.