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

[PATCH v2 31/33] lock_ref_for_update(): don't resolve symrefs

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
May 6, 2016, 16:14 UTC
Message-ID
<6097b21b368baaacdd887deb086219cb919e00d9.1462550456.git.mhagger@alum.mit.edu>
In-Reply-To
<cover.1462550456.git.mhagger@alum.mit.edu>

If a transaction includes a non-NODEREF update to a symbolic reference, we don't have to look it up in lock_ref_for_update(). The reference will be dereferenced anyway when the split-off update is processed.

This change requires that we store a backpointer from the split-off update to its parent update, for two reasons:

* We still want to report the original reference name in error messages.
  So if an error occurs when checking the split-off update's old_sha1,
  walk the parent_update pointers back to find the original reference
  name, and report that one.
* We still need to write the old_sha1 of the symref to its reflog. So
  after we read the split-off update's reference value, walk the
  parent_update pointers back and fill in their old_sha1 fields.

Aside from eliminating unnecessary reads, this change fixes a subtle (though not very serious) race condition: in the old code, the old_sha1 of the symref was resolved before the reference that it pointed at was locked. So it was possible that the old_sha1 value logged to the symref's reflog could be wrong if another process changed the downstream reference before it was locked.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++--------------
 refs/refs-internal.h |  17 ++++++++
 2 files changed, 95 insertions(+), 30 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a9066ba..08ec293 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3371,8 +3371,15 @@ static int split_symref_update(struct ref_update *update,
 			update->new_sha1, update->old_sha1,
 			update->msg);
 
-	/* Change the symbolic ref update to log only: */
+	new_update->parent_update = update;
+
+	/*
+	 * Change the symbolic ref update to log only. Also, it
+	 * doesn't need to check its old SHA-1 value, as that will be
+	 * done when new_update is processed.
+	 */
 	update->flags |= REF_LOG_ONLY | REF_NODEREF;
+	update->flags &= ~REF_HAVE_OLD;
 
 	item->util = new_update;
 
@@ -3380,6 +3387,17 @@ static int split_symref_update(struct ref_update *update,
 }
 
 /*
+ * Return the refname under which update was originally requested.
+ */
+static const char *original_update_refname(struct ref_update *update)
+{
+	while (update->parent_update)
+		update = update->parent_update;
+
+	return update->refname;
+}
+
+/*
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
  * - Read the reference under lock.
@@ -3431,44 +3449,74 @@ static int lock_ref_for_update(struct ref_update *update,
 	lock = update->lock;
 
 	if (update->type & REF_ISSYMREF) {
-		if (read_ref_full(update->refname,
-				  mustexist ? RESOLVE_REF_READING : 0,
-				  lock->old_oid.hash, NULL)) {
-			if (update->flags & REF_HAVE_OLD) {
-				strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-					    update->refname);
+		if (update->flags & REF_NODEREF) {
+			/*
+			 * We won't be reading the referent as part of
+			 * the transaction, so we have to read it here
+			 * to record and possibly check old_sha1:
+			 */
+			if (read_ref_full(update->refname,
+					  mustexist ? RESOLVE_REF_READING : 0,
+					  lock->old_oid.hash, NULL)) {
+				if (update->flags & REF_HAVE_OLD) {
+					strbuf_addf(err, "cannot lock ref '%s': "
+						    "can't resolve old value",
+						    update->refname);
+					return TRANSACTION_GENERIC_ERROR;
+				} else {
+					hashclr(lock->old_oid.hash);
+				}
+			}
+			if ((update->flags & REF_HAVE_OLD) &&
+			    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+				strbuf_addf(err, "cannot lock ref '%s': "
+					    "is at %s but expected %s",
+					    update->refname,
+					    sha1_to_hex(lock->old_oid.hash),
+					    sha1_to_hex(update->old_sha1));
 				return TRANSACTION_GENERIC_ERROR;
-			} else {
-				hashclr(lock->old_oid.hash);
 			}
