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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> ---
>  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...


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]