Message ID | 20210209171839.7911-13-vivek@collabora.com |
---|---|
State | New |
Headers | show |
Series | Implementation of RTLD_SHARED for dlmopen | expand |
On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote: > --- > elf/dl-load.c | 43 +++++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 780bca99e8..13ac2053b8 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -2133,35 +2133,26 @@ _dl_map_object (struct link_map *loader, const char *name, > #endif > > /* Look for this name among those already loaded. */ > - for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next) > + l = _dl_find_dso (name, nsid); > + > + if (l != NULL) > { > - /* If the requested name matches the soname of a loaded object, > - use that object. Elide this check for names that have not > - yet been opened. */ > - if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0)) > - continue; > - if (!_dl_name_match_p (name, l)) > - { > - const char *soname; > - > - if (__glibc_likely (l->l_soname_added) > - || l->l_info[DT_SONAME] == NULL) > - continue; > - > - soname = ((const char *) D_PTR (l, l_info[DT_STRTAB]) > - + l->l_info[DT_SONAME]->d_un.d_val); > - if (strcmp (name, soname) != 0) > - continue; > - > - /* We have a match on a new name -- cache it. */ > - add_name_to_object (l, soname); > - l->l_soname_added = 1; > - } > - > - /* We have a match. */ > +#ifdef SHARED > + /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS > + already opened in the target NS but with RTLD_ISOLATE so it WAS NOT > + created as a proxy we need to error out since we cannot satisfy the > + DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics. */ > + if (!(mode & RTLD_ISOLATE) && > + (l->l_ns != LM_ID_BASE) && > + (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) && > + !l->l_proxy) > + { > + _dl_signal_error (EEXIST, name, NULL, > + N_("object cannot be demoted to a proxy")); > + } > +#endif > return l; I think this patch should reorganized along with the previous, where first you refactor the code to use find_dso and *then* add the required l_proxy change in a subsequent patch. (There are other patch organization that I think should be changed, I will get back to them once I finish reviewing the set) > } > - I usually avoid such spurious change. > /* Display information if we are debugging. */ > if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES) > && loader != NULL) >
On 19/02/2021 11:39, Adhemerval Zanella wrote: > > > On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote: >> --- >> elf/dl-load.c | 43 +++++++++++++++++-------------------------- >> 1 file changed, 17 insertions(+), 26 deletions(-) >> >> diff --git a/elf/dl-load.c b/elf/dl-load.c >> index 780bca99e8..13ac2053b8 100644 >> --- a/elf/dl-load.c >> +++ b/elf/dl-load.c >> @@ -2133,35 +2133,26 @@ _dl_map_object (struct link_map *loader, const char *name, >> #endif >> >> /* Look for this name among those already loaded. */ >> - for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next) >> + l = _dl_find_dso (name, nsid); >> + >> + if (l != NULL) >> { >> - /* If the requested name matches the soname of a loaded object, >> - use that object. Elide this check for names that have not >> - yet been opened. */ >> - if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0)) >> - continue; >> - if (!_dl_name_match_p (name, l)) >> - { >> - const char *soname; >> - >> - if (__glibc_likely (l->l_soname_added) >> - || l->l_info[DT_SONAME] == NULL) >> - continue; >> - >> - soname = ((const char *) D_PTR (l, l_info[DT_STRTAB]) >> - + l->l_info[DT_SONAME]->d_un.d_val); >> - if (strcmp (name, soname) != 0) >> - continue; >> - >> - /* We have a match on a new name -- cache it. */ >> - add_name_to_object (l, soname); >> - l->l_soname_added = 1; >> - } >> - >> - /* We have a match. */ >> +#ifdef SHARED >> + /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS >> + already opened in the target NS but with RTLD_ISOLATE so it WAS NOT >> + created as a proxy we need to error out since we cannot satisfy the >> + DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics. */ >> + if (!(mode & RTLD_ISOLATE) && Also the new RTLD_ISOLATE flag is only defined later in this set. >> + (l->l_ns != LM_ID_BASE) && >> + (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) && >> + !l->l_proxy) >> + { >> + _dl_signal_error (EEXIST, name, NULL, >> + N_("object cannot be demoted to a proxy")); >> + } >> +#endif >> return l; > > I think this patch should reorganized along with the previous, where > first you refactor the code to use find_dso and *then* add the > required l_proxy change in a subsequent patch. > > (There are other patch organization that I think should be changed, > I will get back to them once I finish reviewing the set) > >> } >> - > > I usually avoid such spurious change. > >> /* Display information if we are debugging. */ >> if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES) >> && loader != NULL) >>
> I think this patch should reorganized along with the previous, where > first you refactor the code to use find_dso and *then* add the > required l_proxy change in a subsequent patch. Do you want this combined with the preceding patch or reorganised in some other way?
On 22/02/2021 15:44, Vivek Das Mohapatra wrote: >> I think this patch should reorganized along with the previous, where >> first you refactor the code to use find_dso and *then* add the >> required l_proxy change in a subsequent patch. > > Do you want this combined with the preceding patch or reorganised > in some other way? > My plan is to finish the review and send a overall note of how I think the set should be better organized.
>> Do you want this combined with the preceding patch or reorganised >> in some other way? > > My plan is to finish the review and send a overall note of how I think the set > should be better organized. Ok, thanks.
diff --git a/elf/dl-load.c b/elf/dl-load.c index 780bca99e8..13ac2053b8 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -2133,35 +2133,26 @@ _dl_map_object (struct link_map *loader, const char *name, #endif /* Look for this name among those already loaded. */ - for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next) + l = _dl_find_dso (name, nsid); + + if (l != NULL) { - /* If the requested name matches the soname of a loaded object, - use that object. Elide this check for names that have not - yet been opened. */ - if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0)) - continue; - if (!_dl_name_match_p (name, l)) - { - const char *soname; - - if (__glibc_likely (l->l_soname_added) - || l->l_info[DT_SONAME] == NULL) - continue; - - soname = ((const char *) D_PTR (l, l_info[DT_STRTAB]) - + l->l_info[DT_SONAME]->d_un.d_val); - if (strcmp (name, soname) != 0) - continue; - - /* We have a match on a new name -- cache it. */ - add_name_to_object (l, soname); - l->l_soname_added = 1; - } - - /* We have a match. */ +#ifdef SHARED + /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS + already opened in the target NS but with RTLD_ISOLATE so it WAS NOT + created as a proxy we need to error out since we cannot satisfy the + DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics. */ + if (!(mode & RTLD_ISOLATE) && + (l->l_ns != LM_ID_BASE) && + (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) && + !l->l_proxy) + { + _dl_signal_error (EEXIST, name, NULL, + N_("object cannot be demoted to a proxy")); + } +#endif return l; } - /* Display information if we are debugging. */ if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES) && loader != NULL)