[PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[Vitaly Chikunov]

Avoid calling expensive dlopen at the cost of an extra getenv check when
we already know it would not be needed.

This mirrors the getenv check from debuginfod_query_server.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>
---
 libdwfl/debuginfod-client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index ee604ad9..6fd83900 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -38,6 +38,8 @@ static __typeof__ (debuginfod_find_executable) *fp_debuginfod_find_executable;
 static __typeof__ (debuginfod_find_debuginfo) *fp_debuginfod_find_debuginfo;
 static __typeof__ (debuginfod_end) *fp_debuginfod_end;
 
+static const char *server_urls_envvar = DEBUGINFOD_URLS_ENV_VAR;
+
 /* NB: this is slightly thread-unsafe */
 
 static debuginfod_client *
@@ -101,6 +103,12 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
 void __attribute__ ((constructor))
 __libdwfl_debuginfod_init (void)
 {
+  /* Is there any server we can query?  If not, don't do any work,
+   * just return.  Don't try to dlopen.  */
+  char *urls_envvar = getenv(server_urls_envvar);
+  if (urls_envvar == NULL || urls_envvar[0] == '\0')
+    return;
+
   void *debuginfod_so = dlopen("libdebuginfod-" VERSION ".so", RTLD_LAZY);
 
   if (debuginfod_so == NULL)
-- 
2.25.4

Re: [PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[Mark Wielaard]

Hi Vitaly,

On Thu, 2020-11-05 at 17:44 +0300, Vitaly Chikunov wrote:
> Avoid calling expensive dlopen at the cost of an extra getenv check when
> we already know it would not be needed.
> 
> This mirrors the getenv check from debuginfod_query_server.

I am sympathetic to this. It would have been a nicer way to fix the
valgrind issue I recently had with dlopen having unsuppressible errors
on some older glibc versions. But it also makes it so that an
application can never use the debuginfod support afterwards. The
current debuginfod_query_server check is late enough that an
application could set DEBUGINFOD_URLS just before a Dwarf lookup.

What do you think? Is that a valid use case?

Should we maybe introduce setting debuginfod_urls on Dwfls so that an
application can explicitly indicate they want to use debuginfod lookups
and then dlopen libdebuginfod late? The problem with doing the dlopen
late is that we also need libcurl and initializing libcurl (as done by
libdebuginfod) is not thread-safe.

Cheers,

Mark

Re: [PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[Frank Ch. Eigler]

Hi -

> [...] The problem with doing the dlopen late is that we also need
> libcurl and initializing libcurl (as done by libdebuginfod) is not
> thread-safe.

From reading libcurl code, and that of other clients, I still believe
this concern was & is overrated.  We could back down to simple
debuginfod_begin time mutex-protected curl_global_init calls, and we'd
be as fine as other applications.  We could ditch the dlopen /
dso-ctor issues entirely.

- FChE

Re: [PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[Dmitry V. Levin]

Hi,

On Mon, Nov 09, 2020 at 09:57:57AM -0500, Frank Ch. Eigler via Elfutils-devel wrote:
> Hi -
> 
> > [...] The problem with doing the dlopen late is that we also need
> > libcurl and initializing libcurl (as done by libdebuginfod) is not
> > thread-safe.
> 
> From reading libcurl code, and that of other clients, I still believe
> this concern was & is overrated.  We could back down to simple
> debuginfod_begin time mutex-protected curl_global_init calls, and we'd
> be as fine as other applications.  We could ditch the dlopen /
> dso-ctor issues entirely.

Excuse me, has there been any follow-up to this discussion?


-- 
ldv

Re: [PATCH] libdwfl: Do not dlopen libdebuginfod if DEBUGINFOD_URLS is unset or empty

[Mark Wielaard]

On Wed, 2020-12-09 at 18:35 +0300, Dmitry V. Levin wrote:
> On Mon, Nov 09, 2020 at 09:57:57AM -0500, Frank Ch. Eigler via Elfutils-devel wrote:
> > > [...] The problem with doing the dlopen late is that we also need
> > > libcurl and initializing libcurl (as done by libdebuginfod) is not
> > > thread-safe.
> > 
> > From reading libcurl code, and that of other clients, I still believe
> > this concern was & is overrated.  We could back down to simple
> > debuginfod_begin time mutex-protected curl_global_init calls, and we'd
> > be as fine as other applications.  We could ditch the dlopen /
> > dso-ctor issues entirely.
> 
> Excuse me, has there been any follow-up to this discussion?

Not really. The issue is that the documentation of curl_global_init has
this very scary warning: "This function is not thread safe. You must
not call it when any other thread in the program (i.e. a thread sharing
the same memory) is running. This doesn't just mean no other thread
that is using libcurl." https://curl.se/libcurl/c/curl_global_init.html

So we try to make sure curl_global_init is called as soon as possible
by putting the call in the libdebuginfod.so libdebuginfod_ctor and
libdw has a __libdwfl_debuginfod_init ctor which tries to do the dlopen
of libdebuginfod as early as possible. Using ctors here is done so that
we hopefully call curl_global_init before any threads are started.
Frank might be right that all this is just super paranoia. But given
that the libcurl developers put that scary warning in their
documentation for now I am assuming we have to be that paranoid. I
haven't asked them if they believe that comment is still current
though.

Cheers,

Mark