[PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Oct 1, 2017, 14:56 UTC
- Message-ID
- <6d36f0a8e3c7b81e8c815782fb67fc69f86f2837.1506862824.git.martin.agren@gmail.com>
- In-Reply-To
- <cover.1506862824.git.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