]> Savannah Git Hosting - gnulib.git/commitdiff
clean-temp: Make multithread-safe, part 2.
authorBruno Haible <bruno@clisp.org>
Sat, 4 Jul 2020 12:39:09 +0000 (14:39 +0200)
committerBruno Haible <bruno@clisp.org>
Sat, 4 Jul 2020 12:39:09 +0000 (14:39 +0200)
* lib/fatal-signal.h: Include <signal.h>.
(get_fatal_signal_set): New declaration.
* lib/fatal-signal.c (get_fatal_signal_set): New function.
* lib/clean-temp.c: Include asyncsafe-spin.h, gl_linked_list.h.
(struct closeable_fd): New type.
(fatal_signal_set): New variable.
(init_fatal_signal_set): New function.
(asyncsafe_close, asyncsafe_fclose_variant): New functions.
(cleanup_action): Invoke asyncsafe_close instead of close.
(create_temp_dir): Invoke init_fatal_signal_set.
(register_fd): Use a plain linked list. Add a 'struct closeable_fd *'
element.
(unregister_fd): Remove function.
(close_temp): Cleanup descriptors list on the fly. Invoke
init_fatal_signal_set. Invoke asyncsafe_close instead of close.
(fclose_variant_temp): New function.
(fclose_temp, fwriteerror_temp, close_stream_temp): Use it.
* modules/clean-temp (Depends-on): Add asyncsafe-spin, linked-list.

ChangeLog
lib/clean-temp.c
lib/fatal-signal.c
lib/fatal-signal.h
modules/clean-temp

index e7b38210d6be1e4187a4b15a5c324aa5d26d8e10..d49c9d7a533ef21d0ebe1de4f0a9c9fdeb01a0bb 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2020-07-04  Bruno Haible  <bruno@clisp.org>
+
+       clean-temp: Make multithread-safe, part 2.
+       * lib/fatal-signal.h: Include <signal.h>.
+       (get_fatal_signal_set): New declaration.
+       * lib/fatal-signal.c (get_fatal_signal_set): New function.
+       * lib/clean-temp.c: Include asyncsafe-spin.h, gl_linked_list.h.
+       (struct closeable_fd): New type.
+       (fatal_signal_set): New variable.
+       (init_fatal_signal_set): New function.
+       (asyncsafe_close, asyncsafe_fclose_variant): New functions.
+       (cleanup_action): Invoke asyncsafe_close instead of close.
+       (create_temp_dir): Invoke init_fatal_signal_set.
+       (register_fd): Use a plain linked list. Add a 'struct closeable_fd *'
+       element.
+       (unregister_fd): Remove function.
+       (close_temp): Cleanup descriptors list on the fly. Invoke
+       init_fatal_signal_set. Invoke asyncsafe_close instead of close.
+       (fclose_variant_temp): New function.
+       (fclose_temp, fwriteerror_temp, close_stream_temp): Use it.
+       * modules/clean-temp (Depends-on): Add asyncsafe-spin, linked-list.
+
 2020-07-04  Bruno Haible  <bruno@clisp.org>
 
        clean-temp: Make multithread-safe, part 1.
index 6beea8447837c2a2acbdd340be9c13bfd81bdd00..467453719ac4c7d3f2d2e20833e88e63eb3793b9 100644 (file)
@@ -38,6 +38,7 @@
 
 #include "error.h"
 #include "fatal-signal.h"
+#include "asyncsafe-spin.h"
 #include "pathmax.h"
 #include "tmpdir.h"
 #include "xalloc.h"
@@ -45,6 +46,7 @@
 #include "glthread/lock.h"
 #include "gl_xlist.h"
 #include "gl_linkedhash_list.h"
+#include "gl_linked_list.h"
 #include "gettext.h"
 #if GNULIB_FWRITEERROR
 # include "fwriteerror.h"
@@ -108,12 +110,28 @@ static struct
   size_t tempdir_allocated;
 } cleanup_list /* = { NULL, 0, 0 } */;
 
