@matheusbsbs / matheus-bagliano / commits / df60cf5789

read-cache: leave lock in right state in `write_locked_index()`

If the original version of `write_locked_index()` returned with an error, it didn't roll back the lockfile unless the error occured at the very end, during closing/committing. See commit 03b866477 (read-cache: new API write_locked_index instead of write_index/write_cache, 2014-06-13). In commit 9f41c7a6b (read-cache: close index.lock in do_write_index, 2017-04-26), we learned to close the lock slightly earlier in the callstack. That was mostly a side-effect of lockfiles being implemented using temporary files, but didn't cause any real harm. Recently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05) introduced a subtle bug. If the temporary file is deleted (i.e., the lockfile is rolled back), the tempfile-pointer in the `struct lock_file` will be left dangling. Thus, an attempt to reuse the lockfile, or even just to roll it back, will induce undefined behavior -- most likely a crash. Besides not crashing, we clearly want to make things consistent. The guarantees which the lockfile-machinery itself provides is A) if we ask to commit and it fails, roll back, and B) if we ask to close and it fails, do _not_ roll back. Let's do the same for consistency. Do not delete the temporary file in `do_write_index()`. One of its callers, `write_locked_index()` will thereby avoid rolling back the lock. The other caller, `write_shared_index()`, will delete its temporary file anyway. Both of these callers will avoid undefined behavior (crashing). 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()`. That should keep future readers from wondering why the callers are inconsistent. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Martin Ågren committed Oct 6, 2017 at 22:12 UTC df60cf5789782191b092169f86255aa44525b7d1

5 files changed +13 -11

builtin/difftool.c

-1

@@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,

616 if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||

617 write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {

618 ret = error("could not write %s", buf.buf);

619 - rollback_lock_file(&lock);

619 goto finish;

620 }

621 changed_files(&wt_modified, buf.buf, workdir);

cache.h

@@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *);

616 * split index to the lockfile. If the temporary file for the shared

617 * index cannot be created, fall back to the behavior described in

618 * the previous paragraph.

619 + *

620 + * With `COMMIT_LOCK`, the lock is always committed or rolled back.

621 + * Without it, the lock is closed, but neither committed nor rolled

622 + * back.

623 */

624 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);

625

merge.c

+1 -3

@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,

91 }

92 if (unpack_trees(nr_trees, t, &opts))

93 return -1;

94 - if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {

95 - rollback_lock_file(&lock_file);

94 + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))

95 return error(_("unable to write new index file"));

97 - }

96 return 0;

97 }

read-cache.c

+8 -6

@@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate)

2182 void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)

2183 {

2184 if ((istate->cache_changed || has_racy_timestamp(istate)) &&

2185 - verify_index(istate) &&

2186 - write_locked_index(istate, lockfile, COMMIT_LOCK))

2187 - rollback_lock_file(lockfile);

2185 + verify_index(istate))

2186 + write_locked_index(istate, lockfile, COMMIT_LOCK);

2187 }

2188

2189 /*

@@ -2321,7 +2320,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2320 return -1;

2321 if (close_tempfile_gently(tempfile)) {

2322 error(_("could not close '%s'"), tempfile->filename.buf);

2324 - delete_tempfile(&tempfile);

2323 return -1;

2324 }

2325 if (stat(tempfile->filename.buf, &st))

@@ -2501,7 +2499,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,

2499 (istate->cache_changed & ~EXTMASK)) {

2500 if (si)

2501 hashclr(si->base_sha1);

2504 - return do_write_locked_index(istate, lock, flags);

2502 + ret = do_write_locked_index(istate, lock, flags);

2503 + goto out;

2504 }

2505

2506 if (getenv("GIT_TEST_SPLIT_INDEX")) {

@@ -2517,7 +2516,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,

2516 if (new_shared_index) {

2517 ret = write_shared_index(istate, lock, flags);

2518 if (ret)

2520 - return ret;

2519 + goto out;

2520 }

2521

2522 ret = write_split_index(istate, lock, flags);

@@ -2526,6 +2525,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,

2525 if (!ret && !new_shared_index)

2526 freshen_shared_index(sha1_to_hex(si->base_sha1), 1);

2527

2528 +out:

2529 + if (flags & COMMIT_LOCK)

2530 + rollback_lock_file(lock);

2531 return ret;

2532 }

2533

sequencer.c

-1

@@ -1183,7 +1183,6 @@ static int read_and_refresh_cache(struct replay_opts *opts)

1183 refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);

1184 if (the_index.cache_changed && index_fd >= 0) {

1185 if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {

1186 - rollback_lock_file(&index_lock);

1186 return error(_("git %s: failed to refresh the index"),

1187 _(action_name(opts)));

1188 }