commit 2f17228ee55ecd8c69a9d0dac94841c7894d0d6d from: Stefan Sperling date: Sun May 12 01:18:39 2019 UTC lock branch reference file during 'got commit' to prevent a race 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);