[PATCH 11/11] read-cache: roll back lock on error with `COMMIT_LOCK`
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Oct 1, 2017, 14:56 UTC
- Message-ID
- <26a9c90a6478ec9ddb4ce34923e251ebe9323ab2.1506862824.git.martin.agren@gmail.com>
- In-Reply-To
- <cover.1506862824.git.martin.agren@gmail.com>
When `write_locked_index(..., lock, COMMIT_LOCK)` returns, the lockfile might have been neither committed nor rolled back. This happens if the call to `do_write_index()` early in `do_write_locked_index()` fails, or if `write_shared_index()` fails.
Some callers obviously do not expect the lock to remain locked. They simply kick an error up the call stack without rolling back the lock. They typically have the lock on the stack, so there is now no way of releasing it. (Some callers actually do roll back the lock, and many others simply do not care because they are about to die anyway.) Maybe we don't try to take the lock again within the same process, so this is not a problem at the time, but if we ever learn to try, we could deadlock.
We could teach existing callers to roll back where it matters. That would introduce some boiler-plate code, and future users might re-introduce the same mistake. After all, it does seem reasonable to expect that `COMMIT_LOCK` does what `commit_lockfile()` does: commits or rolls back.
Teach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock before returning. If we have already succeeded and committed, it will be a noop. Simplify the existing callers where we now have a superfluous call to `rollback_lockfile()`. This should keep future readers from wondering why the callers are inconsistent.
For the users which `die()` if `write_locked_index()` fails, this change does not bring any benefit. But it also doesn't hurt -- it simply means we clean up the lockfile in `do_write_locked_index()` instead of in our custom `atexit(3)` handler.
Out-of-tree callers will be affected by this change, but they will be getting the locking-behavior that they almost certainly expect.
For the other flag, `CLOSE_LOCK`, leaving the lock as-is on error is appropriate, since that is how `close_lockfile_gently()` behaves. (There are not many in-tree users and they all `die()` on error.)
Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- builtin/difftool.c | 1 - cache.h | 1 + merge.c | 4 +--- read-cache.c | 13 ++++++++----- sequencer.c | 1 - 5 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c index b2d3ba753..bcc79d188 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 || write_locked_index(&wtindex, &lock, COMMIT_LOCK)) { ret = error("could not write %s", buf.buf); - rollback_lock_file(&lock); goto finish; } changed_files(&wt_modified, buf.buf, workdir); diff --git a/cache.h b/cache.h index 4d09da792..0da90a545 100644 --- a/cache.h +++ b/cache.h @@ -618,6 +618,7 @@ extern int read_index_unmerged(struct index_state *); * split index to the lockfile. If the temporary file for the shared * index cannot be created, fall back to the normal case. * + * With `COMMIT_LOCK`, the lock is always committed or rolled back. * With `CLOSE_LOCK`, the lock will be neither committed nor rolled back. */ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); diff --git a/merge.c b/merge.c index a18a452b5..e5d796c9f 100644 --- a/merge.c +++ b/merge.c @@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head, } if (unpack_trees(nr_trees, t, &opts)) return -1; - if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { - rollback_lock_file(&lock_file); + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) return error(_("unable to write new index file")); - } return 0; } diff --git a/read-cache.c b/read-cache.c index 8e498e879..53f1941c9 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate) void update_index_if_able(struct index_state *istate, struct lock_file *lockfile) { if ((istate->cache_changed || has_racy_timestamp(istate)) && - verify_index(istate) && - write_locked_index(istate, lockfile, COMMIT_LOCK)) - rollback_lock_file(lockfile); + verify_index(istate)) + write_locked_index(istate, lockfile, COMMIT_LOCK); } static int do_write_index(struct index_state *istate, struct tempfile *tempfile, @@ -2498,7 +2497,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, (istate->cache_changed & ~EXTMASK)) { if (si) hashclr(si->base_sha1); - return do_write_locked_index(istate, lock, flags); + ret = do_write_locked_index(istate, lock, flags); + goto out; } if (getenv("GIT_TEST_SPLIT_INDEX")) { @@ -2514,7 +2514,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, if (new_shared_index) { ret = write_shared_index(istate, lock, flags); if (ret) - return ret; + goto out; } ret = write_split_index(istate, lock, flags); @@ -2523,6 +2523,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, if (!ret && !new_shared_index) freshen_shared_index(sha1_to_hex(si->base_sha1), 1); +out: + if (flags & COMMIT_LOCK) + rollback_lock_file(lock); return ret; } diff --git a/sequencer.c b/sequencer.c index 60636ce54..d56c38081 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts) refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed && index_fd >= 0) { if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) { - rollback_lock_file(&index_lock); return error(_("git %s: failed to refresh the index"), _(action_name(opts))); }
-- 2.14.1.727.g9ddaf86