[PATCH 0/20] allowing struct lock_file to be deleted

[PATCH 0/20] allowing struct lock_file to be deleted

[Jeff King]

Since the beginning of time, our code base has had the rule that the
storage for a "struct lock_file" can never go out of scope once it has
been used. This leads to lots of intentional leaks of these structs.

I think it's unlikely that any of the leaks produces truly terrible
behavior, simply because we tend to do a limited number of operations
from a given program (and the structs aren't that big to begin with).
But they make finding _actual_ leaks much harder by polluting
leak-checker output.

And I have to admit that they've just always bugged me.

This series make it possible to free them (and in IMHO makes it harder
to misuse the API in general). I also found a number of interesting
corner cases that turned out to be bugs in the existing code (albeit
quite minor). I've floated the existing fixes to the front, followed by
some preparatory cleanups. The meat is in patches 17 and 18, and then
the last few patches use the new system to clean up some leaks.

There are almost certainly more leaks that can be cleaned. I didn't do
an exhaustive search, but just covered all the ones triggered by t0000
and t0001 (which I've been doggedly trying to get to run clean in a
leak-checker; more on that in a moment).

  [01/20]: write_index_as_tree: cleanup tempfile on error
  [02/20]: setup_temporary_shallow: avoid using inactive tempfile
  [03/20]: setup_temporary_shallow: move tempfile struct into function
  [04/20]: verify_signed_buffer: prefer close_tempfile() to close()
  [05/20]: always check return value of close_tempfile
  [06/20]: tempfile: do not delete tempfile on failed close
  [07/20]: lockfile: do not rollback lock on failed close
  [08/20]: tempfile: prefer is_tempfile_active to bare access
  [09/20]: tempfile: handle NULL tempfile pointers gracefully
  [10/20]: tempfile: replace die("BUG") with BUG()
  [11/20]: tempfile: factor out activation
  [12/20]: tempfile: factor out deactivation
  [13/20]: tempfile: robustify cleanup handler
  [14/20]: tempfile: release deactivated strbufs instead of resetting
  [15/20]: tempfile: use list.h for linked list
  [16/20]: tempfile: remove deactivated list entries
  [17/20]: tempfile: auto-allocate tempfiles on heap
  [18/20]: lockfile: update lifetime requirements in documentation
  [19/20]: ref_lock: stop leaking lock_files
  [20/20]: stop leaking lock structs in some simple cases

 builtin/gc.c               |   8 +-
 builtin/reset.c            |   6 +-
 builtin/update-index.c     |  11 +--
 cache-tree.c               |  37 ++++----
 config.c                   |  24 ++---
 credential-cache--daemon.c |   5 +-
 diff.c                     |  15 ++-
 gpg-interface.c            |  16 ++--
 list.h                     |  38 ++++++++
 lockfile.c                 |   7 +-
 lockfile.h                 |  58 ++++++------
 read-cache.c               |  32 ++++---
 refs/files-backend.c       |  50 +++++-----
 refs/packed-backend.c      |  14 +--
 shallow.c                  |  16 ++--
 tempfile.c                 | 232 +++++++++++++++++++++++----------------------
 tempfile.h                 | 123 +++++++++++-------------
 trailer.c                  |   6 +-
 18 files changed, 358 insertions(+), 340 deletions(-)

-Peff

[PATCH 01/20] write_index_as_tree: cleanup tempfile on error

[Jeff King]

If we failed to write our new index file, we rollback our
lockfile to remove the temporary index. But if we fail
before we even get to the write step (because reading the
old index failed), we leave the lockfile in place, which
makes no sense.

In practice this hasn't been a big deal because failing at
write_index_as_tree() typically results in the whole program
exiting (and thus the tempfile handler kicking in and
cleaning up the files). But this function should
consistently take responsibility for the resources it
allocates.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache-tree.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 2440d1dc89..2690113a6a 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -604,6 +604,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 {
 	int entries, was_valid, newfd;
 	struct lock_file *lock_file;
+	int ret = 0;
 
 	/*
 	 * We can't free this memory, it becomes part of a linked list
@@ -614,8 +615,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 	newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);
 
 	entries = read_index_from(index_state, index_path);
-	if (entries < 0)
-		return WRITE_TREE_UNREADABLE_INDEX;
+	if (entries < 0) {
+		ret = WRITE_TREE_UNREADABLE_INDEX;
+		goto out;
+	}
 	if (flags & WRITE_TREE_IGNORE_CACHE_TREE)
 		cache_tree_free(&index_state->cache_tree);
 
@@ -624,8 +627,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 
 	was_valid = cache_tree_fully_valid(index_state->cache_tree);
 	if (!was_valid) {
-		if (cache_tree_update(index_state, flags) < 0)
-			return WRITE_TREE_UNMERGED_INDEX;
+		if (cache_tree_update(index_state, flags) < 0) {
+			ret = WRITE_TREE_UNMERGED_INDEX;
+			goto out;
+		}
 		if (0 <= newfd) {
 			if (!write_locked_index(index_state, lock_file, COMMIT_LOCK))
 				newfd = -1;
@@ -641,17 +646,19 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 	if (prefix) {
 		struct cache_tree *subtree;
 		subtree = cache_tree_find(index_state->cache_tree, prefix);
-		if (!subtree)
-			return WRITE_TREE_PREFIX_ERROR;
+		if (!subtree) {
+			ret = WRITE_TREE_PREFIX_ERROR;
+			goto out;
+		}
 		hashcpy(sha1, subtree->oid.hash);
 	}
 	else
 		hashcpy(sha1, index_state->cache_tree->oid.hash);
 
+out:
 	if (0 <= newfd)
 		rollback_lock_file(lock_file);
-
-	return 0;
+	return ret;
 }
 
 int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
-- 
2.14.1.721.gc5bc1565f1

[PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile

[Jeff King]

When there are no shallow entries to write, we skip creating
the tempfile entirely and try to return the empty string.

But we do so by calling get_tempfile_path() on the inactive
tempfile object. This will trigger an assertion that kills
the program. The bug was introduced by 6e122b449b
(setup_temporary_shallow(): use tempfile module,
2015-08-10). But nobody seems to have noticed since then
because we do not end up calling this function at all when
there are no shallow items. In other words, this code path
is completely unexercised.

Since the tempfile object is a static global, it _is_
possible that we call the function twice, writing out
shallow info the first time and then "reusing" our tempfile
object the second time. But:

  1. It seems unlikely that this was the intent, as hitting
     this code path would imply somebody clearing the
     shallow_info list between calls.

     And if somebody _did_ call the function multiple times
     without clearing the shallow_info list, we'd hit a
     different BUG for trying to reuse an already-active
     tempfile.

  2. I verified by code inspection that the function is only
     called once per program. And also replacing this code
     with a BUG() and running the test suite demonstrates
     that it is not triggered there.

So we could probably just replace this with an assertion and
confirm that it's never called. However, the original intent
does seem to be that you _could_ call it when the
shallow_info is empty. And that's easy enough to do; since
the return value doesn't need to point to a writable buffer,
we can just return a string literal.

Signed-off-by: Jeff King <peff@peff.net>
---
 shallow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/shallow.c b/shallow.c
index f5591e56da..29194b475a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -307,7 +307,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	 * is_repository_shallow() sees empty string as "no shallow
 	 * file".
 	 */
-	return get_tempfile_path(&temporary_shallow);
+	return "";
 }
 
 void setup_alternate_shallow(struct lock_file *shallow_lock,
-- 
2.14.1.721.gc5bc1565f1

[PATCH 03/20] setup_temporary_shallow: move tempfile struct into function

[Jeff King]

The setup_temporary_shallow() function creates a temporary
file, but we never access the tempfile struct outside of the
function. This is OK, since it means we'll just clean up the
tempfile on exit.  But we can simplify the code a bit by
moving the global tempfile struct to the only function in
which it's used.

Note that it must remain "static" due to tempfile.c's
requirement that tempfile storage never goes away until
program exit.

Signed-off-by: Jeff King <peff@peff.net>
---
 shallow.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/shallow.c b/shallow.c
index 29194b475a..c7fd68ace0 100644
--- a/shallow.c
+++ b/shallow.c
@@ -286,22 +286,21 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
-static struct tempfile temporary_shallow;
-
 const char *setup_temporary_shallow(const struct oid_array *extra)
 {
+	static struct tempfile temp;
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
 	if (write_shallow_commits(&sb, 0, extra)) {
-		fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX"));
+		fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
 
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  get_tempfile_path(&temporary_shallow));
-		close_tempfile(&temporary_shallow);
+				  get_tempfile_path(&temp));
+		close_tempfile(&temp);
 		strbuf_release(&sb);
-		return get_tempfile_path(&temporary_shallow);
+		return get_tempfile_path(&temp);
 	}
 	/*
 	 * is_repository_shallow() sees empty string as "no shallow
-- 
2.14.1.721.gc5bc1565f1

[PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close()

[Jeff King]

We do a manual close() on the descriptor provided to us by
mks_tempfile. But this runs contrary to the advice in
tempfile.h, which notes that you should always use
close_tempfile(). Otherwise the descriptor may be reused
without the tempfile object knowing it, and the later call
to delete_tempfile() could close a random descriptor.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d936f3a32f..455b6c04b4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -215,7 +215,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		delete_tempfile(&temp);
 		return -1;
 	}
-	close(fd);
+	close_tempfile(&temp);
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
-- 
2.14.1.721.gc5bc1565f1

[PATCH 05/20] always check return value of close_tempfile

[Jeff King]

If close_tempfile() encounters an error, then it deletes the
tempfile and resets the "struct tempfile". But many code
paths ignore the return value and continue to use the
tempfile. Instead, we should generally treat this the same
as a write() error.

Note that in the postimage of some of these cases our error
message will be bogus after a failed close because we look
at tempfile->filename (either directly or via get_tempfile_path).
But after the failed close resets the tempfile object, this
is guaranteed to be the empty string. That will be addressed
in a future patch (because there are many more cases of the
same problem than just these instances).

Note also in the hunk in gpg-interface.c that it's fine to
call delete_tempfile() in the error path, even if
close_tempfile() failed and already deleted the file. The
tempfile code is smart enough to know the second deletion is
a noop.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c          | 4 ++--
 gpg-interface.c | 4 ++--
 shallow.c       | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 3d3e553a98..fdb974014b 100644
--- a/diff.c
+++ b/diff.c
@@ -3738,9 +3738,9 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 		blob = buf.buf;
 		size = buf.len;
 	}
-	if (write_in_full(fd, blob, size) != size)
+	if (write_in_full(fd, blob, size) != size ||
+	    close_tempfile(&temp->tempfile))
 		die_errno("unable to write temp-file");
-	close_tempfile(&temp->tempfile);
 	temp->name = get_tempfile_path(&temp->tempfile);
 	oid_to_hex_r(temp->hex, oid);
 	xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
diff --git a/gpg-interface.c b/gpg-interface.c
index 455b6c04b4..05ca6ecbfd 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -209,13 +209,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
 		return error_errno(_("could not create temporary file"));
-	if (write_in_full(fd, signature, signature_size) < 0) {
+	if (write_in_full(fd, signature, signature_size) < 0 ||
+	    close_tempfile(&temp) < 0) {
 		error_errno(_("failed writing detached signature to '%s'"),
 			    temp.filename.buf);
 		delete_tempfile(&temp);
 		return -1;
 	}
-	close_tempfile(&temp);
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
diff --git a/shallow.c b/shallow.c
index c7fd68ace0..f49e6ae122 100644
--- a/shallow.c
+++ b/shallow.c
@@ -295,10 +295,10 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	if (write_shallow_commits(&sb, 0, extra)) {
 		fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
 
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
+		    close_tempfile(&temp) < 0)
 			die_errno("failed to write to %s",
 				  get_tempfile_path(&temp));
-		close_tempfile(&temp);
 		strbuf_release(&sb);
 		return get_tempfile_path(&temp);
 	}
-- 
2.14.1.721.gc5bc1565f1

[PATCH 06/20] tempfile: do not delete tempfile on failed close

[Jeff King]

When close_tempfile() fails, we delete the tempfile and
reset the fields of the tempfile struct. This makes it
easier for callers to return without cleaning up, but it
also makes this common pattern:

  if (close_tempfile(tempfile))
	return error_errno("error closing %s", tempfile->filename.buf);

wrong, because the "filename" field has been reset after the
failed close. And it's not easy to fix, as in many cases we
don't have another copy of the filename (e.g., if it was
created via one of the mks_tempfile functions, and we just
have the original template string).

Let's drop the feature that a failed close automatically
deletes the file. This puts the burden on the caller to do
the deletion themselves, but this isn't that big a deal.
Callers which do:

  if (write(...) || close_tempfile(...)) {
	delete_tempfile(...);
	return -1;
  }

already had to call delete when the write() failed, and so
aren't affected. Likewise, any caller which just calls die()
in the error path is OK; we'll delete the tempfile during
the atexit handler.

Because this patch changes the semantics of close_tempfile()
without changing its signature, all callers need to be
manually checked and converted to the new scheme. This patch
covers all in-tree callers, but there may be others for
not-yet-merged topics. To catch these, we rename the
function to close_tempfile_gently(), which will attract
compile-time attention to new callers. (Technically the
original could be considered "gentle" already in that it
didn't die() on errors, but this one is even more so).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c          |  2 +-
 gpg-interface.c |  2 +-
 lockfile.h      |  8 +++++++-
 read-cache.c    |  7 +++++--
 shallow.c       |  2 +-
 tempfile.c      | 31 ++++++++++++-------------------
 tempfile.h      | 25 +++++++++++++------------
 7 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/diff.c b/diff.c
index fdb974014b..20f68ec389 100644
--- a/diff.c
+++ b/diff.c
@@ -3739,7 +3739,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 		size = buf.len;
 	}
 	if (write_in_full(fd, blob, size) != size ||
-	    close_tempfile(&temp->tempfile))
+	    close_tempfile_gently(&temp->tempfile))
 		die_errno("unable to write temp-file");
 	temp->name = get_tempfile_path(&temp->tempfile);
 	oid_to_hex_r(temp->hex, oid);
diff --git a/gpg-interface.c b/gpg-interface.c
index 05ca6ecbfd..4ea2597ff4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -210,7 +210,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	if (fd < 0)
 		return error_errno(_("could not create temporary file"));
 	if (write_in_full(fd, signature, signature_size) < 0 ||
-	    close_tempfile(&temp) < 0) {
+	    close_tempfile_gently(&temp) < 0) {
 		error_errno(_("failed writing detached signature to '%s'"),
 			    temp.filename.buf);
 		delete_tempfile(&temp);
diff --git a/lockfile.h b/lockfile.h
index 572064939c..dd4259bc40 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -246,7 +246,13 @@ extern char *get_locked_file_path(struct lock_file *lk);
  */
 static inline int close_lock_file(struct lock_file *lk)
 {
-	return close_tempfile(&lk->tempfile);
+	int ret = close_tempfile_gently(&lk->tempfile);
+	if (ret) {
+		int saved_errno = errno;
+		delete_tempfile(&lk->tempfile);
+		errno = saved_errno;
+	}
+	return ret;
 }
 
 /*
diff --git a/read-cache.c b/read-cache.c
index 40da87ea71..51686518e0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2309,8 +2309,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 
 	if (ce_flush(&c, newfd, istate->sha1))
 		return -1;
-	if (close_tempfile(tempfile))
-		return error(_("could not close '%s'"), tempfile->filename.buf);
+	if (close_tempfile_gently(tempfile)) {
+		error(_("could not close '%s'"), tempfile->filename.buf);
+		delete_tempfile(tempfile);
+		return -1;
+	}
 	if (stat(tempfile->filename.buf, &st))
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
diff --git a/shallow.c b/shallow.c
index f49e6ae122..36216febb6 100644
--- a/shallow.c
+++ b/shallow.c
@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 		fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
 
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
-		    close_tempfile(&temp) < 0)
+		    close_tempfile_gently(&temp) < 0)
 			die_errno("failed to write to %s",
 				  get_tempfile_path(&temp));
 		strbuf_release(&sb);
diff --git a/tempfile.c b/tempfile.c
index 6843710670..c6740e562c 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -30,13 +30,12 @@
  *     `fdopen_tempfile()` has been called on the object
  *   - `owner` holds the PID of the process that created the file
  *
- * - Active, file closed (after successful `close_tempfile()`). Same
+ * - Active, file closed (after `close_tempfile_gently()`). Same
  *   as the previous state, except that the temporary file is closed,
  *   `fd` is -1, and `fp` is `NULL`.
  *
- * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, a
- *   failed attempt to create a temporary file, or a failed
- *   `close_tempfile()`). In this state:
+ * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a
+ *   failed attempt to create a temporary file). In this state:
  *
  *   - `active` is unset
  *   - `filename` is empty (usually, though there are transitory
@@ -235,7 +234,7 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)
 	return tempfile->fp;
 }
 
-int close_tempfile(struct tempfile *tempfile)
+int close_tempfile_gently(struct tempfile *tempfile)
 {
 	int fd = tempfile->fd;
 	FILE *fp = tempfile->fp;
@@ -258,14 +257,7 @@ int close_tempfile(struct tempfile *tempfile)
 		err = close(fd);
 	}
 
-	if (err) {
-		int save_errno = errno;
-		delete_tempfile(tempfile);
-		errno = save_errno;
-		return -1;
-	}
-
-	return 0;
+	return err ? -1 : 0;
 }
 
 int reopen_tempfile(struct tempfile *tempfile)
@@ -283,8 +275,10 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
 	if (!tempfile->active)
 		die("BUG: rename_tempfile called for inactive object");
 
-	if (close_tempfile(tempfile))
+	if (close_tempfile_gently(tempfile)) {
+		delete_tempfile(tempfile);
 		return -1;
+	}
 
 	if (rename(tempfile->filename.buf, path)) {
 		int save_errno = errno;
@@ -303,9 +297,8 @@ void delete_tempfile(struct tempfile *tempfile)
 	if (!tempfile->active)
 		return;
 
-	if (!close_tempfile(tempfile)) {
-		unlink_or_warn(tempfile->filename.buf);
-		tempfile->active = 0;
-		strbuf_reset(&tempfile->filename);
-	}
+	close_tempfile_gently(tempfile);
+	unlink_or_warn(tempfile->filename.buf);
+	tempfile->active = 0;
+	strbuf_reset(&tempfile->filename);
 }
diff --git a/tempfile.h b/tempfile.h
index 2f0038decd..d854dcdd3e 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -47,7 +47,7 @@
  *   control of the file.
  *
  * * Close the file descriptor without removing or renaming the
- *   temporary file by calling `close_tempfile()`, and later call
+ *   temporary file by calling `close_tempfile_gently()`, and later call
  *   `delete_tempfile()` or `rename_tempfile()`.
  *
  * Even after the temporary file is renamed or deleted, the `tempfile`
@@ -59,7 +59,7 @@
  * and remove the temporary file.
  *
  * If you need to close the file descriptor yourself, do so by calling
- * `close_tempfile()`. You should never call `close(2)` or `fclose(3)`
+ * `close_tempfile_gently()`. You should never call `close(2)` or `fclose(3)`
  * yourself, otherwise the `struct tempfile` structure would still
  * think that the file descriptor needs to be closed, and a later
  * cleanup would result in duplicate calls to `close(2)`. Worse yet,
@@ -74,9 +74,10 @@
  * `create_tempfile()` returns a file descriptor on success or -1 on
  * failure. On errors, `errno` describes the reason for failure.
  *
- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile()`
- * return 0 on success. On failure they set `errno` appropriately, do
- * their best to delete the temporary file, and return -1.
+ * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
+ * return 0 on success. On failure they set `errno` appropriately and return
+ * -1. `delete` and `rename` (but not `close`) do their best to delete the
+ * temporary file before returning.
  */
 
 struct tempfile {
@@ -203,7 +204,7 @@ static inline int xmks_tempfile(struct tempfile *tempfile,
 /*
  * Associate a stdio stream with the temporary file (which must still
  * be open). Return `NULL` (*without* deleting the file) on error. The
- * stream is closed automatically when `close_tempfile()` is called or
+ * stream is closed automatically when `close_tempfile_gently()` is called or
  * when the file is deleted or renamed.
  */
 extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);
@@ -226,20 +227,20 @@ extern FILE *get_tempfile_fp(struct tempfile *tempfile);
  * If the temporary file is still open, close it (and the file pointer
  * too, if it has been opened using `fdopen_tempfile()`) without
  * deleting the file. Return 0 upon success. On failure to `close(2)`,
- * return a negative value and delete the file. Usually
- * `delete_tempfile()` or `rename_tempfile()` should eventually be
- * called if `close_tempfile()` succeeds.
+ * return a negative value. Usually `delete_tempfile()` or `rename_tempfile()`
+ * should eventually be called regardless of whether `close_tempfile_gently()`
+ * succeeds.
  */
-extern int close_tempfile(struct tempfile *tempfile);
+extern int close_tempfile_gently(struct tempfile *tempfile);
 
 /*
  * Re-open a temporary file that has been closed using
- * `close_tempfile()` but not yet deleted or renamed. This can be used
+ * `close_tempfile_gently()` but not yet deleted or renamed. This can be used
  * to implement a sequence of operations like the following:
  *
  * * Create temporary file.
  *
- * * Write new contents to file, then `close_tempfile()` to cause the
+ * * Write new contents to file, then `close_tempfile_gently()` to cause the
  *   contents to be written to disk.
  *
  * * Pass the name of the temporary file to another program to allow
-- 
2.14.1.721.gc5bc1565f1

[PATCH 07/20] lockfile: do not rollback lock on failed close

[Jeff King]

Since the lockfile code is based on the tempfile code, it
has some of the same problems, including that close_lock_file()
erases the tempfile's filename buf, making it hard for the
caller to write a good error message.

In practice this comes up less for lockfiles than for
straight tempfiles, since we usually just report the
refname. But there is at least one buggy case in
write_ref_to_lockfile(). Besides, given the coupling between
the lockfile and tempfile modules, it's less confusing if
their close() functions have the same semantics.

Just as the previous commit did for close_tempfile(), let's
teach close_lock_file() and its wrapper close_ref() not to
rollback on error. And just as before, we'll give them new
"gently" names to catch any new callers that are added.

Signed-off-by: Jeff King <peff@peff.net>
---
 lockfile.h            | 30 ++++++++++++------------------
 read-cache.c          |  2 +-
 refs/files-backend.c  | 13 +++++++------
 refs/packed-backend.c |  3 ++-
 4 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/lockfile.h b/lockfile.h
index dd4259bc40..c45d2db324 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -69,7 +69,7 @@
  *   `rollback_lock_file()`.
  *
  * * Close the file descriptor without removing or renaming the
- *   lockfile by calling `close_lock_file()`, and later call
+ *   lockfile by calling `close_lock_file_gently()`, and later call
  *   `commit_lock_file()`, `commit_lock_file_to()`,
  *   `rollback_lock_file()`, or `reopen_lock_file()`.
  *
@@ -85,7 +85,7 @@
  *
  * If you need to close the file descriptor you obtained from a
  * `hold_lock_file_for_*()` function yourself, do so by calling
- * `close_lock_file()`. See "tempfile.h" for more information.
+ * `close_lock_file_gently()`. See "tempfile.h" for more information.
  *
  *
  * Under the covers, a lockfile is just a tempfile with a few helper
@@ -104,8 +104,8 @@
  *
  * Similarly, `commit_lock_file`, `commit_lock_file_to`, and
  * `close_lock_file` return 0 on success. On failure they set `errno`
- * appropriately, do their best to roll back the lockfile, and return
- * -1.
+ * appropriately and return -1. The `commit` variants (but not `close`)
+ * do their best to delete the temporary file before returning.
  */
 
 #include "tempfile.h"
@@ -202,8 +202,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
 /*
  * Associate a stdio stream with the lockfile (which must still be
  * open). Return `NULL` (*without* rolling back the lockfile) on
- * error. The stream is closed automatically when `close_lock_file()`
- * is called or when the file is committed or rolled back.
+ * error. The stream is closed automatically when
+ * `close_lock_file_gently()` is called or when the file is committed or
+ * rolled back.
  */
 static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
 {
@@ -241,28 +242,21 @@ extern char *get_locked_file_path(struct lock_file *lk);
  * lockfile over the file being locked. Return 0 upon success. On
  * failure to `close(2)`, return a negative value and roll back the
  * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,
- * or `rollback_lock_file()` should eventually be called if
- * `close_lock_file()` succeeds.
+ * or `rollback_lock_file()` should eventually be called.
  */
-static inline int close_lock_file(struct lock_file *lk)
+static inline int close_lock_file_gently(struct lock_file *lk)
 {
-	int ret = close_tempfile_gently(&lk->tempfile);
-	if (ret) {
-		int saved_errno = errno;
-		delete_tempfile(&lk->tempfile);
-		errno = saved_errno;
-	}
-	return ret;
+	return close_tempfile_gently(&lk->tempfile);
 }
 
 /*
- * Re-open a lockfile that has been closed using `close_lock_file()`
+ * Re-open a lockfile that has been closed using `close_lock_file_gently()`
  * but not yet committed or rolled back. This can be used to implement
  * a sequence of operations like the following:
  *
  * * Lock file.
  *
- * * Write new contents to lockfile, then `close_lock_file()` to
+ * * Write new contents to lockfile, then `close_lock_file_gently()` to
  *   cause the contents to be written to disk.
  *
  * * Pass the name of the lockfile to another program to allow it (and
diff --git a/read-cache.c b/read-cache.c
index 51686518e0..c3be65f8b0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2345,7 +2345,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 	if (flags & COMMIT_LOCK)
 		return commit_locked_index(lock);
 	else if (flags & CLOSE_LOCK)
-		return close_lock_file(lock);
+		return close_lock_file_gently(lock);
 	else
 		return ret;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..bc1899cd4a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store,
 	return ret;
 }
 
-static int close_ref(struct ref_lock *lock)
+static int close_ref_gently(struct ref_lock *lock)
 {
-	if (close_lock_file(lock->lk))
+	if (close_lock_file_gently(lock->lk))
 		return -1;
 	return 0;
 }
@@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 	fd = get_lock_file_fd(lock->lk);
 	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 	    write_in_full(fd, &term, 1) != 1 ||
-	    close_ref(lock) < 0) {
+	    close_ref_gently(lock) < 0) {
 		strbuf_addf(err,
 			    "couldn't write '%s'", get_lock_file_path(lock->lk));
 		unlock_ref(lock);
@@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 		 * the lockfile is still open. Close it to
 		 * free up the file descriptor:
 		 */
-		if (close_ref(lock)) {
+		if (close_ref_gently(lock)) {
 			strbuf_addf(err, "couldn't close '%s.lock'",
 				    update->refname);
 			return TRANSACTION_GENERIC_ERROR;
@@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store,
 			!(type & REF_ISSYMREF) &&
 			!is_null_oid(&cb.last_kept_oid);
 
-		if (close_lock_file(&reflog_lock)) {
+		if (close_lock_file_gently(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
+			rollback_lock_file(&reflog_lock);
 		} else if (update &&
 			   (write_in_full(get_lock_file_fd(lock->lk),
 				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
-			    close_ref(lock) < 0)) {
+			    close_ref_gently(lock) < 0)) {
 			status |= error("couldn't write %s",
 					get_lock_file_path(lock->lk));
 			rollback_lock_file(&reflog_lock);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 412c85034f..dca887c0fe 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 		return -1;
 	}
 
-	if (close_lock_file(&refs->lock)) {
+	if (close_lock_file_gently(&refs->lock)) {
 		strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno));
+		rollback_lock_file(&refs->lock);
 		return -1;
 	}
 
-- 
2.14.1.721.gc5bc1565f1

[PATCH 08/20] tempfile: prefer is_tempfile_active to bare access

[Jeff King]

The tempfile code keeps an "active" flag, and we have a
number of assertions to make sure that the objects are being
used in the right order. Most of these directly check
"active" rather than using the is_tempfile_active()
accessor.

Let's prefer using the accessor, in preparation for it
growing more complicated logic (like checking for NULL).

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index c6740e562c..964c66d504 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -95,7 +95,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)
 		atexit(remove_tempfiles_on_exit);
 	}
 
-	if (tempfile->active)
+	if (is_tempfile_active(tempfile))
 		die("BUG: prepare_tempfile_object called for active object");
 	if (!tempfile->on_list) {
 		/* Initialize *tempfile and add it to tempfile_list: */
@@ -204,7 +204,7 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)
 
 FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: fdopen_tempfile() called for inactive object");
 	if (tempfile->fp)
 		die("BUG: fdopen_tempfile() called for open object");
@@ -215,21 +215,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 
 const char *get_tempfile_path(struct tempfile *tempfile)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: get_tempfile_path() called for inactive object");
 	return tempfile->filename.buf;
 }
 
 int get_tempfile_fd(struct tempfile *tempfile)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: get_tempfile_fd() called for inactive object");
 	return tempfile->fd;
 }
 
 FILE *get_tempfile_fp(struct tempfile *tempfile)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: get_tempfile_fp() called for inactive object");
 	return tempfile->fp;
 }
@@ -264,7 +264,7 @@ int reopen_tempfile(struct tempfile *tempfile)
 {
 	if (0 <= tempfile->fd)
 		die("BUG: reopen_tempfile called for an open object");
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: reopen_tempfile called for an inactive object");
 	tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
 	return tempfile->fd;
@@ -272,7 +272,7 @@ int reopen_tempfile(struct tempfile *tempfile)
 
 int rename_tempfile(struct tempfile *tempfile, const char *path)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		die("BUG: rename_tempfile called for inactive object");
 
 	if (close_tempfile_gently(tempfile)) {
@@ -294,7 +294,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
 
 void delete_tempfile(struct tempfile *tempfile)
 {
-	if (!tempfile->active)
+	if (!is_tempfile_active(tempfile))
 		return;
 
 	close_tempfile_gently(tempfile);
-- 
2.14.1.721.gc5bc1565f1

[PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully

[Jeff King]

The tempfile functions all take pointers to tempfile
objects, but do not check whether the argument is NULL.
This isn't a big deal in practice, since the lifetime of any
tempfile object is defined to last for the whole program. So
even if we try to call delete_tempfile() on an
already-deleted tempfile, our "active" check will tell us
that it's a noop.

In preparation for transitioning to a new system that
loosens the "tempfile objects can never be freed" rule,
let's tighten up our active checks:

  1. A NULL pointer is now defined as "inactive" (so it will
     BUG for most functions, but works as a silent noop for
     things like delete_tempfile).

  2. Functions should always do the "active" check before
     looking at any of the struct fields.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 12 +++++++-----
 tempfile.h |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 964c66d504..861f817133 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -236,13 +236,15 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)
 
 int close_tempfile_gently(struct tempfile *tempfile)
 {
-	int fd = tempfile->fd;
-	FILE *fp = tempfile->fp;
+	int fd;
+	FILE *fp;
 	int err;
 
-	if (fd < 0)
+	if (!is_tempfile_active(tempfile) || tempfile->fd < 0)
 		return 0;
 
+	fd = tempfile->fd;
+	fp = tempfile->fp;
 	tempfile->fd = -1;
 	if (fp) {
 		tempfile->fp = NULL;
@@ -262,10 +264,10 @@ int close_tempfile_gently(struct tempfile *tempfile)
 
 int reopen_tempfile(struct tempfile *tempfile)
 {
-	if (0 <= tempfile->fd)
-		die("BUG: reopen_tempfile called for an open object");
 	if (!is_tempfile_active(tempfile))
 		die("BUG: reopen_tempfile called for an inactive object");
+	if (0 <= tempfile->fd)
+		die("BUG: reopen_tempfile called for an open object");
 	tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
 	return tempfile->fd;
 }
diff --git a/tempfile.h b/tempfile.h
index d854dcdd3e..d30663182d 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -211,7 +211,7 @@ extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);
 
 static inline int is_tempfile_active(struct tempfile *tempfile)
 {
-	return tempfile->active;
+	return tempfile && tempfile->active;
 }
 
 /*
-- 
2.14.1.721.gc5bc1565f1

[PATCH 10/20] tempfile: replace die("BUG") with BUG()

[Jeff King]

Compared to die(), using BUG() triggers abort(). That may
give us an actual coredump, which should make it easier to
get a stack trace. And since the programming error for these
assertions is not in the functions themselves but in their
callers, such a stack trace is needed to actually find the
source of the bug.

In addition, abort() raises SIGABRT, which is more likely to
be caught by our test suite.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 861f817133..813cf6a81c 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -96,7 +96,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)
 	}
 
 	if (is_tempfile_active(tempfile))
-		die("BUG: prepare_tempfile_object called for active object");
+		BUG("prepare_tempfile_object called for active object");
 	if (!tempfile->on_list) {
 		/* Initialize *tempfile and add it to tempfile_list: */
 		tempfile->fd = -1;
@@ -109,7 +109,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)
 		tempfile->on_list = 1;
 	} else if (tempfile->filename.len) {
 		/* This shouldn't happen, but better safe than sorry. */
-		die("BUG: prepare_tempfile_object called for improperly-reset object");
+		BUG("prepare_tempfile_object called for improperly-reset object");
 	}
 }
 
@@ -205,9 +205,9 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)
 FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: fdopen_tempfile() called for inactive object");
+		BUG("fdopen_tempfile() called for inactive object");
 	if (tempfile->fp)
-		die("BUG: fdopen_tempfile() called for open object");
+		BUG("fdopen_tempfile() called for open object");
 
 	tempfile->fp = fdopen(tempfile->fd, mode);
 	return tempfile->fp;
@@ -216,21 +216,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 const char *get_tempfile_path(struct tempfile *tempfile)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: get_tempfile_path() called for inactive object");
+		BUG("get_tempfile_path() called for inactive object");
 	return tempfile->filename.buf;
 }
 
 int get_tempfile_fd(struct tempfile *tempfile)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: get_tempfile_fd() called for inactive object");
