From 450fc2bace48ce7ba07a2431175923bf2d610635 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 19 Aug 2025 15:29:34 -0400 Subject: refs: do not clobber dangling symrefs When given an expected "before" state, the ref-writing code will avoid overwriting any ref that does not match that expected state. We use the null oid as a sentinel value for "nothing should exist", and likewise that is the sentinel value we get when trying to read a ref that does not exist. But there's one corner case where this is ambiguous: dangling symrefs. Trying to read them will yield the null oid, but there is potentially something of value there: the dangling symref itself. For a normal recursive write, this is OK. Imagine we have a symref "FOO_HEAD" that points to a ref "refs/heads/bar" that does not exist, and we try to write to it with a create operation like: oid=$(git rev-parse HEAD) ;# or whatever git symbolic-ref FOO_HEAD refs/heads/bar echo "create FOO_HEAD $oid" | git update-ref --stdin The attempt to resolve FOO_HEAD will actually resolve "bar", yielding the null oid. That matches our expectation, and the write proceeds. This is correct, because we are not writing FOO_HEAD at all, but writing its destination "bar", which in fact does not exist. But what if the operation asked not to dereference symrefs? Like this: echo "create FOO_HEAD $oid" | git update-ref --no-deref --stdin Resolving FOO_HEAD would still result in a null oid, and the write will proceed. But it will overwrite FOO_HEAD itself, removing the fact that it ever pointed to "bar". This case is a little esoteric; we are clobbering a symref with a no-deref write of a regular ref value. But the same problem occurs when writing symrefs. For example: echo "symref-create FOO_HEAD refs/heads/other" | git update-ref --no-deref --stdin The "create" operation asked us to create FOO_HEAD only if it did not exist. But we silently overwrite the existing value. You can trigger this without using update-ref via the fetch followRemoteHEAD code. In "create" mode, it should not overwrite an existing value. But if you manually create a symref pointing to a value that does not yet exist (either via symbolic-ref or with "remote add -m"), create mode will happily overwrite it. Instead, we should detect this case and refuse to write. The correct specification to overwrite FOO_HEAD in this case is to provide an expected target ref value, like: echo "symref-update FOO_HEAD refs/heads/other ref refs/heads/bar" | git update-ref --no-deref --stdin Note that the non-symref "update" directive does not allow you to do this (you can only specify an oid). This is a weakness in the update-ref interface, and you'd have to overwrite unconditionally, like: echo "update FOO_HEAD $oid" | git update-ref --no-deref --stdin Likewise other symref operations like symref-delete do not accept the "ref" keyword. You should be able to do: echo "symref-delete FOO_HEAD ref refs/heads/bar" but cannot (and can only delete unconditionally). This patch doesn't address those gaps. We may want to do so in a future patch for completeness, but it's not clear if anybody actually wants to perform those operations. The symref update case (specifically, via followRemoteHEAD) is what I ran into in the wild. The code for the fix is relatively straight-forward given the discussion above. But note that we have to implement it independently for the files and reftable backends. The "old oid" checks happen as part of the locking process, which is implemented separately for each system. We may want to factor this out somehow, but it's beyond the scope of this patch. (Another curiosity is that the messages in the reftable code are marked for translation, but the ones in the files backend are not. I followed local convention in each case, but we may want to harmonize this at some point). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 905555365b..a4419ef62d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update, */ static enum ref_transaction_error check_old_oid(struct ref_update *update, struct object_id *oid, + struct strbuf *referent, struct strbuf *err) { if (update->flags & REF_LOG_ONLY || - !(update->flags & REF_HAVE_OLD) || - oideq(oid, &update->old_oid)) + !(update->flags & REF_HAVE_OLD)) return 0; + if (oideq(oid, &update->old_oid)) { + /* + * Normally matching the expected old oid is enough. Either we + * found the ref at the expected state, or we are creating and + * expect the null oid (and likewise found nothing). + * + * But there is one exception for the null oid: if we found a + * symref pointing to nothing we'll also get the null oid. In + * regular recursive mode, that's good (we'll write to what the + * symref points to, which doesn't exist). But in no-deref + * mode, it means we'll clobber the symref, even though the + * caller asked for this to be a creation event. So flag + * that case to preserve the dangling symref. + */ + if ((update->flags & REF_NO_DEREF) && referent->len && + is_null_oid(oid)) { + strbuf_addf(err, "cannot lock ref '%s': " + "dangling symref already exists", + ref_update_original_update_refname(update)); + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } + return 0; + } + if (is_null_oid(&update->old_oid)) { strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", @@ -2658,7 +2682,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re if (update->old_target) ret = ref_update_check_old_target(referent.buf, update, err); else - ret = check_old_oid(update, &lock->old_oid, err); + ret = check_old_oid(update, &lock->old_oid, + &referent, err); if (ret) goto out; } else { @@ -2690,7 +2715,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re ret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF; goto out; } else { - ret = check_old_oid(update, &lock->old_oid, err); + ret = check_old_oid(update, &lock->old_oid, + &referent, err); if (ret) { goto out; } -- cgit v1.2.3