Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Jeff King <peff@peff.net>]

On Mon, Dec 28, 2015 at 10:45:19AM +0100, Michael Haggerty wrote:

> > +static int create_ref_symlink(struct ref_lock *lock, const char *target)
> [...]
> > +	char *ref_path = get_locked_file_path(lock->lk);
> > +	unlink(ref_path);
> > +	ret = symlink(target, ref_path);
> [...]
> 
> This function is racy. A reader might see no reference at all in the
> moment between the `unlink()` and the `symlink()`. Moreover, if this
> process is killed at that moment, the symbolic ref would be gone forever.
> 
> I think that the semantics of `rename()` would allow this race to be
> fixed, though, since `symlink()` doesn't have the analogue of
> `O_CREAT|O_EXCL`, one would need a lockfile *and* a second temporary
> filename under which the new symlink is originally created.
> 
> However, this race has always been here, and symlink-based symrefs are
> obsolete, so it's probably not worth fixing.

Yeah. In the commit message I wrote:

> >  - the legacy prefer_symlink_refs path did not do any
> >    locking at all. Admittedly, it is not atomic from a
> >    reader's perspective (and it cannot be; it has to unlink
> >    and then symlink, creating a race), but at least it
> >    cannot conflict with other writers now.

but I think you are right that we could do tricks with rename() on the
symlink. That is in POSIX, though I wouldn't be surprised if it
misbehaves on some obscure systems. Given that using symlinks is only
triggered by an undocumented (!) option that presumably very few people
use, I'm inclined to leave it as-is.

I was actually tempted to rip it out, as the option is mostly a hack
from 2006:

  http://article.gmane.org/gmane.comp.version-control.git/19402

It was done to let people bisect old kernel trees whose build system
_depends_ on .git/HEAD being a symlink. My thought is:

  - people are a lot less likely to bisect back that far anymore

  - ...and if they do, their build system will be totally broken by the
    new ref-backend stuff

  - ...and the right solution is not to put a hack in git, but for the
    bisecting user to "fix up" the tree before testing (by munging the
    symlink themselves, or applying a patch to the build system to use
    `git rev-parse`). This matches what people have to do for every
    other type of weird "the old version doesn't quite build"
    incompatibility that has nothing to do with git.

But that should probably come as a separate patch (it affects this only
in that it makes this patch simpler to do that cleanup first :) ).

> > +	if (adjust_shared_perm(lock->lk->tempfile.filename.buf))
> > +		return error("unable to fix permissions on %s: %s",
> > +			     lock->lk->tempfile.filename.buf, strerror(errno));
> 
> You can skip this step. lock_file() already calls adjust_shared_perm().

Thanks, fixed.

> > +	/* no error check; commit_ref will check ferror */
> > +	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
> > +	if (commit_ref(lock) < 0)
> > +		return error("unable to write symref for %s: %s", ref,
> > +			     strerror(errno));
> > +	update_symref_reflog(lock, ref, target, logmsg);
> 
> Here is another problem that didn't originate with your changes:
> 
> The reflog should be written while holding the reference lock, to
> prevent two processes' trying to write new entries at the same time.
> 
> I think the problem would be solved if you move the call to
> update_symref_reflog() above the call to commit_ref().
> 
> Granted, this could case a reflog entry to be written for a reference
> update whose commit fails, but that's also a risk for non-symbolic
> references. Fixing this residual problem would require the ability to
> roll back reflog changes.

Thanks, I agree with your reasoning (and especially that we should err
on the same side that normal ref-writing does). I'll do it in a
follow-on patch, though, to make it more clear what is going on.

-Peff