From: Linus Torvalds Date: Mon, 26 Sep 2005 01:07:37 GMT Subject: Re: Add "git-update-ref" to update the HEAD (or other) ref Message-ID: In-Reply-To: <7vhdc8n2xb.fsf@assigned-by-dhcp.cox.net> On Sun, 25 Sep 2005, Junio C Hamano wrote: > > Is something like the one at the end acceptable? Looking at the patch closer, no, that's incorrect. "oldsha" doesn't necessarily exist, since there has to be some way to force the new one. So if "oldval" is NULL, we shouldn't re-verify anything. Also, independently of that your patch is buggy because calling "resolve_ref()" again will overwrite the lockpath, since it's re-used by the static buffer in git_path(). That's why the "strdup()" is there. Yeah, yeah, static buffers are evil, but they are also simple and efficient. But something like this (on top of my original one) might work. Linus ---- diff --git a/update-ref.c b/update-ref.c --- a/update-ref.c +++ b/update-ref.c @@ -63,6 +63,19 @@ const char *resolve_ref(const char *path return path; } +static int re_verify(const char *path, unsigned char *oldsha1, unsigned char *currsha1) +{ + char buf[40]; + int fd = open(path, O_RDONLY), nr; + if (fd < 0) + return -1; + nr = read(fd, buf, 40); + close(fd); + if (nr != 40 || get_sha1_hex(buf, currsha1) < 0) + return -1; + return memcmp(oldsha1, currsha1, 20) ? -1 : 0; +} + int main(int argc, char **argv) { char *hex; @@ -108,14 +121,18 @@ int main(int argc, char **argv) unlink(lockpath); die("Unable to write to %s", lockpath); } - + /* - * FIXME! - * - * We should re-read the old ref here, and re-verify that it - * matches "oldsha1". Otherwise there's a small race. + * Re-read the ref after getting the lock to verify */ + if (oldval && re_verify(path, oldsha1, currsha1) < 0) { + unlink(lockpath); + die("Ref lock failed"); + } + /* + * Finally, replace the old ref with the new one + */ if (rename(lockpath, path) < 0) { unlink(lockpath); die("Unable to create %s", path);