Make __libdwfl_debuginfod_find_* functions thread safe

  Individual debuginfod_client objects and their underlying CURLM handles
should not be used concurrently during calls to
__libdwfl_debuginfod_find_debuginfo and
__libdwfl_debuginfod_find_executable.

Add a mutex field to struct Dwfl and synchronize calls to
__libdwfl_debuginfod_find_*.

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 libdwfl/debuginfod-client.c | 9 +++++++--
 libdwfl/dwfl_begin.c        | 2 ++
 libdwfl/dwfl_end.c          | 1 +
 libdwfl/libdwflP.h          | 1 +
 4 files changed, 11 insertions(+), 2 deletions(-)
  

Hi -

> Individual debuginfod_client objects and their underlying CURLM handles
> should not be used concurrently [...]

Yeah, pity eh!  libdebuginfod is perfectly happy with concurrent
calls, but only on different debuginfod_client handles, rather than
the single shared one tracked by libdwfl.  It's unlikely to be worth
the effort to keep a pool of such handles to be shared among threads
... but maybe data could answer that question.

- FChE
  

Hi Aaron,

On Mon, 2025-12-01 at 12:17 -0500, Aaron Merey wrote:
> Individual debuginfod_client objects and their underlying CURLM handles
> should not be used concurrently during calls to
> __libdwfl_debuginfod_find_debuginfo and
> __libdwfl_debuginfod_find_executable.
> 
> Add a mutex field to struct Dwfl and synchronize calls to
> __libdwfl_debuginfod_find_*.

So if I understand correctly there are two issues here.

The debuginfo_client is created and assigned to a Dwfl field lazily. So
you want to prevent concurrent calls to dwfl_get_debuginfod_client.

A debuginfo_client has CURLM handle which cannot be used by different
threads concurrently.

Since we have one unique debuginfo_client per dwfl you use one and the
same lock?

I believe this patch should work. But have you considered having a lock
inside debuginfod-client itself to protect access to the CURLM handles?
That would (maybe) result is slightly simpler code. And would help
other users of libdebuginfod that might use debuginfod_client handles
from multiple threads (gdb?).

> @@ -111,7 +115,8 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
>  }
>  
>  /* Try to get the libdebuginfod library functions.
> -   Only needs to be called once from dwfl_get_debuginfod_client.  */
> +   Only needs to be called once from dwfl_get_debuginfod_client.
> +   A per-Dwfl lock must be held when calling this function.  */
>  static void
>  __libdwfl_debuginfod_init (void)
>  {

Why does it need a per-Dwfl lock? It is already guarded by a
pthread_once. And it doesn't seem to manipulate any Dwfl struct field
structures (and cannot because it isn't passed a Dwfl object).


Cheers,

Mark
  

Hi Frank,

On Mon, Dec 1, 2025 at 4:44 PM Frank Ch. Eigler <fche@redhat.com> wrote:
>
> > Individual debuginfod_client objects and their underlying CURLM handles
> > should not be used concurrently [...]
>
> Yeah, pity eh!  libdebuginfod is perfectly happy with concurrent
> calls, but only on different debuginfod_client handles, rather than
> the single shared one tracked by libdwfl.  It's unlikely to be worth
> the effort to keep a pool of such handles to be shared among threads
> ... but maybe data could answer that question.

Interesting idea, the upcoming systemtap parallelization work will
provide a nice way to test this.

Aaron
  

Hi Mark,

On Wed, Dec 3, 2025 at 11:19 AM Mark Wielaard <mark@klomp.org> wrote:
>
> On Mon, 2025-12-01 at 12:17 -0500, Aaron Merey wrote:
> > Individual debuginfod_client objects and their underlying CURLM handles
> > should not be used concurrently during calls to
> > __libdwfl_debuginfod_find_debuginfo and
> > __libdwfl_debuginfod_find_executable.
> >
> > Add a mutex field to struct Dwfl and synchronize calls to
> > __libdwfl_debuginfod_find_*.
>
> So if I understand correctly there are two issues here.
>
> The debuginfo_client is created and assigned to a Dwfl field lazily. So
> you want to prevent concurrent calls to dwfl_get_debuginfod_client.
>
> A debuginfo_client has CURLM handle which cannot be used by different
> threads concurrently.
>
> Since we have one unique debuginfo_client per dwfl you use one and the
> same lock?
>
> I believe this patch should work. But have you considered having a lock
> inside debuginfod-client itself to protect access to the CURLM handles?
> That would (maybe) result is slightly simpler code. And would help
> other users of libdebuginfod that might use debuginfod_client handles
> from multiple threads (gdb?).

I agree that it is better to have dedicated per-client locks instead.
Theres no need for libdwfl functions to wait on locks held for
independent debuginfod purposes.

>
> > @@ -111,7 +115,8 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
> >  }
> >
> >  /* Try to get the libdebuginfod library functions.
> > -   Only needs to be called once from dwfl_get_debuginfod_client.  */
> > +   Only needs to be called once from dwfl_get_debuginfod_client.
> > +   A per-Dwfl lock must be held when calling this function.  */
> >  static void
> >  __libdwfl_debuginfod_init (void)
> >  {
>
> Why does it need a per-Dwfl lock? It is already guarded by a
> pthread_once. And it doesn't seem to manipulate any Dwfl struct field
> structures (and cannot because it isn't passed a Dwfl object).

You're right since this function is only ever used by pthread_once. I
had general usage of __libdwfl_debuginfod_init in mind when I wrote
that but it's not relevant in this case.

Aaron