+		BUG("get_tempfile_fd() called for inactive object");
 	return tempfile->fd;
 }
 
 FILE *get_tempfile_fp(struct tempfile *tempfile)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: get_tempfile_fp() called for inactive object");
+		BUG("get_tempfile_fp() called for inactive object");
 	return tempfile->fp;
 }
 
@@ -265,9 +265,9 @@ int close_tempfile_gently(struct tempfile *tempfile)
 int reopen_tempfile(struct tempfile *tempfile)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: reopen_tempfile called for an inactive object");
+		BUG("reopen_tempfile called for an inactive object");
 	if (0 <= tempfile->fd)
-		die("BUG: reopen_tempfile called for an open object");
+		BUG("reopen_tempfile called for an open object");
 	tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
 	return tempfile->fd;
 }
@@ -275,7 +275,7 @@ int reopen_tempfile(struct tempfile *tempfile)
 int rename_tempfile(struct tempfile *tempfile, const char *path)
 {
 	if (!is_tempfile_active(tempfile))
-		die("BUG: rename_tempfile called for inactive object");
+		BUG("rename_tempfile called for inactive object");
 
 	if (close_tempfile_gently(tempfile)) {
 		delete_tempfile(tempfile);
-- 
2.14.1.721.gc5bc1565f1

[PATCH 11/20] tempfile: factor out activation

[Jeff King]

There are a few steps required to "activate" a tempfile
struct. Let's pull these out into a function. That saves a
few repeated lines now, but more importantly will make it
easier to change the activation scheme later.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 813cf6a81c..0e6c6b9c18 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -113,6 +113,12 @@ static void prepare_tempfile_object(struct tempfile *tempfile)
 	}
 }
 
