[PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()

[PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()

[Ramsay Jones]

The current implementation of git_path() is essentially the same as
that of vsnpath(), with two minor differences. First, git_path()
currently insists that the git directory path is no longer than
PATH_MAX-100 characters in length. However, vsnpath() does not
attempt this arbitrary 100 character reservation for the remaining
path components. Second, vsnpath() uses the "is_dir_sep()" macro,
rather than comparing directly to '/', to determine if the git_dir
path component ends with a path separator.

In order to benefit from the above improvements, along with increased
compatability with git_snpath() and git_pathdup(), we reimplement the
git_path() function using vsnpath().

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 path.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/path.c b/path.c
index 741ae77..cbbdf7d 100644
--- a/path.c
+++ b/path.c
@@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...)
 
 char *git_path(const char *fmt, ...)
 {
-	const char *git_dir = get_git_dir();
 	char *pathname = get_pathname();
 	va_list args;
-	unsigned len;
+	char *ret;
 
-	len = strlen(git_dir);
-	if (len > PATH_MAX-100)
-		return bad_path;
-	memcpy(pathname, git_dir, len);
-	if (len && git_dir[len-1] != '/')
-		pathname[len++] = '/';
 	va_start(args, fmt);
-	len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args);
+	ret = vsnpath(pathname, PATH_MAX, fmt, args);
 	va_end(args);
-	if (len >= PATH_MAX)
-		return bad_path;
-	return cleanup_path(pathname);
+	return ret;
 }
 
 void home_config_paths(char **global, char **xdg, char *file)
-- 
1.7.12

Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> The current implementation of git_path() is essentially the same as
> that of vsnpath(), with two minor differences. First, git_path()
> currently insists that the git directory path is no longer than
> PATH_MAX-100 characters in length. However, vsnpath() does not
> attempt this arbitrary 100 character reservation for the remaining
> path components. Second, vsnpath() uses the "is_dir_sep()" macro,
> rather than comparing directly to '/', to determine if the git_dir
> path component ends with a path separator.
> In order to benefit from the above improvements,...

In the longer term, I think this goes in the right direction, but
the loss of reservation, especially when we know git_path() is used
by some callers to get the base directory in $GIT_DIR that want to
append stuff after the returned directory path to form the final
pathname, is a bit worrysome.  It may be hiding a bug (lack of
proper limit check) on the callers' side.

> ... along with increased
> compatability with git_snpath() and git_pathdup(), we reimplement the
> git_path() function using vsnpath().
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  path.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/path.c b/path.c
> index 741ae77..cbbdf7d 100644
> --- a/path.c
> +++ b/path.c
> @@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...)
>  
>  char *git_path(const char *fmt, ...)
>  {
> -	const char *git_dir = get_git_dir();
>  	char *pathname = get_pathname();
>  	va_list args;
> -	unsigned len;
> +	char *ret;
>  
> -	len = strlen(git_dir);
> -	if (len > PATH_MAX-100)
> -		return bad_path;
> -	memcpy(pathname, git_dir, len);
> -	if (len && git_dir[len-1] != '/')
> -		pathname[len++] = '/';
>  	va_start(args, fmt);
> -	len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args);
> +	ret = vsnpath(pathname, PATH_MAX, fmt, args);
>  	va_end(args);
> -	if (len >= PATH_MAX)
> -		return bad_path;
> -	return cleanup_path(pathname);
> +	return ret;
>  }
>  
>  void home_config_paths(char **global, char **xdg, char *file)

Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> The current implementation of git_path() is essentially the same as
>> that of vsnpath(), with two minor differences. First, git_path()
>> currently insists that the git directory path is no longer than
>> PATH_MAX-100 characters in length. However, vsnpath() does not
>> attempt this arbitrary 100 character reservation for the remaining
>> path components. Second, vsnpath() uses the "is_dir_sep()" macro,
>> rather than comparing directly to '/', to determine if the git_dir
>> path component ends with a path separator.
>> In order to benefit from the above improvements,...
> 
> In the longer term, I think this goes in the right direction, but
> the loss of reservation, especially when we know git_path() is used
> by some callers to get the base directory in $GIT_DIR that want to
> append stuff after the returned directory path to form the final
> pathname, is a bit worrysome.  It may be hiding a bug (lack of
> proper limit check) on the callers' side.

Hmm, at first I could not see what you found worrysome here.
After all, the number of inputs which leads to success (i.e. does
not result in an "/bad-path/" return) has been *increased* with
this patch.

However, I suppose you are concerned about something like this:

    char *git_dir = git_path("");
    if (strcmp(git_dir, "/bad-path/") != 0) {
        /*
         * Having studied the implementation of git_path(), I know
         * that the buffer pointed to by git_dir has space for an
         * additional 100 chars. This is enough room to concatenate
         * the doberry path, so this is safe ...
         */
        strcat(git_dir, doberry); /* oops */
    }

Yes?

Hmm, yes it would be a little disapointing to see such parasitic code!
;-)

You said above: "... especially when we know git_path() is used
by some callers to get the base directory in $GIT_DIR ...". Can you
point me to an example of such a caller; I have been unable to find
any code which does this.

ATB,
Ramsay Jones