| Message ID | 20221104093000.25169-1-nixiaoming@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | elf/tlsdeschtab.h: Add the Malloc return value check in _dl_make_tlsdesc_dynamic() | expand |
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 >
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 >> > . >
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 > > > > > . > > >
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. */
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(+)