From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com [IPv6:2607:f8b0:4864:20::72f]) by sourceware.org (Postfix) with ESMTPS id 1B683383D026 for ; Wed, 2 Jun 2021 19:18:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B683383D026 Received: by mail-qk1-x72f.google.com with SMTP id i67so3548585qkc.4 for ; Wed, 02 Jun 2021 12:18:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=7uVCNm5lCAhALpcf6G3Rc2MWA9+0+AZWAGaJaqH5WYs=; b=cZMqdNJW7hUfCV1SSK8CRy1Tre+dRyZd3VPHHUPA0Y4FNLhXN1dL3oWVPAhNLMdDgo I8luYYBv/ckhy+2srgsWTP3+zKACOPcNI2WwtWVFCy7lWOIiVY2PkA/krkEsuf6caTXx Up84r7r8Jw4sJAZemk1zFI+12mWmurXV1ZL5CTZvws8xKcvKr4UGOzIcYWF9xlTPnUA1 SCUmvatVEfJTW8tvi1dEsIth4O9r2FZu9V+pxhSl1KDP2a1h5+xJlopCBaYz3qmAa5B1 URgJOUumem32SH2lK3IupWA4zeNemDQHL/hkNg5pz1R4BMaF20BkhGIs0QFzZh5dop2E sD0w== X-Gm-Message-State: AOAM532nh8LyDAvmId6EBcUA2M38NAtPdmynnLDALXXm7YR0rNtrb4fi BoC6In9eh8un8D27SKVBhLuPIxzx7EJnqQ== X-Google-Smtp-Source: ABdhPJxEaYBusYXbQbXqWBl23YZFVkW7UPzHQU6Ry1nNsgouub/WjmZXBt9JA23NPgGsFoHyCPHgVA== X-Received: by 2002:a37:ad08:: with SMTP id f8mr28769230qkm.447.1622661520393; Wed, 02 Jun 2021 12:18:40 -0700 (PDT) Received: from [192.168.1.4] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id 97sm404923qte.20.2021.06.02.12.18.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Jun 2021 12:18:39 -0700 (PDT) Subject: Re: [PATCH 16/16] dlfcn: Rework static dlopen hooks To: Florian Weimer , libc-alpha@sourceware.org References: <3c052fb456744cea69254e95a78a331579068b55.1622469909.git.fweimer@redhat.com> From: Adhemerval Zanella Message-ID: Date: Wed, 2 Jun 2021 16:18:37 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <3c052fb456744cea69254e95a78a331579068b55.1622469909.git.fweimer@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_STOCKGEN, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Jun 2021 19:18:44 -0000 On 31/05/2021 11:12, Florian Weimer via Libc-alpha wrote: > Consolidate all hooks structures into a single one. There are > no static dlopen ABI concerns because glibc 2.34 already comes > with substantial ABI-incompatible chagnes in this area. (Static Typo 'chagnes'. > dlopen requires the exact same dynamic glibc version that was used > for static linking.) > > The new approach uses a pointer to the hooks structure into > _rtld_global_ro and initalizes it in __rtld_static_init. This avoids > a back-and-forth with various callback functions. LGTM, thanks. Reviewed-by: Adhemerval Zanella > --- > dlfcn/dladdr.c | 2 +- > dlfcn/dladdr1.c | 2 +- > dlfcn/dlclose.c | 2 +- > dlfcn/dlerror.c | 32 +---------------- > dlfcn/dlinfo.c | 2 +- > dlfcn/dlmopen.c | 10 ++---- > dlfcn/dlopen.c | 10 ++---- > dlfcn/dlopenold.c | 2 +- > dlfcn/dlsym.c | 2 +- > dlfcn/dlvsym.c | 3 +- > elf/Versions | 1 - > elf/dl-libc.c | 73 ++++---------------------------------- > elf/rtld_static_init.c | 18 ++++++++++ > include/dlfcn.h | 31 ++++++++-------- > sysdeps/generic/ldsodefs.h | 3 ++ > 15 files changed, 54 insertions(+), 139 deletions(-) > > diff --git a/dlfcn/dladdr.c b/dlfcn/dladdr.c > index 3ef1b7f0b6..1cc305f0c4 100644 > --- a/dlfcn/dladdr.c > +++ b/dlfcn/dladdr.c > @@ -25,7 +25,7 @@ __dladdr (const void *address, Dl_info *info) > { > #ifdef SHARED > if (!rtld_active ()) > - return _dlfcn_hook->dladdr (address, info); > + return GLRO (dl_dlfcn_hook)->dladdr (address, info); > #endif > return _dl_addr (address, info, NULL, NULL); > } Ok. > diff --git a/dlfcn/dladdr1.c b/dlfcn/dladdr1.c > index 203d6398e4..78560dbac2 100644 > --- a/dlfcn/dladdr1.c > +++ b/dlfcn/dladdr1.c > @@ -25,7 +25,7 @@ __dladdr1 (const void *address, Dl_info *info, void **extra, int flags) > { > #ifdef SHARED > if (!rtld_active ()) > - return _dlfcn_hook->dladdr1 (address, info, extra, flags); > + return GLRO (dl_dlfcn_hook)->dladdr1 (address, info, extra, flags); > #endif > > switch (flags) Ok. > diff --git a/dlfcn/dlclose.c b/dlfcn/dlclose.c > index 4d5d307ab1..6a013a81bb 100644 > --- a/dlfcn/dlclose.c > +++ b/dlfcn/dlclose.c > @@ -25,7 +25,7 @@ __dlclose (void *handle) > { > #ifdef SHARED > if (!rtld_active ()) > - return _dlfcn_hook->dlclose (handle); > + return GLRO (dl_dlfcn_hook)->dlclose (handle); > #endif > > return _dlerror_run (GLRO (dl_close), handle) ? -1 : 0; Ok. > diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c > index 3df8602f4d..d0194a7cef 100644 > --- a/dlfcn/dlerror.c > +++ b/dlfcn/dlerror.c > @@ -33,7 +33,7 @@ __dlerror (void) > { > # ifdef SHARED > if (!rtld_active ()) > - return _dlfcn_hook->dlerror (); > + return GLRO (dl_dlfcn_hook)->dlerror (); > # endif > > struct dl_action_result *result = __libc_dlerror_result; > @@ -197,33 +197,3 @@ _dlerror_run (void (*operate) (void *), void *args) > } > } > libc_hidden_def (_dlerror_run) > - > -#ifdef SHARED > -struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon)); > -libc_hidden_data_def (_dlfcn_hook) > - > -#else /* !SHARED */ > - > -static struct dlfcn_hook _dlfcn_hooks = > - { > - .dlopen = __dlopen, > - .dlclose = __dlclose, > - .dlsym = __dlsym, > - .dlvsym = __dlvsym, > - .dlerror = __dlerror, > - .dladdr = __dladdr, > - .dladdr1 = __dladdr1, > - .dlinfo = __dlinfo, > - .dlmopen = __dlmopen > - }; > - > -void > -__libc_register_dlfcn_hook (struct link_map *map) > -{ > - struct dlfcn_hook **hook; > - > - hook = (struct dlfcn_hook **) __libc_dlsym_private (map, "_dlfcn_hook"); > - if (hook != NULL) > - *hook = &_dlfcn_hooks; > -} > -#endif /* !SHARED */ Ok. > diff --git a/dlfcn/dlinfo.c b/dlfcn/dlinfo.c > index 15fcbc5dc1..c6f9a1da09 100644 > --- a/dlfcn/dlinfo.c > +++ b/dlfcn/dlinfo.c > @@ -90,7 +90,7 @@ int > ___dlinfo (void *handle, int request, void *arg) > { > if (!rtld_active ()) > - return _dlfcn_hook->dlinfo (handle, request, arg); > + return GLRO (dl_dlfcn_hook)->dlinfo (handle, request, arg); > else > return dlinfo_implementation (handle, request, arg); > } Ok. > diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c > index ae42814bbf..c171c8953d 100644 > --- a/dlfcn/dlmopen.c > +++ b/dlfcn/dlmopen.c > @@ -81,7 +81,7 @@ void * > ___dlmopen (Lmid_t nsid, const char *file, int mode) > { > if (!rtld_active ()) > - return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0)); > + return GLRO (dl_dlfcn_hook)->dlmopen (nsid, file, mode, RETURN_ADDRESS (0)); > else > return dlmopen_implementation (nsid, file, mode, RETURN_ADDRESS (0)); > } > @@ -101,13 +101,7 @@ __dlmopen (Lmid_t nsid, const char *file, int mode, void *dl_caller) > void * > ___dlmopen (Lmid_t nsid, const char *file, int mode) > { > - struct link_map *l = __dlmopen (nsid, file, mode, RETURN_ADDRESS (0)); > - if (l != NULL) > - { > - __libc_register_dl_open_hook (l); > - __libc_register_dlfcn_hook (l); > - } > - return l; > + return __dlmopen (nsid, file, mode, RETURN_ADDRESS (0)); > } > weak_alias (___dlmopen, dlmopen) > static_link_warning (dlmopen) Ok. > diff --git a/dlfcn/dlopen.c b/dlfcn/dlopen.c > index afdc113efb..e04b374b82 100644 > --- a/dlfcn/dlopen.c > +++ b/dlfcn/dlopen.c > @@ -76,7 +76,7 @@ void * > ___dlopen (const char *file, int mode) > { > if (!rtld_active ()) > - return _dlfcn_hook->dlopen (file, mode, RETURN_ADDRESS (0)); > + return GLRO (dl_dlfcn_hook)->dlopen (file, mode, RETURN_ADDRESS (0)); > else > return dlopen_implementation (file, mode, RETURN_ADDRESS (0)); > } > @@ -96,13 +96,7 @@ __dlopen (const char *file, int mode, void *dl_caller) > void * > ___dlopen (const char *file, int mode) > { > - struct link_map *l = __dlopen (file, mode, RETURN_ADDRESS (0)); > - if (l != NULL) > - { > - __libc_register_dl_open_hook (l); > - __libc_register_dlfcn_hook (l); > - } > - return l; > + return __dlopen (file, mode, RETURN_ADDRESS (0)); > } > weak_alias (___dlopen, dlopen) > static_link_warning (dlopen) Ok. > diff --git a/dlfcn/dlopenold.c b/dlfcn/dlopenold.c > index 0fe5f24cc5..9115501ac1 100644 > --- a/dlfcn/dlopenold.c > +++ b/dlfcn/dlopenold.c > @@ -71,7 +71,7 @@ __dlopen_nocheck (const char *file, int mode) > args.mode = mode; > > if (!rtld_active ()) > - return _dlfcn_hook->dlopen (file, mode, RETURN_ADDRESS (0)); > + return GLRO (dl_dlfcn_hook)->dlopen (file, mode, RETURN_ADDRESS (0)); > > return _dlerror_run (dlopen_doit, &args) ? NULL : args.new; > } Ok. > diff --git a/dlfcn/dlsym.c b/dlfcn/dlsym.c > index 6b03b7b7ab..43044cf7bb 100644 > --- a/dlfcn/dlsym.c > +++ b/dlfcn/dlsym.c > @@ -63,7 +63,7 @@ void * > ___dlsym (void *handle, const char *name) > { > if (!rtld_active ()) > - return _dlfcn_hook->dlsym (handle, name, RETURN_ADDRESS (0)); > + return GLRO (dl_dlfcn_hook)->dlsym (handle, name, RETURN_ADDRESS (0)); > else > return dlsym_implementation (handle, name, RETURN_ADDRESS (0)); > } Ok. > diff --git a/dlfcn/dlvsym.c b/dlfcn/dlvsym.c > index de6b340647..9b76f9afa5 100644 > --- a/dlfcn/dlvsym.c > +++ b/dlfcn/dlvsym.c > @@ -66,7 +66,8 @@ void * > ___dlvsym (void *handle, const char *name, const char *version) > { > if (!rtld_active ()) > - return _dlfcn_hook->dlvsym (handle, name, version, RETURN_ADDRESS (0)); > + return GLRO (dl_dlfcn_hook)->dlvsym (handle, name, version, > + RETURN_ADDRESS (0)); > else > return dlvsym_implementation (handle, name, version, RETURN_ADDRESS (0)); > } Ok. > diff --git a/elf/Versions b/elf/Versions > index be88c48e6d..a12d64e8db 100644 > --- a/elf/Versions > +++ b/elf/Versions > @@ -23,7 +23,6 @@ libc { > GLIBC_PRIVATE { > # functions used in other libraries > _dl_addr; > - _dl_open_hook; _dl_open_hook2; > _dl_sym; _dl_vsym; > __libc_dlclose; __libc_dlopen_mode; __libc_dlsym; __libc_dlvsym; > __libc_early_init; Ok. > diff --git a/elf/dl-libc.c b/elf/dl-libc.c > index ed551f6e56..3ac2a0645f 100644 > --- a/elf/dl-libc.c > +++ b/elf/dl-libc.c > @@ -126,32 +126,7 @@ do_dlclose (void *ptr) > GLRO(dl_close) ((struct link_map *) ptr); > } > > -/* This code is to support __libc_dlopen from __libc_dlopen'ed shared > - libraries. We need to ensure the statically linked __libc_dlopen > - etc. functions are used instead of the dynamically loaded. */ > -struct dl_open_hook > -{ > - void *(*dlopen_mode) (const char *name, int mode); > - void *(*dlsym) (void *map, const char *name); > - int (*dlclose) (void *map); > - void *(*dlvsym) (void *map, const char *name, const char *version); > -}; > - > -#ifdef SHARED > -extern struct dl_open_hook *_dl_open_hook; > -libc_hidden_proto (_dl_open_hook); > -struct dl_open_hook *_dl_open_hook __attribute__ ((nocommon)); > -libc_hidden_data_def (_dl_open_hook); > - > -/* The dlvsym member was added retroactively to struct dl_open_hook. > - Static applications which have it will set _dl_open_hook2 in > - addition to _dl_open_hook. */ > -extern struct dl_open_hook *_dl_open_hook2; > -libc_hidden_proto (_dl_open_hook2); > -struct dl_open_hook *_dl_open_hook2 __attribute__ ((nocommon)); > -libc_hidden_data_def (_dl_open_hook2); > - > -#else > +#ifndef SHARED > static void > do_dlsym_private (void *ptr) > { > @@ -169,14 +144,6 @@ do_dlsym_private (void *ptr) > args->map->l_scope, &vers, 0, 0, NULL); > args->loadbase = l; > } > - > -static struct dl_open_hook _dl_open_hook = > - { > - .dlopen_mode = __libc_dlopen_mode, > - .dlsym = __libc_dlsym, > - .dlclose = __libc_dlclose, > - .dlvsym = __libc_dlvsym, > - }; > #endif > > /* ... and these functions call dlerror_run. */ Ok. > @@ -191,16 +158,9 @@ __libc_dlopen_mode (const char *name, int mode) > > #ifdef SHARED > if (!rtld_active ()) > - return _dl_open_hook->dlopen_mode (name, mode); > - return (dlerror_run (do_dlopen, &args) ? NULL : (void *) args.map); > -#else > - if (dlerror_run (do_dlopen, &args)) > - return NULL; > - > - __libc_register_dl_open_hook (args.map); > - __libc_register_dlfcn_hook (args.map); > - return (void *) args.map; > + return GLRO (dl_dlfcn_hook)->libc_dlopen_mode (name, mode); > #endif > + return (dlerror_run (do_dlopen, &args) ? NULL : (void *) args.map); I think you don not need the extra parentheses here. > } > libc_hidden_def (__libc_dlopen_mode) > Ok. > @@ -216,21 +176,6 @@ __libc_dlsym_private (struct link_map *map, const char *name) > return DL_SYMBOL_ADDRESS (sargs.loadbase, sargs.ref); > return NULL; > } > - > -void > -__libc_register_dl_open_hook (struct link_map *map) > -{ > - struct dl_open_hook **hook; > - > - hook = (struct dl_open_hook **) __libc_dlsym_private (map, "_dl_open_hook"); > - if (hook != NULL) > - *hook = &_dl_open_hook; > - > - /* For dlvsym support. */ > - hook = (struct dl_open_hook **) __libc_dlsym_private (map, "_dl_open_hook2"); > - if (hook != NULL) > - *hook = &_dl_open_hook; > -} > #endif > > void * Ok. > @@ -242,7 +187,7 @@ __libc_dlsym (void *map, const char *name) > > #ifdef SHARED > if (!rtld_active ()) > - return _dl_open_hook->dlsym (map, name); > + return GLRO (dl_dlfcn_hook)->libc_dlsym (map, name); > #endif > return (dlerror_run (do_dlsym, &args) ? NULL > : (void *) (DL_SYMBOL_ADDRESS (args.loadbase, args.ref))); > @@ -257,13 +202,7 @@ __libc_dlvsym (void *map, const char *name, const char *version) > { > #ifdef SHARED > if (!rtld_active ()) > - { > - /* The static application is too old and does not provide the > - dlvsym hook. */ > - if (_dl_open_hook2 == NULL) > - return NULL; > - return _dl_open_hook2->dlvsym (map, name, version); > - } > + return GLRO (dl_dlfcn_hook)->libc_dlvsym (map, name, version); > #endif > > struct do_dlvsym_args args; Ok. > @@ -287,7 +226,7 @@ __libc_dlclose (void *map) > { > #ifdef SHARED > if (!rtld_active ()) > - return _dl_open_hook->dlclose (map); > + return GLRO (dl_dlfcn_hook)->libc_dlclose (map); > #endif > return dlerror_run (do_dlclose, map); > } Ok. > diff --git a/elf/rtld_static_init.c b/elf/rtld_static_init.c > index 42efecfbff..3f8abb6800 100644 > --- a/elf/rtld_static_init.c > +++ b/elf/rtld_static_init.c > @@ -25,6 +25,23 @@ > > #include > > +static const struct dlfcn_hook _dlfcn_hook = > + { > + .dlopen = __dlopen, > + .dlclose = __dlclose, > + .dlsym = __dlsym, > + .dlvsym = __dlvsym, > + .dlerror = __dlerror, > + .dladdr = __dladdr, > + .dladdr1 = __dladdr1, > + .dlinfo = __dlinfo, > + .dlmopen = __dlmopen, > + .libc_dlopen_mode = __libc_dlopen_mode, > + .libc_dlsym = __libc_dlsym, > + .libc_dlvsym = __libc_dlvsym, > + .libc_dlclose = __libc_dlclose, > + }; > + > void > __rtld_static_init (struct link_map *map) > { > @@ -45,6 +62,7 @@ __rtld_static_init (struct link_map *map) > extern __typeof (dl->_dl_clktck) _dl_clktck attribute_hidden; > dl->_dl_clktck = _dl_clktck; > #endif > + dl->_dl_dlfcn_hook = &_dlfcn_hook; > extern __typeof (dl->_dl_hwcap) _dl_hwcap attribute_hidden; > dl->_dl_hwcap = _dl_hwcap; > extern __typeof (dl->_dl_hwcap2) _dl_hwcap2 attribute_hidden; Ok. > diff --git a/include/dlfcn.h b/include/dlfcn.h > index 711bbb0f12..d4440c567e 100644 > --- a/include/dlfcn.h > +++ b/include/dlfcn.h > @@ -91,8 +91,12 @@ libc_hidden_proto (_dl_vsym) > extern int _dlerror_run (void (*operate) (void *), void *args); > libc_hidden_proto (_dlerror_run) > > +/* This structure is used to make the outer (statically linked) > + implementation of dlopen and related functions to the inner libc > + after static dlopen, via the GLRO (dl_dlfcn_hook) pointer. */ > struct dlfcn_hook > { > + /* Public interfaces. */ > void *(*dlopen) (const char *file, int mode, void *dl_caller); > int (*dlclose) (void *handle); > void *(*dlsym) (void *handle, const char *name, void *dl_caller); > @@ -104,15 +108,17 @@ struct dlfcn_hook > void **extra_info, int flags); > int (*dlinfo) (void *handle, int request, void *arg); > void *(*dlmopen) (Lmid_t nsid, const char *file, int mode, void *dl_caller); > - void *pad[4]; > -}; > > -extern struct dlfcn_hook *_dlfcn_hook; > -libc_hidden_proto (_dlfcn_hook) > + /* Internal interfaces. */ > + void* (*libc_dlopen_mode) (const char *__name, int __mode); > + void* (*libc_dlsym) (void *map, const char *name); > + void* (*libc_dlvsym) (void *map, const char *name, const char *version); > + int (*libc_dlclose) (void *map); > +}; Ok. > > -/* Note: These prototypes are for initializing _dflcn_hook in static > - libraries. Internal calls in glibc should use the __libc_dl* > - functions defined in elf/dl-libc.c instead. */ > +/* Note: These prototypes are for initializing _dlfcn_hook in static > + builds; see __rtld_static_init. Internal calls in glibc should use > + the __libc_dl* functions defined in elf/dl-libc.c instead. */ > > extern void *__dlopen (const char *file, int mode, void *caller); > extern void *__dlmopen (Lmid_t nsid, const char *file, int mode, > @@ -125,16 +131,7 @@ extern int __dladdr (const void *address, Dl_info *info); > extern int __dladdr1 (const void *address, Dl_info *info, > void **extra_info, int flags); > extern int __dlinfo (void *handle, int request, void *arg); > - > -#ifndef SHARED > -struct link_map; > -extern void * __libc_dlsym_private (struct link_map *map, const char *name) > - attribute_hidden; > -extern void __libc_register_dl_open_hook (struct link_map *map) > - attribute_hidden; > -extern void __libc_register_dlfcn_hook (struct link_map *map) > - attribute_hidden; > -#endif > +extern char *__dlerror (void); > > #endif > #endif Ok. > diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h > index e383aa1dc3..176394de4d 100644 > --- a/sysdeps/generic/ldsodefs.h > +++ b/sysdeps/generic/ldsodefs.h > @@ -687,6 +687,9 @@ struct rtld_global_ro > int (*_dl_discover_osversion) (void); > #endif > > + /* Dynamic linker operations used after static dlopen. */ > + const struct dlfcn_hook *_dl_dlfcn_hook; > + > /* List of auditing interfaces. */ > struct audit_ifaces *_dl_audit; > unsigned int _dl_naudit; > Ok.