Re: [PATCH v2] elf: Make more functions available for binding during dlclose (bug 30425)

[Carlos O'Donell <carlos@redhat.com>]

On 5/25/23 15:06, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 5/22/23 07:32, Florian Weimer via Libc-alpha wrote:
>>> Previously, after destructors for a DSO have been invoked, ld.so refused
>>> to bind against that DSO in all cases.  Relax this restriction somewhat
>>> if the referencing object is itself a DSO that is being unloaded.  This
>>> assumes that the symbol reference is not going to be stored anywhere.
>>
>> The truth here is that the example has a circular reference due to the
>> interposition which makes it have an undefined load and unload
>> order. You wrote a very specific test that *avoids* needing anything
>> from mod1 during mod2's initialization.
> 
> It's possible to create this situation with C++ code due to vague
> linkage.  No explicit interposition is needed.  I didn't want to
> incorporate the C++ test case because it's brittle: it depends on which
> functions the compiler emits.  (It has some leeway because of vague
> linkage.)

I agree that if the test is brittle we should just use what you have.

Don't get me wrong, I think the test you wrote is a *good* test in that you are
trying to exercise a scenario that should work in a consistent fashion. We should
be breaking the circular dependencies in a consistent fashion.

>> Having said all that we should *choose* an unload order that is the
>> opposite of the load order and make it consistent, and if we could
>> load it we should not fail to unload it because of a limitation of the
>> dynamic loader. We might still fail due to some logical dependency in
>> user code though.
>>
>> The DT_NEEDED in mod1 ensures we load: mod2 then mod1.  The closing of
>> mod1 should unload: mod1 then mod2 (opposite order).
>>
>> This consistent order should make it easier for users to debug other
>> problems in their library designs.
> 
> Yeah, sure?  But that's how it's always been?

Except that with bug 30425 it isn't? It crashes? You fixed it! :-)
 
>>> The situation in the test case can arise fairly easily with C++ and
>>> objects that are built with different optimization levels and therefore
>>> define different functions with vague linkage.
>>
>> Please post a v3.
> 
> With the comment updates?  Or some other changes?

My apologies, let me be more specific.

During review I called out the following things:

- Why do we need '__attribute__ ((weak))'? Needs comment.
- Comment update suggestion in mod1 test case.
- Net new comment in test case to explain expectations.

Would you be able to address those for v3?

-- 
Cheers,
Carlos.