+static void activate_tempfile(struct tempfile *tempfile)
+{
+	tempfile->owner = getpid();
+	tempfile->active = 1;
+}
+
 /* Make sure errno contains a meaningful value on error */
 int create_tempfile(struct tempfile *tempfile, const char *path)
 {
@@ -129,8 +135,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
 		strbuf_reset(&tempfile->filename);
 		return -1;
 	}
-	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	if (adjust_shared_perm(tempfile->filename.buf)) {
 		int save_errno = errno;
 		error("cannot fix permission bits on %s", tempfile->filename.buf);
@@ -145,8 +150,7 @@ void register_tempfile(struct tempfile *tempfile, const char *path)
 {
 	prepare_tempfile_object(tempfile);
 	strbuf_add_absolute_path(&tempfile->filename, path);
-	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 }
 
 int mks_tempfile_sm(struct tempfile *tempfile,
@@ -160,8 +164,7 @@ int mks_tempfile_sm(struct tempfile *tempfile,
 		strbuf_reset(&tempfile->filename);
 		return -1;
 	}
-	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	return tempfile->fd;
 }
 
@@ -182,8 +185,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
 		strbuf_reset(&tempfile->filename);
 		return -1;
 	}
-	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	return tempfile->fd;
 }
 
-- 
2.14.1.721.gc5bc1565f1

