git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] config: use a static lock_file struct

From
Jeff King <peff@peff.net>
Date
Aug 30, 2017, 05:55 UTC
Message-ID
<20170830055539.xpgxeu3flmxs55av@sigill.intra.peff.net>
In-Reply-To
<20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net>
On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote:
Show 11 quoted lines
> > I feel like right now we meet (1) and not (2). But I think if we keep to
> > that lower bar of (1), it might not be that bad. We're assuming now that
> > there's no race on the tempfile->active flag, for instance. We could
> > probably make a similar assumption about putting items onto or taking
> > them off of a linked list (it's not really atomic, but a single pointer
> > assignment is probably "atomic enough" for our purposes).
> 
> Something like this, which AFAICT is about as safe as the existing code
> in its list manipulation. It retains the "active" flag as an additional
> check which I think isn't strictly necessary, but potentially catches
> some logic errors.

The patch below demonstrates how this could be used to turn some "xcalloc" lock_files into stack variables that are allowed to go out of scope after we commit or rollback. This solves the three lock-related leaks reported by valgrind when running t0000.

_But_ it also demonstrates an interesting downside of this approach. Some functions are lazy in their error paths. For instance, look at write_index_as_tree(). We take the lock early in the function, but may return before a commit or rollback if we hit an error. With the current code this "leaks" the tempfile, which is wrong. But nobody notices because in practice the program exits soon after and we clean up the tempfile then.

But with a stack variable lock_file, that minor problem becomes a major one: our stack variable got added to a global linked list and the atexit() handler will try to read it. But now of course it will contain garbage, since the variable went out of scope.

So it's probably safer to just let tempfile.c handle the whole lifetime, and have it put all live tempfiles on the heap, and free them when they're deactivated. That means our creation signature becomes more like:

  struct tempfile *create_tempfile(const char *path);

and delete_tempfile() actually calls free() on it (though we'd probably want to skip the free() from a signal handler for the usual reasons).

I don't have a patch for that, but just food for thought while exploring the implications of my earlier suggestion.

---
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d955cd56b3..bf7420b808 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -917,7 +917,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
-	struct lock_file *lock_file;
+	struct lock_file lock_file = LOCK_INIT;
 	struct parse_opt_ctx_t ctx;
 	strbuf_getline_fn getline_fn;
 	int parseopt_state = PARSE_OPT_UNKNOWN;
@@ -1016,11 +1016,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
 	/* we will diagnose later if it turns out that we need to update it */
-	newfd = hold_locked_index(lock_file, 0);
+	newfd = hold_locked_index(&lock_file, 0);
 	if (newfd < 0)
 		lock_error = errno;
 
@@ -1155,11 +1152,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 				exit(128);
 			unable_to_lock_die(get_index_file(), lock_error);
 		}
-		if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
+		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die("Unable to write new index file");
 	}
 
-	rollback_lock_file(lock_file);
+	rollback_lock_file(&lock_file);
 
 	return has_errors ? 1 : 0;
 }
diff --git a/cache-tree.c b/cache-tree.c
index 2440d1dc89..440f92aeca 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -603,19 +603,15 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)
 {
 	int entries, was_valid, newfd;
-	struct lock_file *lock_file;
+	struct lock_file lock_file = LOCK_INIT;
 
-	/*
-	 * We can't free this memory, it becomes part of a linked list
-	 * parsed atexit()
-	 */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
-	newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);
+	newfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
 
 	entries = read_index_from(index_state, index_path);
-	if (entries < 0)
+	if (entries < 0) {
+		rollback_lock_file(&lock_file);
 		return WRITE_TREE_UNREADABLE_INDEX;
+	}
 	if (flags & WRITE_TREE_IGNORE_CACHE_TREE)
 		cache_tree_free(&index_state->cache_tree);
 
