On 12/20/2015 08:34 AM, Jeff King wrote:

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