[PATCH 12/20] tempfile: factor out deactivation

[Jeff King]

When we deactivate a tempfile, we also have to clean up the
"filename" strbuf. Let's pull this out into its own function
to keep the logic in one place (which will become more
important when a future patch makes it more complicated).

Note that we can use the same function when deactivating an
object that _isn't_ actually active yet (like when we hit an
error creating a tempfile). These callsites don't currently
reset the "active" flag to 0, but it's OK to do so (it's
just a noop for these cases).

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 0e6c6b9c18..9d7f0a2f2b 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -119,6 +119,12 @@ static void activate_tempfile(struct tempfile *tempfile)
 	tempfile->active = 1;
 }
 
+static void deactivate_tempfile(struct tempfile *tempfile)
+{
+	tempfile->active = 0;
+	strbuf_reset(&tempfile->filename);
+}
+
 /* Make sure errno contains a meaningful value on error */
 int create_tempfile(struct tempfile *tempfile, const char *path)
 {
@@ -132,7 +138,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
 		tempfile->fd = open(tempfile->filename.buf,
 				    O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		deactivate_tempfile(tempfile);
 		return -1;
 	}
 	activate_tempfile(tempfile);
@@ -161,7 +167,7 @@ int mks_tempfile_sm(struct tempfile *tempfile,
 	strbuf_add_absolute_path(&tempfile->filename, template);
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		deactivate_tempfile(tempfile);
 		return -1;
 	}
 	activate_tempfile(tempfile);
@@ -182,7 +188,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
 	strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, template);
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		deactivate_tempfile(tempfile);
 		return -1;
 	}
 	activate_tempfile(tempfile);
@@ -291,8 +297,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
 		return -1;
 	}
 
-	tempfile->active = 0;
-	strbuf_reset(&tempfile->filename);
+	deactivate_tempfile(tempfile);
 	return 0;
 }
 
@@ -303,6 +308,5 @@ void delete_tempfile(struct tempfile *tempfile)
 
 	close_tempfile_gently(tempfile);
 	unlink_or_warn(tempfile->filename.buf);
-	tempfile->active = 0;
-	strbuf_reset(&tempfile->filename);
+	deactivate_tempfile(tempfile);
 }
-- 
2.14.1.721.gc5bc1565f1

[PATCH 13/20] tempfile: robustify cleanup handler

[Jeff King]

We may call remove_tempfiles() from an atexit handler, or
from a signal handler. In the latter case we must take care
to avoid functions which may deadlock if the process is in
an unknown state, including looking at any stdio handles
(which may be in the middle of doing I/O and locked) or
calling malloc() or free().

The current implementation calls delete_tempfile(). We unset
the tempfile's stdio handle (if any) to avoid deadlocking
there. But delete_tempfile() still calls unlink_or_warn(),
which can deadlock writing to stderr if the unlink fails.

Since delete_tempfile() isn't very long, let's just
open-code our own simple conservative version of the same
thing.  Notably:

  1. The "skip_fclose" flag is now called "in_signal_handler",
     because it should inform more decisions than just the
     fclose handling.

  2. We can replace close_tempfile() with just close(fd).
     That skips the fclose() question altogether. This is
     fine for the atexit() case, too; there's no point
     flushing data to a file which we're about to delete
     anyway.

  3. We can choose between unlink/unlink_or_warn based on
     whether it's safe to use stderr.

  4. We can replace the deactivate_tempfile() call with a
     simple setting of the active flag. There's no need to
     do any further cleanup since we know the program is
     exiting.  And even though the current deactivation code
     is safe in a signal handler, this frees us up in future
     patches to make non-signal deactivation more
     complicated (e.g., by freeing resources).

  5. There's no need to remove items from the tempfile_list.
     The "active" flag is the ultimate answer to whether an
     entry has been handled or not. Manipulating the list
     just introduces more chance of recursive signals
     stomping on each other, and the whole list will go away
     when the program exits anyway. Less is more.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 9d7f0a2f2b..3348ad59dd 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -57,18 +57,24 @@
 
 static struct tempfile *volatile tempfile_list;
 
-static void remove_tempfiles(int skip_fclose)
+static void remove_tempfiles(int in_signal_handler)
 {
 	pid_t me = getpid();
+	struct tempfile *volatile p;
 
-	while (tempfile_list) {
-		if (tempfile_list->owner == me) {
-			/* fclose() is not safe to call in a signal handler */
-			if (skip_fclose)
-				tempfile_list->fp = NULL;
-			delete_tempfile(tempfile_list);
-		}
-		tempfile_list = tempfile_list->next;
+	for (p = tempfile_list; p; p = p->next) {
+		if (!is_tempfile_active(p) || p->owner != me)
+			continue;
+
+		if (p->fd >= 0)
+			close(p->fd);
+
+		if (in_signal_handler)
+			unlink(p->filename.buf);
+		else
+			unlink_or_warn(p->filename.buf);
+
+		p->active = 0;
 	}
 }
 
-- 
2.14.1.721.gc5bc1565f1

[PATCH 14/20] tempfile: release deactivated strbufs instead of resetting

[Jeff King]

When a tempfile is deactivated, we reset its strbuf to the
empty string, which means we hold onto the memory for later
reuse.

Since we'd like to move to a system where tempfile structs
can actually be freed, deactivating one should drop all
resources it is currently using. And thus "release" rather
than "reset" is the appropriate function to call.

In theory the reset may have saved a malloc() when a
tempfile (or a lockfile) is reused multiple times. But in
practice this happened rarely. Most of our tempfiles are
single-use, since in cases where we might actually use many
(like ref locking) we xcalloc() a fresh one for each ref. In
fact, we leak those locks (to appease the rule that tempfile
storage can never be freed). Which means that using reset is
actively hurting us: instead of leaking just the tempfile
struct, we're leaking the extra heap chunk for the filename,
too.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tempfile.c b/tempfile.c
index 3348ad59dd..e655e28477 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -128,7 +128,7 @@ static void activate_tempfile(struct tempfile *tempfile)
 static void deactivate_tempfile(struct tempfile *tempfile)
 {
 	tempfile->active = 0;
-	strbuf_reset(&tempfile->filename);
+	strbuf_release(&tempfile->filename);
 }
 
 /* Make sure errno contains a meaningful value on error */
-- 
2.14.1.721.gc5bc1565f1

[PATCH 15/20] tempfile: use list.h for linked list

[Jeff King]

The tempfile API keeps to-be-cleaned tempfiles in a
singly-linked list and never removes items from the list.  A
future patch would like to start removing items, but removal
from a singly linked list is O(n), as we have to walk the
list to find the predecessor element. This means that a
process which takes "n" simultaneous lockfiles (for example,
an atomic transaction on "n" refs) may end up quadratic in
"n".

Before we start allowing items to be removed, it would be
nice to have a way to cover this case in linear time.

The simplest solution is to make an assumption about the
order in which tempfiles are added and removed from the
list. If both operations iterate over the tempfiles in the
same order, then by putting new items at the end of the list
our removal search will always find its items at the
beginning of the list. And indeed, that would work for the
case of refs. But it creates a hidden dependency between
unrelated parts of the code. If anybody changes the ref code
(or if we add a new caller that opens multiple simultaneous
tempfiles) they may unknowingly introduce a performance
regression.

Another solution is to use a better data structure. A
doubly-linked list works fine, and we already have an
implementation in list.h. But there's one snag: the elements
of "struct tempfile" are all marked as "volatile", since a
signal handler may interrupt us and iterate over the list at
any moment (even if we were in the middle of adding a new
entry).

We can declare a "volatile struct list_head", but we can't
actually use it with the normal list functions. The compiler
complains about passing a pointer-to-volatile via a regular
pointer argument. And rightfully so, as the sub-function
would potentially need different code to deal with the
volatile case.

That leaves us with a few options:

  1. Drop the "volatile" modifier for the list items.

     This is probably a bad idea. I checked the assembly
     output from "gcc -O2", and the "volatile" really does
     impact the order in which it updates memory.

  2. Use macros instead of inline functions. The irony here
     is that list.h is entirely implemented as trivial
     inline functions. So we basically are already
     generating custom code for each call. But sadly there's no
     way in C to declare the inline function to take a more
     generic type.

     We could do so by switching the inline functions to
     macros, but it does make the end result harder to read.
     And it doesn't fully solve the problem (for instance,
     the declaration of list_head needs to change so that
     its "prev" and "next" pointers point to other volatile
     structs).

  3. Don't use list.h, and just make our own ad-hoc
     doubly-linked list. It's not that much code to
     implement the basics that we need here. But if we're
     going to do so, why not add the few extra lines
     required to model it after the actual list.h interface?
     We can even reuse a few of the macro helpers.

So this patch takes option 3, but actually implements a
parallel "volatile list" interface in list.h, where it could
potentially be reused by other code. This implements just
enough for tempfile.c's use, though we could easily port
other functions later if need be.

Signed-off-by: Jeff King <peff@peff.net>
---
 list.h     | 38 ++++++++++++++++++++++++++++++++++++++
 tempfile.c | 13 +++++++------
 tempfile.h |  4 +++-
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/list.h b/list.h
index a226a870dc..eb601192f4 100644
--- a/list.h
+++ b/list.h
@@ -163,4 +163,42 @@ static inline void list_replace_init(struct list_head *old,
 	INIT_LIST_HEAD(old);
 }
 
+/*
+ * This is exactly the same as a normal list_head, except that it can be
+ * declared volatile (e.g., if you have a list that may be accessed from signal
+ * handlers).
+ */
+struct volatile_list_head {
+	volatile struct volatile_list_head *next, *prev;
+};
+
+#define VOLATILE_LIST_HEAD(name) \
+	volatile struct volatile_list_head name = { &(name), &(name) }
+
+static inline void __volatile_list_del(volatile struct volatile_list_head *prev,
+				       volatile struct volatile_list_head *next)
+{
+	next->prev = prev;
+	prev->next = next;
+}
+
+static inline void volatile_list_del(volatile struct volatile_list_head *elem)
+{
+	__volatile_list_del(elem->prev, elem->next);
+}
+
+static inline int volatile_list_empty(volatile struct volatile_list_head *head)
+{
+	return head == head->next;
+}
+
+static inline void volatile_list_add(volatile struct volatile_list_head *newp,
+				     volatile struct volatile_list_head *head)
+{
+	head->next->prev = newp;
+	newp->next = head->next;
+	newp->prev = head;
+	head->next = newp;
+}
+
 #endif /* LIST_H */
diff --git a/tempfile.c b/tempfile.c
index e655e28477..11bda824cf 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -55,14 +55,16 @@
 #include "tempfile.h"
 #include "sigchain.h"
 
