Re: [PATCH] Fix bug of __register_atfork

[Lai Jiangshan <laijs at cn dot fujitsu dot 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);
>      }
>  }
>