+/* A file descriptor to be closed.
+   In multithreaded programs, it is forbidden to close the same fd twice,
+   because you never know what unrelated open() calls are being executed in
+   other threads. So, the 'close (fd)' must be guarded by a once-only guard.  */
+struct closeable_fd
+{
+  /* The file descriptor to close.  */
+  int volatile fd;
+  /* Set to true when it has been closed.  */
+  bool volatile closed;
+  /* Lock that protects the fd from being closed twice.  */
+  asyncsafe_spinlock_t lock;
+  /* Tells whether this list element has been done and can be freed.  */
+  bool volatile done;
+};
+
 /* Lock that protects the descriptors list from concurrent modification in
    different threads.  */
 gl_lock_define_initialized (static, descriptors_lock)
 
 /* List of all open file descriptors to temporary files.  */
-static gl_list_t /* <int> */ volatile descriptors;
+static gl_list_t /* <closeable_fd *> */ volatile descriptors;
 
 
 /* For the subdirs and for the files, we use a gl_list_t of type LINKEDHASH.
@@ -193,6 +211,84 @@ string_hash (const void *x)
 }
 
 
+/* The set of fatal signal handlers.
+   Cached here because we are not allowed to call get_fatal_signal_set ()
+   from a signal handler.  */
+static const sigset_t *fatal_signal_set /* = NULL */;
+
+static void
+init_fatal_signal_set (void)
+{
+  if (fatal_signal_set == NULL)
+    fatal_signal_set = get_fatal_signal_set ();
+}
+
+
+/* Close a file descriptor.
+   Avoids race conditions with normal thread code or signal-handler code that
+   might want to close the same file descriptor.  */
+static _GL_ASYNC_SAFE int
+asyncsafe_close (struct closeable_fd *element)
+{
+  sigset_t saved_mask;
+  int ret;
+  int saved_errno;
+
+  asyncsafe_spin_lock (&element->lock, fatal_signal_set, &saved_mask);
+  if (!element->closed)
+    {
+      ret = close (element->fd);
+      saved_errno = errno;
+      element->closed = true;
+    }
+  else
+    {
+      ret = 0;
+      saved_errno = 0;
+    }
+  asyncsafe_spin_unlock (&element->lock, &saved_mask);
+  element->done = true;
+
+  errno = saved_errno;
+  return ret;
+}
+
+/* Close a file descriptor and the stream that contains it.
+   Avoids race conditions with signal-handler code that might want to close the
+   same file descriptor.  */
+static int
+asyncsafe_fclose_variant (struct closeable_fd *element, FILE *fp,
+                          int (*fclose_variant) (FILE *))
+{
+  if (fileno (fp) != element->fd)
+    abort ();
+
+  /* Flush buffered data first, to minimize the duration of the spin lock.  */
+  fflush (fp);
+
+  sigset_t saved_mask;
+  int ret;
+  int saved_errno;
+
+  asyncsafe_spin_lock (&element->lock, fatal_signal_set, &saved_mask);
+  if (!element->closed)
+    {
+      ret = fclose_variant (fp); /* invokes close (element->fd) */
+      saved_errno = errno;
+      element->closed = true;
+    }
+  else
+    {
+      ret = 0;
+      saved_errno = 0;
+    }
+  asyncsafe_spin_unlock (&element->lock, &saved_mask);
+  element->done = true;
+
+  errno = saved_errno;
+  return ret;
+}
+
 /* The signal handler.  It gets called asynchronously.  */
 static _GL_ASYNC_SAFE void
 cleanup_action (int sig _GL_UNUSED)
@@ -211,8 +307,7 @@ cleanup_action (int sig _GL_UNUSED)
         iter = gl_list_iterator (fds);
         while (gl_list_iterator_next (&iter, &element, NULL))
           {
-            int fd = (int) (uintptr_t) element;
-            close (fd);
+            asyncsafe_close ((struct closeable_fd *) element);
           }
         gl_list_iterator_free (&iter);
       }
