[PATCH] config: use a static lock_file struct
- From
Jeff King <peff@peff.net>
- Date
- Aug 29, 2017, 18:58 UTC
- Message-ID
- <20170829185850.tfmjoa5u5sfuwpgi@sigill.intra.peff.net>
- In-Reply-To
- <20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net>
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@peff.net> --- 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