-		}
-		if ((update->flags & REF_HAVE_OLD) &&
-		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
-			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-				    update->refname,
-				    sha1_to_hex(lock->old_oid.hash),
-				    sha1_to_hex(update->old_sha1));
-			return TRANSACTION_GENERIC_ERROR;
-		}
 
-		if (!(update->flags & REF_NODEREF)) {
+		} else {
+			/*
+			 * Create a new update for the reference this
+			 * symref is pointing at. Also, we will record
+			 * and verify old_sha1 for this update as part
+			 * of processing the split-off update, so we
+			 * don't have to do it here.
+			 */
 			ret = split_symref_update(update, referent.buf, transaction,
 						  affected_refnames, err);
 			if (ret)
 				return ret;
 		}
-	} else if ((update->flags & REF_HAVE_OLD) &&
-		   hashcmp(lock->old_oid.hash, update->old_sha1)) {
-		if (is_null_sha1(update->old_sha1))
-			strbuf_addf(err, "cannot lock ref '%s': reference already exists",
-				    update->refname);
-		else
-			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-				    update->refname,
-				    sha1_to_hex(lock->old_oid.hash),
-				    sha1_to_hex(update->old_sha1));
+	} else {
+		struct ref_update *parent_update;
 
-		return TRANSACTION_GENERIC_ERROR;
+		/*
+		 * If this update is happening indirectly because of a
+		 * symref update, record the old SHA-1 in the parent
+		 * update:
+		 */
+		for (parent_update = update->parent_update;
+		     parent_update;
+		     parent_update = parent_update->parent_update) {
+			oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
+		}
+
+		if ((update->flags & REF_HAVE_OLD) &&
+		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+			if (is_null_sha1(update->old_sha1))
+				strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+					    original_update_refname(update));
+			else
+				strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+					    original_update_refname(update),
+					    sha1_to_hex(lock->old_oid.hash),
+					    sha1_to_hex(update->old_sha1));
+
+			return TRANSACTION_GENERIC_ERROR;
+		}
 	}
 
 	if ((update->flags & REF_HAVE_NEW) &&
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index cccd76b..1bb3d87 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);
  * not exist before update.
  */
 struct ref_update {
+
 	/*
 	 * If (flags & REF_HAVE_NEW), set the reference to this value:
 	 */
 	unsigned char new_sha1[20];
+
 	/*
 	 * If (flags & REF_HAVE_OLD), check that the reference
 	 * previously had this value:
 	 */
 	unsigned char old_sha1[20];
+
 	/*
 	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
 	 * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
 	 * REF_UPDATE_VIA_HEAD:
 	 */
 	unsigned int flags;
+
 	struct ref_lock *lock;
 	unsigned int type;
 	char *msg;
+
+	/*
+	 * If this ref_update was split off of a symref update via
+	 * split_symref_update(), then this member points at that
+	 * update. This is used for two purposes:
+	 * 1. When reporting errors, we report the refname under which
+	 *    the update was originally requested.
+	 * 2. When we read the old value of this reference, we
+	 *    propagate it back to its parent update for recording in
+	 *    the latter's reflog.
+	 */
+	struct ref_update *parent_update;
+
 	const char refname[FLEX_ARRAY];
 };
 

