Patrick Steinhardt <ps@xxxxxx> writes:
> 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 point to not 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.
>
> 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@xxxxxx>
> ---
> refs/files-backend.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
This is a step new in this iteration. I sometimes wonder if the
world were in a much better shape if I didn't record the updates to
underlying branch in the reflog of HEAD (and limited only to record
switching branches), as this is a fallout from the (mis)design.
Anyway, I agree that the change in this patch matches the above
description and takes us in a better place ;-)
Thanks.
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bf6f89b1d19..ba018b0984a 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