@@ -294,8 +389,11 @@ create_temp_dir (const char *prefix, const char *parentdir,
             XNMALLOC (new_allocated, struct tempdir * volatile);
 
           if (old_allocated == 0)
-            /* First use of this facility.  Register the cleanup handler.  */
-            at_fatal_signal (&cleanup_action);
+            {
+              /* First use of this facility.  Register the cleanup handler.  */
+              init_fatal_signal_set ();
+              at_fatal_signal (&cleanup_action);
+            }
           else
             {
               /* Don't use memcpy() here, because memcpy takes non-volatile
@@ -650,30 +748,16 @@ register_fd (int fd)
   gl_lock_lock (descriptors_lock);
 
   if (descriptors == NULL)
-    descriptors = gl_list_create_empty (GL_LINKEDHASH_LIST, NULL, NULL, NULL,
+    descriptors = gl_list_create_empty (GL_LINKED_LIST, NULL, NULL, NULL,
                                         false);
-  gl_list_add_first (descriptors, (void *) (uintptr_t) fd);
 
-  gl_lock_unlock (descriptors_lock);
-}
+  struct closeable_fd *element = XMALLOC (struct closeable_fd);
+  element->fd = fd;
+  element->closed = false;
+  asyncsafe_spin_init (&element->lock);
+  element->done = false;
 
-/* Unregister a file descriptor to be closed.  */
-static void
-unregister_fd (int fd)
-{
-  gl_lock_lock (descriptors_lock);
-
-  gl_list_t fds = descriptors;
-  gl_list_node_t node;
-
-  if (fds == NULL)
-    /* descriptors should already contain fd.  */
-    abort ();
-  node = gl_list_search (fds, (void *) (uintptr_t) fd);
-  if (node == NULL)
-    /* descriptors should already contain fd.  */
-    abort ();
-  gl_list_remove_node (fds, node);
+  gl_list_add_first (descriptors, element);
 
   gl_lock_unlock (descriptors_lock);
 }
@@ -755,64 +839,141 @@ fopen_temp (const char *file_name, const char *mode, bool delete_on_close)
 int
 close_temp (int fd)
 {
-  if (fd >= 0)
-    {
-      /* No blocking of signals is needed here, since a double close of a
-         file descriptor is harmless.  */
-      int result = close (fd);
-      int saved_errno = errno;
+  if (fd < 0)
+    return close (fd);
 
-      /* No race condition here: we assume a single-threaded program, hence
-         fd cannot be re-opened here.  */
+  init_fatal_signal_set ();
 
-      unregister_fd (fd);
+  int result = 0;
+  int saved_errno = 0;
 
-      errno = saved_errno;
-      return result;
-    }
-  else
-    return close (fd);
+  gl_lock_lock (descriptors_lock);
+
+  gl_list_t list = descriptors;
+  if (list == NULL)
+    /* descriptors should already contain fd.  */
+    abort ();
+
+  /* Search through the list, and clean it up on the fly.  */
+  bool found = false;
+  gl_list_iterator_t iter = gl_list_iterator (list);
+  const void *elt;
+  gl_list_node_t node;
+  if (gl_list_iterator_next (&iter, &elt, &node))
+    for (;;)
+      {
+        struct closeable_fd *element = (struct closeable_fd *) elt;
+
+        /* Close the file descriptor, avoiding races with the signal
+           handler.  */
+        if (element->fd == fd)
+          {
+            result = asyncsafe_close (element);
+            saved_errno = errno;
+          }
+
+        bool free_this_node = element->done;
+        struct closeable_fd *element_to_free = element;
+        gl_list_node_t node_to_free = node;
+
+        bool have_next = gl_list_iterator_next (&iter, &elt, &node);
+
+        if (free_this_node)
+          {
+            free (element_to_free);
+            gl_list_remove_node (list, node_to_free);
+          }
+
+        if (!have_next)
+          break;
+      }
+  gl_list_iterator_free (&iter);
+  if (!found)
+    /* descriptors should already contain fd.  */
+    abort ();
+
+  gl_lock_unlock (descriptors_lock);
+
+  errno = saved_errno;
+  return result;
 }
 
-/* Close a temporary file in a temporary directory.
-   Unregisters the previously registered file descriptor.  */
-int
-fclose_temp (FILE *fp)
+static int
+fclose_variant_temp (FILE *fp, int (*fclose_variant) (FILE *))
 {
   int fd = fileno (fp);
-  /* No blocking of signals is needed here, since a double close of a
-     file descriptor is harmless.  */
-  int result = fclose (fp);
-  int saved_errno = errno;
 
-  /* No race condition here: we assume a single-threaded program, hence
-     fd cannot be re-opened here.  */
+  init_fatal_signal_set ();
+
+  int result = 0;
+  int saved_errno = 0;
+
+  gl_lock_lock (descriptors_lock);
+
+  gl_list_t list = descriptors;
+  if (list == NULL)
+    /* descriptors should already contain fd.  */
+    abort ();
+
+  /* Search through the list, and clean it up on the fly.  */
+  bool found = false;
+  gl_list_iterator_t iter = gl_list_iterator (list);
+  const void *elt;
+  gl_list_node_t node;
+  if (gl_list_iterator_next (&iter, &elt, &node))
+    for (;;)
+      {
+        struct closeable_fd *element = (struct closeable_fd *) elt;
+
+        /* Close the file descriptor and the stream, avoiding races with the
+           signal handler.  */
+        if (element->fd == fd)
+          {
+            result = asyncsafe_fclose_variant (element, fp, fclose_variant);
+            saved_errno = errno;
+          }
+
+        bool free_this_node = element->done;
+        struct closeable_fd *element_to_free = element;
+        gl_list_node_t node_to_free = node;
 
-  unregister_fd (fd);
+        bool have_next = gl_list_iterator_next (&iter, &elt, &node);
+
+        if (free_this_node)
+          {
+            free (element_to_free);
+            gl_list_remove_node (list, node_to_free);
+          }
+
+        if (!have_next)
+          break;
+      }
+  gl_list_iterator_free (&iter);
+  if (!found)
+    /* descriptors should have contained fd.  */
+    abort ();
+
+  gl_lock_unlock (descriptors_lock);
 
   errno = saved_errno;
   return result;
 }
 
+/* Close a temporary file in a temporary directory.
+   Unregisters the previously registered file descriptor.  */
+int
+fclose_temp (FILE *fp)
+{
+  return fclose_variant_temp (fp, fclose);
+}
+
 #if GNULIB_FWRITEERROR
 /* Like fwriteerror.
    Unregisters the previously registered file descriptor.  */
 int
 fwriteerror_temp (FILE *fp)
 {
-  int fd = fileno (fp);
-  /* No blocking of signals is needed here, since a double close of a
-     file descriptor is harmless.  */
-  int result = fwriteerror (fp);
-  int saved_errno = errno;
-
-  /* No race condition here: we assume a single-threaded program, hence
-     fd cannot be re-opened here.  */
-
-  unregister_fd (fd);
-
-  errno = saved_errno;
-  return result;
+  return fclose_variant_temp (fp, fwriteerror);
 }
 #endif
 
@@ -822,18 +983,6 @@ fwriteerror_temp (FILE *fp)
 int
 close_stream_temp (FILE *fp)
 {
-  int fd = fileno (fp);
-  /* No blocking of signals is needed here, since a double close of a
-     file descriptor is harmless.  */
-  int result = close_stream (fp);
-  int saved_errno = errno;
-
-  /* No race condition here: we assume a single-threaded program, hence
-     fd cannot be re-opened here.  */
-
-  unregister_fd (fd);
-
-  errno = saved_errno;
-  return result;
+  return fclose_variant_temp (fp, close_stream);
 }
 #endif
index 8b27cc6feb8d7eae29bc90177d0a01446ae68b68..29184e0ff21a7cce6b56e10b2902fb47a9f23c17 100644 (file)
@@ -346,3 +346,10 @@ get_fatal_signals (int signals[64])
     return p - signals;
   }
 }
