| Message ID | 20230318165110.3672749-2-stsp2@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | implement dlmem() function | expand |
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.
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
_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(-)