threads / patch / 31431

patch, 5 partspath.c: Don't discard the return value of vsnpath()

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

## tl;dr

3 messages between Sep 4, 2012 and Sep 7, 2012. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Ramsay Jones· Sep 4, 2012, 17:27 UTC · lore

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(-)

Show changes to path.c +6 −5

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

Junio C Hamano· Sep 4, 2012, 20:53 UTC · re: Ramsay Jones · lore

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

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

Ramsay Jones· Sep 7, 2012, 19:52 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

Show 13 quoted lines

> 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, ... :(

Show 9 quoted lines

> 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