[PATCH 06/15] elf: Fix comments and logic in _dl_add_to_slotinfo

[Adhemerval Zanella]

On 06/04/2021 12:48, Szabolcs Nagy wrote:
> The 04/02/2021 17:50, Adhemerval Zanella via Libc-alpha wrote:
>> On 15/02/2021 08:59, Szabolcs Nagy via Libc-alpha wrote:
>>> From: Szabolcs Nagy <szabolcs dot nagy at arm dot com>
>>>
>>> Since
>>>
>>>   commit a509eb117fac1d764b15eba64993f4bdb63d7f3c
>>>   Avoid late dlopen failure due to scope, TLS slotinfo updates [BZ #25112]
>>>
>>> the generation counter update is not needed in the failure path.
>>
>> It is not clear to me from just the commit reference why it would
>> be safe to remove the GL(dl_tls_generation) update on 
>> _dl_add_to_slotinfo.
>>
>> The dl_open_worker calls update_tls_slotinfo which in turn call
>> might call _dl_add_to_slotinfo *after* the demarcation point.  Will
>> it terminate the process?
> 
> in that commit the logic got changed such that allocations
> happen before the demarcation point in resize_tls_slotinfo.
> 
> this is the reason for the do_add bool argument in
> _dl_add_to_slotinfo: it's called twice and the first call
> with do_add==false is only there to ensure everything is
> allocated before the demarcation point (so module loading
> can be reverted, no need to bump the generation count).
> 
> i guess this is not visible by just looking at the
> _dl_add_to_slotinfo code.

Right, so if I reading correctly once _dl_add_to_slotinfo (..., true)
is called by update_tls_slotinfo, the malloc that create a new
dtv_slotinfo_list won't be called (since it was already allocated
previously) since the entry was already pre-allocated and thus the
search part at line 978-987 will find.  Is that correct?

> 
> note that adding some asserts to ensure there is no allocation
> when do_add==true does not work: rtld uses the same api, but
> without the do_add==false preallocation step since at startup
> time allocation failure is fatal anyway.

Right, the _dl_signal_error will trigger a fatal_error since
lcatch won't be override yet.

Thanks for the explanation.

> 
>>> ---
>>>  elf/dl-tls.c | 11 +----------
>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
>>> index 79b93ad91b..24d00c14ef 100644
>>> --- a/elf/dl-tls.c
>>> +++ b/elf/dl-tls.c
>>> @@ -998,16 +998,7 @@ _dl_add_to_slotinfo (struct link_map *l, bool do_add)
>>>  		+ TLS_SLOTINFO_SURPLUS * sizeof (struct dtv_slotinfo));
>>>        if (listp == NULL)
>>>  	{
>>> -	  /* We ran out of memory.  We will simply fail this
>>> -	     call but don't undo anything we did so far.  The
>>> -	     application will crash or be terminated anyway very
>>> -	     soon.  */
>>> -
>>> -	  /* We have to do this since some entries in the dtv
>>> -	     slotinfo array might already point to this
>>> -	     generation.  */
>>> -	  ++GL(dl_tls_generation);
>>> -
>>> +	  /* We ran out of memory while resizing the dtv slotinfo list.  */
>>>  	  _dl_signal_error (ENOMEM, "dlopen", NULL, N_("\
>>>  cannot create TLS data structures"));
>>>  	}