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

[PATCH 13/20] tempfile: robustify cleanup handler

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

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
Previous: Jeff KingNext: Jeff King
Message 14 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.