Commit Diff

Commit:
2f17228ee55ecd8c69a9d0dac94841c7894d0d6d
From:
Stefan Sperling <stsp@stsp.name>
Date:
Message:
lock branch reference file during 'got commit' to prevent a race
Actions:
Patch | Tree
commit - 03df25b334950c4fa76dc9a86b3df050861298ce
commit + 2f17228ee55ecd8c69a9d0dac94841c7894d0d6d
blob - 5d5cf0446f8b605dd7cd8ba1001a12d2aa7e0a83
blob + bc14b937b2e2688bef36d04f7659e90265482817
--- got/got.c
+++ got/got.c
@@ -261,7 +261,7 @@ check_ancestry(struct got_worktree *worktree, struct g
 	struct got_commit_graph *graph = NULL;
 
 	err = got_ref_open(&head_ref, repo,
-	    got_worktree_get_head_ref_name(worktree));
+	    got_worktree_get_head_ref_name(worktree), 0);
 	if (err)
 		return err;
 
@@ -407,7 +407,7 @@ cmd_checkout(int argc, char *argv[])
 	if (error)
 		goto done;
 
-	error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (error != NULL)
 		goto done;
 
@@ -547,7 +547,7 @@ cmd_update(int argc, char *argv[])
 
 	if (commit_id_str == NULL) {
 		struct got_reference *head_ref;
-		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 		if (error != NULL)
 			goto done;
 		error = got_ref_resolve(&commit_id, repo, head_ref);
@@ -919,7 +919,7 @@ cmd_log(int argc, char *argv[])
 
 	if (start_commit == NULL) {
 		struct got_reference *head_ref;
-		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 		if (error != NULL)
 			return error;
 		error = got_ref_resolve(&id, repo, head_ref);
@@ -929,7 +929,7 @@ cmd_log(int argc, char *argv[])
 		error = got_object_open_as_commit(&commit, repo, id);
 	} else {
 		struct got_reference *ref;
-		error = got_ref_open(&ref, repo, start_commit);
+		error = got_ref_open(&ref, repo, start_commit, 0);
 		if (error == NULL) {
 			int obj_type;
 			error = got_ref_resolve(&id, repo, ref);
@@ -1360,7 +1360,7 @@ cmd_blame(int argc, char *argv[])
 
 	if (commit_id_str == NULL) {
 		struct got_reference *head_ref;
-		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 		if (error != NULL)
 			goto done;
 		error = got_ref_resolve(&commit_id, repo, head_ref);
@@ -1592,7 +1592,7 @@ cmd_tree(int argc, char *argv[])
 
 	if (commit_id_str == NULL) {
 		struct got_reference *head_ref;
-		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 		if (error != NULL)
 			goto done;
 		error = got_ref_resolve(&commit_id, repo, head_ref);
@@ -1744,7 +1744,7 @@ delete_ref(struct got_repository *repo, const char *re
 	const struct got_error *err = NULL;
 	struct got_reference *ref;
 
-	err = got_ref_open(&ref, repo, refname);
+	err = got_ref_open(&ref, repo, refname, 0);
 	if (err)
 		return err;
 
blob - 5c3eed9a3a8b0f5dbc490b7a2cfeccd2fd6f744e
blob + 019df06a6d16642bb74799bcec1df6d150cd3e99
--- include/got_reference.h
+++ include/got_reference.h
@@ -28,10 +28,13 @@ struct got_object_id;
 
 /*
  * Attempt to open the reference with the provided name in a repository.
- * The caller must dispose of it with got_ref_close().
+ * The caller must dispose of the reference with got_ref_close().
+ * Optionally, the underlying reference file can be locked before it is opened
+ * to prevent concurrent modification of the reference, in which case the file
+ * must be unlocked with got_ref_unlock() before got_ref_close() is called.
  */
 const struct got_error *got_ref_open(struct got_reference **,
-    struct got_repository *, const char *);
+    struct got_repository *, const char *, int);
 
 /*
  * Allocate a new reference for a given object ID.
@@ -96,3 +99,6 @@ const struct got_error *got_ref_write(struct got_refer
 /* Delete a reference from its on-disk path in the repository. */
 const struct got_error *got_ref_delete(struct got_reference *,
     struct got_repository *);
+
+/* Unlock a reference which was opened in locked state. */
+const struct got_error *got_ref_unlock(struct got_reference *);
blob - d1b38a65bfd2d837c561df0239d29aab9207b914
blob + 953af3c70ff0826f5cf8d91f4bdeac05b7ad5293
--- lib/reference.c
+++ lib/reference.c
@@ -80,6 +80,8 @@ struct got_reference {
 		struct got_ref ref;
 		struct got_symref symref;
 	} ref;
+
+	struct got_lockfile *lf;
 };
 
 static const struct got_error *
@@ -156,14 +158,21 @@ parse_ref_line(struct got_reference **ref, const char 
 
 static const struct got_error *
 parse_ref_file(struct got_reference **ref, const char *name,
-    const char *abspath)
+    const char *abspath, int lock)
 {
 	const struct got_error *err = NULL;
 	FILE *f;
 	char *line;
 	size_t len;
 	const char delim[3] = {'\0', '\0', '\0'};
+	struct got_lockfile *lf = NULL;
 
+	if (lock) {
+		err = got_lockfile_lock(&lf, abspath);
+		if (err)
+			return (err);
+	}
+
 	f = fopen(abspath, "rb");
 	if (f == NULL)
 		return NULL;
@@ -175,10 +184,19 @@ parse_ref_file(struct got_reference **ref, const char 
 	}
 
 	err = parse_ref_line(ref, name, line);
+	if (err == NULL && lf)
+		(*ref)->lf = lf;
 done:
 	free(line);
-	if (fclose(f) != 0 && err == NULL)
+	if (fclose(f) != 0 && err == NULL) {
 		err = got_error_prefix_errno("fclose");
+		if (lf)
+			got_lockfile_unlock(lf);
+		if (*ref) {
+			got_ref_close(*ref);
+			*ref = NULL;
+		}
+	}
 	return err;
 }
 
@@ -334,7 +352,7 @@ open_packed_ref(struct got_reference **ref, FILE *f, c
 
 static const struct got_error *
 open_ref(struct got_reference **ref, const char *path_refs, const char *subdir,
-    const char *name)
+    const char *name, int lock)
 {
 	const struct got_error *err = NULL;
 	char *path = NULL;
@@ -367,7 +385,7 @@ open_ref(struct got_reference **ref, const char *path_
 		goto done;
 	}
 
-	err = parse_ref_file(ref, absname, normpath);
+	err = parse_ref_file(ref, absname, normpath, lock);
 done:
 	if (!ref_is_absolute && !ref_is_well_known)
 		free(absname);
@@ -378,7 +396,7 @@ done:
 
 const struct got_error *
 got_ref_open(struct got_reference **ref, struct got_repository *repo,
-   const char *refname)
+   const char *refname, int lock)
 {
 	const struct got_error *err = NULL;
 	char *path_refs = NULL;
@@ -401,7 +419,8 @@ got_ref_open(struct got_reference **ref, struct got_re
 
 		/* Search on-disk refs before packed refs! */
 		for (i = 0; i < nitems(subdirs); i++) {
-			err = open_ref(ref, path_refs, subdirs[i], refname);
+			err = open_ref(ref, path_refs, subdirs[i], refname,
+			    lock);
 			if (err || *ref)
 				goto done;
 		}
@@ -413,6 +432,11 @@ got_ref_open(struct got_reference **ref, struct got_re
 			goto done;
 		}
 
+		if (lock) {
+			err = got_lockfile_lock(&(*ref)->lf, packed_refs_path);
+			if (err)
+				goto done;
+		}
 		f = fopen(packed_refs_path, "rb");
 		free(packed_refs_path);
 		if (f != NULL) {
@@ -425,7 +449,7 @@ got_ref_open(struct got_reference **ref, struct got_re
 		}
 	}
 
-	err = open_ref(ref, path_refs, "", refname);
+	err = open_ref(ref, path_refs, "", refname, lock);
 	if (err)
 		goto done;
 done:
@@ -488,7 +512,7 @@ resolve_symbolic_ref(struct got_reference **resolved,
 	struct got_reference *nextref;
 	const struct got_error *err;
 
-	err = got_ref_open(&nextref, repo, ref->ref.symref.ref);
+	err = got_ref_open(&nextref, repo, ref->ref.symref.ref, 0);
 	if (err)
 		return err;
 
@@ -640,7 +664,8 @@ gather_on_disk_refs(struct got_reflist_head *refs, con
 
 		switch (dent->d_type) {
 		case DT_REG:
-			err = open_ref(&ref, path_refs, subdir, dent->d_name);
+			err = open_ref(&ref, path_refs, subdir, dent->d_name,
+			    0);
 			if (err)
 				goto done;
 			if (ref) {
@@ -687,7 +712,7 @@ got_ref_list(struct got_reflist_head *refs, struct got
 		err = got_error_prefix_errno("get_refs_dir_path");
 		goto done;
 	}
-	err = open_ref(&ref, path_refs, "", GOT_REF_HEAD);
+	err = open_ref(&ref, path_refs, "", GOT_REF_HEAD, 0);
 	if (err)
 		goto done;
 	err = insert_ref(&new, refs, ref, repo);
@@ -858,9 +883,11 @@ got_ref_write(struct got_reference *ref, struct got_re
 		}
 	}
 
-	err = got_lockfile_lock(&lf, path);
-	if (err)
-		goto done;
+	if (ref->lf == NULL) {
+		err = got_lockfile_lock(&lf, path);
+		if (err)
+			goto done;
+	}
 
 	/* XXX: check if old content matches our expectations? */
 
@@ -884,7 +911,7 @@ got_ref_write(struct got_reference *ref, struct got_re
 		goto done;
 	}
 done:
-	if (lf)
+	if (ref->lf == NULL && lf)
 		unlock_err = got_lockfile_unlock(lf);
 	if (f) {
 		if (fclose(f) != 0 && err == NULL)
@@ -924,9 +951,11 @@ delete_packed_ref(struct got_reference *delref, struct
 	if (err)
 		goto done;
 
-	err = got_lockfile_lock(&lf, packed_refs_path);
-	if (err)
-		goto done;
+	if (delref->lf == NULL) {
+		err = got_lockfile_lock(&lf, packed_refs_path);
+		if (err)
+			goto done;
+	}
 
 	f = fopen(packed_refs_path, "r");
 	if (f == NULL) {
@@ -1027,7 +1056,7 @@ delete_packed_ref(struct got_reference *delref, struct
 		}
 	}
 done:
-	if (lf)
+	if (delref->lf == NULL && lf)
 		unlock_err = got_lockfile_unlock(lf);
 	if (f) {
 		if (fclose(f) != 0 && err == NULL)
@@ -1066,19 +1095,30 @@ got_ref_delete(struct got_reference *ref, struct got_r
 		goto done;
 	}
 
-	err = got_lockfile_lock(&lf, path);
-	if (err)
-		goto done;
+	if (ref->lf == NULL) {
+		err = got_lockfile_lock(&lf, path);
+		if (err)
+			goto done;
+	}
 
 	/* XXX: check if old content matches our expectations? */
 
 	if (unlink(path) != 0)
 		err = got_error_prefix_errno2("unlink", path);
 done:
-	if (lf)
+	if (ref->lf == NULL && lf)
 		unlock_err = got_lockfile_unlock(lf);
 
 	free(path_refs);
 	free(path);
 	return err ? err : unlock_err;
 }
+
+const struct got_error *
+got_ref_unlock(struct got_reference *ref)
+{
+	const struct got_error *err;
+	err = got_lockfile_unlock(ref->lf);
+	ref->lf = NULL;
+	return err;
+}
blob - bb149215b22b865bfb3a53e4acaa5b5083b9a841
blob + ef82dcbdfbbb231f2e5911319eb6ff7952e6276d
--- lib/repository.c
+++ lib/repository.c
@@ -160,7 +160,7 @@ is_git_repo(struct got_repository *repo)
 		goto done;
 
 	/* Check if the HEAD reference can be opened. */
-	if (got_ref_open(&head_ref, repo, GOT_REF_HEAD) != NULL)
+	if (got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0) != NULL)
 		goto done;
 	got_ref_close(head_ref);
 
blob - da771a2cb37e40dc3f8acea8b76c2e5079339067
blob + ea806c36531cbc279e1c607c5ff3a998ccf0ac9a
--- lib/worktree.c
+++ lib/worktree.c
@@ -2854,7 +2854,7 @@ got_worktree_commit(struct got_object_id **new_commit_
 	if (err)
 		goto done;
 
-	err = got_ref_open(&head_ref, repo, worktree->head_ref_name);
+	err = got_ref_open(&head_ref, repo, worktree->head_ref_name, 0);
 	if (err)
 		goto done;
 	err = got_ref_resolve(&head_commit_id, repo, head_ref);
@@ -2923,13 +2923,13 @@ got_worktree_commit(struct got_object_id **new_commit_
 		goto done;
 
 	/* Check if a concurrent commit to our branch has occurred. */
-	/* XXX ideally we'd lock the reference file here to avoid a race */
 	head_ref_name = got_worktree_get_head_ref_name(worktree);
 	if (head_ref_name == NULL) {
 		err = got_error_prefix_errno("got_worktree_get_head_ref_name");
 		goto done;
 	}
-	err = got_ref_open(&head_ref2, repo, head_ref_name);
+	/* Lock the reference here to prevent concurrent modification. */
+	err = got_ref_open(&head_ref2, repo, head_ref_name, 1);
 	if (err)
 		goto done;
 	err = got_ref_resolve(&head_commit_id2, repo, head_ref2);
@@ -2940,13 +2940,12 @@ got_worktree_commit(struct got_object_id **new_commit_
 		goto done;
 	}
 	/* Update branch head in repository. */
-	err = got_ref_change_ref(head_ref, *new_commit_id);
+	err = got_ref_change_ref(head_ref2, *new_commit_id);
 	if (err)
 		goto done;
-	err = got_ref_write(head_ref, repo);
+	err = got_ref_write(head_ref2, repo);
 	if (err)
 		goto done;
-	/* XXX race has ended here */
 
 	err = got_worktree_set_base_commit_id(worktree, repo, *new_commit_id);
 	if (err)
@@ -2978,7 +2977,11 @@ done:
 	free(head_commit_id2);
 	if (head_ref)
 		got_ref_close(head_ref);
-	if (head_ref2)
+	if (head_ref2) {
+		unlockerr = got_ref_unlock(head_ref2);
+		if (unlockerr && err == NULL)
+			err = unlockerr;
 		got_ref_close(head_ref2);
+	}
 	return err;
 }
blob - 780e093cbeaf08580d0021776e1a2cdfe7e8d209
blob + 1c8251f97cb80f83b6ae8a4939b6cee9122d311d
--- regress/repository/repository_test.c
+++ regress/repository/repository_test.c
@@ -187,7 +187,7 @@ repo_read_log(const char *repo_path)
 	err = got_repo_open(&repo, repo_path);
 	if (err != NULL || repo == NULL)
 		return 0;
-	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (err != NULL || head_ref == NULL)
 		return 0;
 	err = got_ref_resolve(&id, repo, head_ref);
blob - df3b5beb37efbdff4ef25f786d68a5b0f0f2663a
blob + 316594959a4c54ced16b129f9a2a60abedc9cd5e
--- regress/worktree/worktree_test.c
+++ regress/worktree/worktree_test.c
@@ -99,7 +99,7 @@ remove_worktree_base_ref(struct got_worktree *worktree
 		err = got_error_prefix_errno("asprintf");
 		goto done;
 	}
-	err = got_ref_open(&base_ref, repo, absrefname);
+	err = got_ref_open(&base_ref, repo, absrefname, 0);
 	if (err)
 		goto done;
 
@@ -194,7 +194,7 @@ worktree_init(const char *repo_path)
 	err = got_repo_open(&repo, repo_path);
 	if (err != NULL || repo == NULL)
 		goto done;
-	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (err != NULL || head_ref == NULL)
 		goto done;
 
@@ -271,7 +271,7 @@ obstruct_meta_file_and_init(int *ok, struct got_reposi
 	if (!obstruct_meta_file(&path, worktree_path, GOT_WORKTREE_FILE_INDEX))
 		return 0;
 
-	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (err != NULL || head_ref == NULL)
 		return 0;
 
@@ -362,7 +362,7 @@ worktree_checkout(const char *repo_path)
 	err = got_repo_open(&repo, repo_path);
 	if (err != NULL || repo == NULL)
 		goto done;
-	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (err != NULL || head_ref == NULL)
 		goto done;
 
blob - c3da2f33949c3eb4e55a511d97ffeb286484f524
blob + 6513988c9c729deec721e38787c70ba0b83af0f8
--- tog/tog.c
+++ tog/tog.c
@@ -1123,7 +1123,7 @@ get_head_commit_id(struct got_object_id **head_id, str
 
 	*head_id = NULL;
 
-	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+	err = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 	if (err)
 		return err;
 
@@ -3285,7 +3285,7 @@ cmd_blame(int argc, char *argv[])
 
 	if (commit_id_str == NULL) {
 		struct got_reference *head_ref;
-		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD);
+		error = got_ref_open(&head_ref, repo, GOT_REF_HEAD, 0);
 		if (error != NULL)
 			goto done;
 		error = got_ref_resolve(&commit_id, repo, head_ref);