Re: [RFC] stdlib: Make atexit to not act as __cxa_atexit

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 08/07/2019 10:57, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 08/07/2019 08:16, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> For _dl_fini, we already have the link_map for object that calls 
>>>> __cxa_finalize. What we need is to filter out the exit handlers registered
>>>> by the object itself, so its calls only the functions registered by the 
>>>> shared library referenced by the link_map.
>>>>
>>>> Not sure how easily we can accomplish it on exit handlers registration 
>>>> functions (the __dso_handler trick is to make this easier).
>>>
>>> We can find the object that contains the address of __dso_handle, either
>>> at registration time (perhaps better, to keep dlclose cost lower), or
>>> during _dl_fini.
>>
>> We currently have the following:
>>
>>   (libdl) dlclose:
>>           \_ (ld.so) _dl_close_worker
>> 		     - run dt_fini_array
>>                        \_ (libfoo.so) __do_global_dtors_aux
>>                                       \_ (libc.so) __cxa_finalize ((libfoo.so) __dso_handle)
>>
>> What you are suggesting, if I understood correctly is:
>>
>>   (libdl) dlclose:
>>           \_ (ld.so) _dl_close_worker 
>>                      \_ (libc.so) __cxa_finalize ((libfoo.so) __dso_handle)
> 
> Yes.
> 
>> So what we need to get on _dl_close_worked is the (libfoo.so)
>> __dso_handle value.
> 
> Or we call the internal equivalent of dladdr on the supplied
> __dso_handle pointer when the handler is registered.  I expect that will
> result in a slightly smoother execution.
> 
>> We will need to handle some caveats, as to add symbol resolution for
>> local symbols and handle libraries that does not have
>> __do_global_dtors_aux (and thus no __dso_handle).
> 
> Then they rely on ELF constructors exclusively and there is nothing to
> do?  (And there could be __dso_handle present for other reasons, but we
> would not necessarily know about it, given that it's not a dynamic
> symbol, and cannot be.)

I will need to check in which cases __do_global_dtors_aux and thus 
__cxa_finalize is pulled on libgcc.a.

> 
>> I do think it is feasible, we can  add a new field on the link_map to 
>> hold its value or find it on _dl_close_worker.
> 
> I don't think __dso_handle is guaranteed to be unique per object. 8-(
> So we need to be a bit careful there.

My understanding is having multiple values might be problematic, the linker
order might resulting multiple values used in atexit or at_quick_exit.

> 
>>> Have you considered a design which puts the same handler entry on two
>>> different lists?
>>
>> Yes, the problem is we need to handle multiple constraints:
>>
>>   - Any function that register a new exit handle may be called from another
>>     one. For instance atexit my call atexit or __cxa_atexit may call atexit
>>     (or any combination).
>>
>>   - The internal lists follow the GNU principle of adding no limit, so we
>>     support the minimal POSIX states using a static allocated initial list
>>     (so atexit can't fail due malloc failure), and we add a linked list
>>     to make it grow as requested.
>>
>>   - We need to handle BZ#14333, so we need to stop new registered function
>>     once we are over iterating over the list. I don't think it would be
>>     possible with two lists, specially for the case where one callback
>>     can modify the another list.
> 
> It should be possible to have the entry in multiple lists if we add
> reference counters.

I don't think reference counters would help here: atexit can theoretically
call __cxa_atexit and vice-versa, so we will need to iterate over two lists
using the same logic.

I still think it would be simple to change how the internal exit handle list
is organized. I have implemented it on a personal branch [1], along with a
tests that shows a wrong atexit order.

The changes on how atexit is handled are:
    
  1. Add the __atexit symbol which is linked as __cxa_finalize in
     static mode (so __dso_handle is correctly set).  The __atexit
     symbol adds an ef_at exit_function entry on __exit_funcs,
     different than an ef_cxa one from __cxa_atexit.
    
     Old binaries would still call __cxa_atexit, so we do not actually
     need to add a compat symbol.
    
  2. Make __cxa_finalize to handle ef_at as well, similar to ef_cxa.
  
  3. Change how the internal exit handler are organized, so ef_at
     and ef_on handlers (registered by atexit and on_exit) are executed
     before ef_cxa (registered by __cxa_atexit).
    
     Each entry set (struct exit_function_list) has on type associated
     (el_at or el_cxa) to represent the internal handle it contains.
     New insertions (done by the __atexit, __cxa_atexit, etc.) keep the
     node orders, with following constraints:
    
     3.1. el_at nodes should be prior el_cxa.
     3.2. el_at should contain only ef_at, ef_on, or ef_free elements.
     3.3. el_cxa should contain only ef_cxa or ef_free elements.
     3.4. new insertions on each node type should be be kept in lifo order.
     3.5. the original first element should be last one (since it is static
          allocated and 'exit' will deallocated the nodes in order.
    
     So the execution on both __cxa_finalize, exit, or quick_exit will
     iterate over the list by executing first atexit/on_exit handlers and
     then __cxa_atexit ones.  New handlers added by registered functions
     are handled as before, by using the ef_free entry and reseting the
     list iteration


Nat!, if you are still interested you can check on this branch. It should
fix the issues you brought on libc-help.

[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/atexit-order