Re: [PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/29/2016 02:12 PM, Jeff King wrote:
> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:
> 
>> Remember, we're talking about rename_ref() only, not reference deletion
>> in general. rename_ref() is not very robust anyway--it doesn't happen in
>> a single transaction, and it is vulnerable to being defeated by
>> simultaneous reference updates by other processes. It *does* wrap the
>> deletion of newrefname in a transaction; the only question is whether an
>> old_sha1 is supplied to that transaction.
>>
>> So, suppose that newrefname starts at value A, and there are two updates
>> running simultaneously:
>>
>> 1. An update of reference newrefname from A -> B
>>
>> 2. A rename of reference oldrefname to newrefname, which includes
>>    a. read_ref_full("newrefname") and
>>    b. delete_ref("newrefname").
> 
> I wondered at first if you meant "oldrefname" in 2b. That is, a rename
> would involve writing to "newrefname" and then deleting "oldrefname",
> and not doing the old_sha1 and normal locking on the deletion of
> oldrefname would be bad (in case somebody else updated it while we were
> operating).
> 
> But reading the patch again, that's not what's going on. You're talking
> just about the case where we force-overwrite an existing newrefname, and
> delete it first to do so. But what I don't understand is:

It's beyond the ambition of this patch to fix this old rename_ref()
code, but...

>   1. Why do we read_ref_full() at all? Since it is not done under lock
>      anyway, it is useless for helping with race conditions, and I think
>      that is what you are arguing (that we should just be deleting
>      regardless). But then why not just call delete_ref(), and handle
>      the ENOENT case as a noop (which closes another race if somebody
>      deletes it between your 2a and 2b).

I thought about that too, and agree it would be an improvement. But it's
not quite so trivial. The ENOENT doesn't make it out of delete_ref()
(which does a full-fledged ref_transaction internally). The error is
only reported via "strbuf *err".

>   2. Why delete it at all? The point is to overwrite, so I guess it is
>      trying to make room. But we could just rename the loose ref file
>      and reflog overtop the old, couldn't we?
>
> Or am I totally misreading the purpose of this code?

I wouldn't want to just rename the files, because (1) I think it is
better to use the existing ref_transaction code, and (2) that wouldn't
work if there is a D/F conflict between the old and new reference names,
which the existing code handles (though honestly I'm skeptical that it
comes up a lot).

It would be more plausible to use ref_transaction_update() to write
oldrefname's *value* on top of newrefname without actually moving the
file. oldrefname could even be deleted in the same transaction, if you
were willing to stop supporting old/new refnames that D/F conflict with
each other. But we also want to move the reflog, and that should be done
while the newrefname lock and (contrary to the current code) also the
oldrefname lock are held. There's currently no way to slip an arbitrary
action like that into the middle of a transaction.

If I had lots of time and interest to work on this, I think the best
approach would be to add a ref_transaction_rename(), which takes an old
and a new reference name as arguments, and adds a whole rename operation
(including of the reflog) to a transaction. This could probably be
implemented by adding one ref_update() and one ref_delete(), and using
the parent_update pointer that is introduced later to link the two so
that ref_transaction_commit() knows to move the reflog too.

If you were willing to punt on D/F conflicts, you would be done. If not,
then it would be better to teach ref_transaction_commit() to deal with
D/F conflicts in the general case rather than using special-purpose code
in rename_ref().

Then rename_ref() could be omitted from the vtable entirely.

Michael