{"thread":{"id":"4099","subject":"[PATCH] Release config lock if the regex is invalid","startedAt":"2006-05-07T21:36:12Z","lastAt":"2006-05-08T11:28:31Z","messageCount":5,"participants":["Pavel Roskin","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"19666","messageId":"20060507213612.27887.28600.stgit@dv.roinet.com","threadId":"4099","inReplyTo":null,"subject":"[PATCH] Release config lock if the regex is invalid","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2006-05-07T21:36:12Z","receivedAt":"2006-05-07T21:36:12Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"Signed-off-by: Pavel Roskin <proski@gnu.org>\n---\n\n config.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 41066e4..61eacef 100644\n--- a/config.c\n+++ b/config.c\n@@ -516,6 +516,8 @@ int git_config_set_multivar(const char* \n \t\t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\",\n \t\t\t\t\tvalue_regex);\n \t\t\t\tfree(store.value_regex);\n+\t\t\t\tclose(fd);\n+\t\t\t\tunlink(lock_file);\n \t\t\t\tret = 6;\n \t\t\t\tgoto out_free;\n \t\t\t}\n"},{"id":"19670","messageId":"7vr735iezc.fsf@assigned-by-dhcp.cox.net","threadId":"4099","inReplyTo":"20060507213612.27887.28600.stgit@dv.roinet.com","subject":"Re: [PATCH] Release config lock if the regex is invalid","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-07T23:37:11Z","receivedAt":"2006-05-07T23:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"19675","messageId":"Pine.LNX.4.63.0605080229220.32508@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4099","inReplyTo":"20060507213612.27887.28600.stgit@dv.roinet.com","subject":"Re: [PATCH] Release config lock if the regex is invalid","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-05-08T00:32:52Z","receivedAt":"2006-05-08T00:32:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 7 May 2006, Pavel Roskin wrote:\n\n> @@ -516,6 +516,8 @@ int git_config_set_multivar(const char* \n>  \t\t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\",\n>  \t\t\t\t\tvalue_regex);\n>  \t\t\t\tfree(store.value_regex);\n> +\t\t\t\tclose(fd);\n> +\t\t\t\tunlink(lock_file);\n>  \t\t\t\tret = 6;\n>  \t\t\t\tgoto out_free;\n>  \t\t\t}\n> \n\nThis is not enough. There are quite a few exit paths. Notice the \"goto \nout_free\"? That is where this must go.\n\nThis patch is totally untested but obviously correct:\n\ndiff --git a/config.c b/config.c\nindex 30effe3..d8fd94d 100644\n--- a/config.c\n+++ b/config.c\n@@ -445,7 +445,7 @@ int git_config_set_multivar(const char* \n \tconst char* value_regex, int multi_replace)\n {\n \tint i;\n-\tint fd;\n+\tint fd = -1;\n \tint ret;\n \tchar* config_filename = strdup(git_path(\"config\"));\n \tchar* lock_file = strdup(git_path(\"config.lock\"));\n@@ -619,10 +619,14 @@ int git_config_set_multivar(const char* \n \tret = 0;\n \n out_free:\n+\tif (fd > 0)\n+\t\tclose(fd);\n \tif (config_filename)\n \t\tfree(config_filename);\n-\tif (lock_file)\n+\tif (lock_file) {\n+\t\tunlink(lock_file);\n \t\tfree(lock_file);\n+\t}\n \treturn ret;\n }\n \n"},{"id":"19698","messageId":"7v7j4xi6lo.fsf@assigned-by-dhcp.cox.net","threadId":"4099","inReplyTo":"Pine.LNX.4.63.0605080229220.32508@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Release config lock if the regex is invalid","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-08T02:38:11Z","receivedAt":"2006-05-08T02:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> This is not enough. There are quite a few exit paths. Notice the \"goto \n> out_free\"? That is where this must go.\n>\n> This patch is totally untested but obviously correct:\n\nexcept that many places you already close(fd) and\nunlink(lock_file).  \n\nSomehow it vaguely reminds me of recent \"kernel in C++\" thread\nin the other mailing list, which I do not want people to start\ntalking about here, but moving all the clean-up code to exit\npath indeed makes things simpler to read.\n\nHow about doing something like this?\n\n-- >8 --\ndiff --git a/config.c b/config.c\nindex 11d65f8..adb5ce4 100644\n--- a/config.c\n+++ b/config.c\n@@ -420,7 +420,7 @@ int git_config_set_multivar(const char* \n \tconst char* value_regex, int multi_replace)\n {\n \tint i;\n-\tint fd, in_fd;\n+\tint fd = -1, in_fd;\n \tint ret;\n \tchar* config_filename = strdup(git_path(\"config\"));\n \tchar* lock_file = strdup(git_path(\"config.lock\"));\n@@ -478,15 +478,11 @@ int git_config_set_multivar(const char* \n \t\tif ( ENOENT != errno ) {\n \t\t\terror(\"opening %s: %s\", config_filename,\n \t\t\t      strerror(errno));\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_file);\n \t\t\tret = 3; /* same as \"invalid config file\" */\n \t\t\tgoto out_free;\n \t\t}\n \t\t/* if nothing to unset, error out */\n \t\tif (value == NULL) {\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_file);\n \t\t\tret = 5;\n \t\t\tgoto out_free;\n \t\t}\n@@ -514,8 +510,6 @@ int git_config_set_multivar(const char* \n \t\t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\",\n \t\t\t\t\tvalue_regex);\n \t\t\t\tfree(store.value_regex);\n-\t\t\t\tclose(fd);\n-\t\t\t\tunlink(lock_file);\n \t\t\t\tret = 6;\n \t\t\t\tgoto out_free;\n \t\t\t}\n@@ -551,8 +545,6 @@ int git_config_set_multivar(const char* \n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen == 0 && value == NULL) ||\n \t\t\t\t(store.seen > 1 && multi_replace == 0)) {\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_file);\n \t\t\tret = 5;\n \t\t\tgoto out_free;\n \t\t}\n@@ -601,8 +593,6 @@ int git_config_set_multivar(const char* \n \t\tunlink(config_filename);\n \t}\n \n-\tclose(fd);\n-\n \tif (rename(lock_file, config_filename) < 0) {\n \t\tfprintf(stderr, \"Could not rename the lock file?\\n\");\n \t\tret = 4;\n@@ -612,10 +602,14 @@ int git_config_set_multivar(const char* \n \tret = 0;\n \n out_free:\n+\tif (0 <= fd)\n+\t\tclose(fd);\n \tif (config_filename)\n \t\tfree(config_filename);\n-\tif (lock_file)\n+\tif (lock_file) {\n+\t\tunlink(lock_file);\n \t\tfree(lock_file);\n+\t}\n \treturn ret;\n }\n \n"},{"id":"19709","messageId":"Pine.LNX.4.63.0605081327350.28989@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4099","inReplyTo":"7v7j4xi6lo.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Release config lock if the regex is invalid","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-05-08T11:28:31Z","receivedAt":"2006-05-08T11:28:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 7 May 2006, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > This is not enough. There are quite a few exit paths. Notice the \"goto \n> > out_free\"? That is where this must go.\n> >\n> > This patch is totally untested but obviously correct:\n> \n> except that many places you already close(fd) and\n> unlink(lock_file).\n\nYes, my bad. I forgot to look for (and remove) them. Your patch looks fine \nto me.\n\nCiao,\nDscho\n"}]}