Skip to contentSkip to conversation

Close config file handle if the entry to unset is not found

PATCH In review Sven Strickroth 3 participants 10 messages Jun 15, 2016

lore
Share & tools
lore

Sven Strickroth

author
#0 lore raw

PATCH 1/1 Close config file handle if the entry to unset is not found

Without this patch there might be open handle leaks.

Signed-off-by Sup Yut Sum <[email protected]>
Signed-off-by Sven Strickroth <[email protected]>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)
config.c +1 -0
index 9fd275f..89b49e3 100644
@@ -2048,6 +2048,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 		if ((store.seen == 0 && value == NULL) ||
 				(store.seen > 1 && multi_replace == 0)) {
 			ret = CONFIG_NOTHING_SET;
+			close(in_fd);
 			goto out_free;
 		}
Signature · 3 lines
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
#1 lore raw
Quoted 1 line · Sven Strickroth

Without this patch there might be open handle leaks.

Thanks for the patch. A question below...

Quoted 14 lines
Signed-off-by Sup Yut Sum <[email protected]>
Signed-off-by Sven Strickroth <[email protected]>

---

config.c +1 -0
index 9fd275f..89b49e3 100644
@@ -2048,6 +2048,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                if ((store.seen == 0 && value == NULL) ||
                                (store.seen > 1 && multi_replace == 0)) {
                        ret = CONFIG_NOTHING_SET;
+                       close(in_fd);
                        goto out_free;
Quoted 1 line

From a cursory read of the code, it appears that there are several

other places where the open 'in_fd' gets leaked which would deserve the same treatment. So, it's not clear why this patch handles only this one case. Am I missing something?

Quoted 3 lines
                }

--

author
#2 lore raw

Am 14.08.2015 um 21:35 schrieb Eric Sunshine:

Quoted 10 lines
Quoted 10 lines
Signed-off-by Sup Yut Sum <[email protected]>
Signed-off-by Sven Strickroth <[email protected]>

---

config.c +0 -0
index 9fd275f..89b49e3 100644
@@ -2048,6 +2048,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                if ((store.seen == 0 && value == NULL) ||
                                (store.seen > 1 && multi_replace == 0)) {
Quoted 8 lines

config.c int git_config_set_multivar_in_file(const char *config_filename, · line 2052

Quoted 3 lines
config.c +1 -0
                        ret = CONFIG_NOTHING_SET;
+                       close(in_fd);
                        goto out_free;

From a cursory read of the code, it appears that there are several other places where the open 'in_fd' gets leaked which would deserve the same treatment. So, it's not clear why this patch handles only this one case. Am I missing something?

Good point.

I suppose only this place was adjusted, becasue this is the only case which occurred so far.

Shall I re-create another patch?

Signature · 3 lines
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
author
#3 lore raw

PATCH 1/1 Correctly close config file handle in case of error

In reply to Eric Sunshine

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>
---
 config.c | 4 ++++
 1 file changed, 4 insertions(+)
config.c +4 -0
index 9fd275f..c06dc2f 100644
@@ -2010,6 +2010,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 				error("invalid pattern: %s", value_regex);
 				free(store.value_regex);
 				ret = CONFIG_INVALID_PATTERN;
+				close(in_fd);
 				goto out_free;
 			}
 		}

@@ -2034,6 +2035,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 				free(store.value_regex);
 			}
 			ret = CONFIG_INVALID_FILE;
+			close(in_fd);
 			goto out_free;
 		}
 

@@ -2048,6 +2050,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 		if ((store.seen == 0 && value == NULL) ||
 				(store.seen > 1 && multi_replace == 0)) {
 			ret = CONFIG_NOTHING_SET;
+			close(in_fd);
 			goto out_free;
 		}
 

@@ -2062,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			      config_filename, strerror(errno));
 			ret = CONFIG_INVALID_FILE;
 			contents = NULL;
+			close(in_fd);
 			goto out_free;
 		}
 		close(in_fd);
Signature · 3 lines
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
#4 lore raw

Re: [PATCH] Correctly close config file handle in case of error

Quoted 1 line · Sven Strickroth

Without this patch there might be open file handle leaks.

Thanks, this looks better. One comment below...

Quoted 32 lines
Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>

---

config.c +3 -0
index 9fd275f..c06dc2f 100644
@@ -2010,6 +2010,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                                error("invalid pattern: %s", value_regex);
                                free(store.value_regex);
                                ret = CONFIG_INVALID_PATTERN;
+                               close(in_fd);
                                goto out_free;
                        }
                }

@@ -2034,6 +2035,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                                free(store.value_regex);
                        }
                        ret = CONFIG_INVALID_FILE;
+                       close(in_fd);
                        goto out_free;
                }
 

@@ -2048,6 +2050,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                if ((store.seen == 0 && value == NULL) ||
                                (store.seen > 1 && multi_replace == 0)) {
                        ret = CONFIG_NOTHING_SET;
+                       close(in_fd);
                        goto out_free;
                }

config.c int git_config_set_multivar_in_file(const char *config_filename, · line 2069

Quoted 5 lines
config.c +1 -0
@@ -2062,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                              config_filename, strerror(errno));
                        ret = CONFIG_INVALID_FILE;
                        contents = NULL;
+                       close(in_fd);
                        goto out_free;

Each of these cases flows through 'out_free', so an alternate approach would be to close 'in_fd' there instead. Doing so has the benefit that it is less likely for future code changes to make the same mistake of failing to close the file descriptor.

