[01/13] elf: strdup() l_name if no realname [BZ #30100]
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
_dl_close_worker() has this code:
/* This name always is allocated. */
free (imap->l_name);
But in that particular case, while indeed being allocated, l_name
doesn't point to the start of an allocation:
new = (struct link_map *) calloc (sizeof (*new) + audit_space
+ sizeof (struct link_map *)
+ sizeof (*newname) + libname_len, 1);
...
new->l_symbolic_searchlist.r_list = (struct link_map **) ((char *) (new + 1)
+ audit_space);
new->l_libname = newname
= (struct libname_list *) (new->l_symbolic_searchlist.r_list + 1);
newname->name = (char *) memcpy (newname + 1, libname, libname_len);
...
new->l_name = (char *) newname->name + libname_len - 1;
It therefore cannot be freed separately.
Use strdup("") as a simple fix.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
---
elf/dl-object.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On 18/03/23 13:50, Stas Sergeev via Libc-alpha wrote:
> _dl_close_worker() has this code:
> /* This name always is allocated. */
> free (imap->l_name);
>
> But in that particular case, while indeed being allocated, l_name
> doesn't point to the start of an allocation:
> new = (struct link_map *) calloc (sizeof (*new) + audit_space
> + sizeof (struct link_map *)
> + sizeof (*newname) + libname_len, 1);
> ...
> new->l_symbolic_searchlist.r_list = (struct link_map **) ((char *) (new + 1)
> + audit_space);
>
> new->l_libname = newname
> = (struct libname_list *) (new->l_symbolic_searchlist.r_list + 1);
> newname->name = (char *) memcpy (newname + 1, libname, libname_len);
> ...
> new->l_name = (char *) newname->name + libname_len - 1;
>
> It therefore cannot be freed separately.
> Use strdup("") as a simple fix.
This is not required, the l_name alias to newname->name is only used for
__RTLD_OPENEXEC (used by loader on DT_NEEDED) and these handlers are not
meant to be dlclose. So even if the programs get an already opened
handler to a DT_NEEDED object:
void *h = dlopen ("libc.so.6", RTLD_NOW);
assert (h != NULL);
dlclose (h);
The _dl_close_worker comment is indeed correct: the l_name for
dlopen is *always* allocated by open_path so it can always call free on it.
>
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> ---
> elf/dl-object.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/elf/dl-object.c b/elf/dl-object.c
> index f1f2ec956c..ab926cd4bf 100644
> --- a/elf/dl-object.c
> +++ b/elf/dl-object.c
> @@ -122,7 +122,10 @@ _dl_new_object (char *realname, const char *libname, int type,
> #endif
> new->l_name = realname;
> else
> - new->l_name = (char *) newname->name + libname_len - 1;
> + /* When realname="", it is not allocated and points to the constant
> + string. Constness is dropped by an explicit cast. :(
> + So strdup() it here. */
> + new->l_name = __strdup ("");
>
> new->l_type = type;
> /* If we set the bit now since we know it is never used we avoid
29.03.2023 18:54, Adhemerval Zanella Netto пишет:
>
> On 18/03/23 13:50, Stas Sergeev via Libc-alpha wrote:
>> _dl_close_worker() has this code:
>> /* This name always is allocated. */
>> free (imap->l_name);
>>
>> But in that particular case, while indeed being allocated, l_name
>> doesn't point to the start of an allocation:
>> new = (struct link_map *) calloc (sizeof (*new) + audit_space
>> + sizeof (struct link_map *)
>> + sizeof (*newname) + libname_len, 1);
>> ...
>> new->l_symbolic_searchlist.r_list = (struct link_map **) ((char *) (new + 1)
>> + audit_space);
>>
>> new->l_libname = newname
>> = (struct libname_list *) (new->l_symbolic_searchlist.r_list + 1);
>> newname->name = (char *) memcpy (newname + 1, libname, libname_len);
>> ...
>> new->l_name = (char *) newname->name + libname_len - 1;
>>
>> It therefore cannot be freed separately.
>> Use strdup("") as a simple fix.
> This is not required, the l_name alias to newname->name is only used for
> __RTLD_OPENEXEC (used by loader on DT_NEEDED) and these handlers are not
> meant to be dlclose.
But dlmem() can also use "" as the name
if the name is not specified explicitly.
Without that patch it crashes.
I think you mean its not needed w/o dlmem()?
Then its a dlmem-specific patch.
On 29/03/23 11:12, stsp wrote:
>
> 29.03.2023 18:54, Adhemerval Zanella Netto пишет:
>>
>> On 18/03/23 13:50, Stas Sergeev via Libc-alpha wrote:
>>> _dl_close_worker() has this code:
>>> /* This name always is allocated. */
>>> free (imap->l_name);
>>>
>>> But in that particular case, while indeed being allocated, l_name
>>> doesn't point to the start of an allocation:
>>> new = (struct link_map *) calloc (sizeof (*new) + audit_space
>>> + sizeof (struct link_map *)
>>> + sizeof (*newname) + libname_len, 1);
>>> ...
>>> new->l_symbolic_searchlist.r_list = (struct link_map **) ((char *) (new + 1)
>>> + audit_space);
>>>
>>> new->l_libname = newname
>>> = (struct libname_list *) (new->l_symbolic_searchlist.r_list + 1);
>>> newname->name = (char *) memcpy (newname + 1, libname, libname_len);
>>> ...
>>> new->l_name = (char *) newname->name + libname_len - 1;
>>>
>>> It therefore cannot be freed separately.
>>> Use strdup("") as a simple fix.
>> This is not required, the l_name alias to newname->name is only used for
>> __RTLD_OPENEXEC (used by loader on DT_NEEDED) and these handlers are not
>> meant to be dlclose.
> But dlmem() can also use "" as the name
> if the name is not specified explicitly.
> Without that patch it crashes.
> I think you mean its not needed w/o dlmem()?
Yes, I did not take in consideration dlmem inclusion for this. If dlmem breaks
this assumption, it is another issue with the interface.
> Then its a dlmem-specific patch.
29.03.2023 19:19, Adhemerval Zanella Netto пишет:
>
> On 29/03/23 11:12, stsp wrote:
>> 29.03.2023 18:54, Adhemerval Zanella Netto пишет:
>> But dlmem() can also use "" as the name
>> if the name is not specified explicitly.
>> Without that patch it crashes.
>> I think you mean its not needed w/o dlmem()?
> Yes, I did not take in consideration dlmem inclusion for this. If dlmem breaks
> this assumption, it is another issue with the interface.
Could you please suggest what should
be done? dlmem() may create the anonymous
objects, name is optional. I noticed that some
code uses "" and did the same.
What is the suggested solution about
creating an anonymous object?
Of course you can prohibit an empty name
for dlmem(), but that will only force people
to fake it with some "123".
On 29/03/23 11:28, stsp wrote:
>
> 29.03.2023 19:19, Adhemerval Zanella Netto пишет:
>>
>> On 29/03/23 11:12, stsp wrote:
>>> 29.03.2023 18:54, Adhemerval Zanella Netto пишет:
>>> But dlmem() can also use "" as the name
>>> if the name is not specified explicitly.
>>> Without that patch it crashes.
>>> I think you mean its not needed w/o dlmem()?
>> Yes, I did not take in consideration dlmem inclusion for this. If dlmem breaks
>> this assumption, it is another issue with the interface.
> Could you please suggest what should
> be done? dlmem() may create the anonymous
> objects, name is optional. I noticed that some
> code uses "" and did the same.
> What is the suggested solution about
> creating an anonymous object?
> Of course you can prohibit an empty name
> for dlmem(), but that will only force people
> to fake it with some "123".
At least move this change to patch that actually requires it, as an
isolated change it is not required.
29.03.2023 19:30, Adhemerval Zanella Netto пишет:
> At least move this change to patch that actually requires it, as an
> isolated change it is not required.
Will do, thanks.
@@ -122,7 +122,10 @@ _dl_new_object (char *realname, const char *libname, int type,
#endif
new->l_name = realname;
else
- new->l_name = (char *) newname->name + libname_len - 1;
+ /* When realname="", it is not allocated and points to the constant
+ string. Constness is dropped by an explicit cast. :(
+ So strdup() it here. */
+ new->l_name = __strdup ("");
new->l_type = type;
/* If we set the bit now since we know it is never used we avoid