-static struct tempfile *volatile tempfile_list;
+static VOLATILE_LIST_HEAD(tempfile_list);
 
 static void remove_tempfiles(int in_signal_handler)
 {
 	pid_t me = getpid();
-	struct tempfile *volatile p;
+	volatile struct volatile_list_head *pos;
+
+	list_for_each(pos, &tempfile_list) {
+		struct tempfile *p = list_entry(pos, struct tempfile, list);
 
-	for (p = tempfile_list; p; p = p->next) {
 		if (!is_tempfile_active(p) || p->owner != me)
 			continue;
 
@@ -95,7 +97,7 @@ static void remove_tempfiles_on_signal(int signo)
  */
 static void prepare_tempfile_object(struct tempfile *tempfile)
 {
-	if (!tempfile_list) {
+	if (volatile_list_empty(&tempfile_list)) {
 		/* One-time initialization */
 		sigchain_push_common(remove_tempfiles_on_signal);
 		atexit(remove_tempfiles_on_exit);
@@ -110,8 +112,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)
 		tempfile->active = 0;
 		tempfile->owner = 0;
 		strbuf_init(&tempfile->filename, 0);
-		tempfile->next = tempfile_list;
-		tempfile_list = tempfile;
+		volatile_list_add(&tempfile->list, &tempfile_list);
 		tempfile->on_list = 1;
 	} else if (tempfile->filename.len) {
 		/* This shouldn't happen, but better safe than sorry. */
diff --git a/tempfile.h b/tempfile.h
index d30663182d..2ee24f4380 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -1,6 +1,8 @@
 #ifndef TEMPFILE_H
 #define TEMPFILE_H
 
+#include "list.h"
+
 /*
  * Handle temporary files.
  *
@@ -81,7 +83,7 @@
  */
 
 struct tempfile {
-	struct tempfile *volatile next;
+	volatile struct volatile_list_head list;
 	volatile sig_atomic_t active;
 	volatile int fd;
 	FILE *volatile fp;
-- 
2.14.1.721.gc5bc1565f1

[PATCH 16/20] tempfile: remove deactivated list entries

[Jeff King]

Once a "struct tempfile" is added to the global cleanup
list, it is never removed. This means that its storage must
remain valid for the lifetime of the program. For single-use
tempfiles and locks, this isn't a big deal: we just declare
the struct static. But for library code which may take
multiple simultaneous locks (like the ref code), they're
forced to allocate a struct on the heap and leak it.

This is mostly OK in practice. The size of the leak is
bounded by the number of refs, and most programs exit after
operating on a fixed number of refs (and allocate
simultaneous memory proportional to the number of ref
updates in the first place). But:

  1. It isn't hard to imagine a real leak: a program which
     runs for a long time taking a series of ref update
     instructions and fulfilling them one by one. I don't
     think we have such a program now, but it's certainly
     plausible.

  2. The leaked entries appear as false positives to
     tools like valgrind.

Let's relax this rule by keeping only "active" tempfiles on
the list. We can do this easily by moving the list-add
operation from prepare_tempfile_object to activate_tempfile,
and adding a deletion in deactivate_tempfile.

Existing callers do not need to be updated immediately.
They'll continue to leak any tempfile objects they may have
allocated, but that's no different than the status quo. We
can clean them up individually.

Signed-off-by: Jeff King <peff@peff.net>
---
 tempfile.c | 46 ++++++++++++++++++++--------------------------
 tempfile.h | 15 +++++----------
 2 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 11bda824cf..f82a5f3676 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -42,8 +42,7 @@
  *     states in which this condition doesn't hold). Client code should
  *     *not* rely on the filename being empty in this state.
  *   - `fd` is -1 and `fp` is `NULL`
- *   - the object is left registered in the `tempfile_list`, and
- *     `on_list` is set.
+ *   - the object is removed from `tempfile_list` (but could be used again)
  *
  * A temporary file is owned by the process that created it. The
  * `tempfile` has an `owner` field that records the owner's PID. This
@@ -92,36 +91,30 @@ static void remove_tempfiles_on_signal(int signo)
 	raise(signo);
 }
 
-/*
- * Initialize *tempfile if necessary and add it to tempfile_list.
- */
 static void prepare_tempfile_object(struct tempfile *tempfile)
 {
-	if (volatile_list_empty(&tempfile_list)) {
-		/* One-time initialization */
-		sigchain_push_common(remove_tempfiles_on_signal);
-		atexit(remove_tempfiles_on_exit);
-	}
-
-	if (is_tempfile_active(tempfile))
-		BUG("prepare_tempfile_object called for active object");
-	if (!tempfile->on_list) {
-		/* Initialize *tempfile and add it to tempfile_list: */
-		tempfile->fd = -1;
-		tempfile->fp = NULL;
-		tempfile->active = 0;
-		tempfile->owner = 0;
-		strbuf_init(&tempfile->filename, 0);
-		volatile_list_add(&tempfile->list, &tempfile_list);
-		tempfile->on_list = 1;
-	} else if (tempfile->filename.len) {
-		/* This shouldn't happen, but better safe than sorry. */
-		BUG("prepare_tempfile_object called for improperly-reset object");
-	}
+	tempfile->fd = -1;
+	tempfile->fp = NULL;
+	tempfile->active = 0;
+	tempfile->owner = 0;
+	INIT_LIST_HEAD(&tempfile->list);
+	strbuf_init(&tempfile->filename, 0);
 }
 
 static void activate_tempfile(struct tempfile *tempfile)
 {
+	static int initialized;
+
+	if (is_tempfile_active(tempfile))
+		BUG("activate_tempfile called for active object");
+
+	if (!initialized) {
+		sigchain_push_common(remove_tempfiles_on_signal);
+		atexit(remove_tempfiles_on_exit);
+		initialized = 1;
+	}
+
+	volatile_list_add(&tempfile->list, &tempfile_list);
 	tempfile->owner = getpid();
 	tempfile->active = 1;
 }
@@ -130,6 +123,7 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 {
 	tempfile->active = 0;
 	strbuf_release(&tempfile->filename);
+	volatile_list_del(&tempfile->list);
 }
 
 /* Make sure errno contains a meaningful value on error */
diff --git a/tempfile.h b/tempfile.h
index 2ee24f4380..e32b4df092 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -17,12 +17,9 @@
  *
  * The caller:
  *
- * * Allocates a `struct tempfile` either as a static variable or on
- *   the heap, initialized to zeros. Once you use the structure to
- *   call `create_tempfile()`, it belongs to the tempfile subsystem
- *   and its storage must remain valid throughout the life of the
- *   program (i.e. you cannot use an on-stack variable to hold this
- *   structure).
+ * * Allocates a `struct tempfile`. Once the structure is passed to
+ *   `create_tempfile()`, its storage must remain valid until
+ *   `delete_tempfile()` or `rename_tempfile()` is called on it.
  *
  * * Attempts to create a temporary file by calling
  *   `create_tempfile()`.
@@ -52,9 +49,8 @@
  *   temporary file by calling `close_tempfile_gently()`, and later call
  *   `delete_tempfile()` or `rename_tempfile()`.
  *
- * Even after the temporary file is renamed or deleted, the `tempfile`
- * object must not be freed or altered by the caller. However, it may
- * be reused; just pass it to another call of `create_tempfile()`.
+ * After the temporary file is renamed or deleted, the `tempfile`
+ * object may be reused or freed.
  *
  * If the program exits before `rename_tempfile()` or
  * `delete_tempfile()` is called, an `atexit(3)` handler will close
@@ -88,7 +84,6 @@ struct tempfile {
 	volatile int fd;
 	FILE *volatile fp;
 	volatile pid_t owner;
-	char on_list;
 	struct strbuf filename;
 };
 
-- 
2.14.1.721.gc5bc1565f1

[PATCH 17/20] tempfile: auto-allocate tempfiles on heap

[Jeff King]

The previous commit taught the tempfile code to give up
ownership over tempfiles that have been renamed or deleted.
That makes it possible to use a stack variable like this:

  struct tempfile t;

  create_tempfile(&t, ...);
  ...
  if (!err)
          rename_tempfile(&t, ...);
  else
          delete_tempfile(&t);

But doing it this way has a high potential for creating
memory errors. The tempfile we pass to create_tempfile()
ends up on a global linked list, and it's not safe for it to
go out of scope until we've called one of those two
deactivation functions.

Imagine that we add an early return from the function that
forgets to call delete_tempfile(). With a static or heap
tempfile variable, the worst case is that the tempfile hangs
around until the program exits (and some functions like
setup_shallow_temporary rely on this intentionally, creating
a tempfile and then leaving it for later cleanup).

But with a stack variable as above, this is a serious memory
error: the variable goes out of scope and may be filled with
garbage by the time the tempfile code looks at it.  Let's
see if we can make it harder to get this wrong.

Since many callers need to allocate arbitrary numbers of
tempfiles, we can't rely on static storage as a general
solution. So we need to turn to the heap. We could just ask
all callers to pass us a heap variable, but that puts the
burden on them to call free() at the right time.

Instead, let's have the tempfile code handle the heap
allocation _and_ the deallocation (when the tempfile is
deactivated and removed from the list).

This changes the return value of all of the creation
functions. For the cleanup functions (delete and rename),
we'll add one extra bit of safety: instead of taking a
tempfile pointer, we'll take a pointer-to-pointer and set it
to NULL after freeing the object. This makes it safe to
double-call functions like delete_tempfile(), as the second
call treats the NULL input as a noop. Several callsites
follow this pattern.

The resulting patch does have a fair bit of noise, as each
caller needs to be converted to handle:

  1. Storing a pointer instead of the struct itself.

  2. Passing the pointer instead of taking the struct
     address.

  3. Handling a "struct tempfile *" return instead of a file
     descriptor.

We could play games to make this less noisy. For example, by
defining the tempfile like this:

  struct tempfile {
	struct heap_allocated_part_of_tempfile {
                int fd;
                ...etc
        } *actual_data;
  }

Callers would continue to have a "struct tempfile", and it
would be "active" only when the inner pointer was non-NULL.
But that just makes things more awkward in the long run.
There aren't that many callers, so we can simply bite
the bullet and adjust all of them. And the compiler makes it
easy for us to find them all.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c               |  8 ++---
 credential-cache--daemon.c |  5 ++-
 diff.c                     | 15 ++++----
 gpg-interface.c            | 16 ++++-----
 lockfile.c                 |  7 ++--
 lockfile.h                 | 16 ++++-----
 read-cache.c               | 25 +++++++-------
 refs/files-backend.c       |  4 +--
 refs/packed-backend.c      | 11 +++---
 shallow.c                  | 13 ++++---
 tempfile.c                 | 66 +++++++++++++++++++----------------
 tempfile.h                 | 85 +++++++++++++++++++++-------------------------
 trailer.c                  |  6 ++--
 13 files changed, 136 insertions(+), 141 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c78fcb9b1..c22787ac72 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -47,7 +47,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT;
 static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
 static struct argv_array rerere = ARGV_ARRAY_INIT;
 
-static struct tempfile pidfile;
+static struct tempfile *pidfile;
 static struct lock_file log_lock;
 
 static struct string_list pack_garbage = STRING_LIST_INIT_DUP;
@@ -78,7 +78,7 @@ static void process_log_file(void)
 		 */
 		int saved_errno = errno;
 		fprintf(stderr, _("Failed to fstat %s: %s"),
-			get_tempfile_path(&log_lock.tempfile),
+			get_tempfile_path(log_lock.tempfile),
 			strerror(saved_errno));
 		fflush(stderr);
 		commit_lock_file(&log_lock);
@@ -242,7 +242,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	int fd;
 	char *pidfile_path;
 
-	if (is_tempfile_active(&pidfile))
+	if (is_tempfile_active(pidfile))
 		/* already locked */
 		return NULL;
 
@@ -293,7 +293,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 	commit_lock_file(&lock);
-	register_tempfile(&pidfile, pidfile_path);
+	pidfile = register_tempfile(pidfile_path);
 	free(pidfile_path);
 	return NULL;
 }
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 0d5c625094..4dfbc8c9f9 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -5,8 +5,6 @@
 #include "unix-socket.h"
 #include "parse-options.h"
 
-static struct tempfile socket_file;
-
 struct credential_cache_entry {
 	struct credential item;
 	timestamp_t expiration;
@@ -260,6 +258,7 @@ static void init_socket_directory(const char *path)
 
 int cmd_main(int argc, const char **argv)
 {
+	struct tempfile *socket_file;
 	const char *socket_path;
 	int ignore_sighup = 0;
 	static const char *usage[] = {
@@ -285,7 +284,7 @@ int cmd_main(int argc, const char **argv)
 		die("socket directory must be an absolute path");
 
 	init_socket_directory(socket_path);
-	register_tempfile(&socket_file, socket_path);
+	socket_file = register_tempfile(socket_path);
 
 	if (ignore_sighup)
 		signal(SIGHUP, SIG_IGN);
diff --git a/diff.c b/diff.c
index 20f68ec389..7df2227a3c 100644
--- a/diff.c
+++ b/diff.c
@@ -459,7 +459,7 @@ static struct diff_tempfile {
 	 * If this diff_tempfile instance refers to a temporary file,
 	 * this tempfile object is used to manage its lifetime.
 	 */
-	struct tempfile tempfile;
+	struct tempfile *tempfile;
 } diff_temp[2];
 
 struct emit_callback {
@@ -1414,7 +1414,7 @@ static void remove_tempfile(void)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
-		if (is_tempfile_active(&diff_temp[i].tempfile))
+		if (is_tempfile_active(diff_temp[i].tempfile))
 			delete_tempfile(&diff_temp[i].tempfile);
 		diff_temp[i].name = NULL;
 	}
@@ -3720,7 +3720,6 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 			   const struct object_id *oid,
 			   int mode)
 {
-	int fd;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf template = STRBUF_INIT;
 	char *path_dup = xstrdup(path);
@@ -3730,18 +3729,18 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 	strbuf_addstr(&template, "XXXXXX_");
 	strbuf_addstr(&template, base);
 
-	fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1);
-	if (fd < 0)
+	temp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1);
+	if (!temp->tempfile)
 		die_errno("unable to create temp-file");
 	if (convert_to_working_tree(path,
 			(const char *)blob, (size_t)size, &buf)) {
 		blob = buf.buf;
 		size = buf.len;
 	}
-	if (write_in_full(fd, blob, size) != size ||
-	    close_tempfile_gently(&temp->tempfile))
+	if (write_in_full(temp->tempfile->fd, blob, size) != size ||
+	    close_tempfile_gently(temp->tempfile))
 		die_errno("unable to write temp-file");
-	temp->name = get_tempfile_path(&temp->tempfile);
+	temp->name = get_tempfile_path(temp->tempfile);
 	oid_to_hex_r(temp->hex, oid);
 	xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
 	strbuf_release(&buf);
diff --git a/gpg-interface.c b/gpg-interface.c
index 4ea2597ff4..4feacf16e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -202,17 +202,17 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	static struct tempfile temp;
-	int fd, ret;
+	struct tempfile *temp;
+	int ret;
 	struct strbuf buf = STRBUF_INIT;
 
-	fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
-	if (fd < 0)
+	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
+	if (!temp)
 		return error_errno(_("could not create temporary file"));
-	if (write_in_full(fd, signature, signature_size) < 0 ||
-	    close_tempfile_gently(&temp) < 0) {
+	if (write_in_full(temp->fd, signature, signature_size) < 0 ||
+	    close_tempfile_gently(temp) < 0) {
 		error_errno(_("failed writing detached signature to '%s'"),
-			    temp.filename.buf);
+			    temp->filename.buf);
 		delete_tempfile(&temp);
 		return -1;
 	}
@@ -221,7 +221,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 gpg_program,
 			 "--status-fd=1",
 			 "--keyid-format=long",
-			 "--verify", temp.filename.buf, "-",
+			 "--verify", temp->filename.buf, "-",
 			 NULL);
 
 	if (!gpg_status)
diff --git a/lockfile.c b/lockfile.c
index aa69210d8b..efcb7d7dfe 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -72,7 +72,6 @@ static void resolve_symlink(struct strbuf *path)
 /* Make sure errno contains a meaningful value on error */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	int fd;
 	struct strbuf filename = STRBUF_INIT;
 
 	strbuf_addstr(&filename, path);
@@ -80,9 +79,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(&filename);
 
 	strbuf_addstr(&filename, LOCK_SUFFIX);
-	fd = create_tempfile(&lk->tempfile, filename.buf);
+	lk->tempfile = create_tempfile(filename.buf);
 	strbuf_release(&filename);
-	return fd;
+	return lk->tempfile ? lk->tempfile->fd : -1;
 }
 
 /*
@@ -191,7 +190,7 @@ char *get_locked_file_path(struct lock_file *lk)
 {
 	struct strbuf ret = STRBUF_INIT;
 
-	strbuf_addstr(&ret, get_tempfile_path(&lk->tempfile));
+	strbuf_addstr(&ret, get_tempfile_path(lk->tempfile));
 	if (ret.len <= LOCK_SUFFIX_LEN ||
 	    strcmp(ret.buf + ret.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))
 		die("BUG: get_locked_file_path() called for malformed lock object");
diff --git a/lockfile.h b/lockfile.h
index c45d2db324..6ed336e2bc 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -111,7 +111,7 @@
 #include "tempfile.h"
 
 struct lock_file {
-	struct tempfile tempfile;
+	struct tempfile *tempfile;
 };
 
 /* String appended to a filename to derive the lockfile name: */
@@ -180,7 +180,7 @@ static inline int hold_lock_file_for_update(
  */
 static inline int is_lock_file_locked(struct lock_file *lk)
 {
-	return is_tempfile_active(&lk->tempfile);
+	return is_tempfile_active(lk->tempfile);
 }
 
 /*
@@ -208,7 +208,7 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
  */
 static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
 {
-	return fdopen_tempfile(&lk->tempfile, mode);
+	return fdopen_tempfile(lk->tempfile, mode);
 }
 
 /*
@@ -217,17 +217,17 @@ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
  */
 static inline const char *get_lock_file_path(struct lock_file *lk)
 {
-	return get_tempfile_path(&lk->tempfile);
+	return get_tempfile_path(lk->tempfile);
 }
 
 static inline int get_lock_file_fd(struct lock_file *lk)
 {
-	return get_tempfile_fd(&lk->tempfile);
+	return get_tempfile_fd(lk->tempfile);
 }
 
 static inline FILE *get_lock_file_fp(struct lock_file *lk)
 {
-	return get_tempfile_fp(&lk->tempfile);
+	return get_tempfile_fp(lk->tempfile);
 }
 
 /*
@@ -246,7 +246,7 @@ extern char *get_locked_file_path(struct lock_file *lk);
  */
 static inline int close_lock_file_gently(struct lock_file *lk)
 {
-	return close_tempfile_gently(&lk->tempfile);
+	return close_tempfile_gently(lk->tempfile);
 }
 
 /*
@@ -270,7 +270,7 @@ static inline int close_lock_file_gently(struct lock_file *lk)
  */
 static inline int reopen_lock_file(struct lock_file *lk)
 {
-	return reopen_tempfile(&lk->tempfile);
+	return reopen_tempfile(lk->tempfile);
 }
 
 /*
diff --git a/read-cache.c b/read-cache.c
index c3be65f8b0..b211c57af6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2311,7 +2311,7 @@ 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);
+		delete_tempfile(&tempfile);
 		return -1;
 	}
 	if (stat(tempfile->filename.buf, &st))
@@ -2337,7 +2337,7 @@ static int commit_locked_index(struct lock_file *lk)
 static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
 				 unsigned flags)
 {
-	int ret = do_write_index(istate, &lock->tempfile, 0);
+	int ret = do_write_index(istate, lock->tempfile, 0);
 	if (ret)
 		return ret;
 	assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
@@ -2420,34 +2420,33 @@ static int clean_shared_index_files(const char *current_hex)
 	return 0;
 }
 
-static struct tempfile temporary_sharedindex;
-
 static int write_shared_index(struct index_state *istate,
 			      struct lock_file *lock, unsigned flags)
 {
+	struct tempfile *temp;
 	struct split_index *si = istate->split_index;
-	int fd, ret;
+	int ret;
 
-	fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
-	if (fd < 0) {
+	temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+	if (!temp) {
 		hashclr(si->base_sha1);
 		return do_write_locked_index(istate, lock, flags);
 	}
 	move_cache_to_base_index(istate);
-	ret = do_write_index(si->base, &temporary_sharedindex, 1);
+	ret = do_write_index(si->base, temp, 1);
 	if (ret) {
-		delete_tempfile(&temporary_sharedindex);
+		delete_tempfile(&temp);
 		return ret;
 	}
-	ret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex));
+	ret = adjust_shared_perm(get_tempfile_path(temp));
 	if (ret) {
 		int save_errno = errno;
-		error("cannot fix permission bits on %s", get_tempfile_path(&temporary_sharedindex));
-		delete_tempfile(&temporary_sharedindex);
+		error("cannot fix permission bits on %s", get_tempfile_path(temp));
+		delete_tempfile(&temp);
 		errno = save_errno;
 		return ret;
 	}
-	ret = rename_tempfile(&temporary_sharedindex,
+	ret = rename_tempfile(&temp,
 			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
 	if (!ret) {
 		hashcpy(si->base_sha1, si->base->sha1);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bc1899cd4a..36fe8a986b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1747,12 +1747,12 @@ static int create_symref_locked(struct files_ref_store *refs,
 
 	if (!fdopen_lock_file(lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
-			     lock->lk->tempfile.filename.buf, strerror(errno));
+			     lock->lk->tempfile->filename.buf, strerror(errno));
 
 	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
-	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
+	fprintf(lock->lk->tempfile->fp, "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dca887c0fe..321608a114 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -75,7 +75,7 @@ struct packed_ref_store {
 	 * "packed-refs" file. Note that this (and thus the enclosing
 	 * `packed_ref_store`) must not be freed.
 	 */
-	struct tempfile tempfile;
+	struct tempfile *tempfile;
 };
 
 struct ref_store *packed_ref_store_create(const char *path,
@@ -628,7 +628,8 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	 */
 	packed_refs_path = get_locked_file_path(&refs->lock);
 	strbuf_addf(&sb, "%s.new", packed_refs_path);
-	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
+	refs->tempfile = create_tempfile(sb.buf);
+	if (!refs->tempfile) {
 		strbuf_addf(err, "unable to create file %s: %s",
 			    sb.buf, strerror(errno));
 		strbuf_release(&sb);
@@ -636,7 +637,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	}
 	strbuf_release(&sb);
 
-	out = fdopen_tempfile(&refs->tempfile, "w");
+	out = fdopen_tempfile(refs->tempfile, "w");
 	if (!out) {
 		strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
 			    strerror(errno));
@@ -645,7 +646,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 
 	if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
 		strbuf_addf(err, "error writing to %s: %s",
-			    get_tempfile_path(&refs->tempfile), strerror(errno));
+			    get_tempfile_path(refs->tempfile), strerror(errno));
 		goto error;
 	}
 
@@ -657,7 +658,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		if (write_packed_entry(out, iter->refname, iter->oid->hash,
 				       peel_error ? NULL : peeled.hash)) {
 			strbuf_addf(err, "error writing to %s: %s",
-				    get_tempfile_path(&refs->tempfile),
+				    get_tempfile_path(refs->tempfile),
 				    strerror(errno));
 			ref_iterator_abort(iter);
 			goto error;
diff --git a/shallow.c b/shallow.c
index 36216febb6..1cc1c76415 100644
--- a/shallow.c
+++ b/shallow.c
@@ -288,19 +288,18 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 
 const char *setup_temporary_shallow(const struct oid_array *extra)
 {
-	static struct tempfile temp;
+	struct tempfile *temp;
 	struct strbuf sb = STRBUF_INIT;
-	int fd;
 
 	if (write_shallow_commits(&sb, 0, extra)) {
-		fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
+		temp = xmks_tempfile(git_path("shallow_XXXXXX"));
 
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
-		    close_tempfile_gently(&temp) < 0)
+		if (write_in_full(temp->fd, sb.buf, sb.len) != sb.len ||
+		    close_tempfile_gently(temp) < 0)
 			die_errno("failed to write to %s",
-				  get_tempfile_path(&temp));
+				  get_tempfile_path(temp));
 		strbuf_release(&sb);
-		return get_tempfile_path(&temp);
+		return get_tempfile_path(temp);
 	}
 	/*
 	 * is_repository_shallow() sees empty string as "no shallow
diff --git a/tempfile.c b/tempfile.c
index f82a5f3676..5fdafdd2d2 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -91,14 +91,16 @@ static void remove_tempfiles_on_signal(int signo)
 	raise(signo);
 }
 
-static void prepare_tempfile_object(struct tempfile *tempfile)
+static struct tempfile *new_tempfile(void)
 {
+	struct tempfile *tempfile = xmalloc(sizeof(*tempfile));
 	tempfile->fd = -1;
 	tempfile->fp = NULL;
 	tempfile->active = 0;
 	tempfile->owner = 0;
 	INIT_LIST_HEAD(&tempfile->list);
 	strbuf_init(&tempfile->filename, 0);
+	return tempfile;
 }
 
 static void activate_tempfile(struct tempfile *tempfile)
@@ -124,12 +126,13 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 	tempfile->active = 0;
 	strbuf_release(&tempfile->filename);
 	volatile_list_del(&tempfile->list);
+	free(tempfile);
 }
 
 /* Make sure errno contains a meaningful value on error */
-int create_tempfile(struct tempfile *tempfile, const char *path)
+struct tempfile *create_tempfile(const char *path)
 {
-	prepare_tempfile_object(tempfile);
+	struct tempfile *tempfile = new_tempfile();
 
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
@@ -140,48 +143,47 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
 				    O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
-		return -1;
+		return NULL;
 	}
 	activate_tempfile(tempfile);
 	if (adjust_shared_perm(tempfile->filename.buf)) {
 		int save_errno = errno;
 		error("cannot fix permission bits on %s", tempfile->filename.buf);
-		delete_tempfile(tempfile);
+		delete_tempfile(&tempfile);
 		errno = save_errno;
-		return -1;
+		return NULL;
 	}
-	return tempfile->fd;
+
+	return tempfile;
 }
 
-void register_tempfile(struct tempfile *tempfile, const char *path)
+struct tempfile *register_tempfile(const char *path)
 {
-	prepare_tempfile_object(tempfile);
+	struct tempfile *tempfile = new_tempfile();
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	activate_tempfile(tempfile);
+	return tempfile;
 }
 
-int mks_tempfile_sm(struct tempfile *tempfile,
-		    const char *template, int suffixlen, int mode)
+struct tempfile *mks_tempfile_sm(const char *template, int suffixlen, int mode)
 {
-	prepare_tempfile_object(tempfile);
+	struct tempfile *tempfile = new_tempfile();
 
 	strbuf_add_absolute_path(&tempfile->filename, template);
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
-		return -1;
+		return NULL;
 	}
 	activate_tempfile(tempfile);
-	return tempfile->fd;
+	return tempfile;
 }
 
