{"thread":{"id":"44717","subject":"Segfault in git_config_set_multivar_in_file_gently with direct_io in FUSE filesystem","startedAt":"2016-12-16T18:42:16Z","lastAt":"2016-12-20T20:10:07Z","messageCount":7,"participants":["Josh Bleecher Snyder","Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307897","messageId":"CAFAcib_cY8FeLFkW1=MfR+P7xoupGK9DFegNY5boExHSRppAmg@mail.gmail.com","threadId":"44717","inReplyTo":null,"subject":"Segfault in git_config_set_multivar_in_file_gently with direct_io in FUSE filesystem","fromName":"Josh Bleecher Snyder","fromEmail":"josharian@gmail.com","sentAt":"2016-12-16T18:41:25Z","receivedAt":"2016-12-16T18:42:16Z","isPatch":false,"sender":{"key":"josharian@gmail.com","avatar":"https://gravatar.com/avatar/df58f649b62f4d66e2e635810212cb54403228630fabfa080eb5847a6f4cc01b?d=mp&s=160"},"body":"I am using git with a simple in-memory FUSE filesystem. When I enable\ndirect_io, I get a segfault from\ngit_config_set_multivar_in_file_gently during git clone.\n\nI have full reproduction instructions using Go and macOS at\nhttps://github.com/josharian/gitbug. It also includes a stack trace in\ncase anyone wants to try blind debugging. Happy to provide more info\nif it will help.\n\nThanks,\nJosh\n"},{"id":"308008","messageId":"20161219092155.20359-1-pclouds@gmail.com","threadId":"44717","inReplyTo":"CAFAcib_cY8FeLFkW1=MfR+P7xoupGK9DFegNY5boExHSRppAmg@mail.gmail.com","subject":"[PATCH] config.c: handle error case for fstat() calls","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-19T09:21:55Z","receivedAt":"2016-12-19T09:23:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Will this fix the problem I'm replying to? I don't know. I found this\n while checking the code and it should be fixed regardless.\n\n config.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 83fdecb..4973256 100644\n--- a/config.c\n+++ b/config.c\n@@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\tgoto out_free;\n \t\t}\n \n-\t\tfstat(in_fd, &st);\n+\t\tif (fstat(in_fd, &st) == -1) {\n+\t\t\terror_errno(_(\"fstat on %s failed\"), config_filename);\n+\t\t\tret = CONFIG_INVALID_FILE;\n+\t\t\tgoto out_free;\n+\t\t}\n+\n \t\tcontents_sz = xsize_t(st.st_size);\n \t\tcontents = xmmap_gently(NULL, contents_sz, PROT_READ,\n \t\t\t\t\tMAP_PRIVATE, in_fd, 0);\n@@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tgoto unlock_and_out;\n \t}\n \n-\tfstat(fileno(config_file), &st);\n+\tif (fstat(fileno(config_file), &st) == -1) {\n+\t\tret = error_errno(_(\"fstat on %s failed\"), config_filename);\n+\t\tgoto out;\n+\t}\n \n \tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n \t\tret = error_errno(\"chmod on %s failed\",\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308069","messageId":"xmqqtw9zpz0c.fsf@gitster.mtv.corp.google.com","threadId":"44717","inReplyTo":"20161219092155.20359-1-pclouds@gmail.com","subject":"Re: [PATCH] config.c: handle error case for fstat() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-19T18:14:27Z","receivedAt":"2016-12-19T18:15:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Will this fix the problem I'm replying to? I don't know. I found this\n>  while checking the code and it should be fixed regardless.\n\nYeah, from a cursory read, it is a step in the right direction to\ncheck the return value of fstat().  \n\nShouldn't the error-return path in the second hunk rollback the\nlockfile to clean after itself?  The existing \"Oh, we cannot chmod\nto match the original\" check that comes immediately after shares the\nsame issue, so this is not a new problem, but making an existing one\nworse.\n\n>  config.c | 12 ++++++++++--\n>  1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 83fdecb..4973256 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n>  \t\t\tgoto out_free;\n>  \t\t}\n>  \n> -\t\tfstat(in_fd, &st);\n> +\t\tif (fstat(in_fd, &st) == -1) {\n> +\t\t\terror_errno(_(\"fstat on %s failed\"), config_filename);\n> +\t\t\tret = CONFIG_INVALID_FILE;\n> +\t\t\tgoto out_free;\n> +\t\t}\n> +\n>  \t\tcontents_sz = xsize_t(st.st_size);\n>  \t\tcontents = xmmap_gently(NULL, contents_sz, PROT_READ,\n>  \t\t\t\t\tMAP_PRIVATE, in_fd, 0);\n> @@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,\n>  \t\tgoto unlock_and_out;\n>  \t}\n>  \n> -\tfstat(fileno(config_file), &st);\n> +\tif (fstat(fileno(config_file), &st) == -1) {\n> +\t\tret = error_errno(_(\"fstat on %s failed\"), config_filename);\n> +\t\tgoto out;\n> +\t}\n>  \n>  \tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n>  \t\tret = error_errno(\"chmod on %s failed\",\n"},{"id":"308135","messageId":"20161220094746.GA3917@ash","threadId":"44717","inReplyTo":"xmqqtw9zpz0c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config.c: handle error case for fstat() calls","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-20T09:47:46Z","receivedAt":"2016-12-20T09:47:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 19, 2016 at 10:14:27AM -0800, Junio C Hamano wrote:\n> Shouldn't the error-return path in the second hunk rollback the\n> lockfile to clean after itself?  The existing \"Oh, we cannot chmod\n> to match the original\" check that comes immediately after shares the\n> same issue, so this is not a new problem, but making an existing one\n> worse.\n\nOK. How about two more patches on top (or bottom, does not matter)?\nThe second one should fix this. The first is sort of \"good to do\".\n\n[PATCH 1/2] config.c: rename label unlock_and_out\n[PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...\n--\nDuy\n"},{"id":"308136","messageId":"20161220094836.4131-1-pclouds@gmail.com","threadId":"44717","inReplyTo":"20161220094746.GA3917@ash","subject":"[PATCH 1/2] config.c: rename label unlock_and_out","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-20T09:48:35Z","receivedAt":"2016-12-20T09:48:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There are two ways to unlock a file: commit, or revert. Rename it to\ncommit_and_out to avoid confusion.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 4973256..505e0d0 100644\n--- a/config.c\n+++ b/config.c\n@@ -2416,7 +2416,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \n \tif (!(config_file = fopen(config_filename, \"rb\"))) {\n \t\t/* no config file means nothing to rename, no error */\n-\t\tgoto unlock_and_out;\n+\t\tgoto commit_and_out;\n \t}\n \n \tif (fstat(fileno(config_file), &st) == -1) {\n@@ -2478,7 +2478,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t}\n \t}\n \tfclose(config_file);\n-unlock_and_out:\n+commit_and_out:\n \tif (commit_lock_file(lock) < 0)\n \t\tret = error_errno(\"could not write config file %s\",\n \t\t\t\t  config_filename);\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308137","messageId":"20161220094836.4131-2-pclouds@gmail.com","threadId":"44717","inReplyTo":"20161220094836.4131-1-pclouds@gmail.com","subject":"[PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-20T09:48:36Z","receivedAt":"2016-12-20T09:49:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We could rely on atexit() to clean up everything, but let's be\nexplicit when we can. And it's good anyway because the function is\ncalled the second time in the same process, we're in trouble.\n\nThis function should not affect the successful case because after\ncommit_lock_file() is called, rollback_lock_file() becomes no-op.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n config.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.c b/config.c\nindex 505e0d0..e02def4 100644\n--- a/config.c\n+++ b/config.c\n@@ -2483,6 +2483,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tret = error_errno(\"could not write config file %s\",\n \t\t\t\t  config_filename);\n out:\n+\trollback_lock_file(lock);\n \tfree(filename_buf);\n \treturn ret;\n }\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308166","messageId":"xmqqfulijrah.fsf@gitster.mtv.corp.google.com","threadId":"44717","inReplyTo":"20161220094836.4131-2-pclouds@gmail.com","subject":"Re: [PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T20:09:58Z","receivedAt":"2016-12-20T20:10:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> We could rely on atexit() to clean up everything, but let's be\n> explicit when we can. And it's good anyway because the function is\n> called the second time in the same process, we're in trouble.\n>\n> This function should not affect the successful case because after\n> commit_lock_file() is called, rollback_lock_file() becomes no-op.\n\nNot really.  At the point of the first \"goto out\" in this function,\nlock is merely an uninitialized pointer.\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\n\n>  config.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/config.c b/config.c\n> index 505e0d0..e02def4 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2483,6 +2483,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n>  \t\tret = error_errno(\"could not write config file %s\",\n>  \t\t\t\t  config_filename);\n>  out:\n> +\trollback_lock_file(lock);\n>  \tfree(filename_buf);\n>  \treturn ret;\n>  }\n"}]}