On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote:
> It looks like the config code has a minor-ish leak. Patch to follow.
Here it is.
-- >8 --
Subject: [PATCH] config: use a static lock_file struct
When modifying git config, we xcalloc() a struct lock_file
but never free it. This is necessary because the tempfile
code (upon which the locking code is built) requires that
the resulting struct remain valid through the life of the
program. However, it also confuses leak-checkers like
valgrind because only the inner "struct tempfile" is still
reachable; no pointer to the outer lock_file is kept.
Other code paths solve this by using a single static lock
struct. We can do the same here, because we know that we'll
only lock and modify one config file at a time (and
assertions within the lockfile code will ensure that this
remains the case).
That removes a real leak (when we fail to free the struct
after locking fails) as well as removes the valgrind false
positive. It also means that doing N sequential
config-writes will use a constant amount of memory, rather
than leaving stale lock_files for each.
Note that since "lock" is no longer a pointer, it can't be
NULL anymore. But that's OK. We used that feature only to
avoid calling rollback_lock_file() on an already-committed
lock. Since the lockfile code keeps its own "active" flag,
it's a noop to rollback an inactive lock, and we don't have
to worry about this ourselves.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
In the long run we may want to drop the "tempfiles must remain forever"
rule. This is certainly not the first time it has caused confusion or
leaks. And I don't think it's a fundamental issue, just the way the code
is written. But in the interim, this fix is probably worth doing.
config.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/config.c b/config.c
index d0d8ce823a..1603f96e40 100644
--- a/config.c
+++ b/config.c
@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
{
int fd = -1, in_fd = -1;
int ret;
- struct lock_file *lock = NULL;
+ static struct lock_file lock;
char *filename_buf = NULL;
char *contents = NULL;
size_t contents_sz;
@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
* The lock serves a purpose in addition to locking: the new
* contents of .git/config will be written into it.
*/
- lock = xcalloc(1, sizeof(struct lock_file));
- fd = hold_lock_file_for_update(lock, config_filename, 0);
+ fd = hold_lock_file_for_update(&lock, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
free(store.key);
@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
close(in_fd);
in_fd = -1;
- if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
- error_errno("chmod on %s failed", get_lock_file_path(lock));
+ if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
+ error_errno("chmod on %s failed", get_lock_file_path(&lock));
ret = CONFIG_NO_WRITE;
goto out_free;
}
@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
contents = NULL;
}
- if (commit_lock_file(lock) < 0) {
+ if (commit_lock_file(&lock) < 0) {
error_errno("could not write config file %s", config_filename);
ret = CONFIG_NO_WRITE;
- lock = NULL;
goto out_free;
}
- /*
- * lock is committed, so don't try to roll it back below.
- * NOTE: Since lockfile.c keeps a linked list of all created
- * lock_file structures, it isn't safe to free(lock). It's
- * better to just leave it hanging around.
- */
- lock = NULL;
ret = 0;
/* Invalidate the config cache */
git_config_clear();
out_free:
- if (lock)
- rollback_lock_file(lock);
+ rollback_lock_file(&lock);
free(filename_buf);
if (contents)
munmap(contents, contents_sz);
@@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
return ret;
write_err_out:
- ret = write_error(get_lock_file_path(lock));
+ ret = write_error(get_lock_file_path(&lock));
goto out_free;
}
--
2.14.1.721.gc5bc1565f1
