# [PATCH] Release config lock if the regex is invalid 5 messages from 2006-05-07 to 2006-05-08. Participants: Pavel Roskin, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/4099 ## Pavel Roskin, 2006-05-07 21:36 Subject: [PATCH] Release config lock if the regex is invalid Message-ID: <20060507213612.27887.28600.stgit@dv.roinet.com> URL: https://gitlist.dev/e/20060507213612.27887.28600.stgit%40dv.roinet.com ``` Signed-off-by: Pavel Roskin --- config.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/config.c b/config.c index 41066e4..61eacef 100644 --- a/config.c +++ b/config.c @@ -516,6 +516,8 @@ int git_config_set_multivar(const char* fprintf(stderr, "Invalid pattern: %s\n", value_regex); free(store.value_regex); + close(fd); + unlink(lock_file); ret = 6; goto out_free; } ``` ## Junio C Hamano, 2006-05-07 23:37 Subject: Re: [PATCH] Release config lock if the regex is invalid Message-ID: <7vr735iezc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr735iezc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060507213612.27887.28600.stgit@dv.roinet.com> ``` Thanks. ``` ## Johannes Schindelin, 2006-05-08 00:32 Subject: Re: [PATCH] Release config lock if the regex is invalid Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0605080229220.32508%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20060507213612.27887.28600.stgit@dv.roinet.com> ``` Hi, On Sun, 7 May 2006, Pavel Roskin wrote: > @@ -516,6 +516,8 @@ int git_config_set_multivar(const char* > fprintf(stderr, "Invalid pattern: %s\n", > value_regex); > free(store.value_regex); > + close(fd); > + unlink(lock_file); > ret = 6; > goto out_free; > } > This is not enough. There are quite a few exit paths. Notice the "goto out_free"? That is where this must go. This patch is totally untested but obviously correct: diff --git a/config.c b/config.c index 30effe3..d8fd94d 100644 --- a/config.c +++ b/config.c @@ -445,7 +445,7 @@ int git_config_set_multivar(const char* const char* value_regex, int multi_replace) { int i; - int fd; + int fd = -1; int ret; char* config_filename = strdup(git_path("config")); char* lock_file = strdup(git_path("config.lock")); @@ -619,10 +619,14 @@ int git_config_set_multivar(const char* ret = 0; out_free: + if (fd > 0) + close(fd); if (config_filename) free(config_filename); - if (lock_file) + if (lock_file) { + unlink(lock_file); free(lock_file); + } return ret; } ``` ## Junio C Hamano, 2006-05-08 02:38 Subject: Re: [PATCH] Release config lock if the regex is invalid Message-ID: <7v7j4xi6lo.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7j4xi6lo.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > This is not enough. There are quite a few exit paths. Notice the "goto > out_free"? That is where this must go. > > This patch is totally untested but obviously correct: except that many places you already close(fd) and unlink(lock_file). Somehow it vaguely reminds me of recent "kernel in C++" thread in the other mailing list, which I do not want people to start talking about here, but moving all the clean-up code to exit path indeed makes things simpler to read. How about doing something like this? -- >8 -- diff --git a/config.c b/config.c index 11d65f8..adb5ce4 100644 --- a/config.c +++ b/config.c @@ -420,7 +420,7 @@ int git_config_set_multivar(const char* const char* value_regex, int multi_replace) { int i; - int fd, in_fd; + int fd = -1, in_fd; int ret; char* config_filename = strdup(git_path("config")); char* lock_file = strdup(git_path("config.lock")); @@ -478,15 +478,11 @@ int git_config_set_multivar(const char* if ( ENOENT != errno ) { error("opening %s: %s", config_filename, strerror(errno)); - close(fd); - unlink(lock_file); ret = 3; /* same as "invalid config file" */ goto out_free; } /* if nothing to unset, error out */ if (value == NULL) { - close(fd); - unlink(lock_file); ret = 5; goto out_free; } @@ -514,8 +510,6 @@ int git_config_set_multivar(const char* fprintf(stderr, "Invalid pattern: %s\n", value_regex); free(store.value_regex); - close(fd); - unlink(lock_file); ret = 6; goto out_free; } @@ -551,8 +545,6 @@ int git_config_set_multivar(const char* /* if nothing to unset, or too many matches, error out */ if ((store.seen == 0 && value == NULL) || (store.seen > 1 && multi_replace == 0)) { - close(fd); - unlink(lock_file); ret = 5; goto out_free; } @@ -601,8 +593,6 @@ int git_config_set_multivar(const char* unlink(config_filename); } - close(fd); - if (rename(lock_file, config_filename) < 0) { fprintf(stderr, "Could not rename the lock file?\n"); ret = 4; @@ -612,10 +602,14 @@ int git_config_set_multivar(const char* ret = 0; out_free: + if (0 <= fd) + close(fd); if (config_filename) free(config_filename); - if (lock_file) + if (lock_file) { + unlink(lock_file); free(lock_file); + } return ret; } ``` ## Johannes Schindelin, 2006-05-08 11:28 Subject: Re: [PATCH] Release config lock if the regex is invalid Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0605081327350.28989%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7v7j4xi6lo.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Sun, 7 May 2006, Junio C Hamano wrote: > Johannes Schindelin writes: > > > This is not enough. There are quite a few exit paths. Notice the "goto > > out_free"? That is where this must go. > > > > This patch is totally untested but obviously correct: > > except that many places you already close(fd) and > unlink(lock_file). Yes, my bad. I forgot to look for (and remove) them. Your patch looks fine to me. Ciao, Dscho ```