Re: [PATCH v4 13/21] refs: resolve symbolic refs first

On Fri, 201-02-12 at 15:09 +0100, Michael Haggerty wrote:]

On 02/05/2016 08:44 PM, David Turner wrote:

quoted

Before committing ref updates, split symbolic ref updates into two
parts: an update to the underlying ref, and a log-only update to
the
symbolic ref.  This ensures that both references are locked
correctly
while their reflogs are updated.

It is still possible to confuse git by concurrent updates, since
the
splitting of symbolic refs does not happen under lock. So a
symbolic ref
could be replaced by a plain ref in the middle of this operation,
which
would lead to reflog discontinuities and missed old-ref checks.

This patch is doing too much at once for my little brain to follow.

My first hangup is the change to setting RESOLVE_REF_NO_RECURSE
unconditionally in lock_ref_sha1_basic(). I count five callers of
that
function and see no justification for why the change is OK in the
context of each caller. Here are some thoughts:

* The call from files_create_symref() sets REF_NODEREF, so it is
unaffected by this change.

Yes.

* The call from files_transaction_commit() is preceded by a call to
dereference_symrefs(), which I assume effectively replaces the need
for
RESOLVE_REF_NO_RECURSE.

Yes.

* There are two calls from files_rename_ref(). Why is it OK to do
without RESOLVE_REF_NO_RECURSE there?

  * For the oldrefname call, I suppose the justification is the
"(flag &
REF_ISSYMREF)" check earlier in the function. (But does this
introduce a
significant TOCTOU race?)

The refs code as a whole seems likely to have TOCTOU issues. In
general, anywhere we check/set flag & REF_ISSYMREF without holding a
lock, we have a potential problem.  I haven't generally tried to handle
these cases, since they're not presently handled.  

The central problem with this area of the code is that commit interacts
so intimately with the locking machinery.  I understand some of why
it's done that way.  In particular, your change to ref locking to not
hold lots of open files was a big win for us at Twitter.  But this
means that it's hard to deal with cross-backend ref updates: you want
to hold multiple locks, and backends don't have the machinery for it.

We could add backend hooks to specifically lock and unlock refs. Then
the backend commit code would just be handled a bundle of locked refs
and would commit them.  This might be hairy, but it could fix the
TOCTOU problems.  So, first lock the outer refs, then split out updates
for any which are symbolic refs, and lock those. Finally, commit all
updates (split by backend).

One downside of this is that right now, the backend API is relatively
close to the front-end, and this would leak what should be an
implementation detail.  But maybe this is necessary to knit multiple
backends together.  

But I'm not sure that this is necessary right now, because I'm not sure
that I'm actually making TOCTOU issues much worse. 

  * For the newrefname call, I suppose it's because the code a little
higher up tries to delete any existing reference with that name. It
looks to me like the old code was slightly broken: if newrefname was
an
unborn symbolic reference, then: read_ref_full() would fail;
delete_ref() would be skipped; lock_ref_sha1_basic() would lock the
*referred-to* reference; the referred-to reference would be
overwritten
instead of newrefname. So it could be that here REF_NODEREF
indirectly
fixes a bug?

Yes, that's correct.  These two appears to be separable, so I'll make
it an independent patch (and add a test for that case).

* The last call, from files_reflog_expire(), is also questionable 
before your patch. If refname is a symref, then the function is 
expiring the reflog of the symref. But (before this patch) it locks 
not the symref but its referent. 

I can also separate this one.

This was discussed in some length before on the mailing list [1], and
the conclusion was that the current behavior is wrong, but for
backwards compatibility reasons it would be safest to change it to
locking *both* the symref and its referent.

Yes, that would be the right thing to do.  But for the reasons I
discuss above, that requires a serious change in the way that backends
work.  

If possible, it would be better to split this patch up into several:
the
first few would each add the REF_NODEREF flag at one callsite, with a
careful justification of why that is OK. Once all the callsites
(except
the one in files_transaction_commit()) have been changed, then the
last
patch could add the dereference_symrefs() machinery and change the
last
callsite.

(I'm not certain that those steps are actually doable independently,
given that REF_NODEREF has other effects besides setting
RESOLVE_REF_NO_RECURSE.)

I'm not just being pedantic here. The patch as written is really too
big
to review effectively.

That's a legit complaint.  The problem, as you note, is that doing some
of these steps completely independently doesn't work.  But I'll try
splitting out what I can.