git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Sep 5, 2017, 12:14 UTC
Message-ID
<20170905121440.hxbyspsruzrwigtq@sigill.intra.peff.net>
In-Reply-To
<20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net>

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
Previous: Jeff KingNext: Jeff King
Message 10 of 21 in “allowing struct lock_file to be deleted”
  1. 0/20 allowing struct lock_file to be deletedJeff King, Sep 5, 2017
  2. 01/20 write_index_as_tree: cleanup tempfile on errorJeff King, Sep 5, 2017
  3. 02/20 setup_temporary_shallow: avoid using inactive tempfileJeff King, Sep 5, 2017
  4. 03/20 setup_temporary_shallow: move tempfile struct into functionJeff King, Sep 5, 2017
  5. 04/20 verify_signed_buffer: prefer close_tempfile() to close()Jeff King, Sep 5, 2017
  6. 05/20 always check return value of close_tempfileJeff King, Sep 5, 2017
  7. 06/20 tempfile: do not delete tempfile on failed closeJeff King, Sep 5, 2017
  8. 07/20 lockfile: do not rollback lock on failed closeJeff King, Sep 5, 2017
  9. 08/20 tempfile: prefer is_tempfile_active to bare accessJeff King, Sep 5, 2017
  10. 09/20 tempfile: handle NULL tempfile pointers gracefullyJeff King, Sep 5, 2017
  11. 10/20 tempfile: replace die("BUG") with BUG()Jeff King, Sep 5, 2017
  12. 11/20 tempfile: factor out activationJeff King, Sep 5, 2017
  13. 12/20 tempfile: factor out deactivationJeff King, Sep 5, 2017
  14. 13/20 tempfile: robustify cleanup handlerJeff King, Sep 5, 2017
  15. 14/20 tempfile: release deactivated strbufs instead of resettingJeff King, Sep 5, 2017
  16. 16/20 tempfile: remove deactivated list entriesJeff King, Sep 5, 2017
  17. 17/20 tempfile: auto-allocate tempfiles on heapJeff King, Sep 5, 2017
  18. 18/20 lockfile: update lifetime requirements in documentationJeff King, Sep 5, 2017
  19. 19/20 ref_lock: stop leaking lock_filesJeff King, Sep 5, 2017
  20. 20/20 stop leaking lock structs in some simple casesJeff King, Sep 5, 2017
  21. 15/20 tempfile: use list.h for linked listJeff King, Sep 5, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.