Of course, you'd need to initialize 'in_fd' to some "invalid" value (such as -1) which 'out_free' can check, as well as setting 'in_fd' to that invalid value after the legitimate existing close().

    int in_fd = -1;
    ...
    if (whatever_error)
    goto out_free;
    ...
    close(in_fd);
    in_fd = -1;
    ...
    out_free:
    if (in_fd >= 0)
        close(in_fd);
    ...

or something...

Quoted 2 lines
config.c +0 -0
                }
                close(in_fd);
author
#5 lore raw

PATCH 1/1 Correctly close config file handle in case of error

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>
---
 config.c | 3 +++
 1 file changed, 3 insertions(+)
config.c +0 -0
index 9fd275f..8138d5d 100644
@@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char

*config_filename,
goto out_free;
}
close(in_fd);
+ in_fd = -1;

if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
error("chmod on %s failed: %s",
@@ -2148,6 +2149,8 @@ out_free:
free(filename_buf);
if (contents)
munmap(contents, contents_sz);
+ if (in_fd >= 0)
+ close(in_fd);
return ret;

write_err_out:

Signature · 3 lines
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
#6 lore raw

Re: [PATCH] Correctly close config file handle in case of error

Quoted 10 lines · Sven Strickroth

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>

---

config.c +0 -0
index 9fd275f..8138d5d 100644

config.c int git_config_set_multivar_in_file(const char · line 2069

Quoted 5 lines
config.c +1 -0
@@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char
 *config_filename,
                        goto out_free;
                }
                close(in_fd);
+               in_fd = -1;

You also need to initialize 'in_fd' to -1 at its point of declaration since there are goto's to 'out_free' which occur before the `in_fd = open(...)'.

Quoted 12 lines
config.c +2 -1
                if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
                        error("chmod on %s failed: %s",

@@ -2148,6 +2149,8 @@ out_free:
        free(filename_buf);
        if (contents)
                munmap(contents, contents_sz);
+       if (in_fd >= 0)
+               close(in_fd);
        return ret;
 
 write_err_out:
--
author
#7 lore raw

PATCH 1/1 Correctly close config file handle in case of error

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
config.c +4 -1
index 9fd275f..83caa25 100644
@@ -1935,7 +1935,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 				const char *key, const char *value,
 				const char *value_regex, int multi_replace)
 {
-	int fd = -1, in_fd;
+	int fd = -1, in_fd = -1;
 	int ret;
 	struct lock_file *lock = NULL;
 	char *filename_buf = NULL;

@@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			goto out_free;
 		}
 		close(in_fd);
+		in_fd = -1;
 
 		if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
 			error("chmod on %s failed: %s",

@@ -2148,6 +2149,8 @@ out_free:
 	free(filename_buf);
 	if (contents)
 		munmap(contents, contents_sz);
+	if (in_fd >= 0)
+		close(in_fd);
 	return ret;
 
 write_err_out:
Signature · 3 lines
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
Reviewed-by on v1 1/1
#8 lore raw

Re: [PATCH] Correctly close config file handle in case of error

Quoted 4 lines · Sven Strickroth

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>

Better. Thanks.

Reviewed-by Eric Sunshine <[email protected]>
Quoted 35 lines

---

config.c +4 -2
index 9fd275f..83caa25 100644
@@ -1935,7 +1935,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                                const char *key, const char *value,
                                const char *value_regex, int multi_replace)
 {
-       int fd = -1, in_fd;
+       int fd = -1, in_fd = -1;
        int ret;
        struct lock_file *lock = NULL;
        char *filename_buf = NULL;

@@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
                        goto out_free;
                }
                close(in_fd);
+               in_fd = -1;
 
                if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
                        error("chmod on %s failed: %s",

@@ -2148,6 +2149,8 @@ out_free:
        free(filename_buf);
        if (contents)
                munmap(contents, contents_sz);
+       if (in_fd >= 0)
+               close(in_fd);
        return ret;
 
 write_err_out:
--

Best regards,
Sven Strickroth
PGP key id F5A9D4C4 @ any key-server

Reviewed-by Eric Sunshine on v1 1/1
#9 lore raw

Re: [PATCH] Correctly close config file handle in case of error

In reply to Sven Strickroth
Quoted 4 lines · Sven Strickroth

Without this patch there might be open file handle leaks.

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>

Who are these two people and how are they related to this change? I am wondering if "Sup" is the author of this change and "Sven" is only relaying his change, or the other way around or what.

In any case, your log message left a lot of room for improvement; here is my attempt.

-- >8 --
From: Sven Strickroth <[email protected]>
Date: Fri, 14 Aug 2015 22:21:17 +0200
Subject: [PATCH] config: close config file handle in case of error

When updating an existing configuration file, we did not always close the filehandle that is reading from the current configuration file when we encountered an error (e.g. when unsetting a variable that does not exist).

Signed-off-by Sven Strickroth <[email protected]>
Signed-off-by Sup Yut Sum <[email protected]>
Reviewed-by Eric Sunshine <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
config.c +4 -1
index 9fd275f..83caa25 100644
@@ -1935,7 +1935,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 				const char *key, const char *value,
 				const char *value_regex, int multi_replace)
 {
-	int fd = -1, in_fd;
+	int fd = -1, in_fd = -1;
 	int ret;
 	struct lock_file *lock = NULL;
 	char *filename_buf = NULL;

@@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			goto out_free;
 		}
 		close(in_fd);
+		in_fd = -1;
 
 		if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
 			error("chmod on %s failed: %s",

@@ -2148,6 +2149,8 @@ out_free:
 	free(filename_buf);
 	if (contents)
 		munmap(contents, contents_sz);
+	if (in_fd >= 0)
+		close(in_fd);
 	return ret;
 
 write_err_out:
Signature · 1 line
2.5.0-487-g2edca40