-- 
2.8.1
Previous: Michael HaggertyNext: Michael Haggerty
Message 39 of 51 in “Yet more preparation for reference backends”
  1. 00/33 Yet more preparation for reference backendsMichael Haggerty, May 6, 2016
  2. 01/33 t1404: demonstrate a bug resolving referencesMichael Haggerty, May 6, 2016
  3. 02/33 commit_ref(): if there is an empty dir in the way, delete itMichael Haggerty, May 6, 2016
  4. 03/33 read_raw_ref(): don't get confused by an empty directoryMichael Haggerty, May 6, 2016
  5. 04/33 safe_create_leading_directories(): improve docstringMichael Haggerty, May 6, 2016
  6. 05/33 remove_dir_recursively(): add docstringMichael Haggerty, May 6, 2016
  7. 06/33 refname_is_safe(): use skip_prefix()Michael Haggerty, May 6, 2016
  8. 07/33 refname_is_safe(): don't allow the empty stringMichael Haggerty, May 6, 2016
  9. 08/33 refname_is_safe(): insist that the refname already be normalizedMichael Haggerty, May 6, 2016
  10. 09/33 commit_ref_update(): write error message to *err, not stderrMichael Haggerty, May 6, 2016
  11. 10/33 rename_ref(): remove unneeded local variableMichael Haggerty, May 6, 2016
  12. 11/33 ref_transaction_commit(): remove local variable nMichael Haggerty, May 6, 2016
  13. 12/33 read_raw_ref(): rename flags argument to typeMichael Haggerty, May 6, 2016
  14. 13/33 read_raw_ref(): clear *type at start of functionMichael Haggerty, May 6, 2016
  15. 14/33 read_raw_ref(): rename symref argument to referentMichael Haggerty, May 6, 2016
  16. 15/33 read_raw_ref(): improve docstringMichael Haggerty, May 6, 2016
  17. 16/33 read_raw_ref(): move docstring to header fileMichael Haggerty, May 6, 2016
  18. 17/33 lock_ref_sha1_basic(): remove unneeded local variableMichael Haggerty, May 6, 2016
  19. 18/33 refs: make error messages more consistentMichael Haggerty, May 6, 2016
  20. 19/33 ref_transaction_create(): disallow recursive pruningMichael Haggerty, May 6, 2016
  21. 20/33 ref_transaction_commit(): correctly report close_ref() failureMichael Haggerty, May 6, 2016
  22. 21/33 delete_branches(): use resolve_refdup()Michael Haggerty, May 6, 2016
  23. 22/33 refs: allow log-only updatesMichael Haggerty, May 6, 2016
  24. 23/33 refs: don't dereference on renameMichael Haggerty, May 6, 2016
  25. 24/33 verify_refname_available(): adjust constness in declarationMichael Haggerty, May 6, 2016
  26. 25/33 add_update(): initialize the whole ref_updateMichael Haggerty, May 6, 2016
  27. 26/33 lock_ref_for_update(): new functionMichael Haggerty, May 6, 2016
  28. 27/33 unlock_ref(): move definition higher in the fileMichael Haggerty, May 6, 2016
  29. 28/33 ref_transaction_update(): check refname_is_safe() at a minimumMichael Haggerty, May 6, 2016
  30. 29/33 refs: resolve symbolic refs firstMichael Haggerty, May 6, 2016
  31. Jeff KingMay 12, 2016
  32. Jeff KingMay 12, 2016
  33. Michael HaggertyMay 13, 2016
  34. 29/33 refs: resolve symbolic refs firstMichael Haggerty, May 13, 2016
  35. Jeff KingMay 13, 2016
  36. Jeff KingMay 13, 2016
  37. Torsten BögershausenMay 14, 2016
  38. 30/33 lock_ref_for_update(): don't re-read non-symbolic referencesMichael Haggerty, May 6, 2016
  39. 31/33 lock_ref_for_update(): don't resolve symrefsMichael Haggerty, May 6, 2016
  40. 32/33 commit_ref_update(): remove the flags parameterMichael Haggerty, May 6, 2016
  41. 33/33 lock_ref_sha1_basic(): only handle REF_NODEREF modeMichael Haggerty, May 6, 2016
  42. David TurnerMay 9, 2016
  43. Junio C HamanoMay 9, 2016
  44. Michael HaggertyMay 9, 2016
  45. Junio C HamanoMay 9, 2016
  46. Jeff KingMay 12, 2016
  47. Junio C HamanoMay 12, 2016
  48. Junio C HamanoMay 10, 2016
  49. Michael HaggertyJun 10, 2016
  50. Junio C HamanoJun 10, 2016
  51. [ADDENDUM v4] Yet more preparation for reference backendsMichael Haggerty, Jun 13, 2016

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.