[PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`

["Martin Ågren" <martin.agren@gmail.com>]

If `do_write_index(..., struct tempfile *, ...)` fails to close the
temporary file, it deletes it. This resets the pointer to NULL, but only
the pointer which is local to `do_write_index()`, not the pointer that
the caller holds. If the caller ever dereferences its pointer, we have
undefined behavior (most likely a crash). One of the two callers tries
to delete the temporary file on error, so it will dereference it.

We could change the function to take a `struct tempfile **` instead. But
we have another problem here. The other caller, `write_locked_index()`,
passes in `lock->tempfile`. So if we close the temporary file and reset
`lock->tempfile` to NULL, we have effectively rolled back the lock. That
caller is `write_locked_index()` and if it is used with the
`CLOSE_LOCK`-file, it means the lock is being rolled back against the
wishes of the caller. (`write_locked_index()` used to call
`close_lockfile()`, which would have rolled back on error. Commit
83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05)
changed to not rolling back.)

Note also that if we would ever have failed to close the lockfile,
either with `COMMIT_LOCK` or with `CLOSE_LOCK`, that would have happened
already in `do_write_index()` and we would have left the lock in such a
state that trying to, e.g., roll it back or reopen the file would most
likely have crashed.

Do not delete the temporary file in `do_write_index()`. One caller will
avoid rolling back the lock, the other caller will delete its temporary
file anyway, and for both callers we will avoid crashes.

Expand the documentation on `write_locked_index()` to note that the lock
will never be rolled back when using `CLOSE_LOCK`. We can not yet make a
similar claim about `COMMIT_LOCK`; we'll fix that in the next commit.

It does feel a bit unfortunate that we simply "happen" to close the lock
by way of an implementation-detail of lockfiles. But note that we need
to close the temporary file before `stat`-ing it, at least on Windows.
See 9f41c7a6b (read-cache: close index.lock in do_write_index,
2017-04-26).

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 cache.h      | 2 ++
 read-cache.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 32aa8afdf..4d09da792 100644
--- a/cache.h
+++ b/cache.h
@@ -617,6 +617,8 @@ extern int read_index_unmerged(struct index_state *);
  * adjust its permissions and rename it into place, then write the
  * split index to the lockfile. If the temporary file for the shared
  * index cannot be created, fall back to the normal case.
+ *
+ * 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/read-cache.c b/read-cache.c
index 1ec2e8304..8e498e879 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2314,7 +2314,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		return -1;
 	if (close_tempfile_gently(tempfile)) {
 		error(_("could not close '%s'"), tempfile->filename.buf);
-		delete_tempfile(&tempfile);
 		return -1;
 	}
 	if (stat(tempfile->filename.buf, &st))
-- 
2.14.1.727.g9ddaf86