Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2012, 20:53 UTC
Message-ID: <7vipbt8q7r.fsf@alter.siamese.dyndns.org>
In-Reply-To: <50463A1A.9000801@ramsay1.demon.co.uk>

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

Show 47 quoted lines

> The git_snpath() and git_pathdup() functions both use the (static)
> function vsnpath() in their implementation. Also, they both discard
> the return value of vsnpath(), which has the effect of ignoring the
> side effect of calling cleanup_path() in the non-error return path.
>
> In order to ensure that the required cleanup happens, we use the
> pointer returned by vsnpath(), rather than the buffer passed into
> vsnpath(), to derive the return value from git_snpath() and
> git_pathdup().
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  path.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/path.c b/path.c
> index 9eb5333..741ae77 100644
> --- a/path.c
> +++ b/path.c
> @@ -70,21 +70,22 @@ bad:
>  
>  char *git_snpath(char *buf, size_t n, const char *fmt, ...)
>  {
> +	char *ret;
>  	va_list args;
>  	va_start(args, fmt);
> -	(void)vsnpath(buf, n, fmt, args);
> +	ret = vsnpath(buf, n, fmt, args);
>  	va_end(args);
> -	return buf;
> +	return ret;
>  }
>  
>  char *git_pathdup(const char *fmt, ...)
>  {
> -	char path[PATH_MAX];
> +	char path[PATH_MAX], *ret;
>  	va_list args;
>  	va_start(args, fmt);
> -	(void)vsnpath(path, sizeof(path), fmt, args);
> +	ret = vsnpath(path, sizeof(path), fmt, args);
>  	va_end(args);
> -	return xstrdup(path);
> +	return xstrdup(ret);
>  }
>  
>  char *mkpathdup(const char *fmt, ...)

cleanup_path() is a quick-and-dirty hack that only deals with leading ".///" (e.g. "foo//bar" is not reduced to "foo/bar"), and the callers that allocate 4 bytes for buf to hold "foo" may not be able to fit it if for some reason there are some bytes that must be cleaned, e.g. ".///foo".

The worst part of its use is that buf and ret could be different, which means you cannot safely do:

	char *buf = xmalloc(...);
        buf = git_snpath(buf, n, "%s", ...);
        ... use buf ...
        free(buf);

but instead have to do something like:

	char *path, *buf = xmalloc(...);
        path = git_snpath(buf, n, "%s", ...);
        ... use path ...
        free(buf);

And this series goes in a direction of making more callers aware of the twisted calling convention, which smells really bad.

As long as the callers are contained within this file, it is not making things _too_ bad, so...