Re: patch 1/2 debuginfod client

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Tue, 2019-11-19 at 16:22 -0500, Frank Ch. Eigler wrote:
> On Tue, Nov 19, 2019 at 09:15:20PM +0100, Mark Wielaard wrote:
> > > That's what the doc says.  What the code does, as far back as the year
> > > 2001, is have a static flag/counter in curl_global_init() that
> > > prevents multiple initialization.
> > 
> > But the warning isn't about multiple initialization. It is about
> > initialization when other threads are running that might be using any
> > of the libcurl support libraries.
> 
> Since 2001, the curl_global_init function does nothing to interfere
> with any libcurl activity, if the library is already initialized.  Any
> call to the normal libcurl functions first calls this function.  I
> guess I just fail to see a plausible problem scenario short of a
> minuscule race over incrementing an initialization counter, which is a
> race that every other libcurl user has accepted.

But it isn't just about interference with other libcurl activity. If
you look at the curl_global_init code you see that it actually calls a
lot of code in other libraries. It is the curl_global_init code that
shouldn't be run in a multi-threaded environment. That it is acceptable
to others doesn't immediately make it safe to use in our case. We are
slowly trying to make libdw.so into a multi-tread safe library and do
expect it to be used in multi-threaded code. We aren't fully there yet.
But it would be a shame to introduce more issues if we can prevent it.

> > > > That is why I think doing the dlopen of libdebuginfod.so eagerly from a
> > > > libdw.so constructor function or _init might be necessary to make sure
> > > > no other threads are running yet.
> > > 
> > > That's not even enough for "sure" - if we're so deep in the
> > > hypotheticals hole, an application could be dlopen()ing libdw.so
> > > itself.
> > 
> > It could, but I think that would be unlikely. We can at least document
> > that it is unsafe to use libdw.so with dlopen. But if it is done,
> > could we detect it and not do the loading of libdebuginfod.so in that
> > case?
> 
> I don't know how to do that.

I assume you mean the second part. The attached is what I would propose
for the first part. Do you think that is a bad idea?

Thanks,

Mark

diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index 37a4c71f..ee604ad9 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -46,32 +46,7 @@ get_client (Dwfl *dwfl)
   if (dwfl->debuginfod != NULL)
     return dwfl->debuginfod;
 
-  if (fp_debuginfod_begin == NULL)
-    {
-      void *debuginfod_so = dlopen("libdebuginfod-" VERSION ".so", RTLD_LAZY);
-
-      if (debuginfod_so == NULL)
-	debuginfod_so = dlopen("libdebuginfod.so", RTLD_LAZY);
-
-      if (debuginfod_so != NULL)
-	{
-	  fp_debuginfod_begin = dlsym (debuginfod_so, "debuginfod_begin");
-	  fp_debuginfod_find_executable = dlsym (debuginfod_so,
-						 "debuginfod_find_executable");
-	  fp_debuginfod_find_debuginfo = dlsym (debuginfod_so,
-						"debuginfod_find_debuginfo");
-	  fp_debuginfod_end = dlsym (debuginfod_so, "debuginfod_end");
-	}
-
-      if (fp_debuginfod_begin == NULL
-	  || fp_debuginfod_find_executable == NULL
-	  || fp_debuginfod_find_debuginfo == NULL
-	  || fp_debuginfod_end == NULL)
-	fp_debuginfod_begin = (void *) -1; /* never try again */
-    }
-
-  if (fp_debuginfod_begin != NULL
-      && fp_debuginfod_begin != (void *) -1)
+  if (fp_debuginfod_begin != NULL)
     {
       dwfl->debuginfod = (*fp_debuginfod_begin) ();
       return dwfl->debuginfod;
@@ -120,3 +95,37 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
   if (c != NULL)
     (*fp_debuginfod_end) (c);
 }
+
+/* Try to get the libdebuginfod library functions to make sure
+   everything is initialized early.  */
+void __attribute__ ((constructor))
+__libdwfl_debuginfod_init (void)
+{
+  void *debuginfod_so = dlopen("libdebuginfod-" VERSION ".so", RTLD_LAZY);
+
+  if (debuginfod_so == NULL)
+    debuginfod_so = dlopen("libdebuginfod.so", RTLD_LAZY);
+
+  if (debuginfod_so != NULL)
+    {
+      fp_debuginfod_begin = dlsym (debuginfod_so, "debuginfod_begin");
+      fp_debuginfod_find_executable = dlsym (debuginfod_so,
+					     "debuginfod_find_executable");
+      fp_debuginfod_find_debuginfo = dlsym (debuginfod_so,
+					    "debuginfod_find_debuginfo");
+      fp_debuginfod_end = dlsym (debuginfod_so, "debuginfod_end");
+
+      /* We either get them all, or we get none.  */
+      if (fp_debuginfod_begin == NULL
+	  || fp_debuginfod_find_executable == NULL
+	  || fp_debuginfod_find_debuginfo == NULL
+	  || fp_debuginfod_end == NULL)
+	{
+	  fp_debuginfod_begin = NULL;
+	  fp_debuginfod_find_executable = NULL;
+	  fp_debuginfod_find_debuginfo = NULL;
+	  fp_debuginfod_end = NULL;
+	  dlclose (debuginfod_so);
+	}
+    }
+}