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

[PATCH v7 23/38] lockfile: avoid transitory invalid states

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Oct 1, 2014, 10:28 UTC
Message-ID
<1412159322-2622-24-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1412159322-2622-1-git-send-email-mhagger@alum.mit.edu>

Because remove_lock_file() can be called any time by the signal handler, it is important that any lock_file objects that are in the lock_file_list are always in a valid state. And since lock_file objects are often reused (but are never removed from lock_file_list), that means we have to be careful whenever mutating a lock_file object to always keep it in a well-defined state.

This was formerly not the case, because part of the state was encoded by setting lk->filename to the empty string vs. a valid filename. It is wrong to assume that this string can be updated atomically; for example, even

    strcpy(lk->filename, value)
is unsafe.  But the old code was even more reckless; for example,
    strcpy(lk->filename, path);
    if (!(flags & LOCK_NODEREF))
            resolve_symlink(lk->filename, max_path_len);
    strcat(lk->filename, ".lock");

During the call to resolve_symlink(), lk->filename contained the name of the file that was being locked, not the name of the lockfile. If a signal were raised during that interval, then the signal handler would have deleted the valuable file!

We could probably continue to use the filename field to encode the state by being careful to write characters 1..N-1 of the filename first, and then overwrite the NUL at filename[0] with the first character of the filename, but that would be awkward and error-prone.

So, instead of using the filename field to determine whether the lock_file object is active, add a new field "lock_file::active" for this purpose. Be careful to set this field only when filename really contains the name of a file that should be deleted on cleanup.

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h      |  1 +
 lockfile.c   | 37 ++++++++++++++++++++++++++-----------
 read-cache.c |  1 +
 3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 24891a8..8e25fce 100644
--- a/cache.h
+++ b/cache.h
@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
 
 struct lock_file {
 	struct lock_file *next;
+	volatile sig_atomic_t active;
 	int fd;
 	pid_t owner;
 	char on_list;
diff --git a/lockfile.c b/lockfile.c
index 728ce49..d35ac44 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -23,7 +23,7 @@
  * used to prevent a forked process from closing a lockfile created by
  * its parent.
  *
- * A lock_file object can be in several states:
+ * The possible states of a lock_file object are as follows:
  *
  * - Uninitialized.  In this state the object's on_list field must be
  *   zero but the rest of its contents need not be initialized.  As
@@ -32,19 +32,27 @@
  *
  * - Locked, lockfile open (after hold_lock_file_for_update(),
  *   hold_lock_file_for_append(), or reopen_lock_file()). In this
- *   state, the lockfile exists, filename holds the filename of the
- *   lockfile, fd holds a file descriptor open for writing to the
- *   lockfile, and owner holds the PID of the process that locked the
- *   file.
+ *   state:
+ *   - the lockfile exists
+ *   - active is set
+ *   - filename holds the filename of the lockfile
+ *   - fd holds a file descriptor open for writing to the lockfile
+ *   - owner holds the PID of the process that locked the file
  *
  * - Locked, lockfile closed (after successful close_lock_file()).
  *   Same as the previous state, except that the lockfile is closed
  *   and fd is -1.
  *
  * - Unlocked (after commit_lock_file(), rollback_lock_file(), a
- *   failed attempt to lock, or a failed close_lock_file()). In this
- *   state, filename[0] == '\0' and fd is -1. The object is left
- *   registered in the lock_file_list, and on_list is set.
+ *   failed attempt to lock, or a failed close_lock_file()).  In this
+ *   state:
+ *   - active is unset
+ *   - filename[0] == '\0' (usually, though there are transitory states
+ *     in which this condition doesn't hold). Client code should *not*
+ *     rely on this fact!
+ *   - fd is -1
+ *   - the object is left registered in the lock_file_list, and
+ *     on_list is set.
  */
 
 static struct lock_file *lock_file_list;
@@ -175,9 +183,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		atexit(remove_lock_file);
 	}
 
+	if (lk->active)
+		die("BUG: cannot lock_file(\"%s\") using active struct lock_file",
+		    path);
 	if (!lk->on_list) {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
+		lk->active = 0;
 		lk->owner = 0;
 		lk->filename[0] = 0;
 		lk->next = lock_file_list;
@@ -199,6 +211,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		return -1;
 	}
 	lk->owner = getpid();
+	lk->active = 1;
 	if (adjust_shared_perm(lk->filename)) {
 		int save_errno = errno;
 		error("cannot fix permission bits on %s", lk->filename);
@@ -298,7 +311,7 @@ int reopen_lock_file(struct lock_file *lk)
 {
 	if (0 <= lk->fd)
 		die(_("BUG: reopen a lockfile that is still open"));
-	if (!lk->filename[0])
+	if (!lk->active)
 		die(_("BUG: reopen a lockfile that has been committed"));
 	lk->fd = open(lk->filename, O_WRONLY);
 	return lk->fd;
@@ -308,7 +321,7 @@ int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
 
-	if (!lk->filename[0])
+	if (!lk->active)
 		die("BUG: attempt to commit unlocked object");
 
 	if (close_lock_file(lk))
@@ -325,6 +338,7 @@ int commit_lock_file(struct lock_file *lk)
 		return -1;
 	}
 
+	lk->active = 0;
 	lk->filename[0] = 0;
 	return 0;
 }
