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

[PATCH v5 7/9] refs/files: detect race when generating reflog entry for HEAD

From
Patrick Steinhardt <ps@pks.im>
Date
Aug 5, 2025, 15:11 UTC
Message-ID
<20250805-pks-reflog-append-v5-7-050997db09d5@pks.im>
In-Reply-To
<20250805-pks-reflog-append-v5-0-050997db09d5@pks.im>

When updating a reference that is being pointed to HEAD we don't only write a reflog message for that particular reference, but also generate one for HEAD. This logic is handled by `split_head_update()`, where we:

  1. Verify that the condition actually triggered. This is done by
     reading HEAD at the start of the transaction so that we can then
     check whether a given reference update refers to its target.
  2. Queue a new log-only update for HEAD in case it did.

But the logic is unfortunately not free of races, as we do not lock the HEAD reference after we have read its target. This can lead to the following two scenarios:

  - HEAD gets concurrently updated to point to one of the references we
    have already processed. This causes us not writing a reflog message
    even though we should have done so.
  - HEAD gets concurrently updated to no longer point to a reference
    anymore that we have already processed. This causes us to write a
    reflog message even though we should _not_ have done so.

Improve the situation by introducing a new `REF_LOG_VIA_SPLIT` flag that is specific to the "files" backend. If set, we will double check that the HEAD reference still points to the reference that we are creating the reflog entry for after we have locked HEAD. Furthermore, instead of manually resolving the old object ID of that entry, we now use the same old state as for the parent update.

If we detect such a racy update we abort the transaction. This is a bit heavy-handed: the user didn't even ask us to write a reflog update for "HEAD", so it might be surprising if we abort the transaction. That being said:

  - Normal users wouldn't typically hit this case as we only hit the
    relevant code when committing to a branch that is being pointed to
    by "HEAD" directly. Commands like git-commit(1) typically commit to
    "HEAD" itself though.
  - Scripted users that use git-update-ref(1) and related plumbing
    commands are unlikely to hit this case either, as they would have to
    update the pointed-to-branch at the same as "HEAD" is being updated,
    which is an exceedingly rare event.

The alternative would be to instead drop the log-only update completely, but that would require more logic that is hard to verify without adding infrastructure specific for such a test. So we rather do the pragmatic thing and don't worry too much about an edge case that is very unlikely to happen.

Unfortunately, this change only helps with the second race. We cannot reliably plug the first race without locking the HEAD reference at the start of the transaction. Locking HEAD unconditionally would effectively serialize all writes though, and that doesn't seem like an option. Also, double checking its value at the end of the transaction is not an option either, as its target may have flip-flopped during the transaction.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bf6f89b1d1..ba018b0984 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -68,6 +68,12 @@
  */
 #define REF_DELETED_RMDIR (1 << 9)
 
