tempfile: robustify cleanup handler

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/tempfile.c b/tempfile.c
index 9d7f0a2f2b82038fbad1674ca716b6754858d619..3348ad59dd3beb44e3057856edffaddea40a24c1 100644 (file)
--- 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;
        }
 }