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

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

On 04/30/2016 01:21 AM, David Turner wrote:
> On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:
>> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
>>>> const char *newrefname, const char *logms
>>>>  		goto rollback;
>>>>  	}
>>>>  
>>>> -	if (!read_ref_full(newrefname, RESOLVE_REF_READING,
>>>> sha1, NULL) &&
>>>> -	    delete_ref(newrefname, sha1, REF_NODEREF)) {
>>>> +	if (!read_ref_full(newrefname, resolve_flags, sha1,
>>>> NULL) &&
>>>> +	    delete_ref(newrefname, NULL, REF_NODEREF)) {
>>>
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else. So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that
>> a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
>> So
>> the "sha1" check only adds ways for the rename() to fail in
>> situations
>> where nothing harmful would have happened anyway.
>>
>> That being said, this is a very unlikely race, and I don't think it
>> matters much either way. In any case, the change s/sha1/NULL/ here
>> seems
>> orthogonal to the rest of the patch.
>>
>> David, you wrote the original version of this patch. Am I overlooking
>> something?
> 
> I think I might have been handling some weird case related to symbolic
> refs, but I don't recall the details.  Your argument seems right to me.

Doh, of course. I should have just changed it back to `sha1` and run the
test suite, then I would have seen the failure...

The point is that `read_ref_full()` is now called with
`RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic
reference, then `read_ref_full()` sets `sha1` to zeros. But the
pre-check for `delete_ref()` compares `old_sha1` to the recursively
resolved value of the reference, so that check would fail. (In fact,
`ref_transaction_delete()` refuses even to add the deletion to the
transaction if `old_sha1` is zeros, so it doesn't even get that far.)

So for shallow technical reasons we can't pass `sha1` to `delete_ref()`
anymore, and for the deeper reasons discussed in this thread that's not
a problem.

I'll document this in v2 of this patch.

Michael