-int mks_tempfile_tsm(struct tempfile *tempfile,
-		     const char *template, int suffixlen, int mode)
+struct tempfile *mks_tempfile_tsm(const char *template, int suffixlen, int mode)
 {
+	struct tempfile *tempfile = new_tempfile();
 	const char *tmpdir;
 
-	prepare_tempfile_object(tempfile);
-
 	tmpdir = getenv("TMPDIR");
 	if (!tmpdir)
 		tmpdir = "/tmp";
@@ -190,25 +192,25 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
-		return -1;
+		return NULL;
 	}
 	activate_tempfile(tempfile);
-	return tempfile->fd;
+	return tempfile;
 }
 
-int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)
+struct tempfile *xmks_tempfile_m(const char *template, int mode)
 {
-	int fd;
+	struct tempfile *tempfile;
 	struct strbuf full_template = STRBUF_INIT;
 
 	strbuf_add_absolute_path(&full_template, template);
-	fd = mks_tempfile_m(tempfile, full_template.buf, mode);
-	if (fd < 0)
+	tempfile = mks_tempfile_m(full_template.buf, mode);
+	if (!tempfile)
 		die_errno("Unable to create temporary file '%s'",
 			  full_template.buf);
 
 	strbuf_release(&full_template);
-	return fd;
+	return tempfile;
 }
 
 FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