+
+const sigset_t *
+get_fatal_signal_set (void)
+{
+  init_fatal_signal_set ();
+  return &fatal_signal_set;
+}
index 2c0154ffc3e07018faa777d6f055a65bc8c8fcd8..54270fab9217cf658dfccb03fe0b82cafbe26503 100644 (file)
    along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
 
+#ifndef _FATAL_SIGNAL_H
+#define _FATAL_SIGNAL_H
+
+#include <signal.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -77,7 +82,13 @@ extern void unblock_fatal_signals (void);
    Fills signals[0..count-1] and returns count.  */
 extern unsigned int get_fatal_signals (int signals[64]);
 
+/* Return the list of signals that block_fatal_signals/unblock_fatal_signals
+   would block or unblock.  */
+extern const sigset_t * get_fatal_signal_set (void);
+
 
 #ifdef __cplusplus
 }
 #endif
+
+#endif /* _FATAL_SIGNAL_H */
index c1d5c1c4b56e90545d00fc0741d621a8b3741c4a..940869de4ee3ca9b8807c427cf8db0381c6c2722 100644 (file)
@@ -12,6 +12,7 @@ unistd
 lock
 error
 fatal-signal
+asyncsafe-spin
 open
 pathmax
 tmpdir
@@ -20,6 +21,7 @@ rmdir
 xalloc
 xmalloca
 linkedhash-list
+linked-list
 xlist
 gettext-h