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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 29, 2017 at 14:58 UTC f991761eb8c62cef0355c9abc89ac9d3191ae707
1 file changed +7 -17
config.c
+7 -17
@@ -2112,7 +2112,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
2112 {
2113 int fd = -1, in_fd = -1;
2114 int ret;
2115 - struct lock_file *lock = NULL;
2115 + static struct lock_file lock;
2116 char *filename_buf = NULL;
2117 char *contents = NULL;
2118 size_t contents_sz;
@@ -2131,8 +2131,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
2131 * The lock serves a purpose in addition to locking: the new
2132 * contents of .git/config will be written into it.
2133 */
2134 - lock = xcalloc(1, sizeof(struct lock_file));
2135 - fd = hold_lock_file_for_update(lock, config_filename, 0);
2134 + fd = hold_lock_file_for_update(&lock, config_filename, 0);
2135 if (fd < 0) {
2136 error_errno("could not lock config file %s", config_filename);
2137 free(store.key);
@@ -2245,8 +2244,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
2244 close(in_fd);
2245 in_fd = -1;
2246
2248 - if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
2249 - error_errno("chmod on %s failed", get_lock_file_path(lock));
2247 + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
2248 + error_errno("chmod on %s failed", get_lock_file_path(&lock));
2249 ret = CONFIG_NO_WRITE;
2250 goto out_free;
2251 }
@@ -2301,28 +2300,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
2300 contents = NULL;
2301 }
2302
2304 - if (commit_lock_file(lock) < 0) {
2303 + if (commit_lock_file(&lock) < 0) {
2304 error_errno("could not write config file %s", config_filename);
2305 ret = CONFIG_NO_WRITE;
2307 - lock = NULL;
2306 goto out_free;
2307 }
2308
2311 - /*
2312 - * lock is committed, so don't try to roll it back below.
2313 - * NOTE: Since lockfile.c keeps a linked list of all created
2314 - * lock_file structures, it isn't safe to free(lock). It's
2315 - * better to just leave it hanging around.
2316 - */
2317 - lock = NULL;
2309 ret = 0;
2310
2311 /* Invalidate the config cache */
2312 git_config_clear();
2313
2314 out_free:
2324 - if (lock)
2325 - rollback_lock_file(lock);
2315 + rollback_lock_file(&lock);
2316 free(filename_buf);
2317 if (contents)
2318 munmap(contents, contents_sz);
@@ -2331,7 +2321,7 @@ out_free:
2321 return ret;
2322
2323 write_err_out:
2334 - ret = write_error(get_lock_file_path(lock));
2324 + ret = write_error(get_lock_file_path(&lock));
2325 goto out_free;
2326
2327 }