Avoid dlopen on debuginfod-client when not configured

  Loading debuginfod-client is expensive, especially when
FIPS is enabled, as it goes through time intensive self-checks
on load. Avoid dlopen() when no debuginfo url is set.

Signed-off-by: Dirk Müller <dirk@dmllr.de>
---
 libdwfl/ChangeLog           |  5 +++++
 libdwfl/debuginfod-client.c | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)
  

Hi Dirk,

On Mon, Mar 28, 2022 at 11:36:41PM +0200, Dirk Müller wrote:
> Loading debuginfod-client is expensive, especially when
> FIPS is enabled, as it goes through time intensive self-checks
> on load.

I assume this is because debuginfod-client loads and initializes
libcurl, which triggers crypto checks for https support?

> Avoid dlopen() when no debuginfo url is set.

The patch itself looks right. But I am slightly afraid this
(theoretically?) will break some programs which set DEBUGINFOD_URLS
themselves. We currently have no other way to make libdwfl use
debuginfod-client. Thoughts?

Cheers,

Mark

> Signed-off-by: Dirk Müller <dirk@dmllr.de>
> ---
>  libdwfl/ChangeLog           |  5 +++++
>  libdwfl/debuginfod-client.c | 12 +++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index 9c5c8517..1ec13f30 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-03-28  Dirk Müller <dirk@dmllr.de>
> +
> +	* debuginfod-client.c (__libdwfl_debuginfod_init): Skip dlopen
> +	if debuginfod url is unset.
> +
>  2022-02-18  Mark Wielaard  <mark@klomp.org>
>  
>  	* image-header.c (__libdw_image_header): Assign header values for
> diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
> index 99b66b6e..af9d1363 100644
> --- a/libdwfl/debuginfod-client.c
> +++ b/libdwfl/debuginfod-client.c
> @@ -101,7 +101,17 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
>  void __attribute__ ((constructor))
>  __libdwfl_debuginfod_init (void)
>  {
> -  void *debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
> +  void *debuginfod_so;
> +
> +  /* Is there any server we can query?  If not, don't do any work,
> +     just return with ENOSYS.  Don't even access the cache.  */
> +  char *urls_envvar = getenv(DEBUGINFOD_URLS_ENV_VAR);
> +  if (urls_envvar == NULL || urls_envvar[0] == '\0')
> +    {
> +      return;
> +    }
> +
> +  debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
>  
>    if (debuginfod_so != NULL)
>      {
> -- 
> 2.35.1
>
  

Hi -

> > Avoid dlopen() when no debuginfo url is set.
> 
> The patch itself looks right. But I am slightly afraid this
> (theoretically?) will break some programs which set DEBUGINFOD_URLS
> themselves. We currently have no other way to make libdwfl use
> debuginfod-client. Thoughts?

I've been thinking that doing all this dlopen work in the real dwfl
function(s) rather than the solib ctor would be fine.

- FChE
  

Hi Mark,

Am Di., 29. März 2022 um 09:42 Uhr schrieb Mark Wielaard <mark@klomp.org>:

> I assume this is because debuginfod-client loads and initializes
> libcurl, which triggers crypto checks for https support?

Yes, the dependencies of the DSO contain libcurl and other libraries
including the openssl library, which, when patched with FIPS support,
take quite a while to initialize.

> The patch itself looks right. But I am slightly afraid this
> (theoretically?) will break some programs which set DEBUGINFOD_URLS
> themselves. We currently have no other way to make libdwfl use
> debuginfod-client. Thoughts?

I was concerned about that as well, but I couldn't google a case where
this is the case. What we can also do is to do the initialization
lazily upon first use rather than in a
constructor function, but the intention of initializing early has been
specified as avoiding issues in multiple threads with libcurl:

commit fa0226a78a101d26fd80c7e9e70a5f0aa6f9d1cc
Author: Mark Wielaard <mark@klomp.org>
Date:   Sun Nov 17 22:17:26 2019 +0100

   debuginfod: add client context

   Add a mandatory debuginfod_begin()/_end() call pair to manage a client
   object that represents persistent but non-global state.  From libdwfl,
   dlopen the debuginfod.so client library early on.  This hopefully
   makes sure that the code (and the libcurl.so dependency) is loaded
   before the program goes into multi-threaded mode.


What we can do is both, do the reinit at a later point in time if the
environment variable happens to be set at that point in time. then you
are risking the thread-safety issues only when there is no other way.

Or just do it lazily on demand, and work through the libcurl or
whatever the unsafety issue is.

Greetings,
Dirk
  

Hi Dirk,

On Tue, 2022-03-29 at 13:06 +0200, Dirk Müller wrote:
> Am Di., 29. März 2022 um 09:42 Uhr schrieb Mark Wielaard <
> mark@klomp.org>:
> > The patch itself looks right. But I am slightly afraid this
> > (theoretically?) will break some programs which set DEBUGINFOD_URLS
> > themselves. We currently have no other way to make libdwfl use
> > debuginfod-client. Thoughts?
> 
> I was concerned about that as well, but I couldn't google a case where
> this is the case. What we can also do is to do the initialization
> lazily upon first use rather than in a
> constructor function, but the intention of initializing early has been
> specified as avoiding issues in multiple threads with libcurl:
> 
> commit fa0226a78a101d26fd80c7e9e70a5f0aa6f9d1cc
> Author: Mark Wielaard <mark@klomp.org>
> Date:   Sun Nov 17 22:17:26 2019 +0100
> 
>    debuginfod: add client context
> 
>    Add a mandatory debuginfod_begin()/_end() call pair to manage a client
>    object that represents persistent but non-global state.  From libdwfl,
>    dlopen the debuginfod.so client library early on.  This hopefully
>    makes sure that the code (and the libcurl.so dependency) is loaded
>    before the program goes into multi-threaded mode.
> 
> 
> What we can do is both, do the reinit at a later point in time if the
> environment variable happens to be set at that point in time. then you
> are risking the thread-safety issues only when there is no other way.
> 
> Or just do it lazily on demand, and work through the libcurl or
> whatever the unsafety issue is.

This is indeed the (safety) issue I am concerned about. I just sent an
email to the libcurl mailinglist (CCed to the elfutils-devel list) to
ask about how/when/if a library should call curl_global_init.
https://curl.se/mail/lib-2022-03/0118.html

Lets see what the curl developers say and then decide what to do.

Cheers,

Mark