2016-04-14 Florian Weimer [BZ #19431] Run the malloc fork handler as late as possible to avoid deadlocks. * malloc/malloc-internal.h: New file. * malloc/malloc.c: Include it. * malloc/arena.c (ATFORK_MEM): Remove. (__malloc_fork_lock_parent): Rename from ptmalloc_lock_all. Update comment. (__malloc_fork_unlock_parent): Rename from ptmalloc_unlock_all. (__malloc_fork_unlock_child): Rename from ptmalloc_unlock_all2. Remove outdated comment. (ptmalloc_init): Do not call thread_atfork. Remove thread_atfork_static. * malloc/tst-malloc-fork-deadlock.c: New file. * Makefile (tests): Add tst-malloc-fork-deadlock. (tst-malloc-fork-deadlock): Link against libpthread. * manual/memory.texi (Aligned Memory Blocks): Update safety annotation comments. * sysdeps/nptl/fork.c (__libc_fork): Call __malloc_fork_lock_parent, __malloc_fork_unlock_parent, __malloc_fork_unlock_child. * sysdeps/mach/hurd/fork.c (__fork): Likewise. Index: glibc-2.23/malloc/Makefile =================================================================== --- glibc-2.23.orig/malloc/Makefile +++ glibc-2.23/malloc/Makefile @@ -29,7 +29,7 @@ tests := mallocbug tst-malloc tst-valloc tst-malloc-usable tst-realloc tst-posix_memalign \ tst-pvalloc tst-memalign tst-mallopt tst-scratch_buffer \ tst-malloc-backtrace tst-malloc-thread-exit \ - tst-malloc-thread-fail + tst-malloc-thread-fail tst-malloc-fork-deadlock test-srcs = tst-mtrace routines = malloc morecore mcheck mtrace obstack \ @@ -49,6 +49,7 @@ libmemusage-inhibit-o = $(filter-out .os $(objpfx)tst-malloc-backtrace: $(shared-thread-library) $(objpfx)tst-malloc-thread-exit: $(shared-thread-library) $(objpfx)tst-malloc-thread-fail: $(shared-thread-library) +$(objpfx)tst-malloc-fork-deadlock: $(shared-thread-library) # These should be removed by `make clean'. extra-objs = mcheck-init.o libmcheck.a Index: glibc-2.23/malloc/arena.c =================================================================== --- glibc-2.23.orig/malloc/arena.c +++ glibc-2.23/malloc/arena.c @@ -136,10 +136,6 @@ static void *(*save_malloc_hook)(size_t static void (*save_free_hook) (void *__ptr, const void *); static void *save_arena; -# ifdef ATFORK_MEM -ATFORK_MEM; -# endif - /* Magic value for the thread-specific arena pointer when malloc_atfork() is in use. */ @@ -205,14 +201,14 @@ free_atfork (void *mem, const void *call /* Counter for number of times the list is locked by the same thread. */ static unsigned int atfork_recursive_cntr; -/* The following two functions are registered via thread_atfork() to - make sure that the mutexes remain in a consistent state in the - fork()ed version of a thread. Also adapt the malloc and free hooks - temporarily, because the `atfork' handler mechanism may use - malloc/free internally (e.g. in LinuxThreads). */ +/* The following three functions are called around fork from a + multi-threaded process. We do not use the general fork handler + mechanism to make sure that our handlers are the last ones being + called, so that other fork handlers can use the malloc + subsystem. */ -static void -ptmalloc_lock_all (void) +void +__malloc_fork_lock_parent (void) { mstate ar_ptr; @@ -220,7 +216,7 @@ ptmalloc_lock_all (void) return; /* We do not acquire free_list_lock here because we completely - reconstruct free_list in ptmalloc_unlock_all2. */ + reconstruct free_list in __malloc_fork_unlock_child. */ if (mutex_trylock (&list_lock)) { @@ -245,7 +241,7 @@ ptmalloc_lock_all (void) __free_hook = free_atfork; /* Only the current thread may perform malloc/free calls now. save_arena will be reattached to the current thread, in - ptmalloc_lock_all, so save_arena->attached_threads is not + __malloc_fork_lock_parent, so save_arena->attached_threads is not updated. */ save_arena = thread_arena; thread_arena = ATFORK_ARENA_PTR; @@ -253,8 +249,8 @@ out: ++atfork_recursive_cntr; } -static void -ptmalloc_unlock_all (void) +void +__malloc_fork_unlock_parent (void) { mstate ar_ptr; @@ -265,8 +261,8 @@ ptmalloc_unlock_all (void) return; /* Replace ATFORK_ARENA_PTR with save_arena. - save_arena->attached_threads was not changed in ptmalloc_lock_all - and is still correct. */ + save_arena->attached_threads was not changed in + __malloc_fork_lock_parent and is still correct. */ thread_arena = save_arena; __malloc_hook = save_malloc_hook; __free_hook = save_free_hook; @@ -280,15 +276,8 @@ ptmalloc_unlock_all (void) (void) mutex_unlock (&list_lock); } -# ifdef __linux__ - -/* In NPTL, unlocking a mutex in the child process after a - fork() is currently unsafe, whereas re-initializing it is safe and - does not leak resources. Therefore, a special atfork handler is - installed for the child. */ - -static void -ptmalloc_unlock_all2 (void) +void +__malloc_fork_unlock_child (void) { mstate ar_ptr; @@ -324,11 +313,6 @@ ptmalloc_unlock_all2 (void) atfork_recursive_cntr = 0; } -# else - -# define ptmalloc_unlock_all2 ptmalloc_unlock_all -# endif - /* Initialization routine. */ #include extern char **_environ; @@ -397,7 +381,6 @@ ptmalloc_init (void) #endif thread_arena = &main_arena; - thread_atfork (ptmalloc_lock_all, ptmalloc_unlock_all, ptmalloc_unlock_all2); const char *s = NULL; if (__glibc_likely (_environ != NULL)) { @@ -472,14 +455,6 @@ ptmalloc_init (void) __malloc_initialized = 1; } -/* There are platforms (e.g. Hurd) with a link-time hook mechanism. */ -#ifdef thread_atfork_static -thread_atfork_static (ptmalloc_lock_all, ptmalloc_unlock_all, \ - ptmalloc_unlock_all2) -#endif - - - /* Managing heaps and arenas (for concurrent threads) */ #if MALLOC_DEBUG > 1 @@ -828,7 +803,8 @@ _int_new_arena (size_t size) limit is reached). At this point, some arena has to be attached to two threads. We could acquire the arena lock before list_lock to make it less likely that reused_arena picks this new arena, - but this could result in a deadlock with ptmalloc_lock_all. */ + but this could result in a deadlock with + __malloc_fork_lock_parent. */ (void) mutex_lock (&a->mutex); Index: glibc-2.23/malloc/malloc-internal.h =================================================================== --- /dev/null +++ glibc-2.23/malloc/malloc-internal.h @@ -0,0 +1,32 @@ +/* Internal declarations for malloc, for use within libc. + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public License as + published by the Free Software Foundation; either version 2.1 of the + License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; see the file COPYING.LIB. If + not, see . */ + +#ifndef _MALLOC_PRIVATE_H +#define _MALLOC_PRIVATE_H + +/* Called in the parent process before a fork. */ +void __malloc_fork_lock_parent (void) internal_function attribute_hidden; + +/* Called in the parent process after a fork. */ +void __malloc_fork_unlock_parent (void) internal_function attribute_hidden; + +/* Called in the child process after a fork. */ +void __malloc_fork_unlock_child (void) internal_function attribute_hidden; + + +#endif /* _MALLOC_PRIVATE_H */ Index: glibc-2.23/malloc/malloc.c =================================================================== --- glibc-2.23.orig/malloc/malloc.c +++ glibc-2.23/malloc/malloc.c @@ -244,6 +244,7 @@ /* For ALIGN_UP et. al. */ #include +#include /* Debugging: Index: glibc-2.23/malloc/tst-malloc-fork-deadlock.c =================================================================== --- /dev/null +++ glibc-2.23/malloc/tst-malloc-fork-deadlock.c @@ -0,0 +1,220 @@ +/* Test concurrent fork, getline, and fflush (NULL). + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public License as + published by the Free Software Foundation; either version 2.1 of the + License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; see the file COPYING.LIB. If + not, see . */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static int do_test (void); +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" + +enum { + /* Number of threads which call fork. */ + fork_thread_count = 4, + /* Number of threads which call getline (and, indirectly, + malloc). */ + read_thread_count = 8, +}; + +static bool termination_requested; + +static void * +fork_thread_function (void *closure) +{ + while (!__atomic_load_n (&termination_requested, __ATOMIC_RELAXED)) + { + pid_t pid = fork (); + if (pid < 0) + { + printf ("error: fork: %m\n"); + abort (); + } + else if (pid == 0) + _exit (17); + + int status; + if (waitpid (pid, &status, 0) < 0) + { + printf ("error: waitpid: %m\n"); + abort (); + } + if (!WIFEXITED (status) || WEXITSTATUS (status) != 17) + { + printf ("error: waitpid returned invalid status: %d\n", status); + abort (); + } + } + return NULL; +} + +static char *file_to_read; + +static void * +read_thread_function (void *closure) +{ + FILE *f = fopen (file_to_read, "r"); + if (f == NULL) + { + printf ("error: fopen (%s): %m\n", file_to_read); + abort (); + } + + while (!__atomic_load_n (&termination_requested, __ATOMIC_RELAXED)) + { + rewind (f); + char *line = NULL; + size_t line_allocated = 0; + ssize_t ret = getline (&line, &line_allocated, f); + if (ret < 0) + { + printf ("error: getline: %m\n"); + abort (); + } + free (line); + } + fclose (f); + + return NULL; +} + +static void * +flushall_thread_function (void *closure) +{ + while (!__atomic_load_n (&termination_requested, __ATOMIC_RELAXED)) + if (fflush (NULL) != 0) + { + printf ("error: fflush (NULL): %m\n"); + abort (); + } + return NULL; +} + +static void +create_threads (pthread_t *threads, size_t count, void *(*func) (void *)) +{ + for (size_t i = 0; i < count; ++i) + { + int ret = pthread_create (threads + i, NULL, func, NULL); + if (ret != 0) + { + errno = ret; + printf ("error: pthread_create: %m\n"); + abort (); + } + } +} + +static void +join_threads (pthread_t *threads, size_t count) +{ + for (size_t i = 0; i < count; ++i) + { + int ret = pthread_join (threads[i], NULL); + if (ret != 0) + { + errno = ret; + printf ("error: pthread_join: %m\n"); + abort (); + } + } +} + +/* Create a file which consists of a single long line, and assigns + file_to_read. The hope is that this triggers an allocation in + getline which needs a lock. */ +static void +create_file_with_large_line (void) +{ + int fd = create_temp_file ("bug19431-large-line", &file_to_read); + if (fd < 0) + { + printf ("error: create_temp_file: %m\n"); + abort (); + } + FILE *f = fdopen (fd, "w+"); + if (f == NULL) + { + printf ("error: fdopen: %m\n"); + abort (); + } + for (int i = 0; i < 50000; ++i) + fputc ('x', f); + fputc ('\n', f); + if (ferror (f)) + { + printf ("error: fputc: %m\n"); + abort (); + } + if (fclose (f) != 0) + { + printf ("error: fclose: %m\n"); + abort (); + } +} + +static int +do_test (void) +{ + /* Make sure that we do not exceed the arena limit with the number + of threads we configured. */ + if (mallopt (M_ARENA_MAX, 400) == 0) + { + printf ("error: mallopt (M_ARENA_MAX) failed\n"); + return 1; + } + + /* Leave some room for shutting down all threads gracefully. */ + int timeout = 3; + if (timeout > TIMEOUT) + timeout = TIMEOUT - 1; + + create_file_with_large_line (); + + pthread_t fork_threads[fork_thread_count]; + create_threads (fork_threads, fork_thread_count, fork_thread_function); + pthread_t read_threads[read_thread_count]; + create_threads (read_threads, read_thread_count, read_thread_function); + pthread_t flushall_threads[1]; + create_threads (flushall_threads, 1, flushall_thread_function); + + struct timespec ts = {timeout, 0}; + if (nanosleep (&ts, NULL)) + { + printf ("error: error: nanosleep: %m\n"); + abort (); + } + + __atomic_store_n (&termination_requested, true, __ATOMIC_RELAXED); + + join_threads (flushall_threads, 1); + join_threads (read_threads, read_thread_count); + join_threads (fork_threads, fork_thread_count); + + free (file_to_read); + + return 0; +} Index: glibc-2.23/manual/memory.texi =================================================================== --- glibc-2.23.orig/manual/memory.texi +++ glibc-2.23/manual/memory.texi @@ -1051,14 +1051,6 @@ systems that do not support @w{ISO C11}. @c _dl_addr_inside_object ok @c determine_info ok @c __rtld_lock_unlock_recursive (dl_load_lock) @aculock -@c thread_atfork @asulock @aculock @acsfd @acsmem -@c __register_atfork @asulock @aculock @acsfd @acsmem -@c lll_lock (__fork_lock) @asulock @aculock -@c fork_handler_alloc @asulock @aculock @acsfd @acsmem -@c calloc dup @asulock @aculock @acsfd @acsmem -@c __linkin_atfork ok -@c catomic_compare_and_exchange_bool_acq ok -@c lll_unlock (__fork_lock) @aculock @c *_environ @mtsenv @c next_env_entry ok @c strcspn dup ok Index: glibc-2.23/sysdeps/mach/hurd/fork.c =================================================================== --- glibc-2.23.orig/sysdeps/mach/hurd/fork.c +++ glibc-2.23/sysdeps/mach/hurd/fork.c @@ -26,6 +26,7 @@ #include #include "hurdmalloc.h" /* XXX */ #include +#include #undef __fork @@ -107,6 +108,12 @@ __fork (void) /* Run things that prepare for forking before we create the task. */ RUN_HOOK (_hurd_fork_prepare_hook, ()); + /* Acquire malloc locks. This needs to come last because fork + handlers may use malloc, and the libio list lock has an + indirect malloc dependency as well (via the getdelim + function). */ + __malloc_fork_lock_parent (); + /* Lock things that want to be locked before we fork. */ { void *const *p; @@ -604,6 +611,9 @@ __fork (void) nthreads * sizeof (*threads)); } + /* Release malloc locks. */ + __malloc_fork_unlock_parent (); + /* Run things that want to run in the parent to restore it to normality. Usually prepare hooks and parent hooks are symmetrical: the prepare hook arrests state in some way for the @@ -655,6 +665,9 @@ __fork (void) /* Forking clears the trace flag. */ __sigemptyset (&_hurdsig_traced); + /* Release malloc locks. */ + __malloc_fork_unlock_child (); + /* Run things that want to run in the child task to set up. */ RUN_HOOK (_hurd_fork_child_hook, ()); Index: glibc-2.23/sysdeps/nptl/fork.c =================================================================== --- glibc-2.23.orig/sysdeps/nptl/fork.c +++ glibc-2.23/sysdeps/nptl/fork.c @@ -31,7 +31,7 @@ #include #include #include - +#include static void fresetlockfiles (void) @@ -111,6 +111,11 @@ __libc_fork (void) _IO_list_lock (); + /* Acquire malloc locks. This needs to come last because fork + handlers may use malloc, and the libio list lock has an indirect + malloc dependency as well (via the getdelim function). */ + __malloc_fork_lock_parent (); + #ifndef NDEBUG pid_t ppid = THREAD_GETMEM (THREAD_SELF, tid); #endif @@ -168,6 +173,9 @@ __libc_fork (void) # endif #endif + /* Release malloc locks. */ + __malloc_fork_unlock_child (); + /* Reset the file list. These are recursive mutexes. */ fresetlockfiles (); @@ -209,6 +217,9 @@ __libc_fork (void) /* Restore the PID value. */ THREAD_SETMEM (THREAD_SELF, pid, parentpid); + /* Release malloc locks, parent process variant. */ + __malloc_fork_unlock_parent (); + /* We execute this even if the 'fork' call failed. */ _IO_list_unlock ();