[PATCH] elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic()

[PATCH] elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic()

[Xiaoming Ni]

Check the return value of malloc based on the function header comment of
 _dl_make_tlsdesc_dynamic(). If the return value fails, NULL is returned.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 elf/tlsdeschtab.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/elf/tlsdeschtab.h b/elf/tlsdeschtab.h
index 8c02e45a49..82733159e3 100644
--- a/elf/tlsdeschtab.h
+++ b/elf/tlsdeschtab.h
@@ -110,6 +110,8 @@ _dl_make_tlsdesc_dynamic (struct link_map *map, size_t ti_offset)
     }
 
   *entry = td = malloc (sizeof (struct tlsdesc_dynamic_arg));
+  if (! td)
+    return 0;
   /* This may be higher than the map's generation, but it doesn't
      matter much.  Worst case, we'll have one extra DTV update per
      thread.  */
-- 
2.27.0

Re: [PATCH] elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic()

[Szabolcs Nagy]

The 11/04/2022 17:30, Xiaoming Ni wrote:
> Check the return value of malloc based on the function header comment of
>  _dl_make_tlsdesc_dynamic(). If the return value fails, NULL is returned.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>

note that allocation failure is not recoverable here:
the caller cannot really deal with NULL so tls access
will crash.

i think the patch is good, but it will only help if
the failure is propagated to _dl_relocate_object and
handled in dlopen by returning an error.

let me know if you don't have commit access, then i
can commit it for you.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

> ---
>  elf/tlsdeschtab.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/elf/tlsdeschtab.h b/elf/tlsdeschtab.h
> index 8c02e45a49..82733159e3 100644
> --- a/elf/tlsdeschtab.h
> +++ b/elf/tlsdeschtab.h
> @@ -110,6 +110,8 @@ _dl_make_tlsdesc_dynamic (struct link_map *map, size_t ti_offset)
>      }
>  
>    *entry = td = malloc (sizeof (struct tlsdesc_dynamic_arg));
> +  if (! td)
> +    return 0;
>    /* This may be higher than the map's generation, but it doesn't
>       matter much.  Worst case, we'll have one extra DTV update per
>       thread.  */
> -- 
> 2.27.0
> 

Re: [PATCH] elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic()

[Xiaoming Ni]

On 2022/11/4 19:00, Szabolcs Nagy wrote:
> The 11/04/2022 17:30, Xiaoming Ni wrote:
>> Check the return value of malloc based on the function header comment of
>>   _dl_make_tlsdesc_dynamic(). If the return value fails, NULL is returned.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> 
> note that allocation failure is not recoverable here:
> the caller cannot really deal with NULL so tls access
> will crash.
_dl_make_tlsdesc_dynamic() is called by "void elf_machine_rela()"
Whether to add _dl_error_printf() to elf_machine_rela() when 
_dl_make_tlsdesc_dynamic() returns NULL ?
Or change "void elf_machine_rela()"  to non-void ?

> 
> i think the patch is good, but it will only help if
> the failure is propagated to _dl_relocate_object and
> handled in dlopen by returning an error.
> 
> let me know if you don't have commit access, then i
> can commit it for you.
> 
I don't have permission to submit, thank you for your review and help.

> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

Thanks
Xiaoming Ni


> 
>> ---
>>   elf/tlsdeschtab.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/elf/tlsdeschtab.h b/elf/tlsdeschtab.h
>> index 8c02e45a49..82733159e3 100644
>> --- a/elf/tlsdeschtab.h
>> +++ b/elf/tlsdeschtab.h
>> @@ -110,6 +110,8 @@ _dl_make_tlsdesc_dynamic (struct link_map *map, size_t ti_offset)
>>       }
>>   
>>     *entry = td = malloc (sizeof (struct tlsdesc_dynamic_arg));
>> +  if (! td)
>> +    return 0;
>>     /* This may be higher than the map's generation, but it doesn't
>>        matter much.  Worst case, we'll have one extra DTV update per
>>        thread.  */
>> -- 
>> 2.27.0
>>
> .
> 

Re: [PATCH] elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic()

[Szabolcs Nagy]

The 11/05/2022 10:34, Xiaoming Ni wrote:
> On 2022/11/4 19:00, Szabolcs Nagy wrote:
> > The 11/04/2022 17:30, Xiaoming Ni wrote:
> > > Check the return value of malloc based on the function header comment of
> > >   _dl_make_tlsdesc_dynamic(). If the return value fails, NULL is returned.
> > > 
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > 
> > note that allocation failure is not recoverable here:
> > the caller cannot really deal with NULL so tls access
> > will crash.
> _dl_make_tlsdesc_dynamic() is called by "void elf_machine_rela()"
> Whether to add _dl_error_printf() to elf_machine_rela() when
> _dl_make_tlsdesc_dynamic() returns NULL ?
> Or change "void elf_machine_rela()"  to non-void ?

the error has to be propagated up to dlopen in one way or another.

i think this can be done by _dl_signal_error (but it has to be
verified that there are no leaked resources or held locks along
the call stack up to dlopen). and then it's probably better to
do this from _dl_make_tlsdesc_dynamic instead of return NULL.

this is bug 27404, i cannot work on it now, but i can review
if you have patches.

for now i committed this patch so the code matches the comment.

> > i think the patch is good, but it will only help if
> > the failure is propagated to _dl_relocate_object and
> > handled in dlopen by returning an error.
> > 
> > let me know if you don't have commit access, then i
> > can commit it for you.
> > 
> I don't have permission to submit, thank you for your review and help.
> 
> > Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> 
> Thanks
> Xiaoming Ni
> 
> 
> > 
> > > ---
> > >   elf/tlsdeschtab.h | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/elf/tlsdeschtab.h b/elf/tlsdeschtab.h
> > > index 8c02e45a49..82733159e3 100644
> > > --- a/elf/tlsdeschtab.h
> > > +++ b/elf/tlsdeschtab.h
> > > @@ -110,6 +110,8 @@ _dl_make_tlsdesc_dynamic (struct link_map *map, size_t ti_offset)
> > >       }
> > >     *entry = td = malloc (sizeof (struct tlsdesc_dynamic_arg));
> > > +  if (! td)
> > > +    return 0;
> > >     /* This may be higher than the map's generation, but it doesn't
> > >        matter much.  Worst case, we'll have one extra DTV update per
> > >        thread.  */
> > > -- 
> > > 2.27.0
> > > 
> > .
> > 
>