{"thread":{"id":"28985","subject":"[PATCH 2/3] config.c: Fix a static buffer overwrite bug by avoiding mkpath()","startedAt":"2011-11-19T19:42:00Z","lastAt":"2011-11-21T03:32:02Z","messageCount":2,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"179742","messageId":"4EC80688.609@ramsay1.demon.co.uk","threadId":"28985","inReplyTo":null,"subject":"[PATCH 2/3] config.c: Fix a static buffer overwrite bug by avoiding mkpath()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-11-19T19:42:00Z","receivedAt":"2011-11-19T19:42:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nOn cygwin, test number 21 of t3200-branch.sh (git branch -m q q2\nwithout config should succeed) fails. The failure involves the\nfunctions from path.c which parcel out internal static buffers\nfrom the git_path() and mkpath() functions.\n\nIn particular, the rename_ref() function calls safe_create_leading\\\n_directories() with a filename returned by git_path(\"logs/%s\", ref).\nsafe_create_leading_directories(), in turn, calls stat() on each\nelement of the path it is given. On cygwin, this leads to a call\nto git_config() for each component of the path, since this test\nexplicitly removes the config file. git_config() calls mkpath(), so\non the fourth component of the path, the original buffer passed\ninto the function is overwritten with the config filename.\n\nNote that this bug is specific to cygwin and it's schizophrenic\nstat() functions (see commits adbc0b6, 7faee6b and 7974843). The\nlack of a config file and a path with at least four elements is\nalso important to trigger the bug.\n\nIn order to fix the problem, we replace the call to mkpath() with\na call to mksnpath() and provide our own buffer.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n config.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex edf9914..5a9ca84 100644\n--- a/config.c\n+++ b/config.c\n@@ -851,6 +851,7 @@ int git_config_system(void)\n \n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n+\tchar buf[4096];\n \tint ret = 0, found = 0;\n \tconst char *home = NULL;\n \n@@ -865,12 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \thome = getenv(\"HOME\");\n \tif (home) {\n-\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n+\t\tchar *user_config = mksnpath(buf, sizeof(buf), \"%s/.gitconfig\", home);\n \t\tif (!access(user_config, R_OK)) {\n \t\t\tret += git_config_from_file(fn, user_config, data);\n \t\t\tfound += 1;\n \t\t}\n-\t\tfree(user_config);\n \t}\n \n \tif (repo_config && !access(repo_config, R_OK)) {\n-- \n1.7.7\n"},{"id":"179770","messageId":"7v4nxyi219.fsf@alter.siamese.dyndns.org","threadId":"28985","inReplyTo":"4EC80688.609@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/3] config.c: Fix a static buffer overwrite bug by avoiding mkpath()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-21T03:32:02Z","receivedAt":"2011-11-21T03:32:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> In order to fix the problem, we replace the call to mkpath() with\n> a call to mksnpath() and provide our own buffer.\n\nA longer term fix might be to get rid of unwanted git_config_* call from\nstat() callchain, but as a short-term fix, this patch with minor tweaks\nshould do.\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  config.c |    4 ++--\n>  1 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index edf9914..5a9ca84 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -851,6 +851,7 @@ int git_config_system(void)\n>  \n>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n>  {\n> +\tchar buf[4096];\n>  \tint ret = 0, found = 0;\n>  \tconst char *home = NULL;\n\nTwo points. (1) This buffer does not need to be inside the whole function\nscope, no? (2) s/4096/PATH_MAX/ to match what get_pathname() has been\nreturning to the caller?\n\nThat is, something like the attached squashed in?\n\n> @@ -865,12 +866,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n>  \n>  \thome = getenv(\"HOME\");\n>  \tif (home) {\n> -\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n> +\t\tchar *user_config = mksnpath(buf, sizeof(buf), \"%s/.gitconfig\", home);\n>  \t\tif (!access(user_config, R_OK)) {\n>  \t\t\tret += git_config_from_file(fn, user_config, data);\n>  \t\t\tfound += 1;\n>  \t\t}\n> -\t\tfree(user_config);\n>  \t}\n>  \n>  \tif (repo_config && !access(repo_config, R_OK)) {\n\n config.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 5a9ca84..b6d789a 100644\n--- a/config.c\n+++ b/config.c\n@@ -851,7 +851,6 @@ int git_config_system(void)\n \n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n-\tchar buf[4096];\n \tint ret = 0, found = 0;\n \tconst char *home = NULL;\n \n@@ -866,6 +865,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \thome = getenv(\"HOME\");\n \tif (home) {\n+\t\tchar buf[PATH_MAX];\n \t\tchar *user_config = mksnpath(buf, sizeof(buf), \"%s/.gitconfig\", home);\n \t\tif (!access(user_config, R_OK)) {\n \t\t\tret += git_config_from_file(fn, user_config, data);\n"}]}