[PATCH] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595)

[Adhemerval Zanella]

On 24/05/2019 11:42, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 24/05/2019 08:06, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 23/05/2019 16:50, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>> The solution sounds correct, but I don't have a strong opinion if this
>>>>>> is really an improvement over a recursive lock plus a linked list.  It 
>>>>>> potentially adds 'free' calls in fork for multithread mode if list needs 
>>>>>> to be deallocated. Also, since the locks is internal to register-atfork.c
>>>>>> we might have a better control to make the exported interfaces not 
>>>>>> deadlock.
>>>>>
>>>>> Recursive locks do not prevent deadlocks if the callback launches
>>>>> threads.  I'm not sure if this can be considered a stretch.  If
>>>>> there's dlclose involved, their might also be dlopen, and threads
>>>>> launched from an ELF constructor and so on.
>>>>
>>>> That's why I said the core issue is atfork handlers are really
>>>> underspecified, and maybe it would be better if we document more
>>>> thoughtfully what we really aim to support by atfork handlers.
>>>
>>> That's also true.
>>>
>>>>> Furthermore, I'm not sure if the implementation of __unregister_atfork
>>>>> is correct.  I think you have a potential use-after-free issue if the
>>>>> callback deregisters a fork handler.
>>>
>>>> The possible way I can think of is if an atfork handler from a module
>>>> try to dlclose itself and I am not sure how valid it is.
>>>
>>> dlclose'ing itself is not really valid, I think.  It can only work if
>>> the dlclose call is a tail call.
>>
>> I would work towards not make dlclose itself valid, do you see any
>> reason to allow to do so?
> 
> Sorry, what do you mean?  Inspect the caller address and reject the
> dlclose call if it's within the DSO being closed?  This just avoids a
> crash, instead resulting in a dlclose error return.  Not sure if this is
> particularly relevant.

Either a runtime or document this is undefined behaviour.

> 
>>> __unregister_atfork still adds a use-after-free issue, even with the
>>> copy-on-write list: If a dlclose unregisters a fork handler, we will
>>> still attempt to call it subsequently if it was on the list before.  No
>>> data race is involved if the dlclose call happens itself in a fork
>>> handler (so we are back to the territory of the bug).
>>>
>>> So the simple reference counting scheme I proposed is probably not the
>>> right solution, and we need a different data structure
>>
>> My understanding is if the idea of dlclose on atfork handler is to
>> not just unregister the library but also prevent further arfork handler
>> to been called (since __cxa_finalize will call __unregister_atfork), 
>> I still think that the recursive mutex plus double-linked list is
>> the best a suitable solution if we assume two premisses:
>>
>>   - It is invalid to dlclose itself.
>>   - Atfork handlers should be local to modules.
>>
>> Both should avoid imho the use-after-free issue since the callback
>> won't try to deregister an already used entry in atfork handler list
>> walking.
> 
> I'm not sure about that.  The dlclose could remove a fork handler that
> has yet to be called.  The handlers may not have been registered in load
> order.
> 

That's should not be a problem. My understanding is respecting the premisses,
a module unregistering an atfork handler will keep the atfork handler list 
concise (i.e, no dangling pointer or entry with invalid callback handlers).