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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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@pks.im>
> ---
>  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