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

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

[Ramsay Jones]

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, ...)
-- 
1.7.12

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

[Junio C Hamano]

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

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

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

[Ramsay Jones]

Junio C Hamano wrote:
> 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);

Hmm, this seems very unnatural to me; it would never have occurred to
me to use anything other than a stack allocated buffer (or *maybe* a
global buffer) when calling git_snpath().

In this situation (ie a dynamically allocated buffer used to hold the
result), why would you not use git_pathdup()? (which does not suffer
this problem.)

I guess it does not matter what I find unnatural, ... :(

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

Note that, prior to commit aba13e7c ("git_pathdup: returns xstrdup-ed
copy of the formatted path", 27-10-2008), git_snpath() was calling
cleanup_path() in the non-error return path. I suspect that this
commit did not intend to "fix the git_snpath() interface" and only
did so by accident. (That's a guess, of course)

However, I much prefer git_snpath(), git_pathdup() and git_path()
return the same result string given the same inputs! :D

ATB,
Ramsay Jones