This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix bug of __register_atfork
- From: Lai Jiangshan <laijs at cn dot fujitsu dot com>
- To: libc-alpha at sourceware dot org
- Date: Fri, 26 Oct 2007 09:37:29 +0800
- Subject: Re: [PATCH] Fix bug of __register_atfork
- References: <47205A3E.8050807@cn.fujitsu.com>
Does anyone care about this bug? This bug will cause a normal
program "Segmentation fault" or deadlock. __register_atfork and malloc
call each other in the glibc and result in such a bug. It's necessarily
to uncouple them.
This bug was found by Wang Fang <wangf@cn.fujitsu.com>. The first
half of the work is done by her. So, if this patch is to be committed, the
changelog should be:
2007-10-25 Wang Fang <wangf@cn.fujitsu.com>
2007-10-25 Lai Jiangshan <laijs@cn.fujitsu.com>
* nptl/sysdeps/unix/sysv/linux/register-atfork.c
(struct fork_handler_pool) Rewrite
(fork_handler_alloc) Use mmap to alloc memory
(free_mem) Use munmap to free memory
or delete my name.
> Hi, all
> When I tested glibc, I found a bug of __register_atfork.
>
> $ cat test.c
> #include <stdio.h>
> #include <stdlib.h>
> //call glibc __register_atfork:
> #include <pthread.h>
> #define CALL__register_atfork pthread_atfork
>
> int times = 20;
> // usage argv[0] [times]
>
> int main(int argc, char *argv[])
> {
> int i;
> if (argc > 1) {
> times = atoi(argv[1]);
> }
> //malloc hasn't been called until here
>
> // call __register_atfork $times times totally.
> // $i = 1; because __register_atfork was called once
> // in __libc_pthread_init() before.
> for (i = 1; i < times; i++) {
> CALL__register_atfork(NULL, NULL, NULL);
> }
>
> malloc(128);
> return 0;
> }
>
> $ gcc test.c -o test -lpthread # Do not use --static, in static-linked program
> # malloc may be called before main function and
> # the bug will be not occurred.
>
> # Test 1
> $ ./test 48
> Segmentation fault
>
> # Test 2
> $ ./test 49 # or greater
> <the program doesn't exit, it sleeps on syscall futex forever>
>
> I read the code of glibc, and I found that __register_atfork and malloc
> call each other. Glibc malloc calls __register_atfork when its init function
> is called, and __register_atfork calls malloc when its pools are exhausted.
> Especially pools are exhausted when __register_atfork is called at the 49th time.
>
> In the test 1, "malloc(128);" is the first time that malloc was called.
> So malloc calls it's init function, and init function calls __register_atfork,
> but it's the 49th time that __register_atfork is called, so __register_atfork
> calls malloc. So malloc reenters itself and causes "Segmentation fault".
>
> In the test 2, when __register_atfork is called at the 49th time, it
> calls malloc, and malloc calls __register_atfork. So __register_atfork reenters
> itself and sleeps on its low-level-lock.
>
> In a word, malloc and __register_atfork takes each other as its
> foundation, so we have to uncouple them. In my patch, I use mmap instead of
> malloc as __register_atfork's foundation.
>
>
> 2007-10-25 Lai Jiangshan <laijs@cn.fujitsu.com>
>
> * nptl/sysdeps/unix/sysv/linux/register-atfork.c
> (struct fork_handler_pool) Rewrite
> (fork_handler_alloc) Use mmap to alloc memory
> (free_mem) Use munmap to free memory
>
>
> --- glibc.orig/nptl/sysdeps/unix/sysv/linux/register-atfork.c 2005-12-22 06:17:21.000000000 +0800
> +++ glibc.new/nptl/sysdeps/unix/sysv/linux/register-atfork.c 2007-10-23 21:27:12.000000000 +0800
> @@ -20,52 +20,58 @@
> #include <errno.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <unistd.h>
> #include <fork.h>
> +#include <sys/mman.h>
>
>
> /* Lock to protect allocation and deallocation of fork handlers. */
> lll_lock_t __fork_lock = LLL_LOCK_INITIALIZER;
>
> -
> -/* Number of pre-allocated handler entries. */
> -#define NHANDLER 48
> -
> /* Memory pool for fork handler structures. */
> static struct fork_handler_pool
> {
> struct fork_handler_pool *next;
> - struct fork_handler mem[NHANDLER];
> -} fork_handler_pool;
> + struct fork_handler mem[0];
> +} *fork_handler_pool = NULL;
> +static unsigned int fork_handler_pool_size = 0;
> +static unsigned int nhandler = 0;
>
>
> static struct fork_handler *
> fork_handler_alloc (void)
> {
> - struct fork_handler_pool *runp = &fork_handler_pool;
> + struct fork_handler_pool *runp;
> struct fork_handler *result = NULL;
> unsigned int i;
>
> - do
> + /* Make sure fork_handler_pool_size and nhandler have been initialized */
> + if (fork_handler_pool_size == 0)
> + {
> + fork_handler_pool_size = __getpagesize();
> + nhandler = (fork_handler_pool_size - sizeof(struct fork_handler_pool))
> + / sizeof(struct fork_handler);
> + }
> +
> + for (runp = fork_handler_pool; runp != NULL; runp = runp->next)
> {
> /* Search for an empty entry. */
> - for (i = 0; i < NHANDLER; ++i)
> - if (runp->mem[i].refcntr == 0)
> - goto found;
> + for (i = 0; i < nhandler; ++i)
> + if (runp->mem[i].refcntr == 0)
> + goto found;
> }
> - while ((runp = runp->next) != NULL);
>
> /* We have to allocate a new entry. */
> - runp = (struct fork_handler_pool *) calloc (1, sizeof (*runp));
> + runp = (struct fork_handler_pool *) mmap (NULL, fork_handler_pool_size,
> + PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> if (runp != NULL)
> {
> /* Enqueue the new memory pool into the list. */
> - runp->next = fork_handler_pool.next;
> - fork_handler_pool.next = runp;
> + runp->next = fork_handler_pool;
> + fork_handler_pool = runp;
>
> - /* We use the last entry on the page. This means when we start
> - searching from the front the next time we will find the first
> - entry unused. */
> - i = NHANDLER - 1;
> + i = 0;
>
> found:
> result = &runp->mem[i];
> @@ -118,9 +124,9 @@
> __fork_handlers = NULL;
>
> /* Free eventually alloated memory blocks for the object pool. */
> - struct fork_handler_pool *runp = fork_handler_pool.next;
> + struct fork_handler_pool *runp = fork_handler_pool;
>
> - memset (&fork_handler_pool, '\0', sizeof (fork_handler_pool));
> + fork_handler_pool = NULL;
>
> /* Release the lock. */
> lll_unlock (__fork_lock);
> @@ -130,6 +136,6 @@
> {
> struct fork_handler_pool *oldp = runp;
> runp = runp->next;
> - free (oldp);
> + munmap (oldp, fork_handler_pool_size);
> }
> }
>