@@ -339,11 +353,12 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (!lk->filename[0])
+	if (!lk->active)
 		return;
 
 	if (!close_lock_file(lk)) {
 		unlink_or_warn(lk->filename);
+		lk->active = 0;
 		lk->filename[0] = 0;
 	}
 }
diff --git a/read-cache.c b/read-cache.c
index 5ffb1d7..af69f34 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2046,6 +2046,7 @@ static int commit_locked_index(struct lock_file *lk)
 			return -1;
 		if (rename(lk->filename, alternate_index_output))
 			return -1;
+		lk->active = 0;
 		lk->filename[0] = 0;
 		return 0;
 	} else {

-- 
2.1.0
Previous: Michael HaggertyNext: Michael Haggerty
Message 27 of 43 in “Lockfile correctness and refactoring”
  1. 00/38 Lockfile correctness and refactoringMichael Haggerty, Oct 1, 2014
  2. 01/38 unable_to_lock_die(): rename function from unable_to_lock_index_die()Michael Haggerty, Oct 1, 2014
  3. 02/38 api-lockfile: revise and expand the documentationMichael Haggerty, Oct 1, 2014
  4. 03/38 close_lock_file(): exit (successfully) if file is already closedMichael Haggerty, Oct 1, 2014
  5. 04/38 rollback_lock_file(): do not clear filename redundantlyMichael Haggerty, Oct 1, 2014
  6. 05/38 rollback_lock_file(): exit early if lock is not activeMichael Haggerty, Oct 1, 2014
  7. 06/38 rollback_lock_file(): set fd to -1Michael Haggerty, Oct 1, 2014
  8. 07/38 lockfile: unlock file if lockfile permissions cannot be adjustedMichael Haggerty, Oct 1, 2014
  9. 08/38 hold_lock_file_for_append(): release lock on errorsMichael Haggerty, Oct 1, 2014
  10. 09/38 lock_file(): always initialize and register lock_file objectMichael Haggerty, Oct 1, 2014
  11. René ScharfeOct 1, 2014
  12. Michael HaggertyOct 1, 2014
  13. Junio C HamanoOct 1, 2014
  14. 10/38 lockfile.c: document the various states of lock_file objectsMichael Haggerty, Oct 1, 2014
  15. 11/38 cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LENMichael Haggerty, Oct 1, 2014
  16. 12/38 delete_ref_loose(): don't muck around in the lock_file's filenameMichael Haggerty, Oct 1, 2014
  17. 13/38 prepare_index(): declare return value to be (const char *)Michael Haggerty, Oct 1, 2014
  18. 14/38 lock_file(): exit early if lockfile cannot be openedMichael Haggerty, Oct 1, 2014
  19. 15/38 remove_lock_file(): call rollback_lock_file()Michael Haggerty, Oct 1, 2014
  20. 16/38 commit_lock_file(): inline temporary variableMichael Haggerty, Oct 1, 2014
  21. 17/38 commit_lock_file(): die() if called for unlocked lockfile objectMichael Haggerty, Oct 1, 2014
  22. 18/38 close_lock_file(): if close fails, roll backMichael Haggerty, Oct 1, 2014
  23. 19/38 commit_lock_file(): rollback lock file on failure to renameMichael Haggerty, Oct 1, 2014
  24. 20/38 api-lockfile: document edge casesMichael Haggerty, Oct 1, 2014
  25. 21/38 dump_marks(): remove a redundant call to rollback_lock_file()Michael Haggerty, Oct 1, 2014
  26. 22/38 git_config_set_multivar_in_file(): avoid call to rollback_lock_file()Michael Haggerty, Oct 1, 2014
  27. 23/38 lockfile: avoid transitory invalid statesMichael Haggerty, Oct 1, 2014
  28. 24/38 struct lock_file: declare some fields volatileMichael Haggerty, Oct 1, 2014
  29. 25/38 try_merge_strategy(): remove redundant lock_file allocationMichael Haggerty, Oct 1, 2014
  30. 26/38 try_merge_strategy(): use a statically-allocated lock_file objectMichael Haggerty, Oct 1, 2014
  31. 27/38 commit_lock_file(): use a strbuf to manage temporary spaceMichael Haggerty, Oct 1, 2014
  32. 28/38 Change lock_file::filename into a strbufMichael Haggerty, Oct 1, 2014
  33. 29/38 resolve_symlink(): use a strbuf for internal scratch spaceMichael Haggerty, Oct 1, 2014
  34. 30/38 resolve_symlink(): take a strbuf parameterMichael Haggerty, Oct 1, 2014
  35. 31/38 trim_last_path_component(): replace last_path_elm()Michael Haggerty, Oct 1, 2014
  36. 32/38 Extract a function commit_lock_file_to()Michael Haggerty, Oct 1, 2014
  37. 33/38 Rename LOCK_NODEREF to LOCK_NO_DEREFMichael Haggerty, Oct 1, 2014
  38. 34/38 lockfile.c: rename static functionsMichael Haggerty, Oct 1, 2014
  39. 35/38 get_locked_file_path(): new functionMichael Haggerty, Oct 1, 2014
  40. 36/38 hold_lock_file_for_append(): restore errno before returningMichael Haggerty, Oct 1, 2014
  41. 37/38 Move read_index() definition to read-cache.cMichael Haggerty, Oct 1, 2014
  42. 38/38 lockfile.h: extract new header file for the functions in lockfile.cMichael Haggerty, Oct 1, 2014
  43. Junio C HamanoOct 1, 2014

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.