[v2] 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 to serialize calls to
__libdwfl_debuginfod_find_*.

Signed-off-by: Aaron Merey <amerey@redhat.com>
---

v2 changes: Reverted changes to a comment for __libdwfl_debuginfod_init.
Rename dwfl->lock to dwfl->debuginfod_lock.

On Thu, Dec 4, 2025 at 7:03 PM Aaron Merey <amerey@redhat.com> wrote:
> On Wed, Dec 3, 2025 at 11:19 AM Mark Wielaard <mark@klomp.org> wrote:
> > 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.

I experimented with adding thread safety at the libdebuginfod level
instead of libdwfl.  Reasoning about its correctness ended up being
more complex than expected, in part due to the client progressfn
callback that can be called during a query.

After some discussion with Frank I think that the effort to make
libdebuginfod thread safe isn't trivial and may not be worth the time,
as we don't currently have a clear use case that would see a worthwhile
performance benefit.

 libdwfl/debuginfod-client.c | 6 +++++-
 libdwfl/dwfl_begin.c        | 2 ++
 libdwfl/dwfl_end.c          | 1 +
 libdwfl/libdwflP.h          | 3 +++
 4 files changed, 11 insertions(+), 1 deletion(-)
  

Hi Aaron,

On Fri, 2025-12-12 at 20:34 -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 to serialize calls to
> __libdwfl_debuginfod_find_*.
> 
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
> 
> v2 changes: Reverted changes to a comment for __libdwfl_debuginfod_init.
> Rename dwfl->lock to dwfl->debuginfod_lock.

That does make things more clear, thanks.

> On Thu, Dec 4, 2025 at 7:03 PM Aaron Merey <amerey@redhat.com> wrote:
> > On Wed, Dec 3, 2025 at 11:19 AM Mark Wielaard <mark@klomp.org> wrote:
> > > 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.
> 
> I experimented with adding thread safety at the libdebuginfod level
> instead of libdwfl.  Reasoning about its correctness ended up being
> more complex than expected, in part due to the client progressfn
> callback that can be called during a query.
> 
> After some discussion with Frank I think that the effort to make
> libdebuginfod thread safe isn't trivial and may not be worth the time,
> as we don't currently have a clear use case that would see a worthwhile
> performance benefit.

OK, at least internal libdwfl usage is now thread-safe. If the Dwfl
functions that use the debuginfod_client handle are called from
different threads.

The only remaining issue is that the user can call
dwfl_get_debuginfod_client (dwfl) to get the debuginfod_client handle
and use it concurrently with other dwfl operations that might trigger
operations on the same handle.

Not sure what the best "solution" for that is. We could add a new
method to get the debuginfod_lock of the dwfl and require the user uses
that when invoking any method on the debuginfod_client handle. Or maybe
we need dwfl_put_back_debuginfod_client (dwfl) to signal the user is
done with it?

Or we could just add documentation that the handle should only be used
to when not using any other dwfl methods?

The patch looks good otherwise.

Thanks,

Mark

>  libdwfl/debuginfod-client.c | 6 +++++-
>  libdwfl/dwfl_begin.c        | 2 ++
>  libdwfl/dwfl_end.c          | 1 +
>  libdwfl/libdwflP.h          | 3 +++
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
> index 882a5eff..ecb053fc 100644
> --- a/libdwfl/debuginfod-client.c
> +++ b/libdwfl/debuginfod-client.c
> @@ -49,7 +49,7 @@ static void __libdwfl_debuginfod_init (void);
>  
>  static pthread_once_t init_control = PTHREAD_ONCE_INIT;
>  
> -/* NB: this is slightly thread-unsafe */
> +/* dwfl->debuginfod_lock must be held when calling this function.  */
>  
>  debuginfod_client *
>  dwfl_get_debuginfod_client (Dwfl *dwfl)
> @@ -77,10 +77,12 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
>    int fd = -1;
>    if (build_id_len > 0)
>      {
> +      mutex_lock (dwfl->debuginfod_lock);
>        debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl);
>        if (c != NULL)
>  	fd = (*fp_debuginfod_find_executable) (c, build_id_bits,
>  					       build_id_len, NULL);
> +      mutex_unlock (dwfl->debuginfod_lock);
>      }
>  
>    return fd;
> @@ -94,10 +96,12 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
>    int fd = -1;
>    if (build_id_len > 0)
>      {
> +      mutex_lock (dwfl->debuginfod_lock);
>        debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl);
>        if (c != NULL)
>  	fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits,
>  					      build_id_len, NULL);
> +      mutex_unlock (dwfl->debuginfod_lock);
>      }
>  
>    return fd;
> diff --git a/libdwfl/dwfl_begin.c b/libdwfl/dwfl_begin.c
> index b03f5cf4..835de8e3 100644
> --- a/libdwfl/dwfl_begin.c
> +++ b/libdwfl/dwfl_begin.c
> @@ -50,6 +50,8 @@ dwfl_begin (const Dwfl_Callbacks *callbacks)
>        dwfl->offline_next_address = OFFLINE_REDZONE;
>      }
>  
> +  mutex_init (dwfl->debuginfod_lock);
> +
>    return dwfl;
>  }
>  INTDEF (dwfl_begin)
> diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c
> index d9cf569b..c82d83fb 100644
> --- a/libdwfl/dwfl_end.c
> +++ b/libdwfl/dwfl_end.c
> @@ -53,6 +53,7 @@ dwfl_end (Dwfl *dwfl)
>    free (dwfl->lookup_module);
>    free (dwfl->lookup_segndx);
>    free (dwfl->sysroot);
> +  mutex_fini (dwfl->debuginfod_lock);
>  
>    Dwfl_Module *next = dwfl->modulelist;
>    while (next != NULL)
> diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
> index a5d88d60..6e394c26 100644
> --- a/libdwfl/libdwflP.h
> +++ b/libdwfl/libdwflP.h
> @@ -141,6 +141,9 @@ struct Dwfl
>    struct Dwfl_User_Core *user_core;
>    char *sysroot;		/* sysroot, or NULL to search standard system
>  				   paths */
> +
> +  /* Serialize debuginfod_client usage.  */
> +  mutex_define (, debuginfod_lock);
>  };
>  
>  #define OFFLINE_REDZONE		0x10000