Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

[nguyenhu@minatec.inpg.fr]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> a écrit :
>
> So, this re-introduces the bug addressed by commit 05bab3ea. The test number
> is now 29 (rather than 21) but the same test is failing; namely  
> t3200-branch.sh
> test #29 (git branch -m q q2 without config should succeed).
>
> In order to fix the bug, I created the patch given below (on top of  
> this patch).
> (Note that it does not address the above issues).
>
> HTH
>
> ATB,
> Ramsay Jones
>
> -- >8 --
> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Subject: [PATCH] path.c: Fix a static buffer overwrite bug by  
> avoiding mkpath()
>
> The v4 version of the "Read (but not write) from XDG configuration,
> XDG attributes and XDG ignore files" patch, re-introduced the bug
> addressed by commit 05bab3ea ("config.c: Fix a static buffer overwrite
> bug by avoiding mkpath()", 19-11-2011). Note that the patch refactored
> the code to determine the user (or home) configuration filename into
> a new function (home_config_paths()). In doing so, the new code once
> again uses mkpath() rather than mksnpath().
>
> In order to fix the bug, we introduce a new variation of the mkpath()
> function, mkpathdup(), which avoids the use of the internal static
> buffers. As the name implies, the new function returns a pointer to
> the pathname as a dynamically allocated string. It is the callers
> responsibility to free the memory for the returned string.
>
> Having introduced the new function, we can now replace the calls to
> 'xstrdup(mkpath(...))' in the home_config_paths() function with a
> call to mkpathdup() to achieve the same effect, without tickling the
> original bug.
>
> (Also, note that the 'xstrdup(mkpath(...))' pattern occurs in several
> other places in the source ...)
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  cache.h |    2 ++
>  path.c  |   20 +++++++++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 0632503..fbba2d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n,  
> const char *fmt, ...)
>  	__attribute__((format (printf, 3, 4)));
>  extern char *git_pathdup(const char *fmt, ...)
>  	__attribute__((format (printf, 1, 2)));
> +extern char *mkpathdup(const char *fmt, ...)
> +	__attribute__((format (printf, 1, 2)));
>
>  /* Return a statically allocated filename matching the sha1 signature */
>  extern char *mkpath(const char *fmt, ...) __attribute__((format  
> (printf, 1, 2)));
> diff --git a/path.c b/path.c
> index 53f3f53..ca29bdd 100644
> --- a/path.c
> +++ b/path.c
> @@ -87,6 +87,20 @@ char *git_pathdup(const char *fmt, ...)
>  	return xstrdup(path);
>  }
>
> +char *mkpathdup(const char *fmt, ...)
> +{
> +	char path[PATH_MAX];
> +	va_list args;
> +	unsigned len;
> +
> +	va_start(args, fmt);
> +	len = vsnprintf(path, sizeof(path), fmt, args);
> +	va_end(args);
> +	if (len >= sizeof(path))
> +		return xstrdup(bad_path);
> +	return xstrdup(cleanup_path(path));
> +}
> +
>  char *mkpath(const char *fmt, ...)
>  {
>  	va_list args;
> @@ -133,17 +147,17 @@ void home_config_paths(char **global, char  
> **xdg, char *file)
>  			*global = NULL;
>  	} else {
>  		if (!xdg_home) {
> -			to_free = strdup(mkpath("%s/.config", home));
> +			to_free = mkpathdup("%s/.config", home);
>  			xdg_home = to_free;
>  		}
>  		if (global)
> -			*global = xstrdup(mkpath("%s/.gitconfig", home));
> +			*global = mkpathdup("%s/.gitconfig", home);
>  	}
>
>  	if (!xdg_home)
>  		*xdg = NULL;
>  	else
> -		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
> +		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
>
>  	free(to_free);
>  }
> --
> 1.7.10
>

Thank you for having fixed this bug we have re-introduced with your  
patch, we will add your modifications in our next v6 version.