threads / patch / 28985

patch, 3 partsconfig.c: Fix a static buffer overwrite bug by avoiding mkpath()

Subject: [PATCH 2/3] config.c: Fix a static buffer overwrite bug by avoiding mkpath()

## tl;dr

2 messages between Nov 19, 2011 and Nov 21, 2011. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Ramsay Jones· Nov 19, 2011, 19:42 UTC · lore

On cygwin, test number 21 of t3200-branch.sh (git branch -m q q2 without config should succeed) fails. The failure involves the functions from path.c which parcel out internal static buffers from the git_path() and mkpath() functions.

In particular, the rename_ref() function calls safe_create_leading\ _directories() with a filename returned by git_path("logs/%s", ref). safe_create_leading_directories(), in turn, calls stat() on each element of the path it is given. On cygwin, this leads to a call to git_config() for each component of the path, since this test explicitly removes the config file. git_config() calls mkpath(), so on the fourth component of the path, the original buffer passed into the function is overwritten with the config filename.

Note that this bug is specific to cygwin and it's schizophrenic stat() functions (see commits adbc0b6, 7faee6b and 7974843). The lack of a config file and a path with at least four elements is also important to trigger the bug.

In order to fix the problem, we replace the call to mkpath() with a call to mksnpath() and provide our own buffer.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 config.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to config.c +2 −2

diff --git a/config.c b/config.c
index edf9914..5a9ca84 100644
--- a/config.c
+++ b/config.c
@@ -851,6 +851,7 @@ int git_config_system(void)
 
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
+	char buf[4096];
 	int ret = 0, found = 0;
 	const char *home = NULL;
 
@@ -865,12 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home = getenv("HOME");
 	if (home) {
-		char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
+		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
 		if (!access(user_config, R_OK)) {
 			ret += git_config_from_file(fn, user_config, data);
 			found += 1;
 		}
-		free(user_config);
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {

-- 
1.7.7

Junio C Hamano· Nov 21, 2011, 03:32 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/3] config.c: Fix a static buffer overwrite bug by avoiding mkpath()

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

> In order to fix the problem, we replace the call to mkpath() with
> a call to mksnpath() and provide our own buffer.

A longer term fix might be to get rid of unwanted git_config_* call from stat() callchain, but as a short-term fix, this patch with minor tweaks should do.

Show 16 quoted lines

> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  config.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index edf9914..5a9ca84 100644
> --- a/config.c
> +++ b/config.c
> @@ -851,6 +851,7 @@ int git_config_system(void)
>  
>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  {
> +	char buf[4096];
>  	int ret = 0, found = 0;
>  	const char *home = NULL;

Two points. (1) This buffer does not need to be inside the whole function scope, no? (2) s/4096/PATH_MAX/ to match what get_pathname() has been returning to the caller?

That is, something like the attached squashed in?

Show 14 quoted lines

> @@ -865,12 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  
>  	home = getenv("HOME");
>  	if (home) {
> -		char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
> +		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
>  		if (!access(user_config, R_OK)) {
>  			ret += git_config_from_file(fn, user_config, data);
>  			found += 1;
>  		}
> -		free(user_config);
>  	}
>  
>  	if (repo_config && !access(repo_config, R_OK)) {

 config.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to config.c +1 −1

diff --git a/config.c b/config.c
index 5a9ca84..b6d789a 100644
--- a/config.c
+++ b/config.c
@@ -851,7 +851,6 @@ int git_config_system(void)
 
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
-	char buf[4096];
 	int ret = 0, found = 0;
 	const char *home = NULL;
 
@@ -866,6 +865,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home = getenv("HOME");
 	if (home) {
+		char buf[PATH_MAX];
 		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
 		if (!access(user_config, R_OK)) {
 			ret += git_config_from_file(fn, user_config, data);