On Tue, Jan 12, 2016 at 03:22:51PM -0500, Jeff King wrote:
> I had thought that this:
>
> git init
> git commit --allow-empty -m foo
> git symbolic-ref refs/foo refs/heads/master
> old=$(git rev-parse foo)
> git update-ref --no-deref -d refs/foo $old
>
> might trigger a problem (because reading refs/foo with NODEREF will give
> us a blank sha1 to compare against). But of course that is nonsense. The
> actual lock verification is not done by this initial resolve_ref. It
> happens _after_ we take the lock (as it must to avoid races), when
> verify_lock() calls read_ref_full().
>
> But now I'm doubly confused. When we call read_ref_full(), it is _also_
> into lock->old_oid.hash. Which should be overwriting what the earlier
> resolve_ref_unsafe() wrote into it. Which would mean my whole commit is
> wrong; we can just unconditionally do a non-recursive resolution in the
> first place. But when I did so, I ended up with funny reflog values
> (which is why I wrote the patch as I did).
>
> Let me try to dig a little further into that case and see what is going
> on.
Ah, I see. When calling git-symbolic-ref, we don't provide an old_sha1,
and therefore never call verify_lock(). And we get whatever value in
lock->old_oid we happened to read earlier in resolve_ref_unsafe(). Which
happened outside of a lock. Yikes. It seems like we could racily write
the wrong reflog entry in such a case.
So I think we'd want something like this:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 291b18d..c6ce503 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1845,7 +1845,7 @@ static int verify_lock(struct ref_lock *lock,
errno = save_errno;
return -1;
}
- if (hashcmp(lock->old_oid.hash, old_sha1)) {
+ if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
strbuf_addf(err, "ref %s is at %s but expected %s",
lock->ref_name,
sha1_to_hex(lock->old_oid.hash),
@@ -2008,7 +1983,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
goto error_return;
}
}
- if (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {
+ if (verify_lock(lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
}
to make sure that the value in lock->old_oid always comes from what we
read under the lock. And then the resolve_ref() calls in
lock_ref_sha1_basic() really don't matter. They are just about making
sure there is space to create the lockfile.
The patch above is not quite right; I'll work up a series that takes
this approach.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html