Re: Fix the race between atexit() and exit()

Quoting Carlos O'Donell <carlossystemhalted.org>:

> 1. Describe in detail the conditions of the race and
your proposed fix.

current exit() code
==================
while (__exit_funcs != NULL) <==== not atomic, also what
if a new  
handler is registered right after this
{
   struct exit_function_list *old;

   while (__exit_funcs->idx > 0) <==== not atomic,
what if another  
thread increments the index while registering a new handle
{
   const struct exit_function *const f =
	&__exit_funcs->fns[--__exit_funcs->idx]; <====
not atomic
   switch (f->flavor)
	{
	  void (*atfct) (void);
	  void (*onfct) (int status, void *arg);
	  void (*cxafct) (void *arg, int status);

	case ef_free:
	case ef_us:  <=== if the current handler registration is
in process,  
we miss it
	  break;
	...
	...

   old = __exit_funcs; <==== not atomic
   __exit_funcs = __exit_funcs->next; <==== not
atomic
   if (__exit_funcs != NULL)
/* Don't free the last element in the chain, this is the
statically
    allocate element.  */
free (old); <====== not atomic
}


The proposed patch
===================
1. serialises any modification of __exit_funcs list by
locking
2. protects assignments of local variables to list property
values  
like index and next in locks
3. protects change of status (flavor) by atomic operations
3. check if any new handler has been registered in a block
before freeing it
4. fails new registrations once exit() finish processing the
list


> 2. Provide the test case.

Test case
=========
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <fcntl.h>

char *data;

#define ATEXITFUNC1(x) void atexit_func1##x()         
     {       
         *(data - 10 + 96 + x) = x-10;       
     }

#define ATEXITFUNC(x) void atexit_func##x()             
         {       
                 *(data - 10 + 64 + x) = x-10;           
         if(atexit(atexit_func1##x))          
             write(2,"Fn",2); 
         else 
             *(data - 10  + 32 + x) = x -10; 
         }

#define REGISTER_ATEXITFUNC(x) void
*register_atexit_func##x(void *t)   
         {                                               
                 while(thread_start == 0);               
                 if(atexit(atexit_func##x))                 
    
             write(2,"Fn",2); 
         else 
                     *(data - 10  + x) = x -10; 
         }

int thread_start = 0;


ATEXITFUNC1(10)
ATEXITFUNC1(11)
ATEXITFUNC1(12)
ATEXITFUNC1(13)
ATEXITFUNC1(14)
ATEXITFUNC1(15)
ATEXITFUNC1(16)
ATEXITFUNC1(17)
ATEXITFUNC1(18)
ATEXITFUNC1(19)
ATEXITFUNC1(20)
ATEXITFUNC1(21)
ATEXITFUNC1(22)
ATEXITFUNC1(23)
ATEXITFUNC1(24)
ATEXITFUNC1(25)
ATEXITFUNC1(26)
ATEXITFUNC1(27)
ATEXITFUNC1(28)
ATEXITFUNC1(29)
ATEXITFUNC1(30)
ATEXITFUNC1(31)
ATEXITFUNC1(32)
ATEXITFUNC1(33)
ATEXITFUNC1(34)
ATEXITFUNC1(35)
ATEXITFUNC1(36)
ATEXITFUNC1(37)
ATEXITFUNC1(38)
ATEXITFUNC1(39)

ATEXITFUNC(10)
ATEXITFUNC(11)
ATEXITFUNC(12)
ATEXITFUNC(13)
ATEXITFUNC(14)
ATEXITFUNC(15)
ATEXITFUNC(16)
ATEXITFUNC(17)
ATEXITFUNC(18)
ATEXITFUNC(19)
ATEXITFUNC(20)
ATEXITFUNC(21)
ATEXITFUNC(22)
ATEXITFUNC(23)
ATEXITFUNC(24)
ATEXITFUNC(25)
ATEXITFUNC(26)
ATEXITFUNC(27)
ATEXITFUNC(28)
ATEXITFUNC(29)
ATEXITFUNC(30)
ATEXITFUNC(31)
ATEXITFUNC(32)
ATEXITFUNC(33)
ATEXITFUNC(34)
ATEXITFUNC(35)
ATEXITFUNC(36)
ATEXITFUNC(37)
ATEXITFUNC(38)
ATEXITFUNC(39)

REGISTER_ATEXITFUNC(10)
REGISTER_ATEXITFUNC(11)
REGISTER_ATEXITFUNC(12)
REGISTER_ATEXITFUNC(13)
REGISTER_ATEXITFUNC(14)
REGISTER_ATEXITFUNC(15)
REGISTER_ATEXITFUNC(16)
REGISTER_ATEXITFUNC(17)
REGISTER_ATEXITFUNC(18)
REGISTER_ATEXITFUNC(19)
REGISTER_ATEXITFUNC(20)
REGISTER_ATEXITFUNC(21)
REGISTER_ATEXITFUNC(22)
REGISTER_ATEXITFUNC(23)
REGISTER_ATEXITFUNC(24)
REGISTER_ATEXITFUNC(25)
REGISTER_ATEXITFUNC(26)
REGISTER_ATEXITFUNC(27)
REGISTER_ATEXITFUNC(28)
REGISTER_ATEXITFUNC(29)
REGISTER_ATEXITFUNC(30)
REGISTER_ATEXITFUNC(31)
REGISTER_ATEXITFUNC(32)
REGISTER_ATEXITFUNC(33)
REGISTER_ATEXITFUNC(34)
REGISTER_ATEXITFUNC(35)
REGISTER_ATEXITFUNC(36)
REGISTER_ATEXITFUNC(37)
REGISTER_ATEXITFUNC(38)
REGISTER_ATEXITFUNC(39)

int main()
{
     pthread_t tid[30];
     int ret_code[30];
     int pagesize;
     int fd,len;

     fd = open("foo", O_RDWR);
     if(fd<1)
     {
         perror("open");
         exit(1);
     }

     pagesize = getpagesize();
     len=lseek(fd,pagesize,SEEK_CUR);
     if(write(fd, " ", 1 ) != 1)
     {
         perror("write error. ");
         exit(1);
     }

     data = mmap((caddr_t)0, pagesize, PROT_WRITE,
MAP_SHARED, fd, 0);
     if(!data)
     {
         perror("mmap");
         exit(1);
     }

         ret_code[0] = pthread_create(&tid[0], 0,  
register_atexit_func10, &tid[0]);
         ret_code[1] = pthread_create(&tid[1], 0,  
register_atexit_func11, &tid[1]);
         ret_code[2] = pthread_create(&tid[2], 0,  
register_atexit_func12, &tid[2]);
         ret_code[3] = pthread_create(&tid[3], 0,  
register_atexit_func13, &tid[3]);
         ret_code[4] = pthread_create(&tid[4], 0,  
register_atexit_func14, &tid[4]);
         ret_code[5] = pthread_create(&tid[5], 0,  
register_atexit_func15, &tid[5]);
         ret_code[6] = pthread_create(&tid[6], 0,  
register_atexit_func16, &tid[6]);
         ret_code[7] = pthread_create(&tid[7], 0,  
register_atexit_func17, &tid[7]);
         ret_code[8] = pthread_create(&tid[8], 0,  
register_atexit_func18, &tid[8]);
         ret_code[9] = pthread_create(&tid[9], 0,  
register_atexit_func19, &tid[9]);
         ret_code[10] = pthread_create(&tid[10], 0,  
register_atexit_func20, &tid[10]);
         ret_code[11] = pthread_create(&tid[11], 0,  
register_atexit_func21, &tid[11]);
         ret_code[12] = pthread_create(&tid[12], 0,  
register_atexit_func22, &tid[12]);
         ret_code[13] = pthread_create(&tid[13], 0,  
register_atexit_func23, &tid[13]);
         ret_code[14] = pthread_create(&tid[14], 0,  
register_atexit_func24, &tid[14]);
         ret_code[15] = pthread_create(&tid[15], 0,  
register_atexit_func25, &tid[15]);
         ret_code[16] = pthread_create(&tid[16], 0,  
register_atexit_func26, &tid[16]);
         ret_code[17] = pthread_create(&tid[17], 0,  
register_atexit_func27, &tid[17]);
         ret_code[18] = pthread_create(&tid[18], 0,  
register_atexit_func28, &tid[18]);
         ret_code[19] = pthread_create(&tid[19], 0,  
register_atexit_func29, &tid[19]);
         ret_code[20] = pthread_create(&tid[20], 0,  
register_atexit_func30, &tid[20]);
         ret_code[21] = pthread_create(&tid[21], 0,  
register_atexit_func31, &tid[21]);
         ret_code[22] = pthread_create(&tid[22], 0,  
register_atexit_func32, &tid[22]);
         ret_code[23] = pthread_create(&tid[23], 0,  
register_atexit_func33, &tid[23]);
         ret_code[24] = pthread_create(&tid[24], 0,  
register_atexit_func34, &tid[24]);
         ret_code[25] = pthread_create(&tid[25], 0,  
register_atexit_func35, &tid[25]);
         ret_code[26] = pthread_create(&tid[26], 0,  
register_atexit_func36, &tid[26]);
         ret_code[27] = pthread_create(&tid[27], 0,  
register_atexit_func37, &tid[27]);
         ret_code[28] = pthread_create(&tid[28], 0,  
register_atexit_func38, &tid[28]);
         ret_code[29] = pthread_create(&tid[29], 0,  
register_atexit_func39, &tid[29]);

         thread_start = 1;
         exit(0);
}

After running the test, check the result by "hexdump -C
foo"
The first 4 lines indicates the registration status and next
4  
indicates whether the handler was invoked
If they match (excluding the first column) then the test
passed.
Program prints "F" to stderror to indicate failed
registrations

Running without the patch gives
00000000  00 01 02 03 04 05 06 07  08 09 0a 0b 0c 0d 0e 0f 
|................|
00000010  10 11 12 13 14 15 16 17  18 19 1a 1b 1c 1d 00 00 
|................|
00000020  00 01 02 03 04 05 06 07  08 09 0a 0b 00 0d 0e 0f 
|................|
00000030  10 11 12 13 14 15 16 17  18 19 1a 1b 1c 1d 00 00 
|................|
00000040  00 01 02 03 04 05 06 07  08 09 0a 0b 00 0d 0e 0f 
|................|
00000050  10 11 12 13 14 15 16 17  18 19 1a 1b 1c 1d 00 00 
|................|
00000060  00 01 02 03 04 05 06 07  08 09 0a 0b 00 0d 0e 0f 
|................|
00000070  10 11 12 13 14 15 16 17  18 19 1a 1b 1c 1d 00 00 
|................|

We can see that handler "0c" was registered (first
line) but it was  
not invoked (see line 5)

> 3. Run the testsuite for your target to show there were
no
> regressions. Mention the target.
The test was run for about 10 hrs in loop in an x86_64 SMP
target  
without failures.

> 4. When quoting POSIX  please provide references.

POSIX Ref: =>	man 3p atexit

DESCRIPTION
    The  atexit()  function  shall register the function
pointed to by  
func, to be called without arguments at
    normal program termination. At normal program
termination, all  
functions registered by the atexit()  func-
    tion  shall  be called, in the reverse order of their  
registration, except that a function is called after
    any previously registered functions that had already
been called  
at the time  it  was  registered.  Normal
    termination occurs either by a call to exit() or a
return from main().

On Tue, Jun 3, 2008 at 7:49 PM, Carlos O'Donell
<carlossystemhalted.org> wrote:
> On Tue, Jun 3, 2008 at 12:23 PM, Anoop <acvlinux.vnet.ibm.com> wrote:
>>
>> Quoting Carlos O'Donell <carlossystemhalted.org>:
>>
>>> 1. Describe in detail the conditions of the
race and your proposed fix.
>>
>> current exit() code
>> ==================
>> while (__exit_funcs != NULL) <==== not atomic,
also what if a new handler is
>> registered right after this
>
> The standard does not say that exit() is thread-safe or
async-signal
> safe. I recommend using pthread_exit().
>
>>> 2. Provide the test case.
>> #define REGISTER_ATEXITFUNC(x) void
*register_atexit_func##x(void *t)   
>>        {                                           
   
>>                while(thread_start == 0);           
   
>>                if(atexit(atexit_func##x))          
           
>>            write(2,"Fn",2); 
>>        else 
>>                    *(data - 10  + x) = x -10; 
>>        }
>
> The standard does not say that atexit() is thread-safe
or async-signal
> safe. I recommend using pthread_cleanup_* functions.
>
>>> 3. Run the testsuite for your target to show
there were no
>>> regressions. Mention the target.
>>
>> The test was run for about 10 hrs in loop in an
x86_64 SMP target without
>> failures.
>
> When you make a change to a source base, like the C
library, you need
> to run the C library testsuite *before* and *after*
your patch to
> verify that none of the existing functionality was
altered by your
> changes. Have you run the C library test suite? e.g.
make -k check?
>
>>> 4. When quoting POSIX  please provide
references.
>>
>> POSIX Ref: =>   man 3p atexit
>
> The man page is not a good reference, it may not
actually match the
> standard. I recommend http:
//www.opengroup.org/onlinepubs/009695399/.
>
> In general these comments apply only to single threaded
programs that
> are not using signals or threads.
>
> In the case of signals you need to verify that the
functions you are
> calling are reentrant or async-signal safe.
>
> Then you need to verify if the standard says they are
thread safe.
>
> Please remember that reentrant and thread safety are
two separate issues.
>
> For the record I've CC'd libc-alpha, but we should
continue this
> conversation on libc-help.
>
> Cheers,
> Carlos.
>

For the record.

Note: One can't email libc-alpha and libc-help at the same
time.

Cheers,
Carlos.