[PATCH] Release config lock if the regex is invalid

[PATCH] Release config lock if the regex is invalid

[Pavel Roskin]

Signed-off-by: Pavel Roskin <proski@gnu.org>
---

 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;
 			}

Re: [PATCH] Release config lock if the regex is invalid

[Junio C Hamano]

Thanks.

Re: [PATCH] Release config lock if the regex is invalid

[Johannes Schindelin]

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;
 }
 

Re: [PATCH] Release config lock if the regex is invalid

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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;
 }
 

Re: [PATCH] Release config lock if the regex is invalid

[Johannes Schindelin]

Hi,

On Sun, 7 May 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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