+/*
+ * Used to indicate that the reflog-only update has been created via
+ * `split_head_update()`.
+ */
+#define REF_LOG_VIA_SPLIT (1 << 14)
+
 struct ref_lock {
 	char *ref_name;
 	struct lock_file lk;
@@ -2420,9 +2426,10 @@ static enum ref_transaction_error split_head_update(struct ref_update *update,
 
 	new_update = ref_transaction_add_update(
 			transaction, "HEAD",
-			update->flags | REF_LOG_ONLY | REF_NO_DEREF,
+			update->flags | REF_LOG_ONLY | REF_NO_DEREF | REF_LOG_VIA_SPLIT,
 			&update->new_oid, &update->old_oid,
 			NULL, NULL, update->committer_info, update->msg);
+	new_update->parent_update = update;
 
 	/*
 	 * Add "HEAD". This insertion is O(N) in the transaction
@@ -2600,7 +2607,36 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
 
 	update->backend_data = lock;
 
-	if (update->type & REF_ISSYMREF) {
+	if (update->flags & REF_LOG_VIA_SPLIT) {
+		struct ref_lock *parent_lock;
+
+		if (!update->parent_update)
+			BUG("split update without a parent");
+
+		parent_lock = update->parent_update->backend_data;
+
+		/*
+		 * Check that "HEAD" didn't racily change since we have looked
+		 * it up. If it did we must refuse to write the reflog entry.
+		 *
+		 * Note that this does not catch all races: if "HEAD" was
+		 * racily changed to point to one of the refs part of the
+		 * transaction then we would miss writing the split reflog
+		 * entry for "HEAD".
+		 */
+		if (!(update->type & REF_ISSYMREF) ||
+		    strcmp(update->parent_update->refname, referent.buf)) {
+			strbuf_addstr(err, "HEAD has been racily updated");
+			ret = REF_TRANSACTION_ERROR_GENERIC;
+			goto out;
+		}
+
+		if (update->flags & REF_HAVE_OLD) {
+			oidcpy(&lock->old_oid, &update->old_oid);
+		} else {
+			oidcpy(&lock->old_oid, &parent_lock->old_oid);
+		}
+	} else if (update->type & REF_ISSYMREF) {
 		if (update->flags & REF_NO_DEREF) {
 			/*
 			 * We won't be reading the referent as part of
-- 
2.50.1.723.g3e08bea96f.dirty
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 100 of 114 in “refs: fix migration of reflog entries”
  1. 0/8 refs: fix migration of reflog entriesPatrick Steinhardt, Jul 22, 2025
  2. 1/8 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Jul 22, 2025
  3. Junio C HamanoJul 22, 2025
  4. 2/8 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Jul 22, 2025
  5. Justin ToblerJul 23, 2025
  6. Patrick SteinhardtJul 24, 2025
  7. Junio C HamanoJul 24, 2025
  8. 3/8 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Jul 22, 2025
  9. Justin ToblerJul 23, 2025
  10. Karthik NayakJul 24, 2025
  11. Toon ClaesJul 24, 2025
  12. 4/8 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Jul 22, 2025
  13. Justin ToblerJul 23, 2025
  14. Patrick SteinhardtJul 24, 2025
  15. Toon ClaesJul 24, 2025
  16. Patrick SteinhardtJul 25, 2025
  17. SZEDER GáborJul 24, 2025
  18. Junio C HamanoJul 24, 2025
  19. Patrick SteinhardtJul 25, 2025
  20. Junio C HamanoJul 25, 2025
  21. 5/8 ident: fix type of string length parameterPatrick Steinhardt, Jul 22, 2025
  22. 6/8 refs: fix identity for migrated reflogsPatrick Steinhardt, Jul 22, 2025
  23. Justin ToblerJul 23, 2025
  24. Patrick SteinhardtJul 24, 2025
  25. Karthik NayakJul 24, 2025
  26. Toon ClaesJul 24, 2025
  27. 7/8 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Jul 22, 2025
  28. Justin ToblerJul 23, 2025
  29. Patrick SteinhardtJul 24, 2025
  30. Karthik NayakJul 24, 2025
  31. Patrick SteinhardtJul 24, 2025
  32. 8/8 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Jul 22, 2025
  33. Junio C HamanoJul 22, 2025
  34. Patrick SteinhardtJul 23, 2025
  35. 0/8 refs: fix migration of reflog entriesPatrick Steinhardt, Jul 25, 2025
  36. 1/8 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Jul 25, 2025
  37. 2/8 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Jul 25, 2025
  38. 3/8 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Jul 25, 2025
  39. 4/8 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Jul 25, 2025
  40. Kristoffer HaugsbakkJul 28, 2025
  41. Junio C HamanoJul 28, 2025
  42. Karthik NayakJul 28, 2025
  43. Junio C HamanoJul 28, 2025
  44. Karthik NayakJul 30, 2025
  45. Ben KnobleJul 29, 2025
  46. Kristoffer HaugsbakkJul 29, 2025
  47. Patrick SteinhardtJul 29, 2025
  48. Junio C HamanoJul 29, 2025
  49. Patrick SteinhardtJul 30, 2025
  50. Patrick SteinhardtJul 30, 2025
  51. 5/8 ident: fix type of string length parameterPatrick Steinhardt, Jul 25, 2025
  52. 6/8 refs: fix identity for migrated reflogsPatrick Steinhardt, Jul 25, 2025
  53. 7/8 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Jul 25, 2025
  54. Jeff KingJul 25, 2025
  55. Patrick SteinhardtJul 28, 2025
  56. Jeff KingJul 29, 2025
  57. Patrick SteinhardtJul 29, 2025
  58. 8/8 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Jul 25, 2025
  59. 0/9 refs: fix migration of reflog entriesPatrick Steinhardt, Jul 29, 2025
  60. 1/9 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Jul 29, 2025
  61. 2/9 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Jul 29, 2025
  62. 3/9 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Jul 29, 2025
  63. Toon ClaesAug 1, 2025
  64. Patrick SteinhardtAug 4, 2025
  65. 4/9 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Jul 29, 2025
  66. Junio C HamanoJul 29, 2025
  67. Toon ClaesAug 1, 2025
  68. Patrick SteinhardtAug 4, 2025
  69. 5/9 ident: fix type of string length parameterPatrick Steinhardt, Jul 29, 2025
  70. 6/9 refs: fix identity for migrated reflogsPatrick Steinhardt, Jul 29, 2025
  71. 7/9 refs/files: detect race when generating reflog entry for HEADPatrick Steinhardt, Jul 29, 2025
  72. Junio C HamanoJul 29, 2025
  73. Toon ClaesAug 1, 2025
  74. Jeff KingAug 2, 2025
  75. Patrick SteinhardtAug 4, 2025
  76. Jeff KingAug 4, 2025
  77. 8/9 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Jul 29, 2025
  78. 9/9 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Jul 29, 2025
  79. 0/9 refs: fix migration of reflog entriesPatrick Steinhardt, Aug 4, 2025
  80. 1/9 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Aug 4, 2025
  81. 2/9 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Aug 4, 2025
  82. 3/9 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Aug 4, 2025
  83. 4/9 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Aug 4, 2025
  84. 5/9 ident: fix type of string length parameterPatrick Steinhardt, Aug 4, 2025
  85. 6/9 refs: fix identity for migrated reflogsPatrick Steinhardt, Aug 4, 2025
  86. 7/9 refs/files: detect race when generating reflog entry for HEADPatrick Steinhardt, Aug 4, 2025
  87. Jeff KingAug 4, 2025
  88. 8/9 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Aug 4, 2025
  89. 9/9 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Aug 4, 2025
  90. 0/9 refs: fix migration of reflog entriesPatrick Steinhardt, Aug 5, 2025
  91. 1/9 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Aug 5, 2025
  92. Jean-Noël AVILAAug 5, 2025
  93. Junio C HamanoAug 5, 2025
  94. Patrick SteinhardtAug 6, 2025
  95. 2/9 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Aug 5, 2025
  96. 3/9 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Aug 5, 2025
  97. 4/9 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Aug 5, 2025
  98. 5/9 ident: fix type of string length parameterPatrick Steinhardt, Aug 5, 2025
  99. 6/9 refs: fix identity for migrated reflogsPatrick Steinhardt, Aug 5, 2025
  100. 7/9 refs/files: detect race when generating reflog entry for HEADPatrick Steinhardt, Aug 5, 2025
  101. 8/9 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Aug 5, 2025
  102. 9/9 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Aug 5, 2025
  103. Jeff KingAug 5, 2025
  104. Patrick SteinhardtAug 6, 2025
  105. 0/9 refs: fix migration of reflog entriesPatrick Steinhardt, Aug 6, 2025
  106. 1/9 Documentation/git-reflog: convert to use synopsis typePatrick Steinhardt, Aug 6, 2025
  107. 2/9 builtin/reflog: improve grouping of subcommandsPatrick Steinhardt, Aug 6, 2025
  108. 3/9 refs: export `ref_transaction_update_reflog()`Patrick Steinhardt, Aug 6, 2025
  109. 4/9 builtin/reflog: implement subcommand to write new entriesPatrick Steinhardt, Aug 6, 2025
  110. 5/9 ident: fix type of string length parameterPatrick Steinhardt, Aug 6, 2025
  111. 6/9 refs: fix identity for migrated reflogsPatrick Steinhardt, Aug 6, 2025
  112. 7/9 refs/files: detect race when generating reflog entry for HEADPatrick Steinhardt, Aug 6, 2025
  113. 8/9 refs: stop unsetting REF_HAVE_OLD for log-only updatesPatrick Steinhardt, Aug 6, 2025
  114. 9/9 refs: fix invalid old object IDs when migrating reflogsPatrick Steinhardt, Aug 6, 2025

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.