Re: [PATCH 19/29] refs: don't dereference on rename
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Apr 30, 2016, 03:48 UTC
- Message-ID
- <57242B22.4050202@alum.mit.edu>
- In-Reply-To
- <1461972108.4123.43.camel@twopensource.com>
On 04/30/2016 01:21 AM, David Turner wrote:
Show 43 quoted lines
> 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