@@ -624,10 +620,12 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 
 	was_valid = cache_tree_fully_valid(index_state->cache_tree);
 	if (!was_valid) {
-		if (cache_tree_update(index_state, flags) < 0)
+		if (cache_tree_update(index_state, flags) < 0) {
+			rollback_lock_file(&lock_file);
 			return WRITE_TREE_UNMERGED_INDEX;
+		}
 		if (0 <= newfd) {
-			if (!write_locked_index(index_state, lock_file, COMMIT_LOCK))
+			if (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))
 				newfd = -1;
 		}
 		/* Not being able to write is fine -- we are only interested
@@ -641,15 +639,17 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 	if (prefix) {
 		struct cache_tree *subtree;
 		subtree = cache_tree_find(index_state->cache_tree, prefix);
-		if (!subtree)
+		if (!subtree) {
+			rollback_lock_file(&lock_file);
 			return WRITE_TREE_PREFIX_ERROR;
+		}
 		hashcpy(sha1, subtree->oid.hash);
 	}
 	else
 		hashcpy(sha1, index_state->cache_tree->oid.hash);
 
 	if (0 <= newfd)
-		rollback_lock_file(lock_file);
+		rollback_lock_file(&lock_file);
 
 	return 0;
 }
diff --git a/config.c b/config.c
index d0d8ce823a..7931182a54 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;
+	struct lock_file lock = LOCK_INIT;
 	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;
 
 }
diff --git a/lockfile.h b/lockfile.h
index 572064939c..a7ba42bdf2 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -114,6 +114,8 @@ struct lock_file {
 	struct tempfile tempfile;
 };
 
+#define LOCK_INIT { { NULL } }
+
 /* String appended to a filename to derive the lockfile name: */
 #define LOCK_SUFFIX ".lock"
 #define LOCK_SUFFIX_LEN 5
Previous: Jeff KingNext: Michael Haggerty
Message 23 of 43 in “pkt-line: re-'static'-ify buffer in packet_write_fmt_1()”
  1. pkt-line: re-'static'-ify buffer in packet_write_fmt_1()Martin Ågren, Aug 27, 2017
  2. Jeff KingAug 27, 2017
  3. Jeff KingAug 27, 2017
  4. Lars SchneiderAug 27, 2017
  5. Martin ÅgrenAug 27, 2017
  6. Jeff KingAug 27, 2017
  7. Lars SchneiderAug 27, 2017
  8. Jeff KingAug 27, 2017
  9. Martin ÅgrenAug 28, 2017
  10. Stefan BellerAug 28, 2017
  11. Jeff KingAug 28, 2017
  12. Junio C HamanoSep 5, 2017
  13. Lars SchneiderAug 29, 2017
  14. Jeff KingAug 29, 2017
  15. config: use a static lock_file structJeff King, Aug 29, 2017
  16. Brandon WilliamsAug 29, 2017
  17. Brandon WilliamsAug 29, 2017
  18. Jeff KingAug 29, 2017
  19. Michael HaggertyAug 30, 2017
  20. Jeff KingAug 30, 2017
  21. Michael HaggertyAug 30, 2017
  22. Jeff KingAug 30, 2017
  23. Jeff KingAug 30, 2017
  24. Michael HaggertyAug 30, 2017
  25. Jeff KingSep 2, 2017
  26. Jeff KingSep 2, 2017
  27. Michael HaggertyAug 30, 2017
  28. Jeff KingAug 30, 2017
  29. Brandon WilliamsAug 30, 2017
  30. Jeff KingAug 30, 2017
  31. Brandon WilliamsAug 30, 2017
  32. Jeff KingAug 31, 2017
  33. Junio C HamanoSep 6, 2017
  34. Jeff KingSep 6, 2017
  35. Martin ÅgrenAug 29, 2017
  36. Jeff KingAug 29, 2017
  37. Jeff KingAug 30, 2017
  38. Junio C HamanoSep 5, 2017
  39. 0/2 Do not call cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017
  40. 1/2 describe: do not use cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017
  41. SZEDER GáborOct 10, 2017
  42. Junio C HamanoOct 11, 2017
  43. 2/2 merge-ours: do not use cmd_*() as a subroutineJunio C Hamano, Oct 10, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.