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

On 04/30/2016 01:21 AM, David Turner wrote:

On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:

quoted

On 04/27/2016 08:55 PM, Junio C Hamano wrote:

quoted

Michael Haggerty [off-list ref] writes:

quoted

@@ -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