From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jeff King <peff@peff.net>, git@vger.kernel.org
Subject: Re: [PATCH 4/4] create_symref: use existing ref-lock code
Date: Mon, 28 Dec 2015 10:45:19 +0100 [thread overview]
Message-ID: <568104AF.102@alum.mit.edu> (raw)
In-Reply-To: <20151220073414.GD30662@sigill.intra.peff.net>
On 12/20/2015 08:34 AM, Jeff King wrote:
> The create_symref() function predates the existence of
> "struct lock_file", let alone the more recent "struct
> ref_lock". Instead, it just does its own manual dot-locking.
> Besides being more code, this has a few downsides:
>
> - if git is interrupted while holding the lock, we don't
> clean up the lockfile
>
> - we don't do the usual directory/filename conflict check.
> So you can sometimes create a symref "refs/heads/foo/bar",
> even if "refs/heads/foo" exists (namely, if the refs are
> packed and we do not hit the d/f conflict in the
> filesystem).
>
> This patch refactors create_symref() to use the "struct
> ref_lock" interface, which handles both of these things.
> There are a few bonus cleanups that come along with it:
>
> - we leaked ref_path in some error cases
>
> - the symref contents were stored in a fixed-size buffer,
> putting an artificial (albeit large) limitation on the
> length of the refname. We now write through fprintf, and
> handle refnames of any size.
>
> - we called adjust_shared_perm only after the file was
> renamed into place, creating a potential race with
> readers in a shared repository. Now we fix the
> permissions first, and commit only if that succeeded.
> This also makes the update atomic with respect to our
> exit code (whereas previously, we might report failure
> even though we updated the ref).
>
> - 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.
>
> - the result of this patch is hopefully more readable. It
> eliminates three goto labels. Two were for error checking
> that is now simplified, and the third was to reach shared
> code that has been pulled into its own function.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> refs/files-backend.c | 113 +++++++++++++++++++++++++-----------------------
> t/t1401-symbolic-ref.sh | 8 ++++
> 2 files changed, 66 insertions(+), 55 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6bfa139..3d53c42 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2811,74 +2811,77 @@ static int commit_ref_update(struct ref_lock *lock,
> return 0;
> }
>
> -int create_symref(const char *ref, const char *target, const char *logmsg)
> +static int create_ref_symlink(struct ref_lock *lock, const char *target)
> {
> - char *lockpath = NULL;
> - char buf[1000];
> - int fd, len, written;
> - char *ref_path = git_pathdup("%s", ref);
> - unsigned char old_sha1[20], new_sha1[20];
> - struct strbuf err = STRBUF_INIT;
> -
> - if (logmsg && read_ref(ref, old_sha1))
> - hashclr(old_sha1);
> -
> - if (safe_create_leading_directories(ref_path) < 0)
> - return error("unable to create directory for %s", ref_path);
> -
> + int ret = -1;
> #ifndef NO_SYMLINK_HEAD
> - if (prefer_symlink_refs) {
> - unlink(ref_path);
> - if (!symlink(target, ref_path))
> - goto done;
> + char *ref_path = get_locked_file_path(lock->lk);
> + unlink(ref_path);
> + ret = symlink(target, ref_path);
> + free(ref_path);
> +
> + if (ret)
> fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> - }
> #endif
> + return ret;
> +}
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.
> - len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
> - if (sizeof(buf) <= len) {
> - error("refname too long: %s", target);
> - goto error_free_return;
> - }
> - lockpath = mkpathdup("%s.lock", ref_path);
> - fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
> - if (fd < 0) {
> - error("Unable to open %s for writing", lockpath);
> - goto error_free_return;
> - }
> - written = write_in_full(fd, buf, len);
> - if (close(fd) != 0 || written != len) {
> - error("Unable to write to %s", lockpath);
> - goto error_unlink_return;
> - }
> - if (rename(lockpath, ref_path) < 0) {
> - error("Unable to create %s", ref_path);
> - goto error_unlink_return;
> - }
> - if (adjust_shared_perm(ref_path)) {
> - error("Unable to fix permissions on %s", lockpath);
> - error_unlink_return:
> - unlink_or_warn(lockpath);
> - error_free_return:
> - free(lockpath);
> - free(ref_path);
> - return -1;
> - }
> - free(lockpath);
> -
> -#ifndef NO_SYMLINK_HEAD
> - done:
> -#endif
> +static void update_symref_reflog(struct ref_lock *lock, const char *ref,
> + const char *target, const char *logmsg)
> +{
> + struct strbuf err = STRBUF_INIT;
> + unsigned char new_sha1[20];
> if (logmsg && !read_ref(target, new_sha1) &&
> - log_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {
> + log_ref_write(ref, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
> error("%s", err.buf);
> strbuf_release(&err);
> }
> +}
>
> - free(ref_path);
> +static int create_symref_locked(struct ref_lock *lock, const char *ref,
> + const char *target, const char *logmsg)
> +{
> + if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
> + update_symref_reflog(lock, ref, target, logmsg);
> + return 0;
> + }
> +
> + if (!fdopen_lock_file(lock->lk, "w"))
> + return error("unable to fdopen %s: %s",
> + lock->lk->tempfile.filename.buf, strerror(errno));
> +
> + 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().
> + /* 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.
> return 0;
> }
> [...]
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
next prev parent reply other threads:[~2015-12-28 9:52 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-20 7:26 [PATCH 0/4] improve symbolic-ref robustness Jeff King
2015-12-20 7:27 ` [PATCH 1/4] symbolic-ref: propagate error code from create_symref() Jeff King
2015-12-20 7:27 ` [PATCH 2/4] t1401: test reflog creation for git-symbolic-ref Jeff King
2015-12-20 7:29 ` [PATCH 3/4] create_symref: modernize variable names Jeff King
2015-12-28 8:20 ` Michael Haggerty
2015-12-29 5:02 ` Jeff King
2015-12-20 7:34 ` [PATCH 4/4] create_symref: use existing ref-lock code Jeff King
2015-12-21 20:50 ` Junio C Hamano
2015-12-22 0:58 ` Jeff King
2015-12-28 9:45 ` Michael Haggerty [this message]
2015-12-29 5:02 ` Jeff King
2015-12-29 5:41 ` Jeff King
2015-12-29 5:55 ` [PATCH v2 0/3] improve symbolic-ref robustness Jeff King
2015-12-29 5:56 ` [PATCH 1/3] create_symref: modernize variable names Jeff King
2015-12-29 5:57 ` [PATCH 2/3] create_symref: use existing ref-lock code Jeff King
2015-12-29 5:57 ` [PATCH 3/3] create_symref: write reflog while holding lock Jeff King
2015-12-29 6:00 ` [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links Jeff King
2015-12-29 6:03 ` Jeff King
2015-12-29 18:32 ` Junio C Hamano
2015-12-30 6:53 ` Jeff King
2015-12-30 6:56 ` Jeff King
2015-12-29 8:25 ` [PATCH v2 0/3] improve symbolic-ref robustness Michael Haggerty
2015-12-29 18:35 ` Junio C Hamano
2015-12-29 21:24 ` Junio C Hamano
2015-12-30 6:57 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=568104AF.102@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).