>From 9d1ad9c4df04071810d04dcc3142dfdeb5a5f892 Mon Sep 17 00:00:00 2001 From: Bruno Haible Date: Sat, 4 Jul 2020 14:39:09 +0200 Subject: [PATCH 2/2] clean-temp: Make multithread-safe, part 2. * lib/fatal-signal.h: Include . (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 | 22 ++++ lib/clean-temp.c | 303 +++++++++++++++++++++++++++++++++++++++-------------- lib/fatal-signal.c | 7 ++ lib/fatal-signal.h | 11 ++ modules/clean-temp | 2 + 5 files changed, 268 insertions(+), 77 deletions(-) diff --git a/ChangeLog b/ChangeLog index e7b3821..d49c9d7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,27 @@ 2020-07-04 Bruno Haible + clean-temp: Make multithread-safe, part 2. + * lib/fatal-signal.h: Include . + (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 + clean-temp: Make multithread-safe, part 1. * lib/clean-temp.c: Include glthread/lock.h. (cleanup_list_lock): New variable. diff --git a/lib/clean-temp.c b/lib/clean-temp.c index 6beea84..4674537 100644 --- a/lib/clean-temp.c +++ b/lib/clean-temp.c @@ -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 /* */ volatile descriptors; +static gl_list_t /* */ 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 diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c index 8b27cc6..29184e0 100644 --- a/lib/fatal-signal.c +++ b/lib/fatal-signal.c @@ -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; +} diff --git a/lib/fatal-signal.h b/lib/fatal-signal.h index 2c0154f..54270fa 100644 --- a/lib/fatal-signal.h +++ b/lib/fatal-signal.h @@ -16,6 +16,11 @@ along with this program. If not, see . */ +#ifndef _FATAL_SIGNAL_H +#define _FATAL_SIGNAL_H + +#include + #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 */ diff --git a/modules/clean-temp b/modules/clean-temp index c1d5c1c..940869d 100644 --- a/modules/clean-temp +++ b/modules/clean-temp @@ -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 -- 2.7.4