@@ -281,33 +283,39 @@ int reopen_tempfile(struct tempfile *tempfile)
 	return tempfile->fd;
 }
 
-int rename_tempfile(struct tempfile *tempfile, const char *path)
+int rename_tempfile(struct tempfile **tempfile_p, const char *path)
 {
+	struct tempfile *tempfile = *tempfile_p;
+
 	if (!is_tempfile_active(tempfile))
 		BUG("rename_tempfile called for inactive object");
 
 	if (close_tempfile_gently(tempfile)) {
-		delete_tempfile(tempfile);
+		delete_tempfile(tempfile_p);
 		return -1;
 	}
 
 	if (rename(tempfile->filename.buf, path)) {
 		int save_errno = errno;
-		delete_tempfile(tempfile);
+		delete_tempfile(tempfile_p);
 		errno = save_errno;
 		return -1;
 	}
 
 	deactivate_tempfile(tempfile);
+	*tempfile_p = NULL;
 	return 0;
 }
 
-void delete_tempfile(struct tempfile *tempfile)
+void delete_tempfile(struct tempfile **tempfile_p)
 {
+	struct tempfile *tempfile = *tempfile_p;
+
 	if (!is_tempfile_active(tempfile))
 		return;
 
 	close_tempfile_gently(tempfile);
 	unlink_or_warn(tempfile->filename.buf);
 	deactivate_tempfile(tempfile);
+	*tempfile_p = NULL;
 }
diff --git a/tempfile.h b/tempfile.h
index e32b4df092..b8f4b5e145 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -17,22 +17,18 @@
  *
  * The caller:
  *
- * * Allocates a `struct tempfile`. Once the structure is passed to
- *   `create_tempfile()`, its storage must remain valid until
- *   `delete_tempfile()` or `rename_tempfile()` is called on it.
- *
  * * Attempts to create a temporary file by calling
- *   `create_tempfile()`.
+ *   `create_tempfile()`. The resources used for the temporary file are
+ *   managed by the tempfile API.
  *
  * * Writes new content to the file by either:
  *
- *   * writing to the file descriptor returned by `create_tempfile()`
- *     (also available via `tempfile->fd`).
+ *   * writing to the `tempfile->fd` file descriptor
  *
  *   * calling `fdopen_tempfile()` to get a `FILE` pointer for the
  *     open file and writing to the file using stdio.
  *
- *   Note that the file descriptor returned by create_tempfile()
+ *   Note that the file descriptor created by create_tempfile()
  *   is marked O_CLOEXEC, so the new contents must be written by
  *   the current process, not any spawned one.
  *
@@ -50,7 +46,7 @@
  *   `delete_tempfile()` or `rename_tempfile()`.
  *
  * After the temporary file is renamed or deleted, the `tempfile`
- * object may be reused or freed.
+ * object is no longer valid and should not be reused.
  *
  * If the program exits before `rename_tempfile()` or
  * `delete_tempfile()` is called, an `atexit(3)` handler will close
@@ -69,8 +65,8 @@
  * Error handling
  * --------------
  *
- * `create_tempfile()` returns a file descriptor on success or -1 on
- * failure. On errors, `errno` describes the reason for failure.
+ * `create_tempfile()` returns an allocated tempfile on success or NULL
+ * on failure. On errors, `errno` describes the reason for failure.
  *
  * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
  * return 0 on success. On failure they set `errno` appropriately and return
@@ -89,10 +85,10 @@ struct tempfile {
 
 /*
  * Attempt to create a temporary file at the specified `path`. Return
- * a file descriptor for writing to it, or -1 on error. It is an error
- * if a file already exists at that path.
+ * a tempfile (whose "fd" member can be used for writing to it), or
+ * NULL on error. It is an error if a file already exists at that path.
  */
-extern int create_tempfile(struct tempfile *tempfile, const char *path);
+extern struct tempfile *create_tempfile(const char *path);
 
 /*
  * Register an existing file as a tempfile, meaning that it will be
@@ -100,7 +96,7 @@ extern int create_tempfile(struct tempfile *tempfile, const char *path);
  * but it can be worked with like any other closed tempfile (for
  * example, it can be opened using reopen_tempfile()).
  */
-extern void register_tempfile(struct tempfile *tempfile, const char *path);
+extern struct tempfile *register_tempfile(const char *path);
 
 
 /*
@@ -132,70 +128,65 @@ extern void register_tempfile(struct tempfile *tempfile, const char *path);
  * know the (absolute) path of the file that was created, it can be
  * read from tempfile->filename.
  *
- * On success, the functions return a file descriptor that is open for
- * writing the temporary file. On errors, they return -1 and set errno
- * appropriately (except for the "x" variants, which die() on errors).
+ * On success, the functions return a tempfile whose "fd" member is open
+ * for writing the temporary file. On errors, they return NULL and set
+ * errno appropriately (except for the "x" variants, which die() on
+ * errors).
  */
 
 /* See "mks_tempfile functions" above. */
-extern int mks_tempfile_sm(struct tempfile *tempfile,
-			   const char *template, int suffixlen, int mode);
+extern struct tempfile *mks_tempfile_sm(const char *template,
+					int suffixlen, int mode);
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile_s(struct tempfile *tempfile,
-				 const char *template, int suffixlen)
+static inline struct tempfile *mks_tempfile_s(const char *template,
+					      int suffixlen)
 {
-	return mks_tempfile_sm(tempfile, template, suffixlen, 0600);
+	return mks_tempfile_sm(template, suffixlen, 0600);
 }
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile_m(struct tempfile *tempfile,
-				 const char *template, int mode)
+static inline struct tempfile *mks_tempfile_m(const char *template, int mode)
 {
-	return mks_tempfile_sm(tempfile, template, 0, mode);
+	return mks_tempfile_sm(template, 0, mode);
 }
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile(struct tempfile *tempfile,
-			       const char *template)
+static inline struct tempfile *mks_tempfile(const char *template)
 {
-	return mks_tempfile_sm(tempfile, template, 0, 0600);
+	return mks_tempfile_sm(template, 0, 0600);
 }
 
 /* See "mks_tempfile functions" above. */
-extern int mks_tempfile_tsm(struct tempfile *tempfile,
-			    const char *template, int suffixlen, int mode);
+extern struct tempfile *mks_tempfile_tsm(const char *template,
+					 int suffixlen, int mode);
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile_ts(struct tempfile *tempfile,
-				  const char *template, int suffixlen)
+static inline struct tempfile *mks_tempfile_ts(const char *template,
+					       int suffixlen)
 {
-	return mks_tempfile_tsm(tempfile, template, suffixlen, 0600);
+	return mks_tempfile_tsm(template, suffixlen, 0600);
 }
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile_tm(struct tempfile *tempfile,
-				  const char *template, int mode)
+static inline struct tempfile *mks_tempfile_tm(const char *template, int mode)
 {
-	return mks_tempfile_tsm(tempfile, template, 0, mode);
+	return mks_tempfile_tsm(template, 0, mode);
 }
 
 /* See "mks_tempfile functions" above. */
-static inline int mks_tempfile_t(struct tempfile *tempfile,
-				 const char *template)
+static inline struct tempfile *mks_tempfile_t(const char *template)
 {
-	return mks_tempfile_tsm(tempfile, template, 0, 0600);
+	return mks_tempfile_tsm(template, 0, 0600);
 }
 
 /* See "mks_tempfile functions" above. */
-extern int xmks_tempfile_m(struct tempfile *tempfile,
-			   const char *template, int mode);
+extern struct tempfile *xmks_tempfile_m(const char *template, int mode);
 
 /* See "mks_tempfile functions" above. */
-static inline int xmks_tempfile(struct tempfile *tempfile,
-				const char *template)
+static inline struct tempfile *xmks_tempfile(const char *template)
 {
-	return xmks_tempfile_m(tempfile, template, 0600);
+	return xmks_tempfile_m(template, 0600);
 }
 
 /*
@@ -257,7 +248,7 @@ extern int reopen_tempfile(struct tempfile *tempfile);
  * `delete_tempfile()` for a `tempfile` object that has already been
  * deleted or renamed.
  */
-extern void delete_tempfile(struct tempfile *tempfile);
+extern void delete_tempfile(struct tempfile **tempfile_p);
 
 /*
  * Close the file descriptor and/or file pointer if they are still
@@ -268,6 +259,6 @@ extern void delete_tempfile(struct tempfile *tempfile);
  * `rename(2)`. It is a bug to call `rename_tempfile()` for a
  * `tempfile` object that is not currently active.
  */
-extern int rename_tempfile(struct tempfile *tempfile, const char *path);
+extern int rename_tempfile(struct tempfile **tempfile_p, const char *path);
 
 #endif /* TEMPFILE_H */
diff --git a/trailer.c b/trailer.c
index c30e3a0c04..3ba157ed0d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -995,7 +995,7 @@ static void free_all(struct list_head *head)
 	}
 }
 
-static struct tempfile trailers_tempfile;
+static struct tempfile *trailers_tempfile;
 
 static FILE *create_in_place_tempfile(const char *file)
 {
@@ -1017,9 +1017,9 @@ static FILE *create_in_place_tempfile(const char *file)
 		strbuf_add(&template, file, tail - file + 1);
 	strbuf_addstr(&template, "git-interpret-trailers-XXXXXX");
 
-	xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);
+	trailers_tempfile = xmks_tempfile_m(template.buf, st.st_mode);
 	strbuf_release(&template);
-	outfile = fdopen_tempfile(&trailers_tempfile, "w");
+	outfile = fdopen_tempfile(trailers_tempfile, "w");
 	if (!outfile)
 		die_errno(_("could not open temporary file"));
 
-- 
2.14.1.721.gc5bc1565f1

[PATCH 18/20] lockfile: update lifetime requirements in documentation

[Jeff King]

Now that the tempfile system we rely on has loosened the
lifetime requirements for storage, we can adjust our
documentation to match.

Signed-off-by: Jeff King <peff@peff.net>
---
 lockfile.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lockfile.h b/lockfile.h
index 6ed336e2bc..7c1c484d7c 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -37,12 +37,12 @@
  *
  * The caller:
  *
- * * Allocates a `struct lock_file` either as a static variable or on
- *   the heap, initialized to zeros. Once you use the structure to
- *   call the `hold_lock_file_for_*()` family of functions, it belongs
- *   to the lockfile subsystem and its storage must remain valid
- *   throughout the life of the program (i.e. you cannot use an
- *   on-stack variable to hold this structure).
+ * * Allocates a `struct lock_file` with whatever storage duration you
+ *   desire. The struct does not have to be initialized before being
+ *   used, but it is good practice to do so using by setting it to
+ *   all-zeros (or using the LOCK_INIT macro). This puts the object in a
+ *   consistent state that allows you to call rollback_lock_file() even
+ *   if the lock was never taken (in which case it is a noop).
  *
  * * Attempts to create a lockfile by calling `hold_lock_file_for_update()`.
  *
@@ -73,10 +73,8 @@
  *   `commit_lock_file()`, `commit_lock_file_to()`,
  *   `rollback_lock_file()`, or `reopen_lock_file()`.
  *
- * Even after the lockfile is committed or rolled back, the
- * `lock_file` object must not be freed or altered by the caller.
- * However, it may be reused; just pass it to another call of
- * `hold_lock_file_for_update()`.
+ * After the lockfile is committed or rolled back, the `lock_file`
+ * object can be discarded or reused.
  *
  * If the program exits before `commit_lock_file()`,
  * `commit_lock_file_to()`, or `rollback_lock_file()` is called, the
@@ -114,6 +112,8 @@ struct lock_file {
 	struct tempfile *tempfile;
 };
 
+#define LOCK_INIT { NULL }
+
 /* String appended to a filename to derive the lockfile name: */
 #define LOCK_SUFFIX ".lock"
 #define LOCK_SUFFIX_LEN 5
-- 
2.14.1.721.gc5bc1565f1

[PATCH 19/20] ref_lock: stop leaking lock_files

[Jeff King]

Since the tempfile code recently relaxed the rule that
tempfile structs (and thus locks) need to hang around
forever, we no longer have to leak our lock_file structs.

In fact, we don't even need to heap-allocate them anymore,
since their lifetime can just match that of the surrounding
ref_lock (and if we forget to delete a lock, the effect is
the same as before: it will eventually go away at program
exit).

Note that there is a check in unlock_ref() to only rollback
a lock file if it has been allocated. We don't need that
check anymore; we zero the ref_lock (and thus the
lock_file), so at worst we pass a NULL pointer to
delete_tempfile(), which considers that a noop.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 36fe8a986b..f3455609d6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -12,7 +12,7 @@
 
 struct ref_lock {
 	char *ref_name;
-	struct lock_file *lk;
+	struct lock_file lk;
 	struct object_id old_oid;
 };
 
@@ -418,9 +418,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 
 static void unlock_ref(struct ref_lock *lock)
 {
-	/* Do not free lock->lk -- atexit() still looks at them */
-	if (lock->lk)
-		rollback_lock_file(lock->lk);
+	rollback_lock_file(&lock->lk);
 	free(lock->ref_name);
 	free(lock);
 }
@@ -534,11 +532,8 @@ static int lock_raw_ref(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (!lock->lk)
-		lock->lk = xcalloc(1, sizeof(struct lock_file));
-
 	if (hold_lock_file_for_update_timeout(
-			    lock->lk, ref_file.buf, LOCK_NO_DEREF,
+			    &lock->lk, ref_file.buf, LOCK_NO_DEREF,
 			    get_files_ref_lock_timeout_ms()) < 0) {
 		if (errno == ENOENT && --attempts_remaining > 0) {
 			/*
@@ -949,11 +944,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	lock->lk = xcalloc(1, sizeof(struct lock_file));
-
 	lock->ref_name = xstrdup(refname);
 
-	if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {
+	if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
 		last_errno = errno;
 		unable_to_lock_message(ref_file.buf, errno, err);
 		goto error_return;
@@ -1404,14 +1397,14 @@ static int files_rename_ref(struct ref_store *ref_store,
 
 static int close_ref_gently(struct ref_lock *lock)
 {
-	if (close_lock_file_gently(lock->lk))
+	if (close_lock_file_gently(&lock->lk))
 		return -1;
 	return 0;
 }
 
 static int commit_ref(struct ref_lock *lock)
 {
-	char *path = get_locked_file_path(lock->lk);
+	char *path = get_locked_file_path(&lock->lk);
 	struct stat st;
 
 	if (!lstat(path, &st) && S_ISDIR(st.st_mode)) {
@@ -1435,7 +1428,7 @@ static int commit_ref(struct ref_lock *lock)
 		free(path);
 	}
 
-	if (commit_lock_file(lock->lk))
+	if (commit_lock_file(&lock->lk))
 		return -1;
 	return 0;
 }
@@ -1627,12 +1620,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	fd = get_lock_file_fd(lock->lk);
+	fd = get_lock_file_fd(&lock->lk);
 	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 	    write_in_full(fd, &term, 1) != 1 ||
 	    close_ref_gently(lock) < 0) {
 		strbuf_addf(err,
-			    "couldn't write '%s'", get_lock_file_path(lock->lk));
+			    "couldn't write '%s'", get_lock_file_path(&lock->lk));
 		unlock_ref(lock);
 		return -1;
 	}
@@ -1709,7 +1702,7 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {
 	int ret = -1;
 #ifndef NO_SYMLINK_HEAD
-	char *ref_path = get_locked_file_path(lock->lk);
+	char *ref_path = get_locked_file_path(&lock->lk);
 	unlink(ref_path);
 	ret = symlink(target, ref_path);
 	free(ref_path);
@@ -1745,14 +1738,14 @@ static int create_symref_locked(struct files_ref_store *refs,
 		return 0;
 	}
 
-	if (!fdopen_lock_file(lock->lk, "w"))
+	if (!fdopen_lock_file(&lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
-			     lock->lk->tempfile->filename.buf, strerror(errno));
+			     lock->lk.tempfile->filename.buf, strerror(errno));
 
 	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
-	fprintf(lock->lk->tempfile->fp, "ref: %s\n", target);
+	fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
@@ -2853,12 +2846,12 @@ static int files_reflog_expire(struct ref_store *ref_store,
 					strerror(errno));
 			rollback_lock_file(&reflog_lock);
 		} else if (update &&
-			   (write_in_full(get_lock_file_fd(lock->lk),
+			   (write_in_full(get_lock_file_fd(&lock->lk),
 				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
+			    write_str_in_full(get_lock_file_fd(&lock->lk), "\n") != 1 ||
 			    close_ref_gently(lock) < 0)) {
 			status |= error("couldn't write %s",
-					get_lock_file_path(lock->lk));
+					get_lock_file_path(&lock->lk));
 			rollback_lock_file(&reflog_lock);
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to write reflog '%s' (%s)",
-- 
2.14.1.721.gc5bc1565f1

[PATCH 20/20] stop leaking lock structs in some simple cases

[Jeff King]

Now that it's safe to declare a "struct lock_file" on the
stack, we can do so (and avoid an intentional leak). These
leaks were found by running t0000 and t0001 under valgrind
(though certainly other similar leaks exist and just don't
happen to be exercised by those tests).

Initializing the lock_file's inner tempfile with NULL is not
strictly necessary in these cases, but it's a good practice
to model.  It means that if we were to call a function like
rollback_lock_file() on a lock that was never taken in the
first place, it becomes a quiet noop (rather than undefined
behavior).

Likewise, it's always safe to rollback_lock_file() on a file
that has already been committed or deleted, since that
operation is a noop on an inactive lockfile (and that's why
the case in config.c can drop the "if (lock)" check as we
move away from using a pointer).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/reset.c        |  6 +++---
 builtin/update-index.c | 11 ++++-------
 cache-tree.c           | 14 ++++----------
 config.c               | 24 +++++++-----------------
 4 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index d72c7d1c96..f1af9345e4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -367,8 +367,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die_if_unmerged_cache(reset_type);
 
 	if (reset_type != SOFT) {
-		struct lock_file *lock = xcalloc(1, sizeof(*lock));
-		hold_locked_index(lock, LOCK_DIE_ON_ERROR);
+		struct lock_file lock = LOCK_INIT;
+		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
@@ -384,7 +384,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				die(_("Could not reset index file to revision '%s'."), rev);
 		}
 
-		if (write_locked_index(&the_index, lock, COMMIT_LOCK))
+		if (write_locked_index(&the_index, &lock, COMMIT_LOCK))
 			die(_("Could not write new index file."));
 	}
 
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d562f2ec69..655e6d60d3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -915,7 +915,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
-	struct lock_file *lock_file;
+	struct lock_file lock_file = LOCK_INIT;
 	struct parse_opt_ctx_t ctx;
 	strbuf_getline_fn getline_fn;
 	int parseopt_state = PARSE_OPT_UNKNOWN;
@@ -1014,11 +1014,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
 	/* we will diagnose later if it turns out that we need to update it */
-	newfd = hold_locked_index(lock_file, 0);
+	newfd = hold_locked_index(&lock_file, 0);
 	if (newfd < 0)
 		lock_error = errno;
 
@@ -1153,11 +1150,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 				exit(128);
 			unable_to_lock_die(get_index_file(), lock_error);
 		}
-		if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
+		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die("Unable to write new index file");
 	}
 
-	rollback_lock_file(lock_file);
+	rollback_lock_file(&lock_file);
 
 	return has_errors ? 1 : 0;
 }
diff --git a/cache-tree.c b/cache-tree.c
index 2690113a6a..71d092ed51 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -603,16 +603,10 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)
 {
 	int entries, was_valid, newfd;
-	struct lock_file *lock_file;
+	struct lock_file lock_file = LOCK_INIT;
 	int ret = 0;
 
-	/*
-	 * We can't free this memory, it becomes part of a linked list
-	 * parsed atexit()
-	 */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
-	newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);
+	newfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
 
 	entries = read_index_from(index_state, index_path);
 	if (entries < 0) {
@@ -632,7 +626,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 			goto out;
 		}
 		if (0 <= newfd) {
-			if (!write_locked_index(index_state, lock_file, COMMIT_LOCK))
+			if (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))
 				newfd = -1;
 		}
 		/* Not being able to write is fine -- we are only interested
@@ -657,7 +651,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 
 out:
 	if (0 <= newfd)
-		rollback_lock_file(lock_file);
+		rollback_lock_file(&lock_file);
 	return ret;
 }
 
diff --git a/config.c b/config.c
index d0d8ce823a..7931182a54 100644
--- a/config.c
+++ b/config.c
@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 {
 	int fd = -1, in_fd = -1;
 	int ret;
-	struct lock_file *lock = NULL;
+	struct lock_file lock = LOCK_INIT;
 	char *filename_buf = NULL;
 	char *contents = NULL;
 	size_t contents_sz;
@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	lock = xcalloc(1, sizeof(struct lock_file));
-	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	fd = hold_lock_file_for_update(&lock, config_filename, 0);
 	if (fd < 0) {
 		error_errno("could not lock config file %s", config_filename);
 		free(store.key);
@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		close(in_fd);
 		in_fd = -1;
 
-		if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
-			error_errno("chmod on %s failed", get_lock_file_path(lock));
+		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
+			error_errno("chmod on %s failed", get_lock_file_path(&lock));
 			ret = CONFIG_NO_WRITE;
 			goto out_free;
 		}
@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		contents = NULL;
 	}
 
-	if (commit_lock_file(lock) < 0) {
+	if (commit_lock_file(&lock) < 0) {
 		error_errno("could not write config file %s", config_filename);
 		ret = CONFIG_NO_WRITE;
-		lock = NULL;
 		goto out_free;
 	}
 
-	/*
-	 * lock is committed, so don't try to roll it back below.
-	 * NOTE: Since lockfile.c keeps a linked list of all created
-	 * lock_file structures, it isn't safe to free(lock).  It's
-	 * better to just leave it hanging around.
-	 */
-	lock = NULL;
 	ret = 0;
 
 	/* Invalidate the config cache */
 	git_config_clear();
 
 out_free:
-	if (lock)
-		rollback_lock_file(lock);
+	rollback_lock_file(&lock);
 	free(filename_buf);
 	if (contents)
 		munmap(contents, contents_sz);
@@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	return ret;
 
 write_err_out:
-	ret = write_error(get_lock_file_path(lock));
+	ret = write_error(get_lock_file_path(&lock));
 	goto out_free;
 
 }
-- 
2.14.1.721.gc5bc1565f1