[PATCH] Avoid dlopen on debuginfod-client when not configured

[PATCH] Avoid dlopen on debuginfod-client when not configured

[Dirk Müller]

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(-)

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

Re: [PATCH] Avoid dlopen on debuginfod-client when not configured

[Mark Wielaard]

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
> 

Re: [PATCH] Avoid dlopen on debuginfod-client when not configured

[Frank Ch. Eigler]

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

Re: [PATCH] Avoid dlopen on debuginfod-client when not configured

[Dirk Müller]

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

Re: [PATCH] Avoid dlopen on debuginfod-client when not configured